Repository navigation
6: Test prune engine #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 36 commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
6d6cb91
6: super-plan for prune engine
wjduenow 965c2d0
6: bump plan phase to published
wjduenow 80ac03c
6: devolve plan — beads manifest
wjduenow 8c7db22
bd_1-scaffolding-y8y.1: scaffold signalforge.prune subpackage
wjduenow 50dca01
Merge bead bd_1-scaffolding-y8y.1: 6/US-001 subpackage scaffold
wjduenow fb99a31
bd_1-scaffolding-y8y.4: PruneResult / PruneDecision / primitives
wjduenow 7f78624
Merge bead bd_1-scaffolding-y8y.4: 6/US-004 PruneResult / PruneDecision
wjduenow ec037b0
bd_1-scaffolding-y8y.6: PruneError hierarchy
wjduenow 9e6eb1d
bd_1-scaffolding-y8y.6: drop spurious signalforge.errors module
wjduenow 1390cd4
Merge bead bd_1-scaffolding-y8y.6: 6/US-006 PruneError hierarchy
wjduenow 7f4f9cd
bd_1-scaffolding-y8y.7: compiler — failing-rows SQL x 4 test types
wjduenow 1f3abb6
Merge bead bd_1-scaffolding-y8y.7: 6/US-007 compiler
wjduenow 10921f8
bd_1-scaffolding-y8y.8: prune audit — fail-closed prune.jsonl writer
wjduenow a9fdd34
Merge bead bd_1-scaffolding-y8y.8: 6/US-008 audit + AST scan
wjduenow 2f27463
bd_1-scaffolding-y8y.5: PruneConfig + load_prune_config
wjduenow 0eaf437
bd_1-scaffolding-y8y.5: drop dead _DEFAULT_CONFIG_FILENAME constant
wjduenow e2f68bc
Merge bead bd_1-scaffolding-y8y.5: 6/US-005 PruneConfig + load_prune_…
wjduenow 01a4990
bd_1-scaffolding-y8y.2: warehouse adapter timeout_ms plumbing
wjduenow 3333c99
Merge bead bd_1-scaffolding-y8y.2: 6/US-002 adapter timeout_ms plumbing
wjduenow b34f019
bd_1-scaffolding-y8y.9: prune engine — prune_tests orchestrator
wjduenow f025a5f
Merge bead bd_1-scaffolding-y8y.9: 6/US-009 prune_tests orchestrator
wjduenow cb18270
bd_1-scaffolding-y8y.13: signalforge.prune public re-exports
wjduenow 54a3f61
Merge bead bd_1-scaffolding-y8y.13: 6/US-013 public re-exports
wjduenow 88494e1
bd_1-scaffolding-y8y.11: extend logger grep gate to signalforge.prune
wjduenow 1406c78
Merge bead bd_1-scaffolding-y8y.11: 6/US-011 grep gate extension
wjduenow 0c48d09
bd_1-scaffolding-y8y.10: drift detectors for prune read-back models
wjduenow 79015e4
Merge bead bd_1-scaffolding-y8y.10: 6/US-010 drift detectors
wjduenow 801b5be
bd_1-scaffolding-y8y.3: diagnostic cost probe + ops-doc scaffold
wjduenow dd8f321
Merge bead bd_1-scaffolding-y8y.3: 6/US-003 cost probe + ops-doc scaf…
wjduenow b39d811
bd_1-scaffolding-y8y.12: BQ integration test against bigquery-public-…
wjduenow b0a2370
Merge bead bd_1-scaffolding-y8y.12: 6/US-012 BQ integration test
wjduenow d882274
bd_1-scaffolding-y8y.14: docs/prune-ops.md + CLAUDE.md update
wjduenow 2cc75ea
Merge bead bd_1-scaffolding-y8y.14: 6/US-014 docs/prune-ops + CLAUDE.md
wjduenow e64df5a
6/QG: fix bugs surfaced by 4-pass code review
wjduenow 74d67b9
bd_1-scaffolding-y8y.16: distill prune-layer rules into .claude/rules
wjduenow 9a9da06
Merge bead bd_1-scaffolding-y8y.16: 6/PM rules + memory
wjduenow e5fb7cf
6/QG: wire prune scope/sample_size/partition_filter into compiled SQL
wjduenow 1ae462f
6/QG: prune audit short-write loop + SF_RUN_BQ truthy guard + BQ prob…
wjduenow 27e2bee
6/QG: docs/rules corrections + rename make_query_job_config to _make_…
wjduenow File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| # Prune layer (decision routing + fail-closed audit) | ||
|
|
||
| Established by issue #6 (test prune engine). Apply to every module under `signalforge.prune` and to any new code that compiles a candidate test to SQL, runs it against the warehouse, or writes a prune-decision audit record. | ||
|
|
||
| The prune layer sits between the LLM-drafting pipeline (#5) and the quality grader (#7). It enforces SignalForge's load-bearing "signal over volume" commitment: a candidate test that always passes on warehouse samples must be dropped, not shipped. Equally, a candidate test we cannot evaluate must be kept — pruning is allowed to drop only with positive evidence. | ||
|
|
||
| ## Conservative drop-reason taxonomy (DEC-006, DEC-011) | ||
|
|
||
| `DropReason` is a `Literal[...]` of exactly five values: | ||
|
|
||
| - `"always-passes"` — test ran, returned zero failing rows on a representative sample, scope was sufficient. Drop. | ||
| - `"failed-on-known-clean-data"` — test ran against a `trusted_models` opt-in target and returned non-zero failing rows. The model is known clean; the test is wrong. Drop. | ||
| - `"requires-future-data"` — test compiled to a sentinel (e.g. `relationships(to: <unknown>)` where the target model is absent from the manifest). Cannot evaluate; ship it for the operator to revisit when the dependency lands. Kept. | ||
| - `"kept"` — test ran and returned non-zero failing rows on an untrusted model. Real signal. Kept. | ||
| - `"kept-without-evidence"` — total budget elapsed before this test ran, the test SQL was rejected by `validate_identifier`, the warehouse call raised, or any other "cannot positively evaluate" outcome. Kept. | ||
|
|
||
| The decision matrix lives in `engine.py::_classify_decision`. The load-bearing invariant: **kept-without-evidence routes to `decision="kept"`, not `decision="dropped"`.** SignalForge ships tests it cannot evaluate; the LLM proposed them, and absent contradicting warehouse evidence the operator gets to make the call. Dropping a test silently because we couldn't reach the warehouse is exactly the failure mode this commitment exists to prevent. | ||
|
|
||
| If you add a sixth `DropReason` literal in v0.2 (e.g. `"sample-too-small"`), update production `DropReason` AND `StrictPruneDecision` (the drift detector) AND the fixture at `tests/fixtures/prune/prune_event_v1.jsonl` AND the decision-matrix table in `docs/prune-ops.md` in the same change. | ||
|
|
||
| ## Fail-closed audit (DEC-016, mirrors safety/draft) | ||
|
|
||
| `signalforge.prune.audit.write_prune_event` is the project's third fail-closed JSONL writer (after `signalforge.safety.audit.write` and `signalforge.draft.audit.write_response_event`). The contract is identical to those — load-bearing rules: | ||
|
|
||
| 1. **Propagation IS the defence.** Open with `O_APPEND | O_CREAT | 0o600`, write one JSONL line, `os.fsync`, close. Catches **no** exceptions internally — `OSError`, `PermissionError`, encoding failures all propagate. The caller (`prune_tests`) wraps as `PruneAuditWriteError(cause=...)`. **Don't** add try/except inside the writer; the propagation is the contract. | ||
|
|
||
| 2. **Size cap before any file open.** `_PRUNE_AUDIT_RECORD_LIMIT_BYTES = 4000` is checked before the `os.open`, so an oversize record leaves no on-disk artefact. Raises `PruneAuditRecordTooLargeError(size, limit)` which the orchestrator propagates as-is rather than re-wrapping. | ||
|
|
||
| 3. **Per-decision write happens after each test, not in a final batch.** A `prune_tests` run that crashes mid-iteration leaves the prune.jsonl with one durable record per evaluated test up to the failure point. Buffering writes "to amortise fsync" defeats the audit guarantee. | ||
|
|
||
| An unaudited prune decision is, by definition, a `kept`/`dropped` verdict whose justification didn't durably hit disk — exactly the failure mode this layer exists to prevent. The fail-closed pattern is now established in three layers; treat it as the project default for any future audit-write seam. | ||
|
|
||
| ## Symlink-hardened audit path (DEC-016, post-QG fix) | ||
|
|
||
| `signalforge.warehouse._path_safety.canonicalise_path` is the project's standard symlink/containment gate. Apply it to every user-supplied or default-derived path that the writer opens. | ||
|
|
||
| The original prune-audit implementation opened `<project>/.signalforge/prune.jsonl` directly without canonicalising. The QG review caught it: a symlinked `.signalforge/prune.jsonl -> /etc/passwd` (admittedly hostile, but the reader's path safety enforces this gate uniformly elsewhere) would have written outside the project tree. The fix routes the audit path through `canonicalise_path` at writer entry, raising `PruneAuditWriteError` on containment failure. | ||
|
|
||
| When introducing a fourth audit-write seam — grader (#7), diff renderer (#8), or beyond — apply `canonicalise_path` at writer entry. Don't trust the default path because it's "ours"; the reader's three traps from `manifest-readers.md` apply equally to writers. | ||
|
|
||
| ## Identifier shape validation at the compile seam (DEC-024 + post-QG fix) | ||
|
|
||
| LLM-supplied `test.column` / `test.field` strings land in compiled SQL as backtick-quoted identifiers. The drafter's anchor-contract validator (`llm-drafter.md` DEC-003) checks set-membership against `Manifest.columns` but does NOT enforce regex shape — a manifest column named `foo bar` (whitespace) or `users\`; DROP TABLE` (backtick injection) passes the anchor check. | ||
|
|
||
| The compile seam is the layer's last line of defence. `signalforge.prune.compiler` calls `signalforge.warehouse._sql_safety.validate_identifier` on every identifier (`test.column`, `test.field`; `accepted_values.values` go through `escape_bq_string_literal` per DEC-024) **before** quoting. Failures return a sentinel `_InvalidIdentifier` which the engine routes to `kept-without-evidence` (`why="identifier rejected by SQL safety check"`). | ||
|
|
||
| Defence in depth — same principle as the warehouse adapter's DEC-013 (every public-API identifier validated at construction time). Don't skip the compile-seam validation on the assumption that the manifest is trusted; the manifest reader uses `extra="ignore"` for forward-compat and was not designed to enforce identifier shape on user-authored YAML. | ||
|
|
||
| ## Compiler is dialect-driven, not BigQuery-specific (DEC-025) | ||
|
|
||
| `_compile_test(test, table_ref, dialect: Dialect, manifest)` reads `dialect.quote_char`, `dialect.identifier_case`, and `dialect.supports_qualify` from the `Dialect` value object returned by `WarehouseAdapter.dialect()`. **No `from google.cloud import bigquery` anywhere in `signalforge/prune/`.** v0.2 Snowflake/Postgres adapters return their own `Dialect` and the compiler routes accordingly without modification. | ||
|
|
||
| Mirrors the architectural commitment from `warehouse-adapters.md`: keep BigQuery-isms behind the adapter seam from day one. The `relationships` parent join uses `TableRef.from_model` for the same reason (DEC-026) — the table-reference shape is dialect-neutral. | ||
|
|
||
| The NULL-exclusion pattern in compiled SQL matches dbt-core verbatim (DEC-023): `unique` adds `WHERE col IS NOT NULL`; `accepted_values` adds `WHERE col IS NOT NULL AND col NOT IN (...)`; `relationships` adds child-side `WHERE child.col IS NOT NULL`. Snapshot fixtures pin the exact SQL bytes; divergence from dbt-core verdicts is a regression even if it appears semantically equivalent. | ||
|
|
||
| ## Total-budget semantics (DEC-011) | ||
|
|
||
| When `total_budget_seconds` is exceeded, the orchestrator: | ||
|
|
||
| 1. Best-effort cancels the in-flight test (`query_job.cancel()` on BigQuery; ABC-level no-op fallback for adapters that don't expose cancel). | ||
| 2. Marks every remaining un-started test as `kept-without-evidence` with `why="total prune budget exceeded before evaluation"`. | ||
| 3. Emits a final WARNING with the count of un-started tests (`json.dumps`-formatted). | ||
|
|
||
| No partial-evaluation results — a test that was running when the budget tripped is `kept-without-evidence`, not `kept` (the failing-rows count is unknown). The single-threaded sequential model (DEC-028) makes this enforceable; v0.2 concurrency will need a different cancellation contract. | ||
|
|
||
| `signalforge.prune.engine` declares `_sleep = time.sleep` at module scope per DEC-019 so the budget-watchdog tests reassign it to a deterministic stand-in. Mirrors `llm-drafter.md` DEC-004; same rationale (don't monkey-patch `time.sleep` globally). | ||
|
|
||
| ## Single AST scan extension per new audit type (DEC-018) | ||
|
|
||
| `tests/test_audit_completeness.py` was the unified AST-scan suite as of issue #5; issue #6 extends it with a fifth scan: `Call(func=Name(id="PruneEvent"))` is permitted only in `src/signalforge/prune/audit.py`. The sanity check that *at least one* construction exists in the blessed module guards against accidental rename-without-update. | ||
|
|
||
| The pattern: whenever a new audit-event type is added — whether for prune, grade (#7), diff (#8), or beyond — extend `tests/test_audit_completeness.py` with a sixth/seventh/etc. AST scan that gates the type's construction to one module. Mirrors `safety.AuditEvent` and `draft.LLMResponseEvent` precedent. | ||
|
|
||
| If a new module genuinely needs to construct a gated event-type (e.g., a deserialiser for resumption), update the scan's exclusion list AND document the audit-write seam. **Don't suppress the test** — the entire point of the AST scan is to catch unaudited audit-event constructions, and a one-line exclusion entry is cheaper than the hour you'll spend debugging a missing JSONL line. | ||
|
|
||
| ## ANSI-safe lazy-format JSON logger + grep gate (DEC-017) | ||
|
|
||
| Same rule as `safety-layer.md` DEC-022 and `llm-drafter.md` DEC-011 — never f-string-interpolate user-controlled strings into a `_LOGGER` call: | ||
|
|
||
| ```python | ||
| _LOGGER.warning("budget exceeded: %s", json.dumps({"unstarted_count": n, "model": model_unique_id})) | ||
| ``` | ||
|
|
||
| **Never** `_LOGGER.warning(f"budget exceeded: {n} tests for {model_unique_id}")`. A model unique_id containing ANSI escapes (`\x1b[31m...`) would inject into log viewers. JSON encoding handles this; f-string interpolation does not. | ||
|
|
||
| The grep gate at `tests/llm/test_logger_grep_gate.py` scans `src/signalforge/{llm,draft,prune}` and rejects any `_LOGGER\.\w+\(f"` hit. Extend the scan to a fourth directory when the grader (#7) ships, rather than copy-pasting a per-layer gate; the single test is the source of truth. | ||
|
|
||
| ## Custom `__repr__` on result-shaped models (DEC-022, post-QG fix) | ||
|
|
||
| Pydantic v2's default `__repr__` emits every field. `PruneResult` carries the full decision tuple, each `PruneDecision` carries `compiled_sql` and `sample_failures` — an accidental `_LOGGER.warning("result: %s", result)` in a future caller would dump megabytes of SQL and (potentially) sample-row contents to the log. | ||
|
|
||
| `PruneResult.__repr__` shows only `model_unique_id`, `kept_count`, `dropped_count`, `elapsed_ms`. `PruneDecision.__repr__` shows only `test.test_type`, `test.column` (or sentinel `"<model-level>"`), `decision`, `reason`. Compiled SQL and sample failures stay accessible via field access; they just don't slip out the casual debug-print path. | ||
|
|
||
| Apply to any future result-shaped model whose fields include user-content payloads. The pattern is "minimal `__repr__`; rich access via fields" — don't override `__str__` (Pydantic uses it for serialisation). | ||
|
|
||
| User-supplied strings in error messages render via `repr()` (`_format_value` helper) — same DEC-022 rule applies to `PruneError` subclasses. A model unique_id containing `\x1b[31m` quoted via `repr()` shows as `'\\x1b[31m...'` in error output; raw interpolation would inject. | ||
|
|
||
| ## Drift detectors are mandatory for read-back models (DEC-010) | ||
|
|
||
| Every `extra="ignore"` production model — `PruneResult`, `PruneDecision`, `PruneEvent` — pairs with a `Strict<Model>(extra="forbid")` detector in `tests/prune/test_drift_detector.py`, validated against a committed JSON/JSONL fixture (`tests/fixtures/prune/prune_event_v1.jsonl` for the audit type). Adding a field to production without updating the strict mirror OR the fixture breaks the test loudly. | ||
|
|
||
| The QG review for #6 caught one near-miss where a production field landed without updating the strict mirror — the drift detector flagged it before it merged. Bake the rule: production `extra="ignore"` change = strict-model change = fixture refresh, in the same commit. | ||
|
|
||
| The `extra=` placement convention from `safety-layer.md` DEC-015 applies verbatim: | ||
|
|
||
| - `PruneConfig`, `_PruneConfigFile` → `extra="forbid"` (config-shaped; typos like `scop:` must fail loud). | ||
| - `PruneResult`, `PruneDecision`, `PruneEvent` → `extra="ignore"` (read-back shapes; forward-compat matters). | ||
|
|
||
| ## API alignment with adjacent stages (post-QG fix) | ||
|
|
||
| `load_prune_config(project_dir, path=None) -> PruneConfig` matches the signature of `load_safety_config` and `load_draft_config`. The default `audit_path` resolves relative to `project_dir` (`<project_dir>/.signalforge/prune.jsonl` per DEC-016). Same for the orchestrator entry: `prune_tests(model, adapter, candidates, manifest, *, config=None, audit_path=None)` — keyword-only optionals, model-and-adapter front-paired, mirroring `draft_schema(model, adapter, policy, manifest, *, config)` from `llm-drafter.md`. | ||
|
|
||
| The CLI (#9) and any future orchestrator wants one calling convention; new stage configs and orchestrators must match the precedent. If you find yourself writing `load_grade_config(*, project_root)` or `grade_artifacts(adapter, model, ...)` (adapter-first), you've broken the alignment. Match the existing seam. | ||
|
|
||
| ## `signalforge.yml` top-level namespace: `prune:` (DEC-020) | ||
|
|
||
| The prune-stage block is `{ prune: { audit_path, mode, total_budget_seconds, test_timeout_seconds, partition_filter, trusted_models, ... } }`. Sibling top-level keys (`safety:`, `llm:`, future `grade:`/`diff:`) are reserved for other stages and silently ignored by the prune loader. | ||
|
|
||
| `PruneConfig` uses `extra="forbid"` (config-shaped; typos fail loud). The wrapping `_PruneConfigFile` uses `extra="ignore"` at the top level so unknown sibling stages don't break the loader. Mirrors `llm-drafter.md` DEC-027 / `safety-layer.md` DEC-025 verbatim. | ||
|
|
||
| When introducing a new pipeline-stage config, claim its own top-level key. Don't pile under `prune:` — each stage's behaviour-knob block stays separate, and the v2-config migration cost grows linearly with the number of stages whose config got merged. | ||
|
wjduenow marked this conversation as resolved.
Resolved
|
||
|
|
||
| ## `trusted_models` is validated at orchestrator entry, not at config load (DEC-008) | ||
|
|
||
| `PruneConfig.trusted_models: tuple[str, ...]` is a tuple of `unique_id` strings. It is **not** validated at `load_prune_config(...)` time — the manifest isn't loaded yet, so we can't check membership. Validation happens at `prune_tests(...)` entry: every entry must appear in `manifest.models`; otherwise raises `PruneTrustedModelNotFoundError(unique_id=...)` **before any warehouse call**. | ||
|
|
||
| A typo like `trusted_models: ["model.proj.cusotmers"]` (vs. `customers`) MUST fail loud — silent no-op (the model isn't trusted because it doesn't match anything) is exactly the failure mode that leaks "failed-on-known-clean-data" tests through as `kept`. Apply the same "validate-against-manifest at orchestrator entry" pattern to any future config field that references manifest entities by `unique_id`. | ||
|
|
||
| ## Reference | ||
|
|
||
| `plans/super/6-prune-engine.md` — DEC-001 … DEC-028. `src/signalforge/prune/` — current implementation. `docs/prune-ops.md` — operational reference. `tests/prune/test_drift_detector.py` — schema-drift gate. `tests/test_audit_completeness.py` — AST-scan suite (5 scans as of #6). `tests/llm/test_logger_grep_gate.py` — lazy-format logger gate (3 dirs as of #6). `tests/fixtures/prune/prune_event_v1.jsonl` — committed audit fixture for the drift detector. | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.