#116: custom business-rule test generation (plan) - #117
Merged
Conversation
16 implementation stories + Quality Gate + Patterns & Memory; 15 decisions. Phase: detailing (awaiting approval). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
wjduenow
marked this pull request as ready for review
May 23, 2026 01:09
…drift mirror Add a fifth CandidateTest variant CandidateTestCustomSQL (DEC-002) for custom singular SQL business-rule tests: - type: Literal["custom_sql"], sql: str (failing-rows SQL), column: str | None (None = model-level), rationale: str | None = None - frozen Pydantic v2, extra="ignore", non-empty sql validator - added to the CandidateTest discriminated union and __all__ - StrictCandidateTestCustomSQL(extra="forbid") mirror in the draft drift detector + new union member - model-level custom_sql row added to candidate_schema_v1.json fixture Minimal downstream arm to keep pyright/tests green: prune/compiler.py _compile_test now matches CandidateTestRelationships explicitly and raises NotImplementedError for custom_sql (compiler support is a separate bead); behavior for the four existing variants is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend `_strip_string_literals` to also neutralise backtick-quoted identifier spans (`` `...` ``) alongside single/double-quoted string literals. Without this, a stray quote inside a backtick identifier (e.g. `` `it's` ``) opened a phantom single-quoted literal that could swallow a real top-level `;`, letting statement-stacking slip past `validate_test_sql`'s cheap-reject checks (DEC-008 of #116). Backtick spans use the same doubled-quote escape handling as the existing quote chars; all prior behaviour (single/double quotes, balanced-paren depth scan, comment-marker detection) is preserved. This stays a cheap-reject checker — no full SQL parser (warehouse-adapters.md / prune-engine.md DEC-024 preserved). Tests: a `;` masked by a stray quote inside backticks is now caught; a top-level `;` after a backtick span containing `;` is caught; benign backtick identifiers (incl. an in-span `;`) are still allowed; comment markers inside quotes/backticks are ignored; balanced-paren logic is unaffected by backtick content. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…012)
Extend the shared artifact-id seam so the fifth CandidateTest variant
CandidateTestCustomSQL gets a stable args-hash and dotted artifact id.
- model_test_args_hash: add a custom_sql branch hashing {type, column,
sql} as canonical JSON, so distinct SQL -> distinct hash and identical
SQL collides deterministically; column (None vs str) keeps a
column-scoped and model-level custom_sql with the same SQL apart.
- artifact_id_for is generic over test.type and already emits
test.column.<col>.custom_sql / test.model.custom_sql with the
optional .<args_hash> suffix; no formatter change needed.
Tests cover distinct/identical SQL hashing, column distinguishing the
hash, both dotted-path shapes, args-hash suffix, collision +
ordinal-duplicate disambiguation via compute_args_hashes, and
cross-stage byte parity with the grade engine. The cross-stage
is-identity parity test stays green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…solution Expose dbt sources from the manifest and add Jinja-ref relation resolvers (DEC-005 of #116), the manifest-layer foundation for resolving dbt refs in later business-rule-test stories. Stage-0: deterministic, no logging, typed errors with remediation. - models.py: new `Source` read-back model (frozen, extra="ignore", schema_ alias + identifier/name relation_name fallback); `Manifest.sources` field; `Manifest.resolve_ref` / `resolve_source` method wrappers; `Model.resolve_this`. - loader.py: filter `resource_type == "source"` into `Manifest.sources` at load; `resolve_ref(manifest, name, *, package, version)` (matches by Model.name, two-arg package disambiguation, fail-loud on unknown/ambiguous); `resolve_source(manifest, source_name, table_name)` -> TableRef. TableRef imported lazily (deferred) to keep manifest stage-0 import-clean. - errors.py: `RefNotFoundError`, `AmbiguousRefError`, `SourceNotFoundError` (all carry remediation). - __init__.py: re-export `Source`, the two resolvers, and the three errors. - cli/_helpers.py: register the three new errors in the exit-code table (tier 2 input-validation) so the 7th AST scan stays green. - tests: `Source` validate + drift-detector (extra="forbid" mirror) in test_models.py; new test_resolve.py covering ref/source/this resolution, package disambiguation, identifier fallback, and fail-loud unknown/ambiguous. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…riter US-011 of #116 (DEC-010, DEC-014). Ships the two primitives the later `generate --write` CLI write-path will call to emit singular business-rule tests as standalone `.sql` files: - `signalforge.diff._test_file_writer.anchor_to_filename(...)` — injection-safe builder producing a relative `tests/<model>__<descriptor>_<hash>.sql`. Every component slugged to `[A-Za-z0-9_]`; `..`/`/`/`\`/control chars/NUL/absolute paths all collapse to `_` so a crafted manifest model name or LLM-emitted column name cannot escape `tests/`. Exhaustive adversarial unit tests. - `signalforge.diff._test_file_writer.write_test_file(...)` — the project's sixth fail-closed writer, mirroring `diff/_sidecar.py` verbatim: size-check before open -> symlink-hardened canonicalisation -> mkdir -p -> os.open(O_WRONLY|O_CREAT|O_TRUNC, 0o600) -> short-write while-loop -> fsync -> close in try/finally. No except around write/fsync (propagation IS the defence). Prepends a `-- signalforge:generated <hash>` header marker. Two new typed errors (`DiffTestFileWriteError`, `DiffTestFileRecordTooLargeError`) subclass `DiffError`, re-export from the package, and register tier 3 in `_EXCEPTION_TO_EXIT_CODE` (scan 7). Module added to `_FAIL_CLOSED_WRITER_MODULES` (scan 8). CLI does NOT wire `--write` (later bead) — primitives + errors only. Full validation green: ruff, format, pyright (0 errors), pytest (1992 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stom_sql Add the fifth CandidateTest variant (custom_sql, DEC-002) to the drafter's recognised-type set and anchor-contract validator (US-004). config.py: add "custom_sql" to VALID_TEST_TYPES so it is a recognised type and can be named in exclude_tests. parser.py _validate_anchor_contract: bespoke custom_sql handling, collect-all (no short-circuit), matching the existing style: - sql must be non-empty after strip (whitespace-only is a violation; the model's truthiness validator already rejects "") - exempt from the parent-column-equality rule (a column-scoped custom_sql may reference other columns in its SQL); only membership of the declared column matters when column is not None - column=None is a valid model-level business-rule assertion - the existing exclude_tests gate (keyed on test.type) covers custom_sql Structural validation only — SQL Jinja/safety is the resolver/compiler's job. Tests: valid column-scoped + model-level, references-other-columns-not-rejected, empty-sql (column + model level), unknown declared column (column + model level), and exclude_tests rejection (column + model level). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ompt Teach the drafter to propose custom singular SQL business-rule tests (DEC-001, DEC-015 of #116). Two input paths: 1. meta-driven (primary): read meta.signalforge.business_rules at column level (Column.meta) and model level (Model.config.meta), accepting a natural-language str OR list[str]. Mirrors the safety layer's meta.get("signalforge") dict-guard pattern (strict isinstance(dict) check; scalars/lists under the key are treated as config noise). Rules render into the DYNAMIC (non-cached) block as a fenced ## BUSINESS RULES section, model-level first then per-column (columns sorted for byte-stability), deduped. 2. inferred fallback: the system prompt permits the LLM to infer custom_sql tests from the model SQL + column profile when no rules are supplied. Prompt changes (draft/prompts.py): - Add a custom_sql JSON-shape illustration. It lives in a separate _CUSTOM_SQL_CATALOGUE_LINE appended unconditionally after the filtered four standard types — exclude_tests / VALID_TEST_TYPES stay four-typed and unchanged (config.py is owned by a parallel bead). - SCOPE section describes custom_sql as full singular-test failing-rows SELECTs that may use {{ this }} / {{ ref('m') }}, covering both the meta-driven and inferred paths. - _PROMPT_VERSION rotates 2563a71c5e31f0db -> 2e465018c1f6db22; the exclude_tests prompt-version recipe is intact. Cache-stability snapshot test updated in lockstep (cached block unchanged — business rules are dynamic-block-only). Orchestrator: business rules are read from the Model inside _render_dynamic_block, which render_prompt already receives, so no signature change is needed (both render_prompt callers keep working). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add signalforge.manifest.resolve_template_refs(sql, model, manifest) — a
bounded regex substituter (NO Jinja engine, DEC-004) that turns dbt-Jinja
references in singular-test SQL into qualified table names for the prune
compiler (US-007) and ingest reader (US-013) to consume:
- {{ this }} -> model.resolve_this().qualified_name
- {{ ref('m') }} /
{{ ref('pkg','m') }} -> manifest.resolve_ref(...).qualified_name
(last positional = name, leading = package)
- {{ source('s','t') }} -> manifest.resolve_source(...).qualified_name
Whitespace + both quote styles tolerated (mirrors ingest parser's unwrap).
Fails loud on {% ... %} blocks, var()/env_var(), macro calls, dynamic
ref()/source(), and any residual {{ }} after substitution. Underlying
RefNotFoundError / AmbiguousRefError / SourceNotFoundError propagate.
Placement keeps layering + AST scans intact:
- Resolver lives in the manifest layer (stage-0: deterministic, no logging,
typed errors carry remediation); re-exported from signalforge.manifest.
- New TemplateResolutionError(ManifestError) + UnsupportedJinjaError go in
the EXISTING manifest/errors.py (no new errors.py — scan-7 count stays 11).
- Both registered in cli._helpers._EXCEPTION_TO_EXIT_CODE at tier 2; the
test_exit_codes catch-all path constructs them via the layer-base shape.
Tests: each ref form, source, this, pkg disambiguation, version-kwarg
tolerance, whitespace, multi-ref, no-jinja passthrough, all rejection paths,
and propagation of the three manifest resolver errors.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the NotImplementedError placeholder arm for CandidateTestCustomSQL
in prune/compiler.py with a real _compile_custom_sql (DEC-003/006/008/009).
- Resolve dbt-Jinja refs ({{ this }} / {{ ref() }} / {{ source() }}) via
signalforge.manifest.resolve_template_refs; thread the Model under prune
through _compile_test as a keyword-only `model` param (engine call site
passes model=model). Built-ins ignore it; existing call sites unchanged.
- SQL-safety pre-flight (validate_test_sql) on the RESOLVED sql.
- Conservative-bias routing: TemplateResolutionError / UnsupportedJinjaError
/ QuerySyntaxError all return the existing _InvalidIdentifier sentinel so
the engine routes to kept-without-evidence (no 6th DropReason; the
compiler never raises for these — DEC-006).
- Single-table (no JOIN after literal-stripping) -> sample-CTE wrap mirroring
the built-ins (substitute own qualified name with the `sample` alias);
multi-table (JOIN survives) -> full-scan, partition filter applied to the
model's own table only. Dialect-driven via Dialect.quote_char; no
BigQuery-isms.
- Returns the resolved failing-rows SELECT (NOT a pre-wrapped count) so the
adapter's run_test_sql owns the COUNT(*) envelope, avoiding double-count.
Snapshot fixtures pin byte-exact output: custom_sql.sql (single-table full),
custom_sql_sample.sql (single-table sample), custom_sql_fullscan.sql
(multi-table). Tests cover ref/source resolution, unsupported-Jinja/var/
safety-reject -> sentinel, no-model -> sentinel, full-scan partition wrap,
determinism.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…013)
Extend the stage-0 ingest layer to read an operator's existing singular dbt
tests (.sql files under tests/) into CandidateTestCustomSQL records so
prune-existing can later prune them (DEC-013).
- parser.classify_singular_test(sql, *, file_name, model, manifest): reuses
signalforge.manifest.resolve_template_refs to resolve ref()/source()/this
(no regex duplicated). Associated -> CandidateTestCustomSQL(column=None);
references a different / unknown / ambiguous model -> None (not included,
not skip-recorded); unsupported Jinja ({% %}, {{ var() }}, macros, residual
{{ }}) -> SkippedTest(reason="malformed-supported-test"). The closed 3-value
SkipReason is unchanged.
- reader.read_test_files(tests_dir, model, manifest, *, project_dir=None,
existing=None): enumerates *.sql (sorted), size-caps each from stat() before
read (5 MB, mirrors read_schema DEC-005), dedupes associated tests by
(model, "custom_sql", sql_hash) via blake2b-8 of the SQL body, and seeds the
dedupe set from an optional schema.yml-sourced `existing` candidate so the
same test from both sources collapses. Returns a model-level-only
CandidateSchema (no columns; no anchor check — singular tests are
model-level with column=None).
- Re-export read_test_files from signalforge.ingest.
- Stage-0 discipline preserved: no logging, no audit writer, deterministic,
typed errors carry remediation, extra="ignore" read-back models.
Fixtures: tests/fixtures/ingest/custom_sql_files/ (orders ref, customers ref,
unsupported-macro). Tests: tests/ingest/test_test_files.py (classifier matrix +
reader: associate/exclude/skip, dedupe within dir and against existing,
oversize-before-read, missing dir, non-sql ignored, sorted determinism).
Validation green: ruff check + format, pyright (0 errors), full pytest
(2084 passed, 96.58% cov). Logger grep-gate (ingest stays silent) + AST
audit-completeness scans pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ql files US-010 of #116. Teach the diff layer to surface kept singular custom_sql business-rule tests as standalone .sql file proposals (DEC-011) — they are NOT schema.yml blocks. - models.py: add ProposedTestFile frozen model (path + sql); add DiffReport.proposed_test_files tuple; bump audit_schema_version 2 -> 3 in lockstep (sidecar shape evolved). Minimal __repr__ keeps the SQL body out of log sinks. - _emitter.py: _test_args_hash now delegates to the shared _common.artifact_id seam (handles the 5th custom_sql variant); _render_test returns a _SKIP sentinel for custom_sql so the YAML emitter cleanly skips it (never crashes); new emit_proposed_test_files builds one ProposedTestFile per kept custom_sql via the reused anchor_to_filename + _with_marker + shared args-hash. - _renderers.py: AnsiRenderer + MarkdownRenderer render a proposed test-files section (new-file header / fenced sql block) with the same unconditional ANSI-strip + dynamic markdown fence on the SQL content (DEC-007/008). JsonRenderer/sidecar carry it automatically. - engine.py: wire emit_proposed_test_files into DiffReport + INFO log. - Drift detectors (test_drift_detector.py + inline test_models.py) + fixtures (diff_report_v1.json) updated; ProposedTestFile exported on the public surface; snapshot cases + fixtures regenerated; e2e diff.json fixture + docs (diff-ops, audits) bumped to v3. Tier classification + why-cascade unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ql (US-008)
Verify and pin (via tests) that the prune ENGINE routes custom_sql
business-rule tests through the existing decision matrix unchanged. The
engine's per-test routing is deliberately test-type-agnostic — it
dispatches on the compiler's return shape (str / _InvalidIdentifier /
_RequiresFutureData), the warehouse failure_count, and any raised
WarehouseError — so custom_sql flows through the same paths as the four
built-ins with NO engine source change.
Six new fake-adapter tests in tests/prune/test_engine.py:
- always-passes (failure_count=0) -> dropped
- real failure on untrusted model (failure_count>0) -> kept
- unsupported-Jinja -> _InvalidIdentifier sentinel -> kept-without-evidence
(no warehouse call)
- WarehouseError during query -> kept-without-evidence (generic per-test why)
- multi-table full-scan over maximum_bytes_billed (BytesBilledExceededError)
-> kept-without-evidence
- fail-closed audit invariant: one PruneEvent per custom_sql candidate,
PruneEvent shape unchanged (round-trips through read-back model)
DEC-007 why decision (byte-cap case): use the GENERIC per-test handler why
("Test could not be evaluated: BytesBilledExceededError: ...") rather than
a bespoke locked string. The typed class name is already in the why for
reviewer correlation; a distinct why would require special-casing one
WarehouseError subclass in the otherwise error-type-agnostic handler. The
locked 5-value DropReason literal stays unchanged; no new DropReason.
No audit-shape change -> prune_event_v1.jsonl fixture + drift detector
untouched.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (US-016) Complete the operator narrative for the custom singular SQL business-rule test feature (custom_sql, issue #116) across the user-facing docs. Extends the lockstep edits already made by implementation beads (diff-ops/audits audit_schema_version 2→3; cli-ops --force/--tests-dir); does not duplicate. - docs/draft-ops.md: new "Custom business-rule tests (custom_sql)" section — authoring meta.signalforge.business_rules (NL str/list, column + model level), inferred-fallback, {{ this }}/{{ ref }}/{{ source }} support with control-flow Jinja unsupported, a worked rule→JSON example; CandidateTest union + CandidateTestCustomSQL in the API table; exclude_tests exemption. - docs/prune-ops.md: new "custom_sql evaluation" section — Jinja-resolution routing (clean / requires-future-data / kept-without-evidence), single- table sampled vs multi-table full-scan, the maximum_bytes_billed cap as the only multi-table guardrail + tuning note; folded into the expected- drop-rate framing; taxonomy table + v0.2-deferrals updated to five types. - docs/ingest-ops.md: new "Singular tests/*.sql tests" section for read_test_files — ref/source/this resolution, association-to-model, unrelated-files-ignored, unsupported-Jinja skip, dedupe + size cap. - README.md: custom business-rule tests in the feature list; a worked rule→generated .sql→kept/dropped example; prune-existing singular .sql ingestion note. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…itution + multi-table test coverage
P0 (CRITICAL): _compile_custom_sql single-table scope="full" path now
rewrites the model's own qualified name to the substituted table_ref when
the engine handed it a DIFFERENT physical table (the materialised temp
table under sample_strategy="materialised" + scope="sample"). Previously
returned the resolved SQL unchanged, so single-table custom_sql tests
silently full-scanned the production source instead of the materialised
sample — defeating the cost model and risking maximum_bytes_billed over-cap.
Mirrors the built-in compilers, which always FROM table_ref. Multi-table
(DEC-006) and oneshot/full-strategy behaviour unchanged.
P2: single-table self-reference (correlated subquery / self-UNION) now
substitutes ALL occurrences in the sample branch and the full-scope
partition branch (dropped count=1). Multi-table partition replace stays
count=1 (intentional).
Tests (tests/prune/test_engine.py):
- Tightened the over-byte-cap multi-table test to require the full-scan
JOIN shape and reject a WITH sample CTE; switched to two distinct refs
({{ this }} + {{ ref('other_model') }}) so it genuinely exercises the
multi-table classifier.
- Added engine-level multi-table full-scan tests (failures=0 → dropped/
always-passes; failures>0 → kept) asserting no sample CTE is dispatched.
- Added a P0-fix test: single-table custom_sql under materialised+sample
references _SESSION._sf_sample_<run_id> and never the source table.
No DropReason added; no .claude/ edits; no fixture regen needed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e test coverage (bd_1-scaffolding-48o.17)
… + CLAUDE.md surface New .claude/rules/business-rule-tests.md distilling the #116 conventions (custom_sql variant, bounded Jinja resolution, materialised-substitution gotcha, fail-closed .sql writer, conservative-bias routing); CLAUDE.md #116 entry + v0.3 public-API block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements issue #116’s “custom business-rule test generation” by introducing a 5th candidate test variant (custom_sql) representing dbt singular SQL tests, and threading it end-to-end through draft → ingest/prune-existing → prune → diff → CLI write-path, alongside a new bounded dbt-Jinja reference resolver ({{ this }} / ref() / source()) with fail-loud behavior for unsupported Jinja.
Changes:
- Add
CandidateTestCustomSQLto the draft model union, extend prompt/system-prompt content and business-rule meta ingestion (meta.signalforge.business_rules) to drive drafting of singular failing-rows SELECTs. - Add manifest-layer bounded Jinja ref resolution and source registry (
Source,resolve_ref,resolve_source,resolve_template_refs) for singular-test SQL. - Extend ingest to read operator singular
tests/*.sql(read_test_files) and extend diff to surface keptcustom_sqltests as standalone proposed.sqlfiles (proposed_test_files) with a write-path and schema bump.
Reviewed changes
Copilot reviewed 78 out of 78 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/warehouse/test_models.py | Adds test coverage for backtick-aware SQL safety stripping and semicolon/comment detection. |
| tests/test_audit_completeness.py | Updates audit module inventory to include the new fail-closed test-file writer module. |
| tests/manifest/test_template.py | New tests for bounded template ref substitution (this/ref/source) and fail-loud rejection paths. |
| tests/manifest/test_resolve.py | New tests for ref/source/this resolution APIs and loader surfacing of sources. |
| tests/manifest/test_models.py | Adds Source model validation/drift tests (includes a buggy assertion flagged in review). |
| tests/llm/test_prompt_cache_stability.py | Rotates prompt version and documents the #116 prompt change. |
| tests/ingest/test_test_files.py | New end-to-end tests for reading/deduping/classifying singular tests/*.sql. |
| tests/fixtures/prune/compiled_sql/custom_sql.sql | New compiled SQL fixture for custom_sql compilation. |
| tests/fixtures/prune/compiled_sql/custom_sql_sample.sql | New compiled SQL fixture for sampled custom_sql compilation. |
| tests/fixtures/prune/compiled_sql/custom_sql_fullscan.sql | New compiled SQL fixture for join/fullscan custom_sql compilation. |
| tests/fixtures/ingest/custom_sql_files/assert_orders_with_macro.sql | New fixture: unsupported-Jinja singular test to be skip-recorded. |
| tests/fixtures/ingest/custom_sql_files/assert_orders_amount_positive.sql | New fixture: associated singular test for orders. |
| tests/fixtures/ingest/custom_sql_files/assert_customers_have_email.sql | New fixture: unrelated singular test that must be ignored. |
| tests/fixtures/e2e_helpers/happy/.signalforge/diff.json | Updates diff sidecar fixture schema version (audit_schema_version bump). |
| tests/fixtures/draft/candidate_schema_v1.json | Adds a custom_sql row to candidate schema fixture. |
| tests/fixtures/diff/proposed_test_files.md | New snapshot fixture demonstrating proposed singular test files in Markdown. |
| tests/fixtures/diff/proposed_test_files.ansi | New snapshot fixture demonstrating proposed singular test files in ANSI. |
| tests/fixtures/diff/full_with_grade.json | Updates diff fixture for schema bump and proposed_test_files field presence. |
| tests/fixtures/diff/diff_report_v1.json | Updates diff fixture to include proposed_test_files array when present. |
| tests/draft/test_prompts.py | Adds tests for custom_sql prompt catalogue/SCOPE + business_rules meta rendering. |
| tests/draft/test_parser.py | Adds anchor-contract tests for custom_sql (empty SQL, column membership, exclude_tests handling). |
| tests/draft/test_exclude_tests.py | Updates VALID_TEST_TYPES pin to include custom_sql. |
| tests/draft/test_drift_detector.py | Extends strict drift detector union with custom_sql. |
| tests/diff/test_snapshot_fixtures.py | Updates snapshot fixture count to include proposed_test_files cases. |
| tests/diff/test_renderers.py | Adds renderer tests for proposed test files, ANSI stripping, and dynamic fences. |
| tests/diff/test_public_api.py | Exposes ProposedTestFile and new diff writer errors via public API tests. |
| tests/diff/test_models.py | Updates DiffReport audit schema version and adds ProposedTestFile strict mirror/tests. |
| tests/diff/test_engine.py | Adds end-to-end diff test ensuring kept custom_sql becomes a proposed_test_file. |
| tests/diff/test_emitter.py | Adds tests for YAML skipping custom_sql and standalone .sql emission + path safety. |
| tests/diff/test_drift_detector.py | Adds strict mirror parity tests for ProposedTestFile and DiffReport schema bump. |
| tests/diff/test_artifact_id.py | Adds args-hash and artifact-id shape tests for custom_sql. |
| tests/diff/_snapshot_inputs.py | Adds snapshot recipe inputs for proposed test files and updates recipe table. |
| tests/cli/test_prune_existing.py | Extends prune-existing to ingest singular tests via --tests-dir and merge behavior tests. |
| tests/cli/test_generate.py | Adds tests for generate --write writing proposed .sql files and --force overwrite policy. |
| tests/cli/test_exit_codes.py | Adds new diff writer errors to exit-code mapping tests. |
| tests/cli/test_e2e_helpers.py | Adds unit tests for manifest business_rules injection helper. |
| tests/cli/test_e2e_business_rules.py | New gated e2e test validating business-rule drafting+pruning behavior on real services. |
| tests/cli/test_5_surface_parity_force.py | New 5-surface parity test ensuring --force is documented/implemented consistently. |
| tests/cli/_e2e_helpers.py | Adds helper to inject business_rules into a copied manifest fixture. |
| src/signalforge/warehouse/_sql_safety.py | Extends string-literal stripping to include backtick spans for SQL safety checks. |
| src/signalforge/prune/engine.py | Passes model= into prune compilation path (needed for custom_sql compilation). |
| src/signalforge/manifest/template.py | New bounded dbt-Jinja resolver for singular-test SQL (no Jinja engine). |
| src/signalforge/manifest/models.py | Adds Source model, Manifest.sources registry, and resolve_this/resolve methods. |
| src/signalforge/manifest/loader.py | Loads sources registry and adds resolve_ref/resolve_source helper functions. |
| src/signalforge/manifest/errors.py | Adds typed errors for ref/source/template resolution failures. |
| src/signalforge/manifest/init.py | Re-exports new manifest APIs and error types. |
| src/signalforge/ingest/reader.py | Adds read_test_files for singular .sql ingestion + dedupe and size-capped reads. |
| src/signalforge/ingest/parser.py | Adds classify_singular_test using bounded resolver and skip/unrelated/associated routing. |
| src/signalforge/ingest/init.py | Exports read_test_files and updates ingest module docs. |
| src/signalforge/draft/prompts.py | Adds custom_sql catalogue+SCOPE text and renders BUSINESS RULES section from meta. |
| src/signalforge/draft/parser.py | Extends anchor-contract validation to handle custom_sql semantics. |
| src/signalforge/draft/models.py | Adds CandidateTestCustomSQL variant and extends CandidateTest union. |
| src/signalforge/draft/config.py | Adds custom_sql to VALID_TEST_TYPES and updates config docs. |
| src/signalforge/diff/models.py | Adds ProposedTestFile and bumps DiffReport.audit_schema_version to 3 with proposed_test_files. |
| src/signalforge/diff/errors.py | Adds DiffTestFile* writer errors for fail-closed .sql writing. |
| src/signalforge/diff/engine.py | Emits proposed_test_files alongside proposed_yaml into DiffReport. |
| src/signalforge/diff/_renderers.py | Renders proposed standalone test files in ANSI/Markdown outputs with safety stripping. |
| src/signalforge/diff/_emitter.py | Skips custom_sql from YAML and emits ProposedTestFile tuples for kept custom_sql tests. |
| src/signalforge/diff/init.py | Exposes ProposedTestFile and new writer errors as part of public diff API. |
| src/signalforge/cli/prune_existing.py | Adds --tests-dir ingestion of singular tests and merges into prune-existing candidate. |
| src/signalforge/cli/_helpers.py | Maps new manifest and diff writer errors into exit-code taxonomy. |
| src/signalforge/_common/artifact_id.py | Extends args-hash domain to include custom_sql (SQL + column in payload). |
| README.md | Documents custom business-rule tests and how they’re pruned/written. |
| docs/prune-ops.md | Documents pruning semantics for custom_sql including routing and sampling/fullscan rules. |
| docs/ingest-ops.md | Documents singular tests/*.sql ingestion (contains a doc mismatch flagged in review). |
| docs/draft-ops.md | Documents custom_sql drafting and business_rules meta path. |
| docs/diff-ops.md | Documents diff sidecar schema bump and proposed_test_files shape. |
| docs/cli-ops.md | Documents --force overwrite policy and --tests-dir for prune-existing. |
| docs/audits.md | Updates audit schema bump notes for diff sidecar version 3. |
| CLAUDE.md | Updates repo-level shipped-issues notes and public API description for #116. |
| .claude/rules/business-rule-tests.md | New rules doc capturing invariants/DECs for custom_sql end-to-end behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add targeted tests exercising the 19 feature-introduced lines Codecov flagged uncovered on PR #117, all real behaviour assertions (no src changes, no pragmas): - draft/models.py:157 — CandidateTestCustomSQL.sql empty-string validator. - manifest/loader.py:443 — resolve_source missing-schema/identifier branch. - prune/compiler.py:682 — scope='sample' missing sample_size/bucket guard. - prune/compiler.py:719-722 — single-table scope='full' + partition-filter derived-table wrap of the model's own table. - diff/_emitter.py:313 — dedupe continue for two kept custom_sql decisions resolving to the same path. - diff/_test_file_writer.py:275 — os.write-returns-0 short-write guard (mirrors tests/diff/test_sidecar.py). - cli/generate.py:457 — _split_marked_sql no-marker fallback. - cli/generate.py:482-483 — _existing_file_is_signalforge_generated OSError branch (unreadable path → False). - ingest/reader.py:375-376 — read_test_files PathContainmentError wrap. - ingest/reader.py:396 — non-file *.sql glob match skip. - ingest/reader.py:432-433,441-442 — stat/read OSError → IngestSchemaParseError. Full validation green; coverage 96.90%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/diff-ops.md (1)
80-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate stale
audit_schema_versionreferences to 3 in the API/consumer sections.This file now documents
DiffReport.audit_schema_version: 3later on, but these earlier lines still describe version 2. Please align them to avoid incorrect consumer gating guidance.Also applies to: 194-195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/diff-ops.md` at line 80, Update the stale documentation references for the DiffReport Pydantic model so audit_schema_version is consistently documented as Literal[3]; locate occurrences describing DiffReport.audit_schema_version (and any adjacent gating guidance mentioning ">= 2" for consumers) and change them to reflect audit_schema_version: Literal[3] and the correct consumer gate (e.g., ">= 3"); ensure any related counts or notes added in issue `#50` that reference audit_schema_version are updated as well (search for "DiffReport", "audit_schema_version", and the string ">= 2" to find all spots, including the other instances noted around lines 194-195).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/draft-ops.md`:
- Around line 295-296: The docs currently reference the internal symbol
signalforge.manifest.template.resolve_template_refs; update the text to
reference the public API symbol signalforge.manifest.resolve_template_refs
instead so users are directed to the supported resolver surface (replace any
occurrences of the internal module path with the public symbol name).
In `@docs/ingest-ops.md`:
- Around line 230-235: The docs state unsupported Jinja in singular `.sql` tests
is recorded with SkipReason `custom-or-generic-test`, but the implementation and
contract use `malformed-supported-test`; update the text to reference the
correct closed literal `malformed-supported-test` and the canonical construct
`SkippedTest(reason="malformed-supported-test")`, and ensure the sentence that
mentions `IngestResult.skipped` and the detail message reflects this exact
SkipReason value rather than `custom-or-generic-test`.
In `@src/signalforge/cli/prune_existing.py`:
- Around line 179-185: The help text for the --tests-dir option is misleading
about missing-directory behavior; update the help string in
src/signalforge/cli/prune_existing.py so it explicitly states that if the
default <project_dir>/tests is absent the CLI will silently ignore test files,
but if a user supplies --tests-dir and that directory does not exist the command
will fail with an error; reference the --tests-dir option name and adjust the
wording to make this distinction clear.
In `@src/signalforge/diff/__init__.py`:
- Around line 21-24: Update the module docstring to match the current public
API: change the Tier description to the current number/values (replace the
outdated "3-value `Tier`" text) and update the DiffError summary to list the
nine exported error classes (instead of the old "seven-class `DiffError`
hierarchy"), and add mention of the `kept-uncertain` export and the changed
behavior of `ProposedTestFile` described in the diff so the bullets accurately
reflect the symbols exported from this module.
In `@src/signalforge/draft/prompts.py`:
- Around line 169-181: The prompt contains a conflicting scope rule: the
sentence "Propose only {allowed_scope} tests" overrides later instructions to
generate `custom_sql` tests and can suppress those even when BUSINESS RULES are
present; change the wording so the allowed-scope constraint is conditional
(e.g., "Propose only {allowed_scope} tests unless BUSINESS RULES are present")
or add an explicit exception that `custom_sql` tests must be produced when a
BUSINESS RULES section exists; ensure the text referencing `custom_sql` tests,
`column`, and the BUSINESS RULES section remains and is applied as an override
to the {allowed_scope} restriction so `custom_sql` generation is not suppressed.
In `@src/signalforge/ingest/reader.py`:
- Around line 439-445: The except block in the reader code currently only
catches OSError when calling sql_path.read_text(encoding="utf-8"), allowing
UnicodeDecodeError to escape; update the exception handling in the
function/method that contains this call so that it also catches
UnicodeDecodeError (or a broader IOError/Exception that includes decoding
failures) and re-raises IngestSchemaParseError with the original exception as
cause (preserving the existing message format and cause=exc, and keeping the
"from exc" chaining) so decoding errors are converted into
IngestSchemaParseError as intended.
In `@src/signalforge/manifest/template.py`:
- Around line 147-155: The current extraction using _QUOTED_ARG_RE on arglist
treats quoted keyword values as positional (e.g., "'orders', version='1'" yields
["orders","1"]); change the parsing so only quoted positional args are captured:
skip quoted fragments that are part of a kwarg assignment (i.e., preceded by an
identifier and '='). Update the extraction around args =
_QUOTED_ARG_RE.findall(arglist) to filter or use a regex that matches quoted
strings not immediately preceded by a word and '=' (so name = first positional
quoted arg, package = second positional quoted arg if present) before calling
manifest.resolve_ref(name, package=package).qualified_name.
In `@src/signalforge/prune/compiler.py`:
- Around line 680-700: The current rewrite only replaces occurrences of
own_qualified in resolved_sql, so when own_qualified is absent the custom SQL
silently runs against the wrong source; update _compile_test to fail-closed by
detecting when scope == "sample" (and similarly materialized path) and
resolved_sql does not contain own_qualified: either (a) substitute any
engine-aliased self-reference by replacing the {{ this }}/own table marker with
the effective table_ref before calling _render_sample_cte, or (b raise a
ValueError if binding cannot be performed; ensure this check references
own_qualified, resolved_sql, table_ref, sampled_sql and calls _render_sample_cte
with own_table_quoted only after successful substitution so the sample CTE is
always prepended to the SQL (apply same logic to the materialized path code that
returns f"{cte} {sampled_sql}").
In `@tests/manifest/test_models.py`:
- Around line 201-208: The test currently picks an arbitrary source via
next(iter(raw["sources"].values())) which makes assertions flaky; instead
iterate raw["sources"].values() and choose the specific source deterministically
(e.g., find the dict where source_name == "raw" and name == "users" or where
unique_id matches the expected pattern) before calling Source.model_validate;
update the variable assignment that uses _load_fixture and src so the test
asserts against that found source rather than an arbitrary first element.
---
Outside diff comments:
In `@docs/diff-ops.md`:
- Line 80: Update the stale documentation references for the DiffReport Pydantic
model so audit_schema_version is consistently documented as Literal[3]; locate
occurrences describing DiffReport.audit_schema_version (and any adjacent gating
guidance mentioning ">= 2" for consumers) and change them to reflect
audit_schema_version: Literal[3] and the correct consumer gate (e.g., ">= 3");
ensure any related counts or notes added in issue `#50` that reference
audit_schema_version are updated as well (search for "DiffReport",
"audit_schema_version", and the string ">= 2" to find all spots, including the
other instances noted around lines 194-195).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ad77de0-3b39-4f7c-9df8-45e14f462388
📒 Files selected for processing (78)
.claude/rules/business-rule-tests.mdCLAUDE.mdREADME.mddocs/audits.mddocs/cli-ops.mddocs/diff-ops.mddocs/draft-ops.mddocs/ingest-ops.mddocs/prune-ops.mdplans/super/116-business-rule-tests.mdsrc/signalforge/_common/artifact_id.pysrc/signalforge/cli/_helpers.pysrc/signalforge/cli/generate.pysrc/signalforge/cli/prune_existing.pysrc/signalforge/diff/__init__.pysrc/signalforge/diff/_emitter.pysrc/signalforge/diff/_renderers.pysrc/signalforge/diff/_test_file_writer.pysrc/signalforge/diff/engine.pysrc/signalforge/diff/errors.pysrc/signalforge/diff/models.pysrc/signalforge/draft/config.pysrc/signalforge/draft/models.pysrc/signalforge/draft/parser.pysrc/signalforge/draft/prompts.pysrc/signalforge/ingest/__init__.pysrc/signalforge/ingest/parser.pysrc/signalforge/ingest/reader.pysrc/signalforge/manifest/__init__.pysrc/signalforge/manifest/errors.pysrc/signalforge/manifest/loader.pysrc/signalforge/manifest/models.pysrc/signalforge/manifest/template.pysrc/signalforge/prune/compiler.pysrc/signalforge/prune/engine.pysrc/signalforge/warehouse/_sql_safety.pytests/cli/_e2e_helpers.pytests/cli/test_5_surface_parity_force.pytests/cli/test_e2e_business_rules.pytests/cli/test_e2e_helpers.pytests/cli/test_exit_codes.pytests/cli/test_generate.pytests/cli/test_prune_existing.pytests/diff/_snapshot_inputs.pytests/diff/test_artifact_id.pytests/diff/test_drift_detector.pytests/diff/test_emitter.pytests/diff/test_engine.pytests/diff/test_models.pytests/diff/test_public_api.pytests/diff/test_renderers.pytests/diff/test_snapshot_fixtures.pytests/diff/test_test_file_writer.pytests/draft/test_drift_detector.pytests/draft/test_exclude_tests.pytests/draft/test_parser.pytests/draft/test_prompts.pytests/fixtures/diff/diff_report_v1.jsontests/fixtures/diff/full_with_grade.jsontests/fixtures/diff/proposed_test_files.ansitests/fixtures/diff/proposed_test_files.mdtests/fixtures/draft/candidate_schema_v1.jsontests/fixtures/e2e_helpers/happy/.signalforge/diff.jsontests/fixtures/ingest/custom_sql_files/assert_customers_have_email.sqltests/fixtures/ingest/custom_sql_files/assert_orders_amount_positive.sqltests/fixtures/ingest/custom_sql_files/assert_orders_with_macro.sqltests/fixtures/prune/compiled_sql/custom_sql.sqltests/fixtures/prune/compiled_sql/custom_sql_fullscan.sqltests/fixtures/prune/compiled_sql/custom_sql_sample.sqltests/ingest/test_test_files.pytests/llm/test_prompt_cache_stability.pytests/manifest/test_models.pytests/manifest/test_resolve.pytests/manifest/test_template.pytests/prune/test_compiler.pytests/prune/test_engine.pytests/test_audit_completeness.pytests/warehouse/test_models.py
Fix nine review findings across source, tests, and docs:
1. manifest/template.py: _resolve_ref_args split on commas + drop kwargs so
ref('m', version='1') resolves model 'm', not '1'.
2. ingest/reader.py: _read_sql_file catches UnicodeDecodeError → typed
IngestSchemaParseError instead of an untyped escape.
3. draft/prompts.py: custom_sql now participates in exclude_tests filtering;
catalogue line + SCOPE instruction emitted only when allowed; SCOPE phrase
reads "..., plus custom_sql". _PROMPT_VERSION rotates to c9e7ee1f6f465933.
4. prune/compiler.py: _compile_custom_sql fails closed (kept-without-evidence)
when {{ this }} can't bind to the effective sample/materialised table.
5. tests/manifest/test_models.py: deterministic source select + parenthesised
relation_name assertion (was always-truthy).
6. docs/ingest-ops.md: unsupported-Jinja singular .sql skip reason is
malformed-supported-test, not custom-or-generic-test.
7. docs/draft-ops.md: public signalforge.manifest.resolve_template_refs.
8. cli/prune_existing.py: --tests-dir help clarifies only the DEFAULT dir is
silently skipped; explicit missing --tests-dir fails loud.
9. diff/__init__.py: docstring now lists 4-value Tier (kept-uncertain) and the
nine-class DiffError hierarchy.
Regression tests added for items 1-4; tests adjusted for item 3/5.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…CI 3.12) test_read_test_files_stat_oserror_raises_parse_error keyed on the follow_symlinks kwarg to distinguish is_file()'s stat from the size-check stat — that distinction is 3.13-only, so the patched OSError escaped during is_file() on 3.12. Decouple by forcing is_file() True for the target and letting only the size-check stat() raise. Verified on 3.11/3.12/3.13. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review SummaryAll CodeRabbit + Copilot findings addressed (12 threads). No false positives — every item was a genuine issue and is fixed, with regression tests where applicable. Fixes in Fixed (12 items)
False Positives (0 items)None. Full validation green (ruff, pyright, pytest 2173 passed); verified the version-specific fix on Python 3.12 locally. |
Item 1 (Major) — ingest/parser.py classify_singular_test: a singular test
referencing THIS model AND an unresolvable/ambiguous OTHER ref()/source()
was silently dropped when the whole-body resolve raised. Added bounded
per-expression heuristic _raw_sql_references_target (reuses template.py
regexes + _resolve_ref_args; {{ this }} or a ref() resolving to the target
associates). On the resolver-error branch, when this model is referenced we
now carry the RAW unresolved SQL into a CandidateTestCustomSQL so the prune
compiler routes it (requires-future-data / kept-without-evidence, US-019);
only a genuinely-unrelated body returns None. source() is intentionally not
checked — a singular test's target is always a model, never a source.
Item 2 (Minor) — replaced the raw `target not in resolved` substring check
with a word-boundary match (_references_qualified_name) so the target name
inside a comment/string or as a fragment of a longer dotted identifier no
longer false-associates.
Item 3 (Codecov) — added tests/manifest/test_template.py cases for an empty
arg-list fragment (trailing / interior comma) in ref(); template.py now 100%
(the empty-token continue at line 159 is covered).
Item 4 (docs) — docs/draft-ops.md: custom_sql CAN now be excluded via
DraftConfig.exclude_tests (US-021; it is in VALID_TEST_TYPES, the prompt
omits its catalogue/SCOPE blocks when excluded and the parser rejects it
if the LLM defies that). Corrected both the §custom_sql note and the
exclude_tests field description + YAML comment.
Regression tests added in tests/ingest/test_test_files.py (Items 1+2) and
tests/manifest/test_template.py (Item 3). Full suite green (2183 passed),
coverage 96.93%, docs build clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review Summary (round 2)Round-2 CodeRabbit + Copilot findings on the US-021 push, all addressed (
Full validation green (ruff, pyright, pytest 2183, coverage 96.93%); docs build clean; Codecov patch now ~100%. |
This was referenced May 26, 2026
This was referenced Jul 6, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Super plan for #116 — custom business-rule test generation (research Opportunity 2, v2 milestone).
Extends the drafter beyond the four built-in dbt test types to a fifth: custom singular SQL tests encoding business rules — sourced from
meta.signalforge.business_rules(NL) and LLM inference — then run through the existing prune → grade → diff pipeline so always-pass / uninformative rules are dropped.Phase: detailing (awaiting approval)
Stories: 16 implementation + Quality Gate + Patterns & Memory (18 total)
Decisions: 15 captured (DEC-001 … DEC-015)
Locked scope
{{ this }}/{{ ref }}/{{ source }}, cross-model joins).sqlfiles written ongenerate --write(fail-closed writer + injection-safe filenames +--forceoverwrite);prune-existingextended to ingest existingtests/*.sql, read-onlyArchitecture review
No blockers; five concerns resolved into DECs (SQL-injection backtick gap, runaway cost, multi-table sampling, on-disk artifact path-safety, prompt-injection).
Plan document
plans/super/116-business-rule-tests.md.Next steps
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
meta.signalforge.business_rulesin dbt YAMLgeneratecommand now writes proposed singular.sqltest files with--writeflag; use--forceto overwrite existing generated filesprune-existingcommand now ingests and prunes hand-authoredtests/*.sqltests alongside schema tests via new--tests-dirflagDocumentation