diff --git a/.github/meta/harness-review-followups.md b/.github/meta/harness-review-followups.md new file mode 100644 index 0000000000..1bd18a26c7 --- /dev/null +++ b/.github/meta/harness-review-followups.md @@ -0,0 +1,362 @@ +# Completion-gate validators — deferred review findings + +Findings from the review of the deterministic completion-gate validators that are +real but larger than the change they were raised against, plus the ones that were +declined on purpose. Each carries the rationale, so a later pass does not have to +re-derive it. + +Source: review feedback and the end-to-end evidence run on +`feat/deterministic-validators` (2026-08-29). + +--- + +## Deferred — real, but larger than a fix-in-place + +### 1. ~~Custom `model-paths` / `seed-paths` are not honoured~~ — RESOLVED + +`modelsModifiedSince` requires a `models` path segment, and both +`collectProducedNodeNames` and the authored-file scan use a hard-coded directory +list. A project that configures `model-paths: ['analytics']` in `dbt_project.yml` +is invisible to every path-based check in the lane. + +Direction is safe today — the validators under-fire rather than over-fire on such +a project — but the deliverable-names gate can report a name as absent when the +model exists under a custom path, which would block. + +Closed in the consensus pass. `resolveDbtSourcePaths(dbtRoot)` parses +`model-paths`, `seed-paths`, `snapshot-paths`, `analysis-paths`, `macro-paths`, +`test-paths` and `packages-install-path` (plus the pre-1.0 `source-paths` / +`data-paths` spellings), handling inline flow lists, block sequences and bare +scalars. `modelsModifiedSince`, `collectProducedNodeNames` and the authored-file +scan are all driven off it. + +One deliberate carve-out: when the scanned directory has no `dbt_project.yml` +there is nothing to honour, so `modelsModifiedSince` keeps its legacy "any +`models` ancestor" predicate. That keeps the helper usable on a directory that +is not itself a project root. + +### 2. Python models (`.py`) are outside the touched-model set + +`modelsModifiedSince` accepts `.sql` only, so a session that edits +`models/orders.py` produces an empty work list and `dbt-build-green` takes its +`nothing-to-gate` path. + +Why deferred: widening the extension is one line, but the consumers are not +extension-agnostic. `dbt-dialect-guard` and `dbt-incremental-config` would then +run SQL/Jinja regexes over Python source, where `#` is a comment and +`config(materialized=...)` is a `dbt.config()` call — different lexical rules +entirely. The correct shape is a per-consumer file-kind filter, which is a +refactor of the discovery API rather than an added extension. + +### 3. `run_results.json` is trusted as evidence an agent cannot forge + +Nothing stops a session writing a `run_results.json` full of `success` rows +instead of running dbt. Every filesystem-evidence gate in this lane shares that +property. + +Why deferred: closing it means recording dbt invocations from the tool layer and +signing them into the session record — a lane-wide trust model, not a validator +change. Partly mitigated already: build coverage now also reads the model DDL +under `/run/`, so a forgery has to fabricate two artifacts rather than +one. + +### 4. Post-build edit detection is mtime-based, not content-based + +`BUILD_FRESHNESS_TOLERANCE_MS` was raised to 60 s because a formatter or a +trailing-newline fix landing seconds after a green build was blocking sessions. +That trades a false positive for a blind spot: a substantive rewrite inside the +window is not caught. + +The right fix is a content comparison — hash each model at build time and compare +after — which needs a pre-build snapshot the gate does not currently take. Worth +doing when the lane gains a session-scoped artifact store. + +### 5. ~~Compound `{% if is_incremental() and … %}` conditions are not matched~~ — RESOLVED + +Closed in the review sweep. Guard-body extraction was rebuilt on the shared +nesting-aware helper (`extractJinjaIfBlocks` + `jinjaIfBranchHead`) rather than on +a looser regex, which is what this entry said the fix had to wait for. Compound +conditions and nested `{% if %}` / `{% else %}` inside a guard are now handled. +Covered by `review-sweep.test.ts`. + +### 6. ~~`analyses/` counts toward the produced-node inventory~~ — PARTLY RESOLVED + +An `analyses/foo.sql` satisfies a required model named `foo`, even though an +analysis is never materialised as a relation. The requested resource *type* is +also discarded during extraction, so a seed can satisfy a request for a model. + +Half closed in the consensus pass. `analyses/` and `tests/` no longer contribute +to the inventory from either source: the filesystem scan visits only the +configured model/seed/snapshot paths, and manifest nodes are filtered on +`resource_type`. An analysis is compiled but never materialised, so it was never +a relation that could satisfy a deliverable. + +Still open: the requested resource *type* is discarded during extraction, so a +seed can still satisfy a request for a model and vice versa. That needs the noun +carried from the task through to the comparison, changing the +`RequiredDeliverables` shape and the gate's messages. + +### 7. Five copies of the recursive project walker + +`modelsModifiedSince`, `collectProducedNodeNames`, `collectExecutedModelNames`, +`anyAuthoredFileSince` and `projectPrescribesGuards` each carry their own +recurse / skip-hidden / skip-`node_modules` / symlink / depth-cap loop. They have +already drifted (only two follow symlinks; only some skip `target`). + +Why deferred: a shared `walkProject(root, opts)` is a clean refactor but touches +every validator in the lane at once, and doing it in the same change as the +behavioural fixes would make both harder to review. Worth its own change. + +--- + +## Declined — the conservative behaviour is the intended one + +### `IDENTIFIER_RE` requires at least three characters + +Reviewers asked for identifiers of any length so a task requiring `id` is +honoured. Declined: two-character code spans in prose are overwhelmingly not +relation names, and every one that is wrongly accepted becomes a required model +that can never be satisfied. Under-extraction is a miss; over-extraction blocks a +correct session. + +### `hasGuard` accepts any `is_incremental()` occurrence + +Reviewers asked that it require an enclosing `{% if %}`. Declined: a model that +writes `{% set inc = is_incremental() %}{% if inc %}` is correct dbt, and +tightening this creates a new false positive to close a false negative. The +lenient direction is the safe one for a gate that blocks completion. + +### A fresh test-only artifact should hard-fail rather than skip coverage + +Reviewers asked that an artifact containing no model nodes block an edited model. +Declined as stated: `dbt build` followed by `dbt test` is a normal, correct +sequence and leaves exactly that artifact, so blocking on it fires on healthy +sessions. Addressed instead by reading the model DDL under `/run/`, which +a test invocation does not overwrite, and by recording +`verdict: "coverage-inconclusive"` when neither source can speak — so the case is +visible in telemetry rather than silently green. + +### Backslash string escapes in the SQL lexer + +A reviewer asked that `scrubSql` stop treating `\'` as an escaped quote, on the +grounds that none of the target warehouses use backslash as a string-escape +character. Declined: the premise is wrong. Snowflake, BigQuery and Redshift all +support backslash escape sequences in string literals; only DuckDB is +strictly `''`-only. Dropping the branch would mis-lex `'it\'s'` on three of the +four warehouses this lane targets, which is the more common shape than the +literal-trailing-backslash case the reviewer raised. + +### `unique_key` inherited from `dbt_project.yml` + +Full dbt config inheritance is not resolved. Rather than guess, the keyless-upsert +finding is suppressed for the whole project when `dbt_project.yml` mentions +`unique_key` at all. Deliberately blunt: it gives up a true positive in exchange +for never inventing an inconsistency that the merged config does not have. + +--- + +## Deferred — raised in the review sweep, still open + +### 8. `/run/` DDL proves execution, not success + +Build coverage falls back to the model DDL under `/run/` when +`run_results.json` has been overwritten by a later `dbt test`. dbt writes that +DDL *before* the warehouse executes the statement, so it is present for a model +that then failed. A session that runs `dbt build` (a model errors), then +`dbt test`, leaves a failed model with fresh DDL and no failing row in the +surviving artifact, and the gate reports green. + +Why deferred: the obvious narrowing — trust the DDL only when the fresh artifact +carries no model rows at all — breaks the very common `dbt run --select a` then +`dbt run --select b` session, where `a` is covered by DDL alone and the artifact +does carry model rows. That would block healthy sessions, which is the wrong +direction. The real fix is retaining per-invocation run-result history for the +session rather than reading whichever single artifact survived, which is the same +session-scoped artifact store that item 4 needs. + +Partially mitigated in the sweep: staleness is now measured against the DDL's own +mtime rather than the surviving artifact's, so an edit made between the build and +a later test is caught. + +Further mitigated in the consensus pass, and the reasoning above is confirmed +against real dbt 1.8.7: a `dbt build` that errors on a model still leaves that +model's DDL in `/run/`, and a following `dbt test` writes +`run_results.json` with `args.which: "test"` and zero rows. The two evidence +sources are then indistinguishable from a healthy `dbt run` + `dbt test`, so +this still cannot be made to block without firing on correct sessions. + +What did change is the label. Coverage resting on DDL alone now reports +`verdict: "build-unproven"` instead of `fresh-build`. That matters more than it +sounds: the confident verdict was contaminating the shadow telemetry the enable +decision is supposed to rest on, so red builds were being counted as green in +the measurement itself. The gate's pass/fail behaviour is unchanged. + +Closing it properly still needs per-invocation run-result history for the +session — the same session-scoped artifact store item 4 needs. Note the harness +makes this worse on its own: `dbt-tests-pass` spawns `altimate-dbt test` in the +project on every validation pass, so from the second pass onward the surviving +artifact is always a test artifact. + +### 9. Build coverage is keyed on the bare node name, not the package + +In a multi-package project where a dependency and the root project both define +`orders`, a successful `model.dependency.orders` row satisfies coverage for the +local `model.local.orders`. + +Why deferred: matching on the full unique ID means mapping each touched file to +its manifest node via `original_file_path`, which is a new manifest-backed +resolution step rather than a line edit. Much narrower after the sweep: installed +packages are now excluded from the touched-model set, so this needs a genuine +name collision between the root project and a dependency, both selected in the +same session. + +### 10. The dialect guard checks for a guard, not for the *right* guard + +`dbt-dialect-guard` suppresses a warehouse-specific call when it sits anywhere +inside a `{% if … target.type … %}` chain. It does not check that the branch the +call sits in is actually limited to a warehouse that provides the function, so +both of these pass while still breaking on at least one target: + +```jinja +{% if target.type != 'snowflake' %} {{ iff(a, b, c) }} {% endif %} +{% if target.type == 'snowflake' %} … {% else %} safe_cast(x as int) {% endif %} +``` + +Why deferred: closing it needs a mapping from each `DIALECT_FUNCTIONS` entry to +the `target.type` values that provide it, plus evaluation of each branch +condition against that set — `==`, `!=`, `in`, `not in`, and the implicit +complement an `{% else %}` arm carries. That is a feature with its own test +surface, and a half-implementation converts a false negative into a blocking +false positive on correct models. The validator's docstring states the weaker +property it actually checks, so the claim is not overstated in the meantime. + +--- + +## Consensus review (six reviewers, 2026-08-29) — closed in this pass + +Recorded here so a later pass does not re-derive them. Each has a regression +test in `test/altimate/validators/consensus-review.test.ts`. + +- **`dbt compile` artifacts certified builds.** `run_results.json` carries no + statement of which subcommand wrote it, and `dbt compile` emits a full set of + `status: "success"` model rows — verified against real dbt 1.8.7, including + for a model selecting from a non-existent relation. `readRunResults` now reads + `args.which`, and an artifact from a command that executes no model SQL is not + build evidence. An artifact with no `args.which` is still trusted, so the + change is backwards compatible; real dbt always stamps it. +- **`CONFIG_CALL_RE` truncated at the first `)`.** `pre_hook="{{ log_start(run_id) }}"` + ended the non-greedy capture early and silently dropped every argument after + it — reading a correctly-keyed merge model as an unkeyed upsert, and losing + `enabled=false` / `materialized='ephemeral'` exemptions. Replaced with a + paren-depth and quote aware scanner. +- **`dbt-nothing-built` passed on any unrelated edit.** It asked only whether + *anything* had been written under the project, never comparing against + `expectation.required`. A session told to create `fct_orders` cleared it by + touching `macros/helper.sql`. Evidence must now intersect the named + deliverables; the coarse bar is kept only for the opt-in with no named + deliverables. +- **`packages-install-path` was ignored.** The `dbt_packages` / `dbt_modules` + skip was matched on the bare directory name, so a project configuring the + install path sent dependency models through the two *pre-existing* subprocess + validators, and a locally authored directory of that name anywhere in the tree + was wrongly skipped. Now resolved from project config and matched on path. +- **Inactive Jinja exempted live models.** `{% if false %}{{ config(enabled=false) }}` + and the same inside `{% raw %}` read as real exemptions. Both regions are + blanked before config extraction. Limited to these two on purpose — a looser + condition would strip live config and push the gate towards blocking. +- **`insert_overwrite` / `microbatch` were told to add a guard.** Both converge + on re-run by construction, and the prescribed remediation would have changed + what the model does. Now exempt from the guard requirement. +- **`is_incremental()` inside a string literal counted as a guard.** The scan + tested the unmasked source while the file's own comment said otherwise. +- **`[^%]*` in `ownBranchMatches` / `jinjaIfBranchHead`.** A Jinja modulo in a + tag (`{% if loop.index % 2 == 0 %}`) stopped the match dead and lost an arm + from the depth counter. Aligned with `JINJA_IF_OPENER_SOURCE`. +- **Repository text was spliced into the retry prompt.** dbt error messages and + node names were interpolated verbatim into `reason`/`fixHint`, which dispatch + concatenates into a synthetic `role: "user"` turn — so a hostile repo could + place text at instruction position. All untrusted values now go through + `sanitizeForPrompt`. +- **Verdicts that hid a zero-verification pass.** An empty scope and an + exempt-only scope both reported `fresh-build`. Now `nothing-verified` and + `exempt-only`; DDL-only coverage is `build-unproven`; an edit inside the 60 s + grace window is reported in `edited_within_grace` and downgrades the verdict. +- **Unreadable models read as clean.** `dbt-dialect-guard` and + `dbt-incremental-config` now report `models_scanned`, `unreadable_models` and + `coverage_complete` rather than counting an unreadable file as verified. +- **`prompt.ts` claimed shadow mode spawns no subprocesses.** It does. Shadow + suppresses only the retry; every validator runs in full. Comment corrected — + the false claim was load-bearing for "enable in shadow first" advice. + +## Consensus review — open, NOT addressed in this pass + +Ordered by how much they should weigh on an enable decision. + +### 11. Node identity is a bare lowercase name, not a `unique_id` + +Supersedes and widens item 9. Run-result IDs and `/run/` paths are both +reduced to a bare name, discarding package *and* resource type, so +`model.dependency.orders` can satisfy coverage for a local `orders`, and a +snapshot's DDL can stand in for a model's. The honest fix is to resolve each +touched file to its manifest `unique_id` via `original_file_path` and match on +that — a manifest-backed resolution step the lane does not have yet. This is the +single largest remaining source of fabricated coverage. + +### 12. Effective dbt config is never resolved + +`dbt-incremental-config` only sees inline `config()`. A model made incremental +through `dbt_project.yml` or properties YAML is invisible, while any textual +`unique_key:` anywhere in `dbt_project.yml` suppresses the keyless-upsert +finding for every model in the project. dbt supports `unique_key` in SQL, +properties YAML and project config. Needs the effective config for the exact +node from a fresh manifest. + +### 13. `dbt-deliverable-names` mines natural language for a blocking contract + +Assessed by two reviewers as the validator most likely to block correct work. +Code-formatted column names in a Required/Deliverables section become required +models; rename wording can demand both names; negated bullets can demand the +prohibited artifact; `create a model with name fct_orders` is not recognised. +Both contract gates also stop at the first task document that parses any +contract, silently masking a later `REQUIREMENTS.md`. The direction of travel is +an explicit structured contract rather than prose mining, with ambiguous prose +yielding inconclusive instead of a blocking invented requirement. + +### 14. Dialect guard does not check which target owns the function + +Restates item 10, still open, now with a second failure mode: the scrubber does +not handle dollar-quoted strings, so valid text such as `$tag$… iff( …$tag$` +produces a blocking finding. + +### 15. Python models are still outside the touched set + +Item 2, unchanged. Now paired with the `model-paths` fix: discovery honours +custom paths but still accepts `.sql` only, so a `models/orders.py` session takes +the `nothing-to-gate` path. Needs a per-consumer file-kind filter, because the +SQL/Jinja regexes are wrong for Python source. + +### 16. Validator telemetry has no field allowlist + +`details` is attached to telemetry wholesale and includes absolute `dbt_root`, +`task_file` and `run_results_path` plus business model names, against a +telemetry contract that says file paths are not collected. Pre-existing for +`dbt_root`; this lane widens it. Wants an allowlist of bounded counters, +booleans, enum verdicts and durations, with paths dropped or hashed. + +### 17. No whole-check resource budget + +Recursive scanners follow directory symlinks without realpath containment or +cycle detection, several Jinja helpers are near-quadratic on pathological input, +and no deadline is passed through the registry. The 60 s timeout covers +`altimate-dbt` children only, not the in-process checks. + +### 18. Retry budget is shared, and the ceiling is wall-clock, not just waste + +`validatorRetryCount` is one session-scoped counter capped at 3, and the five +new validators register *before* `dbt-schema-verify` / `dbt-tests-pass`. See the +PR discussion for the analysis; not changed here because the counter lives in +`session/prompt.ts`, which this PR otherwise does not touch. The associated +number worth carrying: the two subprocess validators run one `altimate-dbt` +child per touched model at concurrency 4 with a 60 s per-child timeout, across +an initial dispatch plus up to three retries — roughly `480 × ceil(M/4)` seconds +worst case, about 3 h 20 m at 100 touched models. diff --git a/docs/internal/deterministic-checks-engine-split.md b/docs/internal/deterministic-checks-engine-split.md new file mode 100644 index 0000000000..4ae3a38ab8 --- /dev/null +++ b/docs/internal/deterministic-checks-engine-split.md @@ -0,0 +1,252 @@ +# Deterministic completion checks: what belongs in the engine + +**Status:** assessment, input to a build decision +**Date:** 2026-08-28 +**Scope:** the two deterministic checks the plan assigns to the engine rather than to the +validator lane — unguarded division (parse-level) and filter consistency (semantic / +lineage-level). The cheap fs+regex tier lives in +`packages/opencode/src/altimate/validators/` and is out of scope here. + +Engine repo inspected read-only at `/Users/anandgupta/codebase/altimate-core-internal` +(Rust workspace, version `0.7.0`). + +--- + +## Bottom line + +| Check | Engine capability today | Work required | +|---|---|---| +| Unguarded division | **Already implemented and already wired.** Lint rule `L032` (`division_by_column_no_guard`) is a real sqlparser expression-tree walk, reachable from altimate-code right now via `Dispatcher.call("altimate_core.lint", …)`. | **None engine-side.** Consumer-side only: a validator that feeds it compiled model SQL. | +| Filter consistency | **Partial — the wrong granularity.** The engine ships `extract_source_filters`, a *cross-model* sibling-filter primitive already consumed by the review orchestrator. It reads WHERE clauses only, and cannot see predicates attached to sibling aggregates inside one SELECT. | **One new engine analysis** (~a rule-sized addition, not an architecture change) plus a napi export, a dispatcher entry and a consumer tool. | + +Neither check can move into the cheap validator tier: both need a parsed expression tree. +Regex cannot tell `a / b` from `a / nullif(b, 0)` once either side is a function call, a +CASE, or a nested expression, and it certainly cannot group aggregates by the column they +read. + +--- + +## How engine capability reaches altimate-code + +Relevant because it sets the cost of "add something engine-side". + +- Engine crates: `altimate-core` (analysis), `altimate-core-bindings-common` (shared binding + layer), `altimate-core-node` (napi-rs), plus `polyglot-sql` for multi-dialect parse and + transpile. +- Published as the npm package `@altimateai/altimate-core` (per-platform native addon). + altimate-code pins it exactly: `packages/opencode/package.json` → `"@altimateai/altimate-core": "0.7.0"`. +- Consumer binding: `packages/opencode/src/altimate/native/altimate-core.ts` registers 42 + `altimate_core.*` handlers on the dispatcher. Registration is lazy — the napi binary loads + on the first `Dispatcher.call()` (`packages/opencode/src/altimate/native/index.ts`), so a + validator importing `Dispatcher` costs nothing until it actually calls. +- Adding a capability therefore means: engine PR → workspace version bump → publish → + bump the consumer pin → one `register(...)` block in `altimate-core.ts` → optionally a + `Tool.define` file under `src/altimate/tools/`. Four repos-worth of steps, one release + boundary. + +--- + +## Check A — unguarded division + +**What the check is.** Flag a division whose denominator is not wrapped in a `NULLIF` or a +`CASE` guard. The failure it catches is a division-by-zero or silent-NULL result in a ratio +column; it recurs in evaluation traces as a wrong-value defect that builds green and passes +schema checks. + +**Verdict: the engine already does this. Nothing to build engine-side.** + +- Rule: `crates/altimate-core/src/linter/rules/division_by_column_no_guard.rs`, lint code + `L032`, name `division_by_column_no_guard`. +- Implementation is a genuine AST walk, not a text heuristic: it parses with sqlparser and + walks `Expr::BinaryOp { op: Divide }` nodes through CTE bodies, set-op branches, function + arguments and nested expressions (`find_division_by_column_in_set_expr`, and the shared + walkers in `crates/altimate-core/src/linter/rules/mod.rs`). +- The guard semantics fall out of the tree shape: it fires only when the denominator is a + bare `Expr::Identifier` / `Expr::CompoundIdentifier`. A denominator wrapped in + `NULLIF(...)` parses as `Expr::Function` and one wrapped in `CASE` as `Expr::Case`, so + guarded divisions are excluded structurally rather than by pattern-matching the guard. +- `fn check(&self, sql: &str, _schema: &SchemaDefinition)` ignores the schema, so the rule + needs no table/column resolution to be accurate. + +**How altimate-code would call it.** + +```ts +import { Dispatcher } from "../native" + +const result = await Dispatcher.call("altimate_core.lint", { sql, schema_path: "" }) +const findings = (result.data?.findings ?? []).filter((f: any) => f.rule === "L032") +``` + +`altimate_core.lint` is registered at `packages/opencode/src/altimate/native/altimate-core.ts` +(handler 2), calling `core.lint(sql, schema)` in `crates/altimate-core-node/src/safety.rs`. +`schemaOrEmpty()` in the same file means an empty schema is a supported argument. The +composite `altimate_core.check` handler folds the same lint output into `data.lint.findings` +and is already exposed to the agent through +`packages/opencode/src/altimate/tools/altimate-core-check.ts`, which shows the finding shape +(`f.rule`) end to end. + +**The one real consumer-side problem: Jinja.** The engine parses SQL, and a dbt model source +is not SQL — `{{ ref() }}`, `{% if %}` and `{{ config() }}` will not parse. A validator must +feed it the **compiled** SQL from `/compiled//models/**.sql`, which exists +only after a successful compile or build. That makes the division lint naturally sequenced +*after* the build-green gate: no fresh compiled artifact, nothing to lint, skip. The +existing `resolveDbtTargetPath()` in +`packages/opencode/src/altimate/validators/validator-utils.ts` already resolves the artifact +directory including a custom `target-path`. + +**Recommendation.** Build it as a validator in the existing lane, consuming +`altimate_core.lint` and filtering to `L032`, scoped to session-touched models and their +compiled counterparts. No engine work, no version bump. The cost is the compiled-SQL +plumbing, not the analysis. + +--- + +## Check B — filter consistency + +**What the check is.** Detect an exclusion predicate applied on one aggregate path but +omitted on a sibling aggregate over the same source — e.g. two `SUM(CASE WHEN … END)` +measures in one SELECT where one carries an extra exclusion the other lacks. This is the +most-evidenced wrong-logic family: the model builds, the shape is right, the numbers are +quietly inconsistent between columns. + +**Verdict: the engine has the building blocks and a close cousin, but not this check.** + +What exists: + +- `crates/altimate-core/src/review/grain.rs` → `extract_source_filters(sql) -> BTreeMap>`. + Returns, per upstream table, the filter columns applied to it. Exported over napi in + `crates/altimate-core-node/src/review.rs` and registered as the dispatcher key + `altimate_core.source_filters` (`altimate-core.ts`, "Per-upstream WHERE-filter columns, + for cross-model sibling filter-consistency"). +- It is already consumed: `siblingConsistencyLane` in + `packages/opencode/src/altimate/review/orchestrate.ts` compares those filter sets **across + different models reading the same upstream** in a diff, and flags a model missing a filter + its siblings apply. The doc comment on the Rust function cites the real incident that + motivated it (one of three sibling loaders missing a NULL filter). +- `crates/altimate-core/src/filter_analysis/` analyses WHERE-clause quality within a single + query (contradictory, redundant, missing-partition predicates) via logical plans. +- `crates/altimate-core/src/lineage/complete.rs` carries, per output column, a + `lens_code: Vec` — the SQL text of each transformation step — alongside + `transform_type` / `lineage_type` labels. Exposed via `column_lineage` in + `crates/altimate-core-node/src/lineage.rs` (dispatcher key `altimate_core.column_lineage`). + +Why none of it is the check: + +- `extract_source_filters` walks `sel.selection` — the WHERE clause — and attributes filter + *columns* to upstream tables. It never looks inside projection expressions, so a + `CASE WHEN status = 'x' AND NOT is_test THEN amount END` inside a `SUM()` is invisible to + it. Its comparison unit is a model, not a column. +- `filter_analysis` reasons about one predicate set at a time; there is no cross-aggregate + comparison anywhere in it. +- The lineage `lens_code` does carry the aggregate's expression *text* per target column, + which is tantalising but not sufficient: it is unparsed text with a coarse transform + label, so any comparison built on it would be string diffing — exactly the fuzzy matching + a deterministic gate must not do. Two logically identical predicates written differently + would read as inconsistent, and a genuinely asymmetric one written similarly could slip + through. + +**What would need to be added engine-side.** One new analysis pass, following the shape of +the existing rules: + +1. Walk the SELECT projection, collecting aggregate calls (`SUM`, `COUNT`, `AVG`, …) whose + argument is a `CASE` expression. The helpers already exist next door in + `crates/altimate-core/src/linter/rules/mod.rs` and `review/grain.rs` (aggregate + detection, bare/qualified column extraction). +2. For each, extract the CASE `WHEN` condition set as parsed predicates, plus the base + column being aggregated. +3. Group the aggregates by base column (and by upstream relation, reusing the attribution + `walk_query_filters` already performs). +4. Diff the predicate sets within a group and emit a finding for an asymmetric exclusion, + normalising predicates structurally rather than textually so that reordered or + differently-spelled equivalents do not produce noise. + +Then, one of two wiring paths — they are not the same amount of work: + +- **As a lint rule (cheaper, preferred).** Add the code in `safety.rs::lint` and consume + the existing `altimate_core.lint` handler. No napi export and no dispatcher key are + needed: `altimate-core.ts` already exposes lint results end to end, to the agent and to + `altimate_core.check`. +- **As a bespoke analysis API.** Only this path needs a new napi export in + `crates/altimate-core-node/src/review.rs` plus a dispatcher entry in `altimate-core.ts`. + Choose it only if the finding needs a richer result shape than a lint code carries. + +Either way, a validator consumes the result. + +**Why this cannot live in the validator lane.** Step 1 needs the projection's expression +tree; step 2 needs parsed boolean predicates; step 3 needs column attribution through CTEs +and aliases; step 4 needs structural predicate comparison. Every one of those is SQL +analysis. A regex approximation would fire on formatting differences and miss the real +asymmetries, which is the worst outcome for a gate that blocks completion — a lane that +cries wolf gets its retry budget burned and then disabled. + +**Sizing.** Rule-sized, not architecture-sized: it reuses existing sqlparser walkers, has a +direct precedent in `extract_source_filters`, and needs no change to lineage or to the +binding architecture. The dominant cost is predicate normalisation (step 4), which is where +the false-positive risk concentrates and where the test corpus has to be built. + +--- + +## Decision inputs + +1. **Unguarded division is free.** It is a consumer-side validator over + `altimate_core.lint` + compiled SQL. Do it in the lane; no engine ticket, no version bump. +2. **Filter consistency is the only item here that needs an engine ticket**, and it is worth + scoping as a lint rule (code alongside `L032`) rather than a bespoke review API, because + the lint path is already wired end to end — engine rule → `lint` → dispatcher → + `altimate_core.check` → agent and validators. That collapses the wiring work to the rule + itself. +3. **Sequencing.** Both checks depend on compiled SQL, so both sit behind the build-green + gate in the completion lane. Neither is worth building before that gate demonstrably + fires on real sessions. + +--- + +## Placement of the shipped completion-gate validators + +**Status:** assessment, addendum to the above +**Date:** 2026-08-28 +**Scope:** the five completion-gate validators actually shipped in +`packages/opencode/src/altimate/validators/`: `dbt-nothing-built`, `dbt-build-green`, +`dbt-deliverable-names`, `dbt-incremental-config`, `dbt-dialect-guard`. Question: does any +of this — or its logic — belong in `altimate-core-internal` instead of `altimate-code`, so +other engine consumers (the dbt PR reviewer, CI tooling, other products) can reuse it? + +### Per-validator table + +| Validator | What it analyzes | Reuse value for other engine consumers | Engine-feasible (does the engine see the inputs)? | Recommendation | +|---|---|---|---|---| +| `dbt-nothing-built` | Filesystem: any authored file under `models/seeds/snapshots/data/analyses/macros/tests` since session start, `run_results.json` freshness. Plus a markdown/regex scan of a task-instruction file for literal required deliverables. | None. The check *is* "did this session's filesystem change since a wall-clock timestamp" — meaningless outside a live agent session. | No. The engine's API is SQL-in/findings-out; it has no concept of session start time, a live project directory to `stat`, or a task-instruction document. | **Stays in altimate-code.** Not a SQL question at any point. | +| `dbt-build-green` | Filesystem: `/run_results.json` parsed, cross-referenced against edited-model mtimes for coverage/staleness/failure status. | None. Same category as above — needs a live build artifact plus session-scoped mtime comparisons. | No. Same as above; also no dbt subprocess invocation happens in-engine. | **Stays in altimate-code.** | +| `dbt-deliverable-names` | Filesystem inventory (`models/seeds/snapshots/data/analyses`) + `manifest.json` node names/aliases, diffed against literal deliverable names extracted from a task document via markdown/regex parsing. | None directly — this is a naming-contract check against a task doc, not a SQL property. | No. Requires a live project directory and a task-instruction file; the engine never sees either. | **Stays in altimate-code.** | +| `dbt-incremental-config` | Two of its three findings are dbt **config** semantics — `incremental_strategy`/`unique_key` presence, `is_incremental()` guard presence — read via regex over `{{ config(...) }}` args and grep over the model body. The third is a regex scan for non-deterministic functions (`current_timestamp`, `random()`, …) inside the `is_incremental()` predicate block. | **High, and already realized elsewhere.** `crates/altimate-core/src/review/dbt_config.rs::dbt_config_lint` implements the *same two* checks — `DBT001` (`incremental_no_guard`) and `DBT002` (`incremental_no_unique_key`) — as a real minijinja-based structural config parser, not regex. It is already wired dispatcher-side as `altimate_core.dbt_config_lint` and is **already consumed** by the dbt-PR-reviewer (`packages/opencode/src/altimate/review/runner.ts:338`). | Yes for findings 1 & 2 — the engine already does this, on raw (uncompiled) model SQL, no compiled-artifact dependency. Finding 3 (non-determinism *specifically inside* the `is_incremental()` predicate) has no engine equivalent yet; the closest existing rule, `L049 clock_in_filter`, catches clock functions in any WHERE/JOIN/HAVING predicate but doesn't scope to the incremental guard block specifically. | **Rewire, don't reimplement.** Findings 1 & 2 should call `altimate_core.dbt_config_lint` and filter to `DBT001`/`DBT002` instead of carrying a parallel regex implementation — the validator's own comment tier ("grep-level … config declared in `dbt_project.yml` is intentionally not resolved") describes exactly the ceiling the engine's minijinja parser already clears. The idempotency-gating policy (only escalate the missing-guard finding to a blocker when the task doc says "idempotent") is genuinely consumer-side — it depends on a task-instruction file the engine doesn't see — and stays in the validator as a thin policy layer over the engine's finding. **Worth an engine follow-up ticket:** add a `DBT007`-style rule to the same `dbt_config.rs` file for "non-deterministic function inside an `is_incremental()` predicate" — the parsing/masking infrastructure (`mask_comments_and_strings`, predicate-block extraction) already exists in that file, so it's a small addition, not new architecture. | +| `dbt-dialect-guard` | Raw (uncompiled) model text: (a) does the project already establish a `target.type`-guard convention (fs scan for the string across `models/`+`macros/`); (b) a curated 19-entry list of warehouse-specific function names, regex-matched; (c) whether each match sits inside a `{% if …target.type… %}…{% endif %}` block, detected by blanking out guarded regions before matching. | Partial, and one-directional. The engine's `L033 non_portable_function` rule is a genuine sqlparser AST walk with a much larger curated function set (the excerpt alone showed 50+ names vs. the validator's 19) — the validator is duplicating a *subset* of engine data with a worse matcher. But L033 cannot do what this validator does: it requires parseable SQL, and compiling a dbt model resolves away whichever `{% if target.type %}` branch wasn't taken — the information "was this guarded" is gone by the time SQL reaches the engine's parser. | No, for the guard-detection half — that is a category error: the engine's SQL-in/findings-out contract has nothing to say about un-rendered Jinja branches, because by the time it sees valid SQL the branch has already been chosen. Yes, in principle, for the *function-name curation* half, which is pure data the engine already owns more completely. | **Hybrid, but the "hybrid" is data-sharing, not logic-splitting.** The guard-detection orchestration must stay in altimate-code (it needs live, un-rendered Jinja source — a live-project concern by definition). The curated dialect-function list should stop being hand-maintained twice: either export `non_portable_function_set()` (or a subset annotated with source dialect) via a small napi accessor so the validator sources its match list from the engine, or accept the current duplication and add a periodic reconciliation check. This is a maintenance/consistency follow-up, not an engine ticket for new analysis logic. | + +### Overall recommendation — does this need to go into the engine? + +**No, not as a wholesale move.** Three of the five validators (`dbt-nothing-built`, +`dbt-build-green`, `dbt-deliverable-names`) never touch SQL semantics at all — they reason +about filesystem state, build artifacts (`run_results.json`, `manifest.json`), and a task +document's prose. None of that is representable in the engine's SQL-in/findings-out +dispatcher contract, and none of it has any value to another engine consumer (the dbt PR +reviewer doesn't care whether *this* session's clock wrote a file). Pushing them +engine-side would be the same category error the prior assessment already named for +division-guard and filter-consistency, just in the other direction: these validators need a +live project directory and session-scoped timestamps, and the engine's API surface is +SQL text in, findings out. There's nothing to move. + +The one real finding here is `dbt-incremental-config`, and it isn't "should this move to +the engine" — **it already lives there.** `dbt_config_lint` (`DBT001`/`DBT002`) is shipped, +wired through the dispatcher, and already consumed by the dbt-PR-reviewer's `runner.ts`. +The validator built a second, weaker (regex-only) implementation of the same two checks +instead of calling the existing one. The fix is a rewire of the validator's detection +layer onto `altimate_core.dbt_config_lint`, keeping only the task-doc-driven idempotency +policy consumer-side (exactly the hybrid pattern the division-guard case established: +engine finds it, consumer decides when it's blocking). One small engine follow-up is worth +filing — a `DBT007` rule for non-determinism scoped to the `is_incremental()` predicate, +reusing infrastructure already in `dbt_config.rs` — but it's rule-sized, not a new +capability. + +`dbt-dialect-guard` is the only genuinely mixed case, and even there the "engine work" is +data reconciliation (share `L033`'s function-name set) rather than new analysis: the guard +-detection logic itself has no engine home because it depends on un-rendered Jinja +branches, which the engine's compiled-SQL contract structurally cannot see. diff --git a/docs/internal/validator-e2e-evidence.md b/docs/internal/validator-e2e-evidence.md new file mode 100644 index 0000000000..92ea1be993 --- /dev/null +++ b/docs/internal/validator-e2e-evidence.md @@ -0,0 +1,625 @@ +# Completion-gate validators: end-to-end evidence + +> ## ⚠️ SUPERSEDED for the enable decision — read this first +> +> This report measured commit `7aa9087`, **before** the six-reviewer consensus +> review. Its false-positive inventory and its "shadow only" verdict describe +> code that no longer exists, so do **not** carry either into an enable/hold +> decision. It is kept because the measurement itself was real and the +> methodology section is still the right protocol; the conclusions are not. +> +> What changed under it: +> +> * The review found a class of failure this report did **not** measure: +> `dbt-build-green` certifying builds that failed or never ran. A `dbt compile` +> artifact records every model as `success` without executing anything, and was +> read as `verdict: "fresh-build", ok: true`. That is now rejected. Any +> `fresh-build` verdict in the tables below may be one of these. +> * Because of that, **the shadow-mode numbers in this report are not a clean +> baseline.** Red builds were recorded as green, so the very telemetry an +> enable decision would rest on was contaminated. Re-measure before deciding. +> * Several of the five false positives were addressed (custom `model-paths`, +> `insert_overwrite` incremental models, config args truncated at a nested +> hook). The remaining inventory has not been re-measured. +> * `dbt-build-green` verdicts are now finer-grained: `fresh-build` means every +> in-scope model has a success row from a model-executing dbt command. +> `build-unproven`, `coverage-inconclusive`, `nothing-verified`, `exempt-only` +> and `non-executing-artifact` split apart states this report could not tell +> from a verified pass. +> * **Correction on shadow mode.** This report treats shadow as observation-only. +> It is not free: shadow suppresses only the retry. Every applicable validator +> still runs in full, including the two that spawn one `altimate-dbt` child per +> touched model. Shadow costs the same filesystem scans, subprocess time and +> warehouse work as enforcement. Budget wall-clock for it accordingly — the +> subprocess timeout is per child, not per validator. +> +> Open items the review left unclosed are tracked in +> `.github/meta/harness-review-followups.md`. + +**Status:** superseded measurement report — see the banner above +**Date:** 2026-08-29 +**Scope:** the five validators added in PR #1175 — +`dbt-nothing-built`, `dbt-build-green`, `dbt-deliverable-names`, +`dbt-incremental-config`, `dbt-dialect-guard` — measured on real dbt projects +rather than on unit-test fixtures. + +> **Sample-size disclosure, up front — read this before the A/B section.** +> The planned A/B was 10 tasks × 2 arms × 2 rollouts = 40 live sessions. +> **Three complete sessions were achieved: N = 1 paired task (arm A + arm B) +> plus one unpaired arm-A run.** The batch was terminated for machine capacity: +> these sessions ran on a single laptop that was concurrently hosting another +> workstream's agent fleet, the machine had already crashed twice under load, +> and local session execution was stopped outright at a load average above 20. +> **No conversion claim can be made from N = 1.** The A/B numbers below are +> reported for completeness and transparency, not as evidence of effect in +> either direction. +> +> The false-positive, true-positive, negative-control and cost sections do +> **not** carry this limitation. Those are deterministic probes — the validator +> code paths run directly against real project states with no model in the loop +> — and their N (38 known-good states, 11 known-bad states) is what the safety +> conclusions rest on. The negative control is a completed live session. +> +> What a properly powered A/B would need is specified in +> [What a real A/B would require](#what-a-real-ab-would-require). + +--- + +## Bottom line + +| Question | Answer | +|---|---| +| Do the five fire on healthy, complete dbt projects? | **Not in ordinary end-states** — 0 firings across 27 naturalistic known-good states. | +| Are there false positives at all? | **Yes, five reproducible ones**, all in `dbt-build-green` (3) and `dbt-dialect-guard` (2), each triggered by an ordinary dbt practice rather than by a defect. | +| Do they fire on genuinely-unfinished work? | **Partially** — 8 of 11 constructed known-bad states fired; 3 were silent (see [True-positive discrimination](#true-positive-discrimination-known-bad-states-n--11)). | +| Do they execute in a non-dbt repo? | **No.** Zero validators executed; confirmed in a live session with the lane and the artifact opt-in both forced on. | +| Runtime cost? | ~2–10 ms per dispatch on a small project; ~1–3.5 s on a 2 000-model project, because each validator re-walks the tree independently. | +| Does enforcement convert failures into passes? | **Unknown — not measured.** N = 1 paired task; the run needed no retry, so there was nothing to convert. | +| Verdict | **Shadow only.** Two reasons, independently sufficient: five reproducible false positives, and no conversion evidence at all. Details in [Verdict](#verdict). | + +--- + +## Methodology + +### Code under test + +`feat/deterministic-validators` at `14747ac6c9`, worktree `/tmp/validators-build`. +Validators at `packages/opencode/src/altimate/validators/`; dispatch hook at +`packages/opencode/src/session/prompt.ts:1360`. + +The lane registers **seven** validators, not five: the two pre-existing members +(`dbt-schema-verify`, `dbt-tests-pass`) are registered alongside this PR's five. +To attribute results to this PR rather than to the lane as a whole, an +**experiment instrument** was added to `validators/index.ts` for the duration of +the measurement: an `ALTIMATE_VALIDATORS_ONLY` env allowlist filtering which +validators get registered (unset ⇒ register everything, i.e. shipped behaviour +unchanged). **That instrument is reverted in the commit that carries this +document** — it exists only so the numbers below describe the five. + +### Model + +altimate-code exposes a ChatGPT-subscription (Codex) provider +(`packages/opencode/src/plugin/codex.ts`, provider id `openai`, OAuth), but +**this environment has no credential for it** — `auth list` shows Anthropic +(oauth), Azure, Google, Vertex, Vertex (Anthropic), and the plugin's OAuth flow +needs an interactive browser login. Per the standing rule, no Vertex/GCP +Anthropic path was used. + +The three completed sessions therefore ran against a **self-hosted internal +staging endpoint**, registered as a custom OpenAI-compatible provider: + +```text +provider custom (npm: @ai-sdk/openai-compatible), internal staging endpoint +context 65536, output limit 8192 +sampling temperature 0.2, seed 1001 (pinned per rollout) +agent builder, --max-turns 40, --yolo, --format json +``` + +The model is a mid-capability coding model, not a frontier one. That matters +for reading the A/B: a stronger model would need the gates less often, a weaker +one more, so the fire rates here do not transfer directly to production traffic. +It does **not** affect the false-positive, true-positive, cost or +negative-control results, none of which involve a model. + +### Projects + +1. **An internal dbt task corpus** (10 tasks, all 10 used for the offline + probes, 2 reached in the live A/B) — real dbt projects (dbt-core + 1.8.7 + dbt-duckdb 1.8.3), each with a task prompt and a `verify.sh` grader + that rebuilds from scratch and diffs against a golden expectation file. + Every workspace was **copied** to a scratch directory before use; the + pristine corpus was never written to, and `verify.sh` was always invoked + with an explicit workspace argument. +2. **`dbt-labs/jaffle-shop-classic`** (public, Apache-2.0), cloned fresh and + pointed at a local DuckDB profile. Used for the false-positive work because + it is a complete, correct, third-party project with no injected defects. + +### Instruments + +* `validator-probe.ts` (repo root, **not committed**) — imports the registry + and runs `appliesTo()` + `check()` against an arbitrary directory with an + explicit `sessionStartMs`, emitting JSON. This is how the false-positive + sweep is executed: it exercises the exact validator code paths the session + hook calls, without a model in the loop, so results are deterministic and + repeatable. +* Live sessions via `bun run packages/opencode/src/index.ts run …` with + `ALTIMATE_VALIDATORS_DEBUG=1`, which mirrors every `validator_hook_reached` + and `dispatch_result` event to stderr. +* `altimate-dbt` was rebuilt from `packages/dbt-tools` and put first on `PATH`; + the globally-installed copy is stale and does not implement `schema-verify`. + +--- + +## Measurement 1 — false-positive rate (safety) + +A false positive here means: **a validator returns `ok:false` on a state a +competent engineer would call finished.** In enforce mode that state costs the +session a synthetic retry turn for nothing. + +### Set A — naturalistic known-good states (n = 27) + +For each of the seven `real-*` tasks whose pristine workspace builds green +(01, 02, 04, 05, 08, 09, 10), three end-states were probed: + +* **kg1a** — every project file written during the session, then `dbt build` + green ("the agent authored this and built it"). +* **kg1b** — project pre-existing, session ran only the build. +* **kg4** — read-only session: nothing written, nothing built. + +Plus six jaffle_shop states: whole project authored + built green; build-only; +read-only; a new correct model added and built green; the same with an explicit +`TASK.md` naming the deliverable (so the contract-driven validators activate); +and a correctly-configured incremental model built green. + +**Result: 0 firings from any of the five, across all 27 states.** + +The three `real-*` tasks whose pristine workspace does *not* build green +(03, 06, 07) were excluded from the known-good set — they are genuinely +unfinished, and firings there are true positives (recorded below). + +### Set B — constructed known-good states, each an ordinary dbt practice (n = 11) + +| # | State | Fired? | +|---|---|---| +| B1 | dialect-specific function inside a `{% if target.type … %}` guard | — | +| B2 | same, but the guard block contains a **nested `{% if %}`** | **FP — `dbt-dialect-guard`** | +| B3 | dialect function name appears only in a `--` SQL comment | — | +| B4 | dialect function name appears only inside a **string literal** | **FP — `dbt-dialect-guard`** | +| B5 | incremental config declared in `dbt_project.yml`, not in `config()` | — | +| B6 | last dbt command of the session was `dbt test` | — | +| B7 | green build, then the model file is **touched 3 s later** (reformat/comment) | **FP — `dbt-build-green`** | +| B8 | session edits an **ephemeral** model, builds green | **FP — `dbt-build-green`** | +| B9 | session **disables** a model on purpose (`enabled=false`), builds green | **FP — `dbt-build-green`** | +| B10 | session builds only the changed model with `--select` | — | +| B11 | session's build was `dbt run` (no tests) | — | + +### False-positive table + +| Validator | FPs | Known-good states where it could fire | Mechanism | +|---|---|---|---| +| `dbt-nothing-built` | **0** | 4 | — | +| `dbt-build-green` | **3** | 38 | see FP-1..3 | +| `dbt-deliverable-names` | **0** | 4 | — | +| `dbt-incremental-config` | **0** | 38 | — | +| `dbt-dialect-guard` | **2** | 7 | see FP-4..5 | + +Total: **5 false positives across 38 known-good states.** All five are +deterministic and reproduce on every run. + +#### FP-1 — `dbt-build-green`: any post-build write to a model file blocks + +State B7. The session writes a model, runs `dbt build` green, then appends a +trailing newline (a formatter, a comment, a tidy-up) three seconds later. + +```text +The build is not green: 1 model(s) were edited after the last build: extra_green2. + "stale_build": ["extra_green2"], "failed_in_scope": [], "not_built": [] +``` + +`BUILD_FRESHNESS_TOLERANCE_MS` is 1 000 ms. Any edit after that window — even a +whitespace-only one that cannot change compiled SQL — flips the model into +`stale_build` and blocks the session. "Build, then tidy, then summarise" is a +common agent trajectory, so this is not a corner case. + +#### FP-2 — `dbt-build-green`: editing an **ephemeral** model always blocks + +State B8. dbt does not emit a `run_results` node for an ephemeral model, so the +coverage assertion ("is every model I edited present in the artifact?") can +never be satisfied for one. + +```text +The build is not green: 1 model(s) you edited were never built: eph_helper. + "not_built": ["eph_helper"], "model_nodes_in_artifact": 6 +``` + +The build was green; the ephemeral model was compiled into its consumer, which +built and passed. There is no defect to fix, and no action the agent can take +that clears the gate short of changing the materialization. + +#### FP-3 — `dbt-build-green`: deliberately disabling a model always blocks + +State B9. `{{ config(enabled=false) }}` removes the node from the manifest, so +the same coverage assertion fires. Retiring a model is a legitimate, common +change. + +```text +The build is not green: 1 model(s) you edited were never built: retired_model. +``` + +FP-2 and FP-3 share one root cause: `not_built` treats "absent from +`run_results`" as evidence of an unbuilt model, but dbt legitimately omits +ephemeral and disabled nodes. + +#### FP-4 — `dbt-dialect-guard`: a nested `{% if %}` breaks guard detection + +State B2. `listagg()` sits inside a `{% if target.type == 'snowflake' %}` block +that also contains an inner `{% if var(...) %}…{% endif %}`: + +```text +1 unguarded warehouse-specific construct(s) in 1 model(s) you edited: nested_guard. + findings: [{"model":"nested_guard","function":"listagg()","dialects":"Snowflake / Redshift / Oracle"}] +``` + +`TARGET_TYPE_GUARD_RE` is non-greedy from `{% if … target.type … %}` to the +**first** `{% endif %}`. The inner `{% endif %}` closes the blanked region +early, so the still-guarded remainder is scanned and reported. Nested Jinja +inside a dialect guard is normal dbt. + +#### FP-5 — `dbt-dialect-guard`: string literals are not masked + +State B4. `'listagg('` as a string literal is flagged. + +Isolating the two halves of the composite probe shows the boundary precisely: +a `--` comment containing `listagg( … )` is **not** flagged (so +`stripSqlComments` works), while `select 'listagg(' as never_executed` **is**. +Lower frequency than FP-1..4, but the same class of bug: text matching without +lexical masking. + +### True-positive discrimination (known-bad states, n = 11) + +**8 of 11 known-bad states fired; 3 were silent.** This is the other half of the +safety question — a gate that never fires is also useless. + +| Known-bad state | Fired | +|---|---| +| `real-03` pristine: build genuinely red (`team_game_counts` errors) | `dbt-build-green` ✓ | +| `real-06`, `real-07` pristine: models edited, project does not parse, no artifact | `dbt-build-green` ✓ | +| `real-07`: required `stg_nba_games` does not exist | `dbt-nothing-built` ✓, `dbt-deliverable-names` ✓ | +| jaffle + `listagg()` unguarded in a project that establishes the guard convention | `dbt-dialect-guard` ✓ | +| jaffle + `incremental_strategy='delete+insert'` with no `unique_key` | `dbt-incremental-config` ✓ | + +The three silent known-bad states were `real-03` read-only, `real-06` build-only +and `real-06` read-only. These are not all the same kind of gap, and the two +buckets should not be conflated: + +* **`real-03` read-only is an intentional no-op, not a recall miss.** + `DbtBuildGreenValidator.check()` returns `verdict: "nothing-to-gate"` + (`packages/opencode/src/altimate/validators/dbt-build-green.ts:132-134`) + whenever a session touches no model files *and* produces no fresh build + artifact of its own — there is no claim of success to check. A read-only + session on `real-03` (nothing written, nothing built) hits exactly that + path by design. Real-03's defect is a genuine build error + (`team_game_counts`), not a missing deliverable, so none of the + contract-driven validators below are candidates for it either — this state + has no validator that could plausibly have caught it while doing nothing. +* **`real-06` build-only and `real-06` read-only are more likely the same + `nothing-to-gate` no-op for `dbt-build-green`** — the table above already + records that real-06's project does not parse and produces no artifact, so + the same "no edits, no fresh artifact" path plausibly applies regardless of + whether the session ran `dbt build` or nothing at all. This document does + not independently record `dbt-build-green`'s per-state telemetry for these + two variants, so that half of the explanation is inferred from the general + no-artifact/no-edit design path rather than separately measured. What *is* + a genuine recall gap on these two states is that `dbt-nothing-built` and + `dbt-deliverable-names` — the validators built to catch a missing + deliverable like real-06's absent `stg_nba_teams.sql` — never activated at + all, for the task-document parsing reason below. + +**Two more gaps worth noting, neither a false positive** — the first is the +genuine recall gap in the task-document parser referenced above; the second is +a narrow, by-design activation precondition rather than a parsing miss: + +* `real-06`'s prompt reads *"Add the missing `models/staging/stg_nba_teams.sql`"*. + `REQUIREMENT_VERB_RE` covers `creat|build|produc|implement|deliver|materiali[sz]|generat|writ|deploy` + — **not `add`**. `real-07` says *"Implement the missing …"* and is picked up. + One word decides whether the contract-driven validators activate at all: of + ten real task documents, exactly one (`real-07`) yielded a literal contract. +* `dbt-dialect-guard` never activated on any of the ten `real-*` tasks or on + stock jaffle_shop: none of those projects establishes a `target.type` + convention, which is the validator's activation precondition. Its real-world + coverage is therefore narrow by design. + +### Adjacent finding: the two pre-existing lane members (out of PR scope) + +Not part of PR #1175, but it matters to anyone deciding whether to switch +`ALTIMATE_VALIDATORS_ENABLED=1` on, because that flag turns on all **seven** +registered validators, not five. + +Probing the full lane against a green, complete `real-01` workspace produced: + +```text +dbt-schema-verify ok:false errored=3/5 models elapsed_ms=10864 +dbt-tests-pass ok:false errored=4/5 models elapsed_ms=13919 +``` + +Zero actual mismatches and zero actual test failures — every one of those is a +subprocess that did not return a parseable result, and both validators treat +"could not verify" as "blocks". Two contributing causes were observed: the +globally-installed `altimate-dbt` predates `schema-verify` entirely (so the +subprocess errors out), and even with a freshly built binary on `PATH` the +validators fan out at `concurrency_limit: 4` against a single DuckDB file, which +is single-writer. They also cost **11–14 s each**, three orders of magnitude +more than the five under test. + +Separately, `schema-verify` treats columns present in the model but absent from +`schema.yml` as `columns_extra` — and partially-documented models are the norm +in real dbt projects, so that is a large latent false-positive surface on its +own. None of this is PR #1175's doing, but it means "enable the lane" and +"enable these five" are very different decisions. + +--- + +## Measurement 2 — A/B conversion (does it help?) + +**Read the sample-size disclosure at the top of this document first. This +section does not support a conclusion about conversion.** + +### Design (as intended) + +Same model, same prompts, same pinned seed (1001) and temperature (0.2), same +`--max-turns 40`, same task workspaces. Arm A: no validator env set. Arm B: +`ALTIMATE_VALIDATORS_ENABLED=1` with the five under test registered. Grading by +each task's own `verify.sh`, which wipes `target/`, rebuilds from scratch and +diffs against a golden expectation file — so the grader is independent of +anything the validators looked at. + +### Achieved N + +| | planned | achieved | +|---|---|---| +| tasks | 10 | 2 | +| arms per task | 2 | 2 for `real-01`, 1 for `real-07` | +| rollouts per cell | 2 | 1 | +| total sessions | 40 | **3** | +| complete A/B pairs | 20 | **1** | + +Terminated for machine capacity, not because the result was in. Two further +arm-B sessions (`real-07`, `real-10`) were in flight and were killed; their +partial data is discarded, not reported. + +### Runs completed + +| Task | Arm | verify.sh | wall clock | assistant steps | tool calls | validator dispatches | validator retries | validators that fired | +|---|---|---|---|---|---|---|---|---| +| `real-01-home-team-join` | A (off) | **pass** | 478 s | 14 | 24 | 0 | 0 | — | +| `real-01-home-team-join` | B (on) | **pass** | 392 s | 13 | 16 | 1 | 0 | — | +| `real-07-add-game-staging` | A (off) | **pass** | 867 s | 29 | 37 | 0 | 0 | — | + +### What can and cannot be read from this + +**Cannot be read:** any pass-rate difference, any conversion effect, any wall-clock +or turn-count overhead. One pair is one pair; the 478 s → 392 s difference between +the two `real-01` arms is ordinary run-to-run variance in a nondeterministic +agent loop, not a measured effect of the gate, and it would be dishonest to +present it as one. + +**Can be read**, because they are structural observations rather than statistics: + +1. **The hook fires where it is supposed to and nowhere else.** In arm A the + session logged `validator_hook_reached` on every step with + `validatorsEnabled: false` and **zero** `dispatch_enter` events — the flag-off + path costs nothing, as the code comment claims. In arm B the dispatch ran + exactly once, on the single step where the model declared a clean stop. + +2. **Only two of the five ever became applicable on this task.** + `dbt-nothing-built`, `dbt-deliverable-names` and `dbt-dialect-guard` all + returned `appliesTo: false`, consistent with the offline sweep: the first two + need a task document with a literal deliverable contract (1 of 10 real task + prompts produced one), the third needs a project that already uses + `target.type` guards (0 of 11 projects tested). + +3. **`dbt-build-green`, the lane's central gate, degraded to a no-op on this + real session.** Its own telemetry from the live dispatch: + + ```json + {"name":"dbt-build-green","ok":true,"details":{ + "models_touched":1,"run_results_fresh":true,"verdict":"fresh-build", + "coverage_assertable":false,"model_nodes_in_artifact":0, + "failed_in_scope":[],"not_built":[],"stale_build":[]}} + ``` + + `model_nodes_in_artifact: 0` means the agent's final dbt command left a + `run_results.json` containing test nodes only, so the coverage assertion was + skipped by design (`coverage_assertable: false`). The gate returned `ok:true` + **without checking anything** — it would have passed identically had the + model never been built. The same blind spot reproduces deterministically in + the offline probe (state B6). This is the documented conservative fallback, + but it means the gate's real-world discriminating power depends on which dbt + command the agent happens to run last, which the gate does not control. + +### What a real A/B would require + +On a GCP VM, isolated from developer machines: + +* **Scale:** 10 tasks × 2 arms × 3 rollouts = 60 sessions. At the observed + 392–867 s per session, that is ~9 h serial, or ~2 h at concurrency 5 on a + machine that can take it (each session is one `bun` process plus a DuckDB + build; ~4–6 GB RSS observed per session, so size for ≥8 vCPU / 32 GB). +* **A third arm.** Arm A (off) and arm B (enforce) are not enough, because a + fired validator changes the trajectory and destroys its own counterfactual. + Add **arm S (`ALTIMATE_VALIDATORS_SHADOW=1`)**: validators run and log but do + not enforce, so shadow tells you the true fire rate on unperturbed sessions, + and the arm-A/arm-S verdict difference should be zero (a sanity check on the + harness). Note that arm S is **not** cheaper than arm B in wall-clock: shadow + suppresses the retry, not the work, and still spawns one `altimate-dbt` child + per touched model. Size arm S like arm B minus the retries. +* **Tasks that can actually fire the gates.** Of the 10 `real-*` prompts, only + one yields a deliverable contract and none establishes a `target.type` + convention, so three of the five validators are structurally unreachable on + this task set. A conversion study needs a task set where each validator has a + reachable failure mode — otherwise arm B is arm A with extra logging. +* **A pre-grade workspace snapshot.** `verify.sh` deletes `target/` and + rebuilds, which destroys the very artifact state the validators inspect. + Snapshot the workspace before grading so post-hoc probes are valid. +* **Report per-dispatch telemetry, not just pass rates** — `dispatch_result` + already carries everything needed (`coverage_assertable`, `not_built`, + `stale_build`, per-validator `elapsed_ms`). + +--- + +## Negative control — non-dbt project + +A live session in a small TypeScript repo (`/tmp/valexp/negctl/repo`: two source +files, a `bun test` suite, a README, no dbt anywhere), with the **full lane** +enabled and the artifact gate additionally forced on: + +```text +ALTIMATE_VALIDATORS_ENABLED=1 +ALTIMATE_VALIDATORS_REQUIRE_ARTIFACTS=1 +(ALTIMATE_VALIDATORS_ONLY unset — all seven validators registered) +``` + +Task: *"Add a `stddev` function to src/stats.ts and a unit test for it. Run +`bun test`."* The session completed the work (`stddev` present in both files) +and stopped cleanly. + +Validator events, verbatim from the session's stderr: + +```text +validator_hook_reached step=1..5 finish=tool-calls validatorCount=7 +validator_hook_reached step=6 finish=stop validatorCount=7 +dispatch_enter step=6 +dispatch_result step=6 checks_count=0 results=[] +``` + +**Zero validators executed.** All seven were registered and the dispatch ran, +but every `appliesTo()` returned false because `findDbtProjectRoot()` finds no +`dbt_project.yml`. No synthetic message was injected (`grep -c +"altimate-validator:" trace.jsonl` → 0). The gate is inert outside dbt, as +designed, even with the most aggressive opt-in set. + +--- + +## Cost overhead + +Per-validator `check()` wall time, measured across 50 probe invocations on the +small projects (5–6 models): + +| Validator | mean | max | +|---|---|---| +| `dbt-build-green` | 1.4 ms | 4 ms | +| `dbt-incremental-config` | 1.7 ms | 5 ms | +| `dbt-dialect-guard` | 1.2 ms | 2 ms | +| `dbt-nothing-built` | 1.3 ms | 2 ms | +| `dbt-deliverable-names` | 0.8 ms | 2 ms | + +Whole-dispatch wall time on those projects: **mean 10 ms, max 39 ms.** +Negligible. + +**Scaling is the caveat.** On a synthetic 2 005-model project: + +| Scenario | `dbt-build-green` | `dbt-incremental-config` | dispatch total | +|---|---|---|---| +| all 2 005 models modified this session | 300 ms | 738 ms | 2 853 ms | +| zero models modified this session | 649 ms | (skipped) | 3 454 ms | + +Cost is dominated by the directory walk, not by the analysis, and **it is paid +even when nothing was touched**. Each validator runs its own independent +`modelsModifiedSince()` / project scan with no shared work or caching, so the +tree is walked up to five times per dispatch, and the dispatch fires on every +clean stop. On a large monorepo that is seconds of wall time per turn boundary. + +The dominant cost of enforce mode is not CPU — it is the injected retry turn, +which is a full model turn plus whatever tool calls the model makes in response. + +--- + +## Verdict + +**Shadow only. Do not enable by default yet.** Two independent reasons. + +**1. Five reproducible false positives, and they are not exotic.** Editing an +ephemeral model, disabling a model, touching a file after the build, nesting a +Jinja `if` inside a dialect guard — these are ordinary dbt work, not defects. In +enforce mode each one costs a session a synthetic retry turn, and the fix hint +tells the agent to do something that is either impossible (`eph_helper` can never +appear in `run_results`) or wrong (rebuild after a whitespace change). A gate +that burns its retry budget on non-problems is exactly the failure mode the +lane's own design doc warns about. All four of the highest-frequency ones are +small, contained fixes: + +* `dbt-build-green`: exclude nodes that dbt legitimately omits from + `run_results` — resolve `manifest.json` for `enabled: false` and for + `materialized: ephemeral` before asserting coverage. +* `dbt-build-green`: compare **content**, not mtime, for the stale check, or + raise the tolerance far above 1 s and skip files whose comment-stripped SQL is + unchanged since the build. +* `dbt-dialect-guard`: match `{% if %}`/`{% endif %}` by nesting depth instead + of a non-greedy regex; mask string literals as well as comments. + +**2. There is no conversion evidence at all.** N = 1 paired task, and that pair +needed no retry. Nothing here shows the gate turns a failure into a pass, and +nothing here shows it does not. The claim in PR #1175 that these gates improve +outcomes is, on this evidence, **untested** — which is a different and more +honest statement than "disproven". + +**What the evidence does support.** The safety envelope is genuinely +conservative where it was designed to be: 0 firings across 27 naturalistic +known-good end-states, 0 false positives from three of the five validators, a +clean negative control in a non-dbt repo with the most aggressive opt-in forced +on, and negligible CPU cost on normal projects. The true-positive side works in +part: 8 of 11 constructed known-bad states fired, and 3 were silent. The problem +is not that the lane is +reckless; it is that its blast radius includes a handful of legitimate dbt +practices, and its benefit is unmeasured. + +**Recommended sequence.** + +1. Ship the five behind `ALTIMATE_VALIDATORS_SHADOW=1` only. The lane already + emits per-validator `validator_check` telemetry with `enforced: false`, which + is enough to measure the real fire rate on real traffic. +2. Fix FP-1 through FP-5 (above) and add the five constructed known-good states + as regression tests — they are a few lines each and all five reproduce + deterministically. +3. Widen `REQUIREMENT_VERB_RE` to include `add` (and probably `convert`, + `rename`, `fix`, `repair`); today one verb decides whether two of the five + validators activate at all. +4. Look hard at whether `dbt-build-green`'s coverage assertion should survive + `dbt test` overwriting `run_results.json`. Reading `manifest.json` (which is + not overwritten by `dbt test`) alongside the run artifact would close the + blind spot that made the gate a no-op on the one real session measured here. +5. Only then run the VM-based three-arm A/B described above, and decide on + enable-by-default from those numbers. + +**Not recommended:** reverting. The checks are cheap, the design is sound, the +true-positive behaviour is real, and the defects found are localized bugs rather +than a flaw in the approach. + +**Also not recommended:** flipping `ALTIMATE_VALIDATORS_ENABLED=1` as a lane-wide +default, on the strength of these five. That flag also enables +`dbt-schema-verify` and `dbt-tests-pass`, which in this environment blocked a +green, complete project on subprocess errors alone and cost 11–14 s each. Those +two need their own evidence before the lane ships enabled. + +--- + +## Reproducing this + +Probe harness (not committed; recreate at the repo root): + +```ts +// validator-probe.ts — bun run validator-probe.ts