#154: dbt-expectations prune+grade adapter via manifest compiled_code - #266
Merged
Conversation
…ifest compiled_code Single cohesive plan (not an epic): reuse the custom_sql pipeline to prune+grade dbt-expectations (and other generic/namespaced) tests read from manifest.json compiled_code. 18 decisions, 8 implementation stories + Quality Gate + Patterns. Key shape: built on sqlglot AST (determinism / aggregate-shape / comment-tolerant validation); scope=sample deferred to full-scope in pass 1; opt-in --from-manifest + --grade on prune-existing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR adds manifest-ingested ChangesManifest-ingested custom_sql feature
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
…st.tests sibling filter Add a frozen GenericTest Pydantic model (+ nested TestMetadata) carrying a test node's compiled_code / test_metadata / column_name / depends_on / attached_node / file_key_name, and surface resource_type=="test" nodes into a new Manifest.tests dict via a sibling filter in _load (parallel to filtered_sources). nodes stays model-only. Add associate_test_model(): a feature-detect precedence ladder (attached_node for v10+ -> depends_on.nodes disambiguated by file_key_name / kwargs.model ref for v9) so association never version-branches (the loader discards the detected version). Silent-degrade on absent/null compiled_code (mirrors Column.data_type=None, #159); the "not a silent skip" surfacing is a downstream ingest concern. Tests: StrictGenericTest(extra="forbid") drift detector over a hand-authored generic_test_nodes.json fixture (both v10-attached and v9-no-attached shapes) + poisoned-key rejection; association-ladder coverage for both version shapes, single/multi/no-dep and ambiguous cases; load() surfaces test nodes into Manifest.tests while keeping nodes model-only; empty-default construction; and confirmation that the frozen model_copy catalog overlay passes Manifest.tests through untouched. Traces to DEC-008, DEC-009 (plans/super/154-dbt-expectations-prune.md).
… test SQL Add src/signalforge/ingest/_compiled_sql.py — three pure, string-in/verdict-out functions over a dbt-compiled ``compiled_code`` body, all sqlglot-AST-based (#154 US-002, DEC-006/DEC-004/DEC-012/DEC-013): - is_deterministic_sql: flags TABLESAMPLE + non-deterministic funcs (RAND/RANDOM/ RND, CURRENT_TIMESTAMP/CURRENT_DATE/NOW/GETDATE, UUID/GENERATE_UUID) via AST function-node inspection, so a column named random_id / current_timestamp_col and those tokens inside a string literal or comment never false-positive. - is_row_returning: rejects scalar/aggregate bodies (all top-level projections collapsing aggregates + no GROUP BY) while treating windowed aggregates and subquery aggregates as row-returning — the row_count_between silent-wrong-kept guard (AR row 10). - validate_ingested_sql: comment-tolerant sibling of validate_test_sql; strips --/ /* */ comments (string-literal-aware) before the ;/unbalanced-paren injection scan, but still fails loud on real injection (AR row 11). NOT a reuse of the #116 validator. Placement documented in the module docstring: lives under ingest (the primary consumer via the manifest-test bridge, US-003) as a pure str->verdict seam carrying no ingest domain types, so the prune compiler (US-004) can import it without cross-stage coupling. Extends the sqlglot confinement (#159 DEC-008) from draft/parser to this one module. tests/ingest/test_compiled_sql.py: 42 tests incl. a planted-violation negative per helper (column-name / string-literal / windowed-aggregate / subquery- aggregate / comment-hidden-injection cases). Full validation green.
…ipt (#154 US-006) Add tests/fixtures/dbt_project_expectations/ — the first committed manifest built via real `dbt deps && dbt compile` (dbt-duckdb + dbt-expectations 0.10.4) rather than `dbt parse`, so its five `resource_type == "test"` nodes carry populated `compiled_code`. #154's manifest-test bridge reads that Jinja-resolved SQL off test nodes; every prior fixture is `dbt parse` output (compiled_code null, zero test nodes). The five dbt-expectations tests engineer the four #154 prune outcomes through the macro args (dbt resolves them statically, per the row_count_between e2e precedent): - expect_column_values_to_be_between, impossible bounds [1000,2000] on amount (100/200) -> returns failing rows -> KEPT - expect_column_values_to_be_between, vacuous bounds [0,1e6] -> ALWAYS-PASSES - expect_column_values_to_not_be_null on natural-NOT-NULL order_id -> ALWAYS-PASSES - expect_table_row_count_to_be_between -> aggregate COUNT(*) -> SKIP - expect_row_values_to_have_recent_data -> compiled SQL has now() -> non-deterministic SKIP Extends tests/fixtures/regenerate.sh with a `dbt deps && dbt compile` run and a broader jq scrub that also nulls every per-node `created_at` epoch (verified: two independent compiles produce byte-identical scrubbed output). The compiled manifest is committed at target/manifest.json via a fixture-local .gitignore negation so `signalforge.manifest.load(fixture_dir)` finds it at the default path. tests/manifest/test_expectations_fixture_loads.py is the network-free loads guard: it asserts the manifest loads via signalforge.manifest.load and (reading the JSON directly) that all five compiled test nodes survive. Tolerant of the sibling Manifest.tests surface (bead .1) not yet being merged — the Manifest.tests assertions run only when that attribute exists. Traces to DEC-017, DEC-008 of plans/super/154-dbt-expectations-prune.md.
…TestCustomSQL (#154 US-003) Add read_manifest_tests(manifest, model, *, project_dir=None) -> IngestResult: walk Manifest.tests, select nodes associated to the target model via associate_test_model, and route each by the US-002 sqlglot helpers — - absent/blank compiled_code -> SkippedTest(custom-or-generic-test), detail naming dbt compile (DEC-010); one prepended summary skip when ALL associated nodes lack compiled_code (not a silent skip). - NOT is_row_returning (aggregate/scalar) -> SkippedTest(malformed-supported-test) (DEC-004; aggregate support deferred to #267). - NOT is_deterministic_sql -> SkippedTest(malformed-supported-test) (DEC-012). - validate_ingested_sql (comment-tolerant) raises -> SkippedTest(malformed). - else -> model-level CandidateTestCustomSQL(sql=compiled_code, column=None) with a synthesized envelope-safe rationale (DEC-001, DEC-011). Rationale = '<namespace-label> <macro>(column=..., <args>)' built at frozen construction; the </ARTIFACT> close tag (literal + whitespace-split) is stripped so a hostile macro arg cannot fail-close the grade run. Macro identity rides on the rationale so the diff why cascade names it (DEC-015). SkipReason stays the closed 3-value literal; stage-0 (no logging). Re-exported from signalforge.ingest. Tests cover all five dispositions, the </ARTIFACT> strip, the all-missing summary, association filtering, and the real committed dbt-compiled fixture.
…t node -> CandidateTestCustomSQL)
…e, not proposed files (#154 US-005) DEC-015 of #154 — two behaviours for INGESTED (read-only) manifest/external tests flowing through the custom_sql prune pipeline: 1. Macro name into the diff `why`. The ingest bridge (US-003) carries the source-macro identity on the custom_sql test's `rationale` ("dbt-expectations expect_...(args)"). The KEPT tier already surfaces it via the rationale -> evidence -> fallback cascade; the DROPPED and kept-uncertain tiers bypassed that cascade and showed only decision.why, leaving the operator no way to locate the test to remove. New `_macro_why` threads the macro identity (front, so it survives max_why_chars truncation) ahead of the prune verdict/cause. Scoped to `custom_sql` so a built-in/drafted test with a drafter rationale keeps the issue-#50 carve-out (decision.why only) — the drop CATEGORY still rides the separate drop_reason column; the #50 cause stays load-bearing on kept-uncertain. 2. Ingested tests on the kept/dropped/flagged table, NEVER proposed_test_files. New `emit_test_files: bool = True` kwarg on render_diff gates emit_proposed_test_files. prune-existing (read-only; never drafts, so every custom_sql it prunes is external) passes emit_test_files=False, so an ingested test appears only as a table row, never re-surfaced as an authored .sql file. generate keeps the default True (#116 behaviour unchanged). tier stays the 4-value literal; no audit_schema_version bump (no shape change); no origin marker added to CandidateTestCustomSQL. tests/diff/test_engine.py adds kept/dropped/kept-uncertain macro-why coverage, the emit_test_files gate, and a built-in-unchanged regression pin.
…emit_test_files gate
…routing + comment-tolerant compile (#154 US-004) Route manifest-ingested custom_sql candidates (dbt compiled_code, already Jinja-resolved) to full-scope-against-source evaluation regardless of the configured --scope, and validate them comment-tolerantly. dbt renders the model relation with its own quoting, which the {{ this }} string-substitution cannot match — so under scope=sample every ingested test silently degraded to kept-without-evidence (AR row 8). Now they run full-scope against the source (never a _SESSION._sf_sample_* temp table), bounded by maximum_bytes_billed. Marker (DEC-007): CandidateTestCustomSQL gains a runtime-only from_manifest: bool = Field(default=False, exclude=True) provenance flag — the "typed category at the source" discriminator (grade-layer.md), set by read_manifest_tests, read by the engine + compiler. exclude=True keeps it out of model_dump / model_dump_json, so the diff candidate_hash, proposed YAML, and every committed custom_sql fixture stay byte-identical; drafted custom_sql (from_manifest=False) keeps its existing sample behaviour unchanged (DEC-016). - engine: _test_requires_source_table returns True for ingested custom_sql under either sample strategy (joins the metadata-aggregate bypass set); one INFO fires when scope=sample was requested for an ingested batch. - compiler: _compile_custom_sql takes an early full-scope branch for ingested bodies — skips the {{ this }}/sample substitution, uses the comment-tolerant validate_ingested_sql (DEC-013), and keeps the belt-and-braces is_deterministic_sql -> _InvalidIdentifier -> kept-without-evidence fallback (DEC-012). DropReason stays the locked 5-value Literal; no PruneEvent field, no _PRUNE_AUDIT_SCHEMA_VERSION bump. Tests: 11 new (engineered-determinism via FakeBigQueryClient) covering ingested tautology drop / real-failure kept / non-deterministic kept-without-evidence / scope=sample -> full-scope with one INFO and source-not-temp dispatched SQL / comment-bearing not false-rejected / drafted custom_sql byte-unchanged / a mixed drafted+ingested batch pinning the per-test override arm.
…comment-tolerant compile
#154 US-007) Add two opt-in flags to `signalforge prune-existing`: - --from-manifest (DEC-005): ALSO ingest the model's dbt-compiled manifest test nodes via ingest.read_manifest_tests and merge the row-returning + deterministic compiled_code bodies (as from_manifest=True custom_sql candidates) into the prune set alongside the --schema / tests/*.sql candidates. Manifest skip records fold into the existing skipped-test report. Off by default: byte-identical to the pre-#154 behaviour. - --grade (DEC-002 / DEC-018): opt-in LLM-as-judge grade stage on the ingested manifest tests. Requires --from-manifest (schema.yml / singular tests carry rationale=None, so grading them is noise) — --grade alone is an input-validation failure (CliInputError, exit 2) raised at handler entry. Wires load_grade_config + grade_artifacts between prune and diff, feeds grading_report into render_diff (enabling the flagged tier), writes grade.json / grade.jsonl, and renumbers progress to [N/4]. Credential gate is implicit (missing key -> LLMAuthError tier 3 at call time). 6-surface parity: argparse help, handler/module docstrings, docs/cli-ops.md (flag reference + prose sections), test names + docstrings, and SKILL.md. Tests cover: --grade-without---from-manifest -> exit 2, --from-manifest OFF (no manifest ingestion), --from-manifest ON (manifest custom_sql on the diff table via the real dbt_project_expectations fixture), skip folding, grade stage runs (faked grade_artifacts), and [N/4] progress renumber.
…de on prune-existing
…s prune+grade adapter (#154 US-008) Document the manifest-compiled-SQL prune path (issue #154) across the five ops docs, verified against the shipped code and the committed tests/fixtures/dbt_project_expectations/ fixture: - ingest-ops.md: read_manifest_tests recognition path — the four gates (presence / row-returning / deterministic / comment-tolerant safety), the closed-SkipReason routing, macro-identity rationale, and the "run dbt compile" remediation surface. Documents the KEY FINDING that dbt-expectations wraps every macro (incl. expect_table_row_count) in a row-returning validation_errors shell, so aggregate-SKIP fires only on bare SELECT COUNT(*) bodies. - prune-ops.md: how ingested compiled tests route through the custom_sql pipeline; the DEC-007 full-scope decision (scope=full always; --scope= sample emits an INFO and still runs full); comment-tolerant validation; the from_manifest source-vs-drafted split. - diff-ops.md: macro identity into the diff why; ingested tests appear on the kept/dropped/flagged table, never as proposed .sql files (read-only). - cli-ops.md: verified the US-007 --from-manifest/--grade rows; filled the exit-code list (CliInputError tier 2, LLM/grade errors tier 3) and added a worked example pruning a real dbt-expectations schema.yml. - grade-ops.md: --grade behavior on prune-existing (opt-in LLM grade of ingested tests via the synthesized rationale; requires --from-manifest; off by default; credential/cost implications). mkdocs build (non-strict) emits no new anchor-breakage vs baseline.
Fixes from 4 diverse-lens review passes (correctness / rules / test-signal / integration): - diff macro-why: scope to from_manifest ingested custom_sql only — was gated on type==custom_sql, leaking the drafter rationale into generate's dropped/kept-uncertain why and re-breaking the issue-#50 carve-out (confirmed regression, 2 reviewers). - prune-existing --grade: grade ONLY the manifest-ingested tests, not the merged candidate — grading the operator's schema.yml built-ins (empty rationale) wasted LLM calls and could flip a kept built-in to the flagged tier (DEC-002 intent). - ingest _compiled_sql: backslash-escape-aware literal blanking; drop the cross-layer warehouse _strip_string_literals private import (self-contained _blank_sql_literals). - tests: pin from_manifest exclude=True byte-identity (unguarded DEC-016 invariant); grade->LLMAuthError exit-3 path; drafted-custom_sql why regression guard; prune-existing never emits proposed_test_files (read-only). - fixture comment + cli-ops caveat: row-count macro is pruned (validation_errors wrapper), not aggregate-skipped; proposed-files suppression scoped in the byte-identical claim.
- manifest-readers.md: GenericTest + Manifest.tests sibling filter + feature-detect association - ingest-layer.md: read_manifest_tests source + four-gate sqlglot classification + SkipReason reuse - prune-engine.md: from_manifest full-scope routing + comment-tolerant validation + DropReason lock - business-rule-tests.md: custom_sql third source (manifest); gate on from_manifest not type - llm-drafter.md: sqlglot confinement 2nd/3rd importer (foreign-SQL = AST, not string-sub)
wjduenow
changed the title
wjduenow
marked this pull request as ready for review
July 6, 2026 00:12
There was a problem hiding this comment.
Pull request overview
This PR implements issue #154’s core “manifest compiled SQL” adapter so dbt-compiled generic/namespaced tests (e.g., dbt-expectations) can be pruned and optionally graded by reusing the existing custom_sql pipeline, with conservative-bias routing preserved.
Changes:
- Extend the manifest loader to surface
resource_type == "test"nodes asManifest.tests(GenericTest) and add association logic (associate_test_model). - Add an ingest bridge (
read_manifest_tests) plus sqlglot-based compiled-SQL gates (determinism, scalar/aggregate-shape detection, comment-tolerant safety scan) and wirefrom_manifest=Truerouting into prune + diff. - Add
prune-existing --from-manifestand--gradewiring, plus a realdbt compilefixture project and comprehensive tests/docs.
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/prune/test_engine.py | Adds prune-engine routing tests for manifest-ingested custom_sql (full-scope bypass under sample). |
| tests/prune/test_compiler.py | Adds compiler unit tests for from_manifest=True behavior (no sampling, comment-tolerant validation, determinism fallback). |
| tests/manifest/test_models.py | Adds GenericTest/Manifest.tests model tests + drift sentinel fixture usage. |
| tests/manifest/test_loader.py | Tests loader filtering of test nodes into Manifest.tests and association ladder behavior. |
| tests/manifest/test_expectations_fixture_loads.py | Smoke test ensuring the committed dbt-expectations compiled manifest fixture loads and contains compiled test SQL. |
| tests/ingest/test_manifest_tests.py | End-to-end unit tests for read_manifest_tests classification, rationale synthesis, and skip recording. |
| tests/ingest/test_compiled_sql.py | Tests sqlglot-based determinism/row-shape analysis and comment-tolerant safety validation. |
| tests/fixtures/regenerate.sh | Adds regen logic for the dbt-expectations compiled fixture (dbt deps + dbt compile + scrubbing). |
| tests/fixtures/README.md | Documents the new expectations fixture and how to regenerate it. |
| tests/fixtures/manifest/generic_test_nodes.json | Hand-authored generic test node fragments for loader/model drift + association tests. |
| tests/fixtures/dbt_project_expectations/profiles.yml | DuckDB profile committed for offline fixture regeneration. |
| tests/fixtures/dbt_project_expectations/packages.yml | Pins dbt-expectations dependency for the compiled fixture. |
| tests/fixtures/dbt_project_expectations/package-lock.yml | Commits a reproducible dbt deps resolution for the fixture. |
| tests/fixtures/dbt_project_expectations/models/schema.yml | Declares dbt-expectations tests used to exercise prune outcomes. |
| tests/fixtures/dbt_project_expectations/models/orders.sql | Provides a deterministic 2-row model for compiled test outcomes. |
| tests/fixtures/dbt_project_expectations/dbt_project.yml | Fixture project config + vars needed for deterministic compilation. |
| tests/fixtures/dbt_project_expectations/.gitignore | Ensures only target/manifest.json is committed from compiled artifacts. |
| tests/draft/test_models.py | Guards that from_manifest stays excluded from serialization for hash/byte-identity invariants. |
| tests/draft/test_drift_detector.py | Updates strict drift model mirror to include runtime-only from_manifest. |
| tests/diff/test_engine.py | Ensures diff why includes macro identity for ingested tests and suppresses proposed test files when requested. |
| tests/cli/test_prune_existing.py | Adds CLI tests for --from-manifest, --grade dependency, progress renumbering, and proposed-file suppression. |
| src/signalforge/skills/signalforge/SKILL.md | Documents new prune-existing flags and behavior. |
| src/signalforge/prune/engine.py | Routes from_manifest custom_sql to source-table/full-scope under sampling and emits one INFO. |
| src/signalforge/prune/compiler.py | Adds manifest-ingested custom_sql compile branch with determinism + comment-tolerant validation fallback. |
| src/signalforge/manifest/models.py | Introduces GenericTest, TestMetadata, and Manifest.tests. |
| src/signalforge/manifest/loader.py | Filters test nodes into Manifest.tests and adds associate_test_model ladder. |
| src/signalforge/manifest/init.py | Re-exports GenericTest and associate_test_model. |
| src/signalforge/ingest/reader.py | Implements read_manifest_tests bridge and rationale synthesis + skip recording. |
| src/signalforge/ingest/_compiled_sql.py | New sqlglot-based helpers for determinism/row-shape analysis and comment-tolerant safety scanning. |
| src/signalforge/ingest/init.py | Exposes read_manifest_tests as a public ingest entry point. |
| src/signalforge/draft/models.py | Adds runtime-only from_manifest flag to CandidateTestCustomSQL (excluded from serialization). |
| src/signalforge/diff/engine.py | Threads macro identity into dropped/kept-uncertain why for ingested tests; adds emit_test_files option. |
| src/signalforge/cli/prune_existing.py | Adds --from-manifest and --grade, grade-stage wiring, and always suppresses proposed test files. |
| docs/prune-ops.md | Documents routing and validation differences for manifest-ingested vs drafted custom_sql. |
| docs/ingest-ops.md | Documents read_manifest_tests gates, skip reasons, and macro identity behavior. |
| docs/grade-ops.md | Documents prune-existing --grade behavior for ingested manifest tests. |
| docs/diff-ops.md | Documents macro identity threading into diff why for ingested tests and read-only behavior. |
| docs/cli-ops.md | Documents new CLI flags, behaviors, and exit-code implications. |
| .claude/rules/prune-engine.md | Updates project rules with #154 routing/validation/determinism notes. |
| .claude/rules/manifest-readers.md | Documents the new Manifest.tests surface and association ladder. |
| .claude/rules/llm-drafter.md | Updates sqlglot confinement notes for the new ingest compiled-SQL module. |
| .claude/rules/ingest-layer.md | Documents read_manifest_tests classification gates and skip surfaces. |
| .claude/rules/business-rule-tests.md | Notes the new custom_sql source and from_manifest scoping requirements across layers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/signalforge/prune/compiler.py (1)
70-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose compiled-SQL helpers through a public seam.
prune.compilernow reaches intosignalforge.ingest._compiled_sql, which makes prune depend on an ingest-private module. Please re-export these helpers throughsignalforge.ingest.__all__or move the shared SQL validation seam to a common/warehouse module before importing it here. As per coding guidelines, "Ensure the public API of each subpackage is defined via__all__; contract details are in matchingdocs/*-ops.mdfiles. Prefix internal implementation details with_."🤖 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 `@src/signalforge/prune/compiler.py` at line 70, `prune.compiler` is importing compiled-SQL helpers from an ingest-private module instead of a public seam. Move `is_deterministic_sql` and `validate_ingested_sql` behind a public API by re-exporting them from `signalforge.ingest.__all__` or relocating the shared SQL validation helpers to a common/warehouse module, then update the import in `prune.compiler` to use that public symbol path.Source: Coding guidelines
src/signalforge/ingest/_compiled_sql.py (1)
214-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the two hand-rolled quote-scanners.
_strip_sql_commentsand_blank_sql_literalseach independently re-implement the same quote/backslash/doubled-quote tracking state machine (~40 lines apiece), differing only in what happens to comment/literal content. Sincevalidate_ingested_sqlcomposes both, a future fix to quote-handling in one (e.g. a new escape convention) risks not being mirrored in the other, silently reintroducing a bypass. Consider extracting a shared low-level scanner (e.g., a generator yielding(char, in_quote, in_comment)or span classification) that both functions consume.🤖 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 `@src/signalforge/ingest/_compiled_sql.py` around lines 214 - 357, The quote/backslash/doubled-quote handling is duplicated in _strip_sql_comments and _blank_sql_literals, so they can drift and create inconsistent SQL scanning behavior. Extract the shared quote-state machine into a common helper used by both functions, and have validate_ingested_sql rely on that shared logic for comment stripping and literal blanking so any future escape-rule change is applied consistently.tests/fixtures/regenerate.sh (1)
140-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
mv -ffor the scrub temp-file swap.Line 152's
mv "$out.tmp" "$out"should use-fto avoid any interactive overwrite prompt, consistent with every other file operation in this script and the repo's shell guideline.🛠️ Proposed fix
' "$in" >"$out.tmp" - mv "$out.tmp" "$out" + mv -f "$out.tmp" "$out" }As per coding guidelines, "Use
mv -finstead ofmvto force overwrite without prompting."🤖 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 `@tests/fixtures/regenerate.sh` around lines 140 - 153, The temp-file swap in scrub_compiled_manifest should force overwrite instead of relying on plain mv. Update the mv call that replaces "$out" so it uses the force flag, matching the script’s non-interactive file operation pattern and the repo shell guideline.Source: Coding guidelines
🤖 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/ingest-ops.md`:
- Around line 419-423: The heading under the prunable vs. skip-recorded section
has a gate-count mismatch: it says “Three gates” while the described flow in the
same paragraph lists four checks. Update the heading in the markdown section so
it matches the actual four-step sequence described around the bridge checks,
keeping the wording consistent with the surrounding explanation.
In `@plans/super/154-dbt-expectations-prune.md`:
- Around line 16-22: Remove the accidental diff artifact in the prune-gate note
by updating the text around the `custom_sql` mention so it reads as a normal
sentence. The issue is the leading `+` before `custom_sql`, so edit the markdown
content in this plan document to eliminate that marker and keep the phrase
listing the built-ins and `custom_sql` grammatically consistent.
In `@src/signalforge/diff/engine.py`:
- Around line 471-472: The concatenation in the preserve-why path is letting a
long macro rationale overwrite the kept-uncertain base cause, so update the
logic in the relevant `engine.py` helper(s) around the `base_why`/`macro`
combination to always retain `decision.why`. Reserve part of `max_why_chars` for
the base cause before calling `_truncate_why`, or shorten the macro text first,
and apply the same fix anywhere the same pattern appears (including the later
duplicate block).
In `@src/signalforge/manifest/loader.py`:
- Around line 506-510: _UPDATE_REF_CALL_RE in loader.py so the ref() extractor
matches both single-quoted and double-quoted arguments, including the one-arg
and two-arg forms. Keep the existing purpose of resolving the v9
no-attached_node path in the generic test metadata parsing, but broaden the
regex used by the bounded ref() disambiguator so ref("my_model") and ref("pkg",
"name") are handled the same as the single-quoted variants.
In `@src/signalforge/manifest/models.py`:
- Around line 128-189: The package surface is missing TestMetadata even though
GenericTest.test_metadata exposes it publicly. Update signalforge.manifest’s
__init__ exports to import TestMetadata from models.py and include it in
__all__, alongside GenericTest and the other manifest model re-exports, so
callers can access it directly from signalforge.manifest.
In `@src/signalforge/prune/compiler.py`:
- Line 704: The determinism check in compiler.py is still using the helper
default dialect instead of the active one, so compiled SQL for non-BigQuery
dialects can be misclassified; update the `is_deterministic_sql` call inside the
pruning logic to pass `dialect.name` from the surrounding compiler flow so it
uses the current `Dialect` consistently with sqlglot.
---
Nitpick comments:
In `@src/signalforge/ingest/_compiled_sql.py`:
- Around line 214-357: The quote/backslash/doubled-quote handling is duplicated
in _strip_sql_comments and _blank_sql_literals, so they can drift and create
inconsistent SQL scanning behavior. Extract the shared quote-state machine into
a common helper used by both functions, and have validate_ingested_sql rely on
that shared logic for comment stripping and literal blanking so any future
escape-rule change is applied consistently.
In `@src/signalforge/prune/compiler.py`:
- Line 70: `prune.compiler` is importing compiled-SQL helpers from an
ingest-private module instead of a public seam. Move `is_deterministic_sql` and
`validate_ingested_sql` behind a public API by re-exporting them from
`signalforge.ingest.__all__` or relocating the shared SQL validation helpers to
a common/warehouse module, then update the import in `prune.compiler` to use
that public symbol path.
In `@tests/fixtures/regenerate.sh`:
- Around line 140-153: The temp-file swap in scrub_compiled_manifest should
force overwrite instead of relying on plain mv. Update the mv call that replaces
"$out" so it uses the force flag, matching the script’s non-interactive file
operation pattern and the repo shell guideline.
🪄 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: 4058c05b-a42e-4b01-8ad4-319f19df6104
📒 Files selected for processing (45)
.claude/rules/business-rule-tests.md.claude/rules/ingest-layer.md.claude/rules/llm-drafter.md.claude/rules/manifest-readers.md.claude/rules/prune-engine.mddocs/cli-ops.mddocs/diff-ops.mddocs/grade-ops.mddocs/ingest-ops.mddocs/prune-ops.mdplans/super/154-dbt-expectations-prune.mdsrc/signalforge/cli/prune_existing.pysrc/signalforge/diff/engine.pysrc/signalforge/draft/models.pysrc/signalforge/ingest/__init__.pysrc/signalforge/ingest/_compiled_sql.pysrc/signalforge/ingest/reader.pysrc/signalforge/manifest/__init__.pysrc/signalforge/manifest/loader.pysrc/signalforge/manifest/models.pysrc/signalforge/prune/compiler.pysrc/signalforge/prune/engine.pysrc/signalforge/skills/signalforge/SKILL.mdtests/cli/test_prune_existing.pytests/diff/test_engine.pytests/draft/test_drift_detector.pytests/draft/test_models.pytests/fixtures/README.mdtests/fixtures/dbt_project_expectations/.gitignoretests/fixtures/dbt_project_expectations/dbt_project.ymltests/fixtures/dbt_project_expectations/models/orders.sqltests/fixtures/dbt_project_expectations/models/schema.ymltests/fixtures/dbt_project_expectations/package-lock.ymltests/fixtures/dbt_project_expectations/packages.ymltests/fixtures/dbt_project_expectations/profiles.ymltests/fixtures/dbt_project_expectations/target/manifest.jsontests/fixtures/manifest/generic_test_nodes.jsontests/fixtures/regenerate.shtests/ingest/test_compiled_sql.pytests/ingest/test_manifest_tests.pytests/manifest/test_expectations_fixture_loads.pytests/manifest/test_loader.pytests/manifest/test_models.pytests/prune/test_compiler.pytests/prune/test_engine.py
- diff/engine.py (MAJOR): _macro_why is tier-aware — kept-uncertain preserves the
load-bearing kept-without-evidence cause under truncation (cause_priority), dropped
keeps macro-locator-first (drop_reason column carries the category). +test.
- prune/compiler.py: pass dialect.name to is_deterministic_sql so Snowflake/Databricks
compiled SQL parses under the right dialect (a parse failure returned a false pass).
- manifest/loader.py: _REF_CALL_RE accepts double-quoted ref("name") (dbt/Jinja allow it).
- manifest/__init__.py: export TestMetadata (GenericTest.test_metadata exposes it).
- cli/prune_existing.py: reword the grade comment — grade_artifacts also scores the
model-level doc artifacts; only manifest TESTS can surface as flagged.
- regenerate.sh: mv -f for the scrub temp-swap (non-interactive-safe).
- docs/ingest-ops.md: Three -> Four gates heading; plan doc: drop stray + marker.
Owner
Author
PR Review SummaryAll 8 review threads addressed in Fixed (8 items)
False Positives (0)None — every comment was a real, actionable issue. Validation after fixes: |
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 #154 — a prune+grade adapter that consumes dbt-expectations (and
other generic/namespaced) tests' already-compiled SQL from
manifest.jsonand runsthem through the existing
custom_sqlpipeline, so they get the samekept/kept-uncertain/dropped/flagged treatment as the built-ins. Closes the
un-graded half of the prune gate for teams that aren't greenfield.
Phase: detailing (awaiting approval)
Structure: single cohesive plan (not an epic) — reuses
custom_sql, one design.Decisions: 18 (DEC-001…018)
Stories: 8 implementation + Quality Gate + Patterns & Memory
Key shape decisions
CandidateTestCustomSQL— no 7th test type (DEC-001).comment-tolerant validation all need AST parsing because
compiled_codeisdbt's foreign-rendered SQL; the
#116string-substitution machinery is safebut wrong-tool (DEC-006).
scope=sampledeferred → full-scope for ingested tests in pass 1 (dbt'squoted relation can't be string-substituted; sampling silently degrades to
kept-without-evidence) (DEC-007).gate (wrapping a scalar in
COUNT(*) AS failuresyieldsfailures=1always →silent wrong "kept") (DEC-004).
--from-manifest+--gradeonprune-existing;--graderequires--from-manifest, off by default; existing runs byte-unchanged (DEC-002/005/018).dbt compilefixture (dbt-duckdb + dbt-expectations), per-nodehand-patch fallback (DEC-017).
Architecture review
Four focused reviews (SQL-handling/determinism · manifest read-back · grade wiring ·
fixture feasibility). Two blockers, both resolvable (compiled fixture; scope=sample
substitution) — no design dead-ends. Full pass/concern/blocker table in the plan doc.
Deferred (filed as follow-up issues)
scope=samplesupport for ingested tests via sqlglot AST relation-rewriting.Plan document
See
plans/super/154-dbt-expectations-prune.md.Next steps
Summary by CodeRabbit
New Features
manifest.json, surfaced ascustom_sqlcandidates.prune-existingnow supports--from-manifestand optional--grade, adding an extra grading stage and producing grade artifacts.Bug Fixes
compiled_code: comment-tolerant safety checks, determinism validation, and safer fallback routing.Tests
--from-manifest/--gradeworkflow and “why” construction plus read-only diff output.