#159: drafter column-type awareness - #161
Merged
Merged
Conversation
Plan for closing the test_e2e_business_rules flake where the drafter emits type-incoherent custom_sql (INT64 vs STRING comparison), prune correctly routes to kept-without-evidence, and the test asserts kept-with-evidence. Root cause: tests/fixtures/dbt_project_austin/target/manifest.json carries data_type: null for every column. The drafter / safety / prompt pipeline already supports types end-to-end — the gap is upstream. Scope (DEC-001): A + B + D. A. populate data_type in the Austin fixture B. merge target/catalog.json types into Column.data_type at load time D. add sqlglot AST type-coherence check in _validate_anchor_contract 13 decisions captured (DEC-001 … DEC-013). 4 implementation stories + Quality Gate + Patterns & Memory. No new CLI flag, no new config knob, no _PROMPT_VERSION rotation, no new error class — fix lives inside existing extensibility seams. 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:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@plans/super/159-drafter-column-types.md`:
- Around line 299-309: The duplicate markdown heading "Beads manifest (Phase 7 —
pending)" appears twice; remove the redundant block so only a single "Beads
manifest (Phase 7 — pending)" heading remains, ensuring you keep the intended
content under the retained heading and delete the other repeated heading and its
empty spacer lines to satisfy MD024 and avoid confusion.
- Line 233: The "Done when" acceptance criterion for US-003 conflicts with its
dependencies (US-001 and US-002) because it requires the git diff to show ONLY
two fixture file changes while the dependencies introduce non-fixture changes;
update the US-003 acceptance criterion text (the "Done when" line referencing
fixture-only diffs) to be story-local and unambiguous (e.g., "US-003 scoped
commit" or "fixture files changed in this story step") and/or remove
US-001/US-002 from US-003's dependency list so the criterion no longer conflicts
with dependent work; ensure you update the US-003 description and dependency
list consistently in the document.
🪄 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: ee54596d-0b81-4256-b885-d0b82185fc8c
📒 Files selected for processing (1)
plans/super/159-drafter-column-types.md
…for Column.data_type (#159) Implements US-001 of the #159 plan (DEC-001, DEC-002, DEC-007, DEC-010). The manifest loader now reads a sibling 'target/catalog.json' when present and overlays its per-column 'type' onto Column.data_type via the frozen-model 'model_copy(update={...})' pattern. Case-insensitive column matching covers Snowflake (uppercased identifiers) and BigQuery (preserved-case) catalogs. Missing / malformed / unreadable catalog → silent no-op (stage-0 invariant); phantom columns are dropped; manifest columns absent from the catalog keep data_type=None. The catalog path is canonicalised through signalforge._common.path_safety.canonicalise_path, so a symlink escape raises PathContainmentError (security gate is NOT in the silent-degrade set). Tests: 19 new cases under tests/manifest/test_loader.py covering the 8 TDD acceptance cases plus parametrised shape-degrade guards. Fixtures under tests/fixtures/manifest/ (canonical/case-mismatch/phantom/partial catalogs + a minimal manifest carrying three columns).
…ce for custom_sql (#159) Add a sqlglot-based type-coherence check to _validate_anchor_contract in signalforge.draft.parser. For each custom_sql test, parse the SQL with sqlglot.parse_one(dialect=...), annotate types, and append violations for direct Column <op> Column comparisons where both known types are incompatible. Skip silently on every other shape per DEC-006 (Cast / SafeCast / Coalesce / function call / subquery / literal / NULL / window / unparseable SQL / unknown types). A {{ this }} / {{ ref(...) }} placeholder is substituted with a stable identifier before parsing so sqlglot can read the surrounding SQL — the parser defence runs pre-resolution and the LLM almost always references the model via Jinja. Bidirectional COERCES_TO compatibility lets numeric-family coercions (INT64↔FLOAT64, NUMERIC↔BIGNUMERIC) pass while INT64 vs STRING / DATE is flagged. Type violations append to the existing LLMOutputAnchorContractError.violations list (DEC-011) — no new error subclass. parse_draft_response gains two keyword-only params: - model_columns_by_type: Mapping[str, str | None] | None = None - dialect_name: str = 'bigquery' draft_from_request builds the type map from model.columns_list and threads through; v0.1 hard-codes 'bigquery' with a TODO referencing v0.2 multi-warehouse work (DEC-013). sqlglot promoted to a direct runtime dep (sqlglot>=30,<31) in [project].dependencies + [project.optional-dependencies].dev + [dependency-groups].dev (mirror per python-build.md). Previously a dev-only transitive via fakesnow; type-defence correctness is load-bearing. Confinement: sqlglot imports live ONLY in src/signalforge/draft/parser.py (DEC-008). Convention only in v0.1; no AST scan. Traces to DEC-001 / DEC-003 / DEC-005 / DEC-006 / DEC-008 / DEC-011 / DEC-012 / DEC-013 in plans/super/159-drafter-column-types.md. 17 new tests in tests/draft/test_parser.py: - 3 planted positives (int64 vs string × 2 ops, int64 vs date) - 9 planted negatives (numeric coercion × 2, cast / safe_cast / coalesce / null / literal / function / subquery) - 5 robustness (unparseable, both-unknown, partial-unknown, None-map-skips-arm, collect-all-with-structural-violation) All 41 tests in tests/draft/test_parser.py pass; full canonical validation (ruff check + ruff format + pyright + pytest) green with 2604 tests passing and 97.44% coverage.
…alog.json (#159) Populate real BigQuery data_type values on every column of stg_bikeshare_trips in the Austin manifest fixture and ship a sibling target/catalog.json with the same types so US-001's catalog-merge read-path is exercised end-to-end by the e2e smoke. Types verified via 'bq show --schema bigquery-public-data:austin_bikeshare.bikeshare_trips': - trip_id, subscriber_type, bike_id, end_station_id STRING - start_time TIMESTAMP - start_station_id, duration_minutes INT64 end_station_id is STRING but start_station_id is INT64 — exactly the type mismatch the original #159 BQ error 'No matching signature for operator != for argument types: INT64, STRING' flagged in the production prune.jsonl. DEC-008 of #47 (demo-fixture parity) requires the shipped src/signalforge/_demo/ tree stay byte-equal to the e2e fixture except for the two documented rewrites; both manifest.json and the new catalog.json are mirrored into _demo/target/ in lockstep. Traces to DEC-001 sub-option A + DEC-004 in plans/super/159-drafter-column-types.md.
…ess (#159) Documents the two new behaviours added in US-001 (catalog.json sibling merge) and US-002 (sqlglot parser type-coherence defence), per cli-layer.md "Multi-surface parity for behaviour changes". Surfaces touched: - .claude/rules/manifest-readers.md — new "Catalog.json sibling merge" section: 5 load-bearing rules, no _PROMPT_VERSION rotation rationale, no drift detector required. - .claude/rules/llm-drafter.md — new "Sqlglot type-coherence check" subsection under "Whole-draft fail-loud anchor contract": dual-defence framing (prompt + parser), 6 load-bearing rules incl. skip-when-uncertain + Jinja substitution + ParseError silent skip + sqlglot confinement. - docs/manifest-loader-ops.md — operator-facing "Column types from catalog.json" section: how to run dbt docs generate, failure modes (all silent), refresh notes. - docs/draft-ops.md — operator-facing "Type-coherence defence" section inserted after "Hard JSON validation + anchor-contract": what it catches, what it skips, threading API, dialect support, dep note. - CHANGELOG.md — one Added entry (catalog.json merge), one Fixed entry (sqlglot parser defence). Validation: ruff/format/pyright/pytest all green; 2623 passed, 97.46% coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QG review pass 1 (low effort) surfaced one finding: docs/draft-ops.md § "Type-coherence defence" claimed the check covers "plus IN-list comparisons" but parser.py:_check_custom_sql_type_coherence only matches (EQ, NEQ, GT, LT, GTE, LTE) — no exp.In handler. Per DEC-006 skip-when-uncertain, removing the IN claim is the right fix (matches actual conservative behaviour); adding IN handling is deferred to a follow-up if real-world drift demands it. Passes 2 (medium), 3 (high), and 4 (max) review surfaced no additional real-bug findings. The implementation honors all 13 #159 DECs: - DEC-008 sqlglot confined to signalforge.draft.parser (grep verified) - DEC-006 skip-when-uncertain (Cast/SafeCast/Coalesce/IfNull/function/ subquery/literal/NULL/window/unknown-type/parse-error all skip) - DEC-009 no _PROMPT_VERSION rotation - DEC-011 violations append to existing LLMOutputAnchorContractError - DEC-012/013 model_columns_by_type + dialect_name kwargs threaded - DEC-002/007/010 catalog.json sibling lookup, case-insensitive, silent degradation CodeRabbit skill not registered in this session; manual review only. Canonical validation green: ruff/format/pyright all clean; pytest 2623 passed, 97.46% coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
wjduenow
marked this pull request as ready for review
May 29, 2026 16:08
wjduenow
changed the title
There was a problem hiding this comment.
Pull request overview
This PR implements column-type awareness to reduce flaky custom_sql drafting by (1) populating Column.data_type from target/catalog.json when available and (2) adding a sqlglot-based parser defence that rejects obviously type-incoherent Column <op> Column comparisons in drafted custom_sql.
Changes:
- Merge sibling
target/catalog.jsontypes into the loaded dbt manifest’sColumn.data_typefields (silent degradation on missing/malformed catalog; path-safety enforced). - Add sqlglot-based type-coherence validation for drafted
custom_sql, threaded via amodel_columns_by_typemap fromdraft_from_request. - Update fixtures/docs/changelog and add extensive unit/integration tests for the new behaviours.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Promotes/pins sqlglot in the lock to support runtime parsing/validation. |
pyproject.toml |
Adds sqlglot>=30,<31 to runtime + dev dependency sets. |
src/signalforge/manifest/loader.py |
Applies a sibling catalog.json overlay to populate Column.data_type. |
tests/manifest/test_loader.py |
Adds integration tests covering catalog overlay happy-path + degradation/security cases. |
tests/fixtures/manifest/manifest_with_columns.json |
New manifest fixture with columns to validate overlay behaviour. |
tests/fixtures/manifest/catalog_canonical.json |
New catalog fixture (canonical types). |
tests/fixtures/manifest/catalog_case_mismatch.json |
New catalog fixture to validate case-insensitive column matching. |
tests/fixtures/manifest/catalog_phantom_column.json |
New catalog fixture ensuring phantom catalog columns are ignored. |
tests/fixtures/manifest/catalog_partial.json |
New catalog fixture ensuring missing catalog columns leave data_type=None. |
src/signalforge/draft/parser.py |
Adds sqlglot-based custom_sql type-coherence checks and threads new kwargs through validation. |
tests/draft/test_parser.py |
Adds unit tests for type-coherence violations and skip-when-uncertain shapes. |
src/signalforge/draft/schema.py |
Builds model_columns_by_type from manifest columns and passes dialect/type map into the parser. |
tests/fixtures/dbt_project_austin/target/manifest.json |
Populates data_type for Austin e2e fixture columns. |
tests/fixtures/dbt_project_austin/target/catalog.json |
Adds sibling catalog fixture for the Austin project. |
src/signalforge/_demo/target/manifest.json |
Populates data_type values in the demo manifest fixture. |
src/signalforge/_demo/target/catalog.json |
Adds sibling catalog fixture for the demo project. |
docs/manifest-loader-ops.md |
Documents catalog-driven type sourcing in the manifest loader. |
docs/draft-ops.md |
Documents the parser-side type-coherence defence and skip policy. |
CHANGELOG.md |
Adds release notes for catalog merge + type-coherence rejection. |
.claude/rules/manifest-readers.md |
Records the catalog overlay conventions and invariants (stage-0, silent degrade, path safety). |
.claude/rules/llm-drafter.md |
Documents the dual-defence pattern and sqlglot confinement convention. |
plans/super/159-drafter-column-types.md |
Adds the detailed plan/DEC log for #159. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ence (#159) Codecov flagged 11 missing patch-coverage lines in src/signalforge/draft/ parser.py — all in `_check_custom_sql_type_coherence` and its model-level call site: - 121: `_types_compatible` `a == b` short-circuit (same-type case) - 126: reverse-direction `a in COERCES_TO[b]` branch - 168-171: defensive `except sqlglot.errors.SqlglotError` (non-ParseError) - 174: `if parsed is None: return ()` (parse_one can return None) - 184-187: defensive `except Exception` around `annotate_types` - 209-210: defensive `except Exception` around `DataType.build` - 353-354: model-level custom_sql type-coherence call site Per memory `qg-pass-3-defer-defensive-tests-fails-codecov` ("if a Pass-3 nice-to-have covers lines inside the patch diff, upgrade it to must-fix; codecov holds patch coverage to project standard regardless"), these defensive branches need explicit tests rather than skipping them. 7 new tests pin each branch: - test_custom_sql_same_type_comparison_skipped (121) - test_custom_sql_reverse_coerce_direction_accepted (126) - test_check_custom_sql_type_coherence_sqlglot_non_parse_error_skipped (168-171, monkeypatch) - test_check_custom_sql_type_coherence_parser_returns_none_handled (174, monkeypatch — parse_one returning None varies by sqlglot release) - test_check_custom_sql_type_coherence_annotate_types_failure_skipped (184-187, monkeypatch) - test_custom_sql_invalid_type_string_skipped (209-210, opaque type string) - test_model_level_custom_sql_type_mismatch_is_rejected (353-354) Mix of organic-input tests where possible (lines 121/126/209-210/353-354) and monkeypatch tests for the sqlglot-internals defensive catches that can't be triggered organically without mocking (168-171/174/184-187). Imports the private `_check_custom_sql_type_coherence` helper alongside `_LLMResultMeta` per the existing pattern (private-under-test). Result: parser.py at 100% coverage (was 92% / 11 missing). Full project coverage up to 97.61% (from 97.46%). 2630 tests pass (+7). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…chema (#159) Codecov flags any uncovered line in a file modified by the PR — even pre-existing defensive branches that #159 never touched. The catalog overlay landed in loader.py and the type-coherence kwargs threaded into schema.py, so both files surfaced their pre-existing gaps on the PR view. Closing those completes the project-coverage picture for #159. schema.py (2 lines): - test_draft_from_request_record_too_large_propagates_typed (line 250): pin that LLMResponseAuditRecordTooLargeError raised by write_response_ event is re-raised as-is, NOT wrapped under LLMResponseAuditWriteError - test_draft_from_request_keyboard_interrupt_propagates_untouched (line 254): pin that a Ctrl-C signal mid-audit-write propagates with its identity intact — never silently demoted to an audit error loader.py (6 lines): - test_load_project_dir_is_a_file_raises_manifest_not_found (line 231): non-directory project_dir → ManifestNotFoundError (distinct from FileNotFoundError caught one branch up) - test_load_non_dict_metadata_raises_manifest_error (line 283): manifest with metadata as a non-dict → ManifestError before version-sniff - test_load_disabled_with_non_list_value_continues (line 308): defensive shape-drift skip in the disabled-models loop - test_schema_version_non_string_returns_empty_string (line 485): the free-function schema_version() returns "" for a non-string version - test_get_model_resolver_index_cache_hit_returns_cached (line 598): the in-memory resolver-index cache is reused on second get_model call - test_get_model_by_file_path_for_disabled_model_raises_disabled (line 679): file-path lookup for disabled model raises ModelDisabledError (the path-lookup branch needs the same disabled detection the unique_id branch already has) Result: - src/signalforge/draft/parser.py: 100% (was 100%) - src/signalforge/draft/schema.py: 100% (was 96%, 2 missing) - src/signalforge/manifest/loader.py: 100% (was 97%, 6 missing) - Project coverage: 97.72% (was 97.61%) - 2638 tests pass (+8) Per memory `qg-pass-3-defer-defensive-tests-fails-codecov`. Most tests use monkeypatch or carefully-crafted JSON fixtures to hit the precise defensive branch; lines that can be exercised organically (231, 283, 598, 679) use real Manifest construction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses all 5 unresolved review comments on PR #161: Thread 1 (CodeRabbit, plan US-003 "Done when"): - Rewrote the criterion to be story-local: scopes only THIS story's own commit, not the cumulative diff against the base branch. Adds the demo mirror files explicitly (the DEC-008 of #47 parity gate the original scope missed). Clarifies why US-001 + US-002 are real dependencies. Thread 2 (CodeRabbit, duplicate "Beads manifest" heading): - Already fixed during devolve (Phase 7 cleanup committed earlier); only one heading remains at line 302. No code change. Thread 3 (Copilot, test_loader.py Windows skipif chain): - `@pytest.mark.skipif(os.geteuid() == 0, ...)` ran at collection time on Windows even when the prior `sys.platform == "win32"` skipif fired, because pytest evaluates ALL skipif predicates. On Windows os.geteuid doesn't exist → AttributeError breaking collection. Combined into a single skipif with `hasattr(os, "geteuid")` short-circuit. Thread 4 (Copilot, docs/manifest-loader-ops.md "all silent" framing): - Added the path-containment caveat: catalog.json that escapes the project via symlink raises PathContainmentError loud — the one non-silent failure mode. Reworded section header from "all silent" to "all silent except the path-safety gate". Thread 5 (Copilot, CHANGELOG mechanism wording): - The previous wording implied sqlglot's annotator received a `schema=` kwarg. The actual mechanism walks binary comparison nodes and looks each operand's column name up in the model's data_type map directly, then tests via TypeAnnotator.COERCES_TO. Reworded to match. Validation: ruff + format + pyright + pytest all green; 2638 tests pass, 97.72% coverage. No production-code changes (docs / plan / test decorator only). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review Summary — 5 threads addressedCommit: Fixed (5 items)
False positivesNone — every finding was a real issue. Validation
|
This was referenced May 30, 2026
Merged
Merged
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 #159 —
test_e2e_business_rulesflake where the drafter emits type-incoherentcustom_sql(INT64 vs STRING), prune correctly routes tokept-without-evidence, and the test assertskept-with-evidence.Phase: detailing (awaiting approval)
Stories: 4 implementation + Quality Gate + Patterns & Memory
Decisions: 13 (DEC-001 … DEC-013)
Root cause
The Austin fixture's
manifest.jsoncarriesdata_type: nullfor every column. The drafter / safety / prompt pipeline already supports column types end-to-end (prompts.py:347rendersColumn.data_type or "UNKNOWN";safety/request.py:161flows it into the dynamic block). The gap is upstream — nothing populatesdata_typefor this fixture, anddbt parsedoesn't populate it for any project; onlydbt docs generate(which producescatalog.json) carries types.Scope (DEC-001) — A + B + D
loader.py+ 8 tests + 3 fixturesparser.py+schema.py+pyproject.toml+ 17 testsdata_type+ addcatalog.jsonKey load-bearing facts surfaced in architecture review
fakesnow), NOT viadbt-core. Promoting to runtime is a real ~15MB dependency add forsignalforge-dbtPyPI users; DEC-005 pinssqlglot>=30,<31in[project].dependencies._PROMPT_VERSIONrotation needed (DEC-009). The version is a template hash; per-project rendered-byte variation has always been allowed. The cache-stability golden usesfct_orders(already typed); unaffected.LLMOutputAnchorContractError.violationstuple.Plan document
Full plan:
plans/super/159-drafter-column-types.mdNext steps
🤖 Generated with Claude Code
Summary by CodeRabbit
#159outlining planned improvements to column-type support in the LLM drafter, including detailed specifications, test cases, and acceptance criteria for upcoming enhancements.