#268: scope=sample for ingested dbt tests via sqlglot AST relation-rewriting (plan) - #271
Merged
Conversation
|
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 (1)
📝 WalkthroughWalkthroughManifest-ingested dbt tests can use materialized sample tables when rewrite and batching gates pass. The change adds fail-closed SQL analysis, compiler and engine routing, audit schema v4 provenance, bounded audit SQL, count-scalar handling, dialect-aware ingestion, fixtures, and live BigQuery coverage. ChangesManifest-ingested sample pruning
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Three latent #154 bugs, all in the stage-0 ingest layer (DEC-012(1)(2), DEC-013): 1. RecursionError / ValueError escaped every _compiled_sql helper's `except sqlglot.errors.SqlglotError`. A ~2000-deep nested-paren body blows sqlglot's recursive-descent parser (RecursionError) and an unknown dialect= name raises ValueError — neither is a SqlglotError, so both aborted the whole prune run with no audit rows written (fail-OPEN on the fail-closed audit contract). A new `_PARSE_FAILURES` tuple is now caught in all three helpers, each returning its existing conservative verdict. 2. No size cap on `compiled_code`. The 5 MB _INGEST_SCHEMA_SIZE_LIMIT_BYTES guards file reads only; read_manifest_tests takes an already-parsed Manifest, so a pathological body reached sqlglot unbounded. New 256 KiB _COMPILED_CODE_SIZE_LIMIT_BYTES, checked before any parse; over-cap bodies are skip-recorded as `malformed-supported-test` (the closed 3-value SkipReason is NOT grown). 3. read_manifest_tests called the gates with the default dialect="bigquery" while prune/compiler.py passes dialect.name — two parses of the same body under different dialects can disagree. New keyword-only `dialect: str = "bigquery"` on read_manifest_tests / _classify_manifest_test, threaded into every gate. The CLI's prune_existing._merge/_ingest_manifest_tests keeps the default (the adapter, and hence the live Dialect, is constructed after the ingest step); documented rather than plumbed. Stage-0 posture preserved: no logging, no SQL building, no new error class, no 4th SkipReason, no sqlglot import under prune/.
#268 US-002) Two pure-ANALYSIS helpers in ingest/_compiled_sql (DEC-001: they return data, never SQL — so no sqlglot lands under prune/ and no 4th-importer confinement scan is owed): * plan_relation_rewrite — parse; resolve scopes via sqlglot.optimizer.scope; normalize_identifiers on both sides (the per-dialect fold rule, never hand-rolled); EXACT full-tuple match (never suffix); enforce a SINGLE physical relation on the AST (DEC-006 — a \bjoin\b regex misses comma-joins, correlated subqueries and NOT EXISTS); refuse on a CTE-alias collision (DEC-005 / AR-B2 — a dotted `proj.ds.tbl` alias defeats a naive CTE-name exclusion set); return inclusive-end CHARACTER spans (DEC-015), each proved to re-tokenize against the identical str object. * verify_relation_rewrite — the DEC-004 post-condition on the REWRITTEN SQL: parses clean, ZERO residual source tables, exactly expected_n temp tables. A count cross-check is NOT an integrity proof (AR-B1). Refusals carry a machine-readable reason from a closed 5-value set (RELATION_REWRITE_REASONS) for the DEC-014 histogram. No new error class, no new SkipReason value. 44+ tests incl. every adversarial body from the plan; the AR-B1, AR-B2 and residual-source defences are each pinned by a mutation-verified failing test.
…ate + rewrite verification
…m rewrite + fail-closed guard
…ampled ingested tests
…urce + compiled_sql truncation
…O + per-test DEBUG
…rse-guard (#268 US-007)
…pins + ungated sqlglot parse-guard
…cert for sampled ingested tests
- audit: truncate SQL fields by JSON-escaped byte cost, not code points, so an adversarial multibyte compiled_code (emoji escaping to \uXXXX\uXXXX under ensure_ascii) can no longer blow the 4000-byte cap and abort the run (QG pass 2) - ingest: pin the compiled_code size-cap constant value (262_144), so a silent RAISE of the cap can't slip past the co-varying over-cap test (QG pass 4) - ingest: remove an inaccurate '# pragma: no cover' — the zero-match arm is reachable via a single-backtick dotted relation; pin it with a test (QG pass 1)
Update .claude/rules/ to match the shipped sampling of manifest-ingested tests: - prune-engine.md: retire the #154 full-scope-only DEC-007 claim; new 'Sampled manifest-ingested tests (#268)' section (locate/splice split, verify_relation_rewrite post-condition, _IngestedSamplePlan precompute, DEC-009 fallback, DEC-014 observability, JSON-escaped audit truncation); bypassed_to_source + audit v3->4 history - business-rule-tests.md: retire 'scope=sample deferred'; add the ingested Direction-1/2 precedent - ingest-layer.md: four->five gates (size cap); plan/verify_relation_rewrite helpers; totalised parse guards - llm-drafter.md: why sqlglot confinement stayed at 2 importers (locate-in-ingest/splice-in-compiler)
There was a problem hiding this comment.
Pull request overview
Implements the #268 plan to enable --scope=sample for manifest-ingested dbt tests by locating the model’s relation in dbt-rendered compiled_code via sqlglot AST analysis (in ingest), then performing a byte-preserving token splice in the prune compiler, with integrity verification and a gated BigQuery live e2e merge gate.
Changes:
- Add sqlglot-based relation rewrite planning + verification for foreign-rendered SQL, and integrate a verified
ingested_sql_overrideinto prune compilation with a fail-closed guard. - Harden ingest/audit against hostile inputs (RecursionError/unknown dialect, compiled_code size cap) and prevent run-aborting audit oversize by truncating SQL fields for audit records; bump audit schema to v4 with
bypassed_to_source. - Add extensive unit fixtures + an ungated parse-guard for rewritten SQL, plus a gated BigQuery live end-to-end certification test.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/signalforge/ingest/_compiled_sql.py | Adds total parse guards and the relation rewrite planner + AST post-condition verifier. |
| src/signalforge/ingest/reader.py | Adds compiled_code size cap and threads dialect through sqlglot gates. |
| src/signalforge/prune/compiler.py | Adds pure string splice helper and ingested override + fail-closed guard in compiler. |
| src/signalforge/prune/models.py | Adds bypassed_to_source to prune decision model. |
| src/signalforge/prune/audit.py | Bumps audit schema to v4, truncates SQL fields for audit safety, persists bypassed_to_source. |
| src/signalforge/cli/prune_existing.py | Documents manifest-ingest behavior and dialect handling at prune-existing ingest seam. |
| tests/ingest/test_compiled_sql.py | Pins hostile-input totality behavior (RecursionError/unknown dialect) for sqlglot gate helpers. |
| tests/ingest/test_manifest_tests.py | Tests compiled_code size cap and dialect-threading into ingest gate calls. |
| tests/ingest/test_relation_rewrite.py | Comprehensive adversarial tests for relation-locate planning + rewrite verification. |
| tests/prune/test_compiler.py | Adds compiler-level tests for verified ingested override behavior and fail-closed guard. |
| tests/prune/test_audit.py | Updates audit schema assertions, adds truncation regression pins, adds bypassed_to_source tests. |
| tests/prune/test_drift_detector.py | Updates strict drift models + adds v3→v4 replay tests for new audit field. |
| tests/prune/test_ingested_rewrite_parse_guard.py | Ungated parse guard validating rewritten fixture SQL and integrity invariants. |
| tests/cli/_e2e_helpers.py | Adds manifest test node injection helper and threads bypass flag when reading decisions. |
| tests/cli/test_e2e_bigquery_ingested_sample.py | Gated BigQuery live e2e merge gate for sampled ingested tests. |
| tests/fixtures/prune/prune_event_v1.jsonl | Updates committed audit fixture lines to schema v4 and exercises both bypass states. |
| tests/fixtures/prune/compiled_sql/ingested/index.json | Index for ingested compiled_sql rewrite fixtures and expected span counts. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_between.in.sql | Input fixture for BigQuery dbt-expectations body pre-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_between.out.sql | Output fixture for BigQuery dbt-expectations body post-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_not_null.in.sql | Input fixture for BigQuery not-null dbt-expectations body pre-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_not_null.out.sql | Output fixture for BigQuery not-null dbt-expectations body post-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_row_count_between.in.sql | Input fixture for BigQuery row-count-between body pre-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_row_count_between.out.sql | Output fixture for BigQuery row-count-between body post-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_self_join.in.sql | Input fixture for BigQuery self-join pre-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_self_join.out.sql | Output fixture for BigQuery self-join post-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_simple_where.in.sql | Input fixture for simple BigQuery WHERE pre-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_simple_where.out.sql | Output fixture for simple BigQuery WHERE post-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_subquery_and_comment.in.sql | Input fixture for subquery/comments pre-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/bigquery_subquery_and_comment.out.sql | Output fixture for subquery/comments post-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/databricks_dbt_expectations_not_null.in.sql | Input fixture for Databricks not-null body pre-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/databricks_dbt_expectations_not_null.out.sql | Output fixture for Databricks not-null body post-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/snowflake_dbt_expectations_not_null.in.sql | Input fixture for Snowflake not-null body pre-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/snowflake_dbt_expectations_not_null.out.sql | Output fixture for Snowflake not-null body post-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/snowflake_self_join.in.sql | Input fixture for Snowflake self-join pre-rewrite. |
| tests/fixtures/prune/compiled_sql/ingested/snowflake_self_join.out.sql | Output fixture for Snowflake self-join post-rewrite. |
| docs/prune-ops.md | Updates operational docs for sampled ingested tests, routing observability, and audit v4. |
| docs/ingest-ops.md | Updates ingest ops docs for new gate order, size cap, dialect threading, and sampling behavior. |
| CHANGELOG.md | Documents behavior/cost change, audit schema bump, and fixes for latent #154 bugs. |
| plans/super/268-ingest-sample-scope.md | Adds the detailed super-plan document for #268 with DECs and story breakdown. |
| .claude/rules/prune-engine.md | Updates project rules to reflect new sampled-ingested behavior and audit schema v4. |
| .claude/rules/ingest-layer.md | Updates ingest-layer rules for size cap, totality, and relation-locate helpers. |
| .claude/rules/business-rule-tests.md | Updates business-rule tests rules to reflect #268 landing and integrity post-condition guidance. |
| .claude/rules/llm-drafter.md | Documents why sqlglot stayed out of prune/ via locate-in-ingest/splice-in-compiler split. |
💡 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: 9
🧹 Nitpick comments (2)
CHANGELOG.md (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense these entries into release-facing outcomes.
These bullets re-narrate implementation gates, internal decisions, and failure mechanics. Retain behavior, compatibility, and cost impacts, then refer readers to
plans/super/268-ingest-sample-scope.mdand the operational docs.As per coding guidelines, CHANGELOG entries must be curated release records and refer to plans or ADRs rather than re-narrating shipped work.
Also applies to: 14-16
🤖 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 `@CHANGELOG.md` at line 9, Condense the verbose changelog entry into concise release-facing outcomes: retain the supported sample-scope behavior, unchanged full-scope/oneshot compatibility, materialisation cost impact, fallback behavior, and audit/observability changes. Remove implementation gates, internal routing details, failure mechanics, and test-specific narrative, and refer readers to plans/super/268-ingest-sample-scope.md plus the relevant operational documentation.Source: Coding guidelines
src/signalforge/prune/compiler.py (1)
772-790: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated determinism + safety-scan check into a shared helper.
The raw-body check and the override re-check run the identical two-step validation (determinism, then comment-tolerant safety scan), differing only in the reason-string prefix. This codebase explicitly calls out "two-conditional drift" as a bug class elsewhere (
_test_requires_source_table) — worth consolidating here too.♻️ Proposed extraction
+def _revalidate_ingested_body( + sql: str, dialect: Dialect, *, rewritten: bool +) -> _InvalidIdentifier | None: + prefix = "relation-rewritten ingested" if rewritten else "ingested" + if not is_deterministic_sql(sql, dialect=dialect.name): + return _InvalidIdentifier( + reason=f"{prefix} custom_sql is non-deterministic " + "(TABLESAMPLE / RAND / time-dependent function)" + ) + try: + validate_ingested_sql(sql) + except QuerySyntaxError: + return _InvalidIdentifier( + reason=f"{prefix} custom_sql rejected by the comment-tolerant SQL safety scan" + ) + return None + ... - if not is_deterministic_sql(test.sql, dialect=dialect.name): - return _InvalidIdentifier( - reason=( - "ingested custom_sql is non-deterministic " - "(TABLESAMPLE / RAND / time-dependent function)" - ) - ) - try: - validate_ingested_sql(test.sql) - except QuerySyntaxError: - return _InvalidIdentifier( - reason="ingested custom_sql rejected by the comment-tolerant SQL safety scan" - ) + if (invalid := _revalidate_ingested_body(test.sql, dialect, rewritten=False)) is not None: + return invalidand, in the override branch:
- if not is_deterministic_sql(ingested_sql_override, dialect=dialect.name): - return _InvalidIdentifier( - reason=( - "relation-rewritten ingested custom_sql is non-deterministic " - "(TABLESAMPLE / RAND / time-dependent function)" - ) - ) - try: - validate_ingested_sql(ingested_sql_override) - except QuerySyntaxError: - return _InvalidIdentifier( - reason=( - "relation-rewritten ingested custom_sql rejected by the " - "comment-tolerant SQL safety scan" - ) - ) + if ( + invalid := _revalidate_ingested_body(ingested_sql_override, dialect, rewritten=True) + ) is not None: + return invalid return ingested_sql_overrideAlso applies to: 877-892
🤖 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` around lines 772 - 790, Extract the repeated determinism and comment-tolerant safety validation into a shared helper near the relevant compiler logic, preserving the existing validation order and QuerySyntaxError handling. Have the helper accept the reason-string prefix or equivalent context so raw-body and override callers retain their distinct _InvalidIdentifier reasons, then replace both validation blocks with calls to it.
🤖 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 @.claude/rules/ingest-layer.md:
- Around line 91-102: Repair the Markdown inline-code formatting in the
five-gate classification paragraph: adjust the backtick delimiters around the
size-cap expression and _COMPILED_CODE_SIZE_LIMIT_BYTES so the intended text
uses valid, non-nested code spans and eliminates MD038 warnings. Preserve the
paragraph’s content and emphasis.
In @.claude/rules/prune-engine.md:
- Around line 313-316: Rewrite the final sentence in the DEC-016
“Live-certified” paragraph to remove the double-modal phrasing, while preserving
the meaning that oneshot sampling remains a follow-up because its required CTE
approach failed during execution.
In `@CHANGELOG.md`:
- Line 14: The CHANGELOG entry incorrectly documents the audit SQL truncation as
a 1000-character prefix. Update the entry to state the implementation retains up
to 1200 JSON-escaped bytes, while preserving the surrounding behavior and
forensic details.
In `@docs/ingest-ops.md`:
- Around line 71-81: The documentation for read_manifest_tests must consistently
include the dialect parameter, including the signature shown around the later
usage section. Replace the claim that dialect disagreement is only conservative
with an accurate statement that downstream compilation remains fail-closed when
parsing or compilation fails, while preserving the guidance to pass the active
warehouse dialect.
In `@docs/prune-ops.md`:
- Around line 281-289: The documented sampling policy must not treat two
candidates as an unconditional CTAS break-even. Update docs/prune-ops.md lines
281-289, plans/super/268-ingest-sample-scope.md lines 298-301 (DEC-010), and
.claude/rules/business-rule-tests.md lines 218-228 to define the chosen
cost-aware signal or explicit heuristic; remove the unconditional two-candidate
assertion in tests/prune/test_engine.py lines 6062-6067 and update
tests/prune/test_engine.py lines 6173-6213 to verify that policy boundary or
cost signal instead.
- Line 858: The `bypassed_to_source` documentation incorrectly lists
materialisation failures as `false`, although source fallback records `true`.
Update the table description to reserve `false` for failures that produce no
dispatch and blanket `kept-without-evidence`, while documenting
materialisation-failure source fallback as `true`.
In `@src/signalforge/ingest/reader.py`:
- Around line 696-704: Update the compiled-code sizing check around cc.encode in
the ingest reader to catch UnicodeEncodeError for invalid Unicode, then return
the same SkippedTest using test_name=label, test.column_name,
reason="malformed-supported-test", and _OVERSIZE_SKIP_DETAIL; preserve the
existing byte-length limit behavior for encodable values.
In `@src/signalforge/prune/engine.py`:
- Around line 1841-1844: Update the materialisation-failure handling around
ingested_plans to replace only plans that were previously samplable; preserve
existing bypass plans and their reasons for scalar, unparseable, or
multi-relation candidates. Keep the materialisation-failed reason for candidates
that reached sampling and then failed.
In `@tests/prune/test_ingested_rewrite_parse_guard.py`:
- Around line 128-133: Update the fixture inventory assertions in the test to
enumerate both `.out.sql` and `.in.sql` files, and assert that each suffix’s
discovered case names exactly matches the indexed `_CASE_NAMES`; retain the
existing minimum fixture-count validation and per-case file checks.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 9: Condense the verbose changelog entry into concise release-facing
outcomes: retain the supported sample-scope behavior, unchanged
full-scope/oneshot compatibility, materialisation cost impact, fallback
behavior, and audit/observability changes. Remove implementation gates, internal
routing details, failure mechanics, and test-specific narrative, and refer
readers to plans/super/268-ingest-sample-scope.md plus the relevant operational
documentation.
In `@src/signalforge/prune/compiler.py`:
- Around line 772-790: Extract the repeated determinism and comment-tolerant
safety validation into a shared helper near the relevant compiler logic,
preserving the existing validation order and QuerySyntaxError handling. Have the
helper accept the reason-string prefix or equivalent context so raw-body and
override callers retain their distinct _InvalidIdentifier reasons, then replace
both validation blocks with calls to it.
🪄 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: 123c681a-fb07-459d-b5ca-473b2d49aa4a
📒 Files selected for processing (45)
.claude/rules/business-rule-tests.md.claude/rules/ingest-layer.md.claude/rules/llm-drafter.md.claude/rules/prune-engine.mdCHANGELOG.mddocs/ingest-ops.mddocs/prune-ops.mdplans/super/268-ingest-sample-scope.mdsrc/signalforge/cli/prune_existing.pysrc/signalforge/ingest/_compiled_sql.pysrc/signalforge/ingest/reader.pysrc/signalforge/prune/audit.pysrc/signalforge/prune/compiler.pysrc/signalforge/prune/engine.pysrc/signalforge/prune/models.pytests/cli/_e2e_helpers.pytests/cli/test_e2e_bigquery_ingested_sample.pytests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_between.in.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_between.out.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_not_null.in.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_not_null.out.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_row_count_between.in.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_dbt_expectations_row_count_between.out.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_self_join.in.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_self_join.out.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_simple_where.in.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_simple_where.out.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_subquery_and_comment.in.sqltests/fixtures/prune/compiled_sql/ingested/bigquery_subquery_and_comment.out.sqltests/fixtures/prune/compiled_sql/ingested/databricks_dbt_expectations_not_null.in.sqltests/fixtures/prune/compiled_sql/ingested/databricks_dbt_expectations_not_null.out.sqltests/fixtures/prune/compiled_sql/ingested/index.jsontests/fixtures/prune/compiled_sql/ingested/snowflake_dbt_expectations_not_null.in.sqltests/fixtures/prune/compiled_sql/ingested/snowflake_dbt_expectations_not_null.out.sqltests/fixtures/prune/compiled_sql/ingested/snowflake_self_join.in.sqltests/fixtures/prune/compiled_sql/ingested/snowflake_self_join.out.sqltests/fixtures/prune/prune_event_v1.jsonltests/ingest/test_compiled_sql.pytests/ingest/test_manifest_tests.pytests/ingest/test_relation_rewrite.pytests/prune/test_audit.pytests/prune/test_compiler.pytests/prune/test_drift_detector.pytests/prune/test_engine.pytests/prune/test_ingested_rewrite_parse_guard.py
Code fixes: - ingest/reader: skip-record compiled_code that is not valid UTF-8 (a lone surrogate from a manifest JSON escape crashed the size-cap encode AND would resurface at compiled_sql_hash — a run-aborting bug, same class as US-001) [CR] - prune/engine: on materialisation-failure fallback, only demote SAMPLABLE plans to 'materialisation-failed'; a plan already refused (multi-relation/scalar/…) keeps its reason so the DEC-014 histogram reports the true cause [CR] - test parse-guard: enumerate orphan .in.sql fixtures too, not just .out.sql [CR] Doc/rules accuracy: - prune_existing.py + ingest-ops: the dialect-default safety argument is downstream fail-closed compilation, NOT 'gates only more conservative' — the gates are permissive on parse failure and could ADMIT a body [Copilot+CR] - CHANGELOG + prune-ops: audit truncation is a 1200 JSON-escaped-byte budget, not a 1000-character prefix [Copilot+CR] - prune-ops: bypassed_to_source is TRUE on the DEC-009 source-fallback (only the blanket no-compile degrade is false) [CR] - prune-ops: the >=2 gate is a coarse heuristic, not a break-even guarantee [CR] - ingest-layer: repair malformed MD038 inline-code span; prune-engine: reword garbled sentence [CR] Regression tests added for all three code fixes.
Owner
Author
PR Review SummaryAll 12 review threads (CodeRabbit + Copilot) addressed in 026cea6. No false positives — every finding was legitimate, including three real code issues. Fixed — code (3)
Fixed — docs/rules accuracy (9 threads)
Validation after fixes: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/ingest/test_manifest_tests.py`:
- Around line 605-624: The test docstring in
test_lone_surrogate_compiled_code_does_not_crash_the_size_cap describes the
wrong surrogatepass/sqlglot mechanism; update it to state that the size-cap
encoding catches UnicodeEncodeError and skip-records the candidate immediately.
Tighten the final assertion to require the specific malformed-supported-test
skip reason, while preserving the no-crash and single-skipped-result checks.
🪄 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: 45225e85-0234-491f-97b6-66e39bb08bc2
📒 Files selected for processing (11)
.claude/rules/ingest-layer.md.claude/rules/prune-engine.mdCHANGELOG.mddocs/ingest-ops.mddocs/prune-ops.mdsrc/signalforge/cli/prune_existing.pysrc/signalforge/ingest/reader.pysrc/signalforge/prune/engine.pytests/ingest/test_manifest_tests.pytests/prune/test_engine.pytests/prune/test_ingested_rewrite_parse_guard.py
🚧 Files skipped from review as they are similar to previous changes (9)
- src/signalforge/cli/prune_existing.py
- tests/prune/test_ingested_rewrite_parse_guard.py
- .claude/rules/ingest-layer.md
- docs/ingest-ops.md
- docs/prune-ops.md
- src/signalforge/ingest/reader.py
- CHANGELOG.md
- .claude/rules/prune-engine.md
- src/signalforge/prune/engine.py
- test docstring described the abandoned surrogatepass+downstream-gate mechanism; correct it to match reader.py (UnicodeEncodeError caught + skip-recorded at the size-cap step, because a surrogate body would else resurface at compiled_sql_hash) - tighten the assertion to pin reason == 'malformed-supported-test'
Owner
Author
|
Fixed in bc870f0: the surrogate test's docstring now matches |
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 #268 — enable
--scope=samplefor manifest-ingested dbt tests by rewriting the model's own relation in dbt'scompiled_codevia sqlglot AST analysis instead of string substitution.Phase: detailing (awaiting approval)
Stories: 9 implementation + Quality Gate + Patterns & Memory
Decisions: 16 (DEC-001 … DEC-016)
What the review found
An empirical sqlglot prototype (30.2.1, run against the real fixtures) de-risked the mechanism — but the architecture review turned up five blockers, three of which are latent bugs in the #154 code that already shipped:
ast_count == span_countinvariant is defeatable.select proj.ds.tbl.c from+ "proj.ds.tbl" +FROMon production. A dotted CTE alias shadowing the relation likewise gets rewritten to the sample. Both end in a real test being silently deleted.from_manifestarm never readstable_reftoday, so narrowing the engine gate alone would full-scan production while recording an evidence-backed verdict atscope="sample".RecursionErrorescapes the sqlglot guards and aborts the whole prune run; there is no size cap oncompiled_code; and a real ~1.8KB dbt-expectations body already blows the 4000-byte audit cap (the body is serialised twice) → exit 3, run aborted mid-batch.All have accepted mitigations recorded as DECs.
Scope calls
prune/— locate in ingest (analysis only), splice in the compiler. No 4th importer, so no confinement scan is owed.oneshotkeeps bypassing to source (the CTE alternative failed on execution). This deviates from the ticket's stated acceptance — theoneshothalf is a follow-up.Plan document
See
plans/super/268-ingest-sample-scope.md.Next steps
Summary by CodeRabbit
New Features
custom_sqltests can now run under--scope=sampleby routing to materialized samples when rewrite safety, batching, and cost gates pass.Bug Fixes
Documentation