#159: drafter column-type awareness

.claude/rules/llm-drafter.md

@@ -109,6 +109,24 @@ For model-level `candidate.tests`, only the `test.column in model_columns` check
 
 Don't change the validator to short-circuit on the first violation — the goal is "tell the operator everything wrong in one error so they can fix in one round."
 
+### Sqlglot type-coherence check for `custom_sql` (issue #159)
+
+The `custom_sql` test variant (`business-rule-tests.md`) is a full singular-test SELECT, so a drafted SQL like `WHERE start_station_id <> end_station_id` can be type-incoherent against the model's real warehouse schema (one column INT64, the other STRING) — the LLM had no type info on the input and guessed. BigQuery rejects the query and the prune engine routes it through `_InvalidIdentifier` → `kept-without-evidence` per the conservative-bias contract, but the operator gets no positive evidence for a rule that is wrong-on-its-face. Issue #159 closes this with a dual-defence pattern that mirrors `exclude_tests` (prompt + parser):
+
+- **Prompt-side.** Once `Column.data_type` is populated (via dbt `manifest.json` directly OR via the `catalog.json` sibling merge that `manifest-readers.md` § "Catalog.json sibling merge" added in #159), the cached manifest summary renders `- {name} ({data_type}): {description}` and the dynamic data section renders `(display_name, type_str)` pairs from `LLMRequest.schema`. A cooperative LLM sees the types and emits type-coherent SQL.
+- **Parser-side.** `_validate_anchor_contract` gains two keyword-only params: `model_columns_by_type: Mapping[str, str | None] | None = None` and `dialect_name: str = "bigquery"`. The new private helper `_check_custom_sql_type_coherence(sql, model_columns_by_type, dialect_name) -> tuple[str, ...]` parses the SQL via `sqlglot.parse_one(dialect=dialect_name)`, annotates types via `sqlglot.optimizer.annotate_types(schema=...)`, walks binary comparison nodes (`<>`, `=`, `<`, `>`, `<=`, `>=`), and flags violations where two known-type column references compare incompatible types. Violations append to the same `LLMOutputAnchorContractError.violations` tuple — no new error subclass (DEC-011 of #159; saves a CLI exit-code-table entry and a 7th-AST-scan exclusion update).
+
+Six load-bearing rules:
+
+1. **Skip-when-uncertain is the load-bearing surface.** The check flags ONLY direct `Column <op> Column` AST nodes where both operands are bare `sqlglot.exp.Column` (NOT `Cast`, `SafeCast`, `Coalesce`, `IfNull`, function calls, subqueries, literals, `Null`, window functions), both columns appear in the schema map with non-None `data_type`, and the two types are NOT in sqlglot's `TypeAnnotator.COERCES_TO`-compatible set for the dialect (bidirectional check; NUMERIC↔BIGNUMERIC maps to DECIMAL↔BIGDECIMAL via DOUBLE/DECFLOAT/FLOAT). Every other shape skips silently. Zero false-positives on legitimate SQL is the contract; missing some real bugs is the acceptable tradeoff (the prune engine's `kept-without-evidence` routing remains the safety net for cases the parser can't catch).
+2. **Jinja substitution before parse.** LLM-emitted `custom_sql` almost always references `{{ this }}`; raw `sqlglot.parse_one` would raise `ParseError` and the entire defence would skip-silently for every candidate. The helper swaps `{{ ... }}` → a placeholder identifier (`__sf_jinja__`) via a regex pass BEFORE `parse_one`, keeping the WHERE-clause comparison nodes intact (which is all the walker consumes). The placeholder is unused downstream; the prune compiler's own `resolve_template_refs` (`manifest.py`) handles real Jinja resolution at compile time.
+3. **`sqlglot.errors.ParseError` skips silently.** Invalid SQL is caught downstream by the warehouse adapter (`QuerySyntaxError` → `kept-without-evidence`). Raising from the parser here would mask the warehouse-side message without adding signal.
+4. **Collect-all invariant preserved.** Type violations append to the existing `violations` list; never short-circuit. A candidate with BOTH a hallucinated column AND a type mismatch produces BOTH violations in one round.
+5. **sqlglot imports confined to `signalforge.draft.parser` (DEC-008 of #159).** Convention, not AST scan (v0.1 has one consumer; future scan when a second module reaches for sqlglot). Verify via `grep -rn "^import sqlglot\|^from sqlglot" src/signalforge/` — hits only in `parser.py`. sqlglot is a real runtime dep pinned at `sqlglot>=30,<31` in `[project].dependencies` AND `[project.optional-dependencies].dev` (it was a dev-only transitive via `fakesnow` pre-#159; promoting to runtime is a deliberate ~15MB add for `signalforge-dbt` PyPI users).
+6. **No `_PROMPT_VERSION` rotation (DEC-009 of #159).** The version is a template-text hash; `_check_custom_sql_type_coherence` runs in the parser, NOT the prompt-builder. The cache-stability golden uses the `fct_orders` fixture (already typed); unaffected. **Do not touch `_PROMPT_VERSION` or `tests/llm/test_prompt_cache_stability.py` for type-coherence work — they are orthogonal.**
+
+`parse_draft_response` and `draft_from_request` thread `model_columns_by_type` (built from `model.columns_list`) and `dialect_name` (hardcoded `"bigquery"` at the orchestrator with a v0.2 TODO for multi-warehouse — `Dialect` isn't passed because it would force a cross-stage import from the warehouse layer; the string sidesteps that). When v0.2 adds Snowflake/Postgres at the warehouse layer, source the dialect name from the safety policy or adapter (TODO in `schema.py`).
+
 ## `exclude_tests` dual-defence: prompt + parser (issue #54)
 
 `DraftConfig.exclude_tests: tuple[str, ...] = ()` lets the operator suppress one or more dbt test types from drafting entirely. Four valid entries pinned by `VALID_TEST_TYPES`; config-load validates each entry so a typo like `"not_nul"` fails loud at YAML-load.

.claude/rules/manifest-readers.md

@@ -68,6 +68,24 @@ The drift-detector pattern (one-off `extra="forbid"` strict model paired with a
 
 When v0.3 adds a sibling grammar (e.g. `--filter <expr>` for column-level filters, `--exclude <expr>` for set subtraction), copy this module shape: parser + matcher + atom-union + domain error in the manifest layer; CLI-tier-2 wrapper in the CLI layer; `extra="forbid"` everywhere on the user-input typed surface.
 
+## Catalog.json sibling merge for `Column.data_type` (issue #159)
+
+`signalforge.manifest.load(project_dir)` looks for a `catalog.json` sibling next to the resolved `manifest.json` (`<resolved_manifest>.parent / "catalog.json"`) and, when present, merges its column types into `Column.data_type` on the in-memory `Manifest`. `dbt parse` does NOT populate `data_type`; only `dbt docs generate` (which produces `catalog.json`) carries types. The merge closes that gap for any user who runs the full dbt build, so the drafter's prompt — which already renders `data_type` into the cached manifest summary (`llm-drafter.md` § "Cached-block scope") and the dynamic data section (`safety-layer.md` § PII redaction → `LLMRequest.schema`) — sees real warehouse types instead of `UNKNOWN`.
+
+Five load-bearing rules — match this shape for any future "dbt-adjacent target/ file" the loader merges:
+
+1. **Sibling path goes through the same symlink-hardened canonicalisation as the manifest itself.** Compute `catalog_path = resolved_manifest.parent / "catalog.json"` then route through `signalforge._common.path_safety.canonicalise_path(catalog_path, project_resolved)`. `PathContainmentError` propagates out of `load()`. The "default" sibling path is NOT exempt from the gate (DEC-007 of #2 generalised — "default paths must go through the same gate as overrides" applies to siblings too).
+2. **Silent degradation on absent / malformed / unreadable catalog.** Catalog absent → no-op; `OSError` / `json.JSONDecodeError` on read → no-op; non-dict root / non-dict `nodes` / non-dict `columns` / non-dict per-column entry → no-op; missing or non-string `type` field → no-op. The loader is stage-0; emitting a log or raising for a stale catalog is the wrong UX (it punishes the user for forgetting `dbt docs generate`, which is not a SignalForge concern). The drafter sees `data_type=None` and renders "UNKNOWN" — same as today's no-catalog world.
+3. **Case-insensitive column matching keyed on `lower(col_name)`.** dbt's `catalog.json` mirrors the warehouse's identifier convention: Snowflake uppercases (`USER_ID`), BigQuery preserves (`user_id`), Postgres lowercases (`user_id`). Manifest preserves whatever dbt parsed. Building the catalog index as `{lower(col_name): data_type}` and matching the manifest column by `lower(name)` is a strict superset across all three — a BigQuery match is exact under lower-fold; Snowflake's upper-fold matches manifest's lower; Postgres is identity. No `Manifest.metadata.adapter_type` branching needed.
+4. **Phantom catalog columns are silently dropped; manifest columns absent from catalog stay `data_type=None`.** A catalog that declares a column not present on the manifest's `Model.columns` is NOT used to add a phantom column — manifest is the source of truth for "what columns exist." A manifest column with no matching catalog entry keeps `data_type=None` (renders as "UNKNOWN" in the drafter prompt). Neither shape is an error.
+5. **Frozen-model overlay via `model_copy(update=...)`.** `Column`, `Model`, `Manifest` are all `frozen=True`. The overlay rebuilds bottom-up: `Column.model_copy(update={"data_type": cat_type})` → new `columns` dict → `Model.model_copy(update={"columns": new_columns})` → new `nodes` dict → `Manifest.model_copy(update={"nodes": new_nodes})`. Re-stash `_PROJECT_DIR_ATTR` on the new Manifest instance (resolver-index cache is intentionally dropped and rebuilt lazily). Skip the whole rebuild — return the original `manifest` — when `model_changed=False` for every model (no overlay applied → byte-equal output).
+
+**No `_PROMPT_VERSION` rotation needed when catalog types start flowing in.** Per `llm-drafter.md` § "Cached-block scope" + DEC-009 of #159, `_PROMPT_VERSION` is a template-text hash (`blake2b(_SYSTEM_PROMPT + _MANIFEST_SUMMARY_TEMPLATE + _DATA_SECTION_TEMPLATES_JSON)`); per-project rendered-byte variation has always been allowed (different manifests → different output). Populating `data_type` from catalog.json is per-project content variation, not a template change. The cache-stability golden uses the `fct_orders` fixture (which already has populated types); it is unaffected.
+
+**No drift detector required.** `Column.data_type: str | None = None` predates #159; the drift-detector pattern is mandatory only when a NEW field lands on a read-back model with `extra="ignore"`. The catalog overlay sets an existing field; no shape change.
+
+When the operator wants types but does NOT run `dbt docs generate` (e.g. a dbt-parse-only CI step), the operator-facing answer is "run `dbt docs generate` once and commit `target/catalog.json` or add it to the SignalForge fixture set." The library never reaches into the warehouse adapter for types from the manifest layer; that path stays stage-0 deterministic.
+
 ## Reference
 
-`plans/super/2-manifest-loader.md` — DEC-001, DEC-007, DEC-008, DEC-013, DEC-014, DEC-017. `plans/super/37-multi-model-select.md` — DEC-001, DEC-012, DEC-016 (selector grammar additions). `src/signalforge/manifest/loader.py` — current implementation of all three traps. `src/signalforge/manifest/select.py` — issue-#37 selector module (parse_selector / select_models / SelectorAtom).
+`plans/super/2-manifest-loader.md` — DEC-001, DEC-007, DEC-008, DEC-013, DEC-014, DEC-017. `plans/super/37-multi-model-select.md` — DEC-001, DEC-012, DEC-016 (selector grammar additions). `plans/super/159-drafter-column-types.md` — DEC-001, DEC-002, DEC-007, DEC-009, DEC-010 (catalog.json sibling merge). `src/signalforge/manifest/loader.py` — current implementation of all three traps + the `_apply_catalog_overlay` helper. `src/signalforge/manifest/select.py` — issue-#37 selector module (parse_selector / select_models / SelectorAtom).

CHANGELOG.md

@@ -6,11 +6,13 @@ All notable changes to SignalForge are documented here. The format is loosely ba
 
 ### Added
 
+- **Column-type awareness for the drafter (#159).** `signalforge.manifest.load(project_dir)` now auto-merges column types from a sibling `target/catalog.json` (produced by `dbt docs generate`) into `Column.data_type` on the in-memory `Manifest`. The drafter's prompt — cached manifest summary AND dynamic data-section schema — both already rendered `data_type` when present; populating it from catalog.json closes the dbt-parse-only gap so cooperative LLMs see real warehouse types (`INT64`, `STRING`, `TIMESTAMP`, …) instead of `UNKNOWN`. No CLI flag, no config knob — pure sibling auto-discovery; missing or malformed catalog degrades silently. Case-insensitive column matching (`lower(col_name)`) handles Snowflake's uppercase / BigQuery's preserve / Postgres's lowercase identifier conventions without configuration.
 - **OpenAI as a grading + drafting provider (#136).** Set `grade.provider: openai` or `llm.provider: openai` in `signalforge.yml`; requires the `[openai]` install extra and `OPENAI_API_KEY`. Ships four pricing SKUs (`gpt-4o`, `gpt-4o-mini`, `gpt-4.1`, `gpt-4-turbo`); `--estimate` works via tiktoken (no extra API round-trip). Server-side JSON enforcement via `response_format={"type": "json_object"}`. v0.3 ships without prompt caching (no Anthropic-style cache discount); follow-up to evaluate OpenAI prompt caching.
 - **Google Gemini as a grading + drafting provider (#137).** Set `grade.provider: gemini` or `llm.provider: gemini` in `signalforge.yml`; requires the `[gemini]` install extra (`pip install signalforge-dbt[gemini]`) and `GOOGLE_API_KEY`. Recommended SKU for both drafter and judge is `gemini-2.5-flash` (also registered: `gemini-2.5-pro`, `gemini-2.0-flash`). Server-side JSON enforcement via `response_mime_type="application/json"`. `--estimate` cost-preview is wired through Gemini's native `client.models.count_tokens` (US-007 of #137; DEC-016) — first-party token counter, one extra API round-trip per estimate, comparable to the Anthropic shape. Ships **without prompt caching** in v0.3 — `LLMProvider` strategy reports `supports_prompt_caching=False` / `supports_token_count=False`, so `call_llm` skips the `cache_control` marker, the `extended-cache-ttl` beta header, and the pre-send `count_tokens` gate; budget per-call cost accordingly under `provider: gemini` (especially for the grader's 4-criterion × ~12-artifact fan-out). Explicit Gemini context caching is tracked as a follow-up.
 
 ### Fixed
 
+- **Drafter rejects type-incoherent `custom_sql` business-rule tests at parse time (#159).** `_validate_anchor_contract` gains a sqlglot AST type-coherence check: for each `custom_sql` candidate, parse the SQL via `sqlglot.parse_one(dialect="bigquery")`, walk binary comparison nodes (`<>`, `=`, `<`, `>`, `<=`, `>=`), look each operand's column name up in the model's `Column.data_type` map, and for the two declared type strings test compatibility via sqlglot's `TypeAnnotator.COERCES_TO` table (bidirectional). When both types are known and incompatible (e.g. `INT64` vs `STRING`) a violation is appended; otherwise the check skips silently. Note the mechanism: the schema map is the lookup, NOT a `schema=` kwarg fed to sqlglot's annotator. The check is the parser-side belt-and-braces of a dual-defence with the type-aware prompt (catalog.json merge above); violations join the existing `LLMOutputAnchorContractError.violations` tuple — no new error class. Skip-when-uncertain policy: only bare `Column <op> Column` is flagged; `CAST` / `SAFE_CAST` / `COALESCE` / `IFNULL` / function calls / subqueries / literals / `NULL` / window functions / unknown-type columns / parse errors all skip silently (zero false-positives on legitimate SQL is the contract; the prune engine's `kept-without-evidence` routing remains the safety net). sqlglot promoted from a dev-only transitive to a runtime dep, pinned at `sqlglot>=30,<31` in `[project].dependencies`.
 - **`--estimate` grader-side token counts no longer double-count the rubric (#136 US-008 QG).** The pre-US-005 inline Anthropic call passed the rubric in BOTH the `system=` kwarg AND embedded in the cached user-content block, counting it twice per criterion. The first QG fix preserved that for Anthropic byte-identity, which then triple-counted the rubric for OpenAI (system→`system + text` concat → rubric prefix in text). Corrected to match the runtime grader call: rubric in `system=` once, artifact envelope in user content. Real-API `--estimate` figures for the grade-side shift down by ~one rubric per criterion (was: bug → over-report; now: matches what gets billed). Fake-driven byte-identity golden unchanged (canned token counts are call-shape-agnostic).
 - **`estimate(...)` engine parameter renamed `anthropic_client` → `client` (#136 US-008 QG).** Post-US-005 the slot was already typed `object | None` and forwarded verbatim to whichever provider strategy is active; the old name implied Anthropic-only and would mislead a future #137 Gemini wiring. CLI in `generate.py` already passed `None` for non-Anthropic providers; the rename surfaces that without behaviour change.