feat(validators): deterministic completion gates — zero-write, build-green, literal deliverables, config/dialect lints - #1175
Open
anandgupta42 wants to merge 21 commits into
Open
feat(validators): deterministic completion gates — zero-write, build-green, literal deliverables, config/dialect lints#1175anandgupta42 wants to merge 21 commits into
anandgupta42 wants to merge 21 commits into
Conversation
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.
Issue for this PR
Closes #1174
Type of change
What does this PR do?
Adds five deterministic completion-gate validators to the existing
ALTIMATE_VALIDATORS_ENABLEDlane, and closes a structural blind spot in that lane. Every check is answer-free — it asserts structure, invariants, or the task's own literal contract, never a known-correct output — so the gates work on unseen tasks.Closes the zero-write blind spot. Both pre-existing validators key on "did the session modify models", so a session that authored nothing passed every gate by default.
dbt-nothing-builtis an inverse gate: in a dbt project, with no session-authored files and no fresh successful run artifact, the session is not done. It is deliberately conservative —appliesToreturns false unless a task document literally names required deliverables, orALTIMATE_VALIDATORS_REQUIRE_ARTIFACTS=1is set — so genuinely read-only/analysis sessions are unaffected.New validators
dbt-nothing-builtdbt-build-greendbt-deliverable-namesdbt-incremental-configmerge/delete+insertwithoutunique_key; missingis_incremental()guard where the task literally requires idempotency; non-deterministic calls inside the guard predicatedbt-dialect-guardtarget.typeguards (or opt-in via env)Conservative by construction. No fuzzy matching anywhere — required names come from three literal tiers only. No discoverable source of required names means a silent skip, never a false failure.
dbt_project.yml-inherited config is intentionally not resolved rather than guessed. Non-determinism outside anis_incremental()predicate is advisory detail, never blocking. Out-of-scope build failures are telemetry, not a block. Per the lane's existing contract, a validator that throws soft-passes, so a buggy check cannot brick a session.Also adds
docs/internal/deterministic-checks-engine-split.md, assessing two further candidate checks that need real SQL parsing rather than filesystem/regex analysis, and where each belongs relative to the engine's existing capabilities.Reviewer-critical: these gates almost never get to run on a current build
None of the dispatch code is in this PR.
git diff main...HEADtouches no file underpackages/opencode/src/session/. The hook already exists onmain; this PR only adds validators to the registry it calls. The consequence is that the value of this PR is contingent on how often that hook fires, which is a property of the harness, not of these five validators.The gate condition, at
packages/opencode/src/session/prompt.ts:1405-1412:Two conditions skip dispatch entirely, and neither logs anything beyond the
validator_hook_reachedline:processor.message.finish === "stop"— the model must declare a clean stop. Any other finish reason (tool-calls,length,unknown, or an error) skips.result !== "compact"—processor.process()returns"compact"when the step ended in compaction (packages/opencode/src/session/processor.ts:661), and those steps skip too.result === "stop"(blocked, or an assistant-message error) skips as well.Measured on a pre-#1171 build, 9 trials / 72
validator_hook_reachedevents:finishresulttool-callscontinuetool-callscompactstopcontinueThe gate fired on 1.4% of step-ends. All seven validators registered every time, so registration is fine, and the one dispatch went hook →
dispatch_enter→dispatch_resultwith no throw or hang: the mechanism is correct whenever it is reached. The accurate word is structurally starved, not broken — sessions on that build end by exhausting turns or by compacting, not by declaring completion.#1171 is the dependency. It replaces trusting a bare provider finish-stop with explicit
DONE-token termination (session/termination.ts). Until it lands, enabling this lane on a current build is close to a no-op. A reviewer evaluating this PR on merit should know that the pre-#1171 measurements say nothing about whether these gates work.What the single dispatch showed, reported because it is the only enforcement event in the run and a reviewer should hear it here rather than find it later:
Three of this PR's five validators did not declare themselves applicable at all. The two that ran both passed. The only validator that failed was
dbt-tests-pass, which is pre-existing, not one of the five — and it converted nothing: theoff,shadowandenforcearms all passed that task, so the retry it triggered was spent on work that would have succeeded anyway. This is N = 1 on a build where the gate is starved; it is not a verdict on the five, in either direction.Open design question for the reviewer, deliberately not acted on here. The two endings where dispatch is silent — budget exhaustion and compaction — are arguably the two where a completion gate is most valuable, because they are the endings most likely to hand a user unfinished work. There is a defensible reading (the gates check a claim of completion; no claim, nothing to validate; a stalled run has already failed by other means) and a worrying one (this is a gate that is quiet precisely when it should be loud, which is the same shape as the
dbt-build-green-passes-with-zero-models bug this PR exists to fix). Changing it means running validators on abnormal finish reasons and converting a silent stall into an explicit failure — a change to shared harness code outside this PR's scope. Flagging it for a decision rather than making it.Recommendation: shadow mode first, not enable-by-default
docs/internal/validator-e2e-evidence.mdreports an end-to-end run against real dbt projects rather than fixtures. The recommendation from it isALTIMATE_VALIDATORS_SHADOW=1only, and it rests on two independent findings.1. Five reproducible false positives, since fixed — and the class of bug matters more than the count. Across 38 known-good states the gates fired five times, every one on ordinary dbt practice rather than on a defect: editing an ephemeral model, disabling a model on purpose, touching a file seconds after a green build, a nested
{% if %}inside atarget.typeguard, and a dialect function name inside a string literal. In enforce mode each costs a session a synthetic retry turn, and in two of the five the fix hint asked for something impossible. All five now have regression tests asserting no firing on the known-good state, and the evidence run also found thatdbt-build-greenreturned green having checked nothing when the session's last dbt command was adbt test(which overwritesrun_results.jsonwith test nodes only). That is fixed too — coverage now also reads the model DDL under<target>/run/, which a test invocation does not touch.Fixing the observed false positives does not establish that no others exist. Shadow mode is how you find out, on real traffic, without spending anyone's retry budget.
2. There is no conversion evidence in either direction. The planned A/B was 10 tasks x 2 arms x 2 rollouts. Three sessions completed: N = 1 paired task plus one unpaired run, terminated for machine capacity. The single pair needed no retry, so there was nothing to convert. Nothing here shows these gates turn a failure into a pass, and nothing here shows they do not — the honest word is untested, not disproven. What a properly powered run would need (three arms including shadow, a task set where each gate is reachable, a pre-grade workspace snapshot) is specified in the evidence document.
Cost. ~2-10 ms per dispatch on a small project, but ~1-3.5 s on a 2 000-model project even when the session touched no models, because each validator walks the tree independently. That cost is paid on every dispatch, and consolidating the walks is recorded as a follow-up rather than done here.
ALTIMATE_VALIDATORS_ENABLED=1is not the same decision as "enable these five." That flag activates all seven registered validators, including the two pre-existing ones. Probed against a green, complete project,dbt-schema-verifyanddbt-tests-passboth returnedok:falsewith zero actual mismatches and zero actual test failures — every one was a subprocess that did not return a parseable result, and both treat "could not verify" as "blocks". They also cost 11-14 s each. None of that is this PR's doing, but it is what switching the lane on today would actually do.Suggested sequence: shadow these five and measure the real fire rate; then run the three-arm A/B on isolated infrastructure; then decide on enforcement from those numbers.
How did you verify your code works?
bun test test/altimate/validators/-> 574 pass, 132 skip, 0 fail. That includes regression tests for each observed false positive, for the test-overwrite blind spot and its discriminating converse, and for the shared task-parsing and SQL-scrubbing utilities.bun test test/session/ test/altimate/-> 5039 pass, 649 skip, 2 fail. Both failures are pre-existing and were confirmed by re-running them on a detachedorigin/maincheckout: a 5 s timeout intest/session/prompt.test.tsand a PostgreSQL driver E2E that requires a local database.bun run typecheckclean; marker guard (--markers --base main --strict) clean.Not verified, stated plainly:
Deferred and declined review findings are recorded with rationale in
.github/meta/harness-review-followups.md.Repo note:
script/upstream/analyze.tsfails out of the box in a fresh worktree withCannot find package 'minimatch'(no longer a transitive dep sinceglob@13). Worked around transiently to run the marker check; worth fixing separately.Screenshots / recordings
N/A - no user-visible surface; these run inside the completion-gate lane.
Checklist
Note
Medium Risk
These gates can block session completion and inject synthetic retries from filesystem/task parsing heuristics; mitigations are opt-in/shadow mode and conservative skips, but documented gaps (prose mining, bare node names, mutable task files) remain.
Overview
Adds five opt-in completion validators to the altimate validator lane and registers them before the existing schema-verify / tests-pass checks so “nothing built” and “build not green” run first.
dbt-nothing-builtblocks finish when a task (orALTIMATE_VALIDATORS_REQUIRE_ARTIFACTS) names deliverables but the session authored nothing matching those names and has no fresh successful build evidence—including treating existing on-disk deliverables as satisfied so stale task docs don’t trap unrelated work.dbt-build-greenties session-edited models to fresh build evidence: rejects stale or non-executingrun_results.json(e.g.dbt compile), falls back to<target>/run/DDL when tests overwrite artifacts, exempts ephemeral/disabled models, and emits finer verdicts (fresh-build,build-unproven,nothing-verified, etc.) so shadow telemetry doesn’t count unverified passes as green.dbt-deliverable-namescompares literal task contract names to produced model/seed/snapshot inventory (workspace + dbt roots for required files).dbt-incremental-configanddbt-dialect-guardlint touched models for incremental contradictions and unguarded warehouse-specific calls (dialect guard only when the project already usestarget.typeguards or env opt-in).Shared behaviour implied by the new modules includes
resolveDbtSourcePaths, split run-results predicates for model coverage vs deliverable production,sanitizeForPrompton retry text, and a 60s post-build mtime grace window.Also adds internal docs: deferred review inventory (
.github/meta/harness-review-followups.md), engine-vs-lane split assessment, and a superseded E2E evidence report with rollout guidance (shadow first).Reviewed by Cursor Bugbot for commit dd858ef. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Closes #1174 by adding five deterministic completion gates to the altimate validator lane and closing its zero-write blind spot — sessions that declared done with nothing built, stale builds, missing deliverables, contradictory incremental configs, or unguarded warehouse-specific SQL. The gates are answer-free, off by default, and conservative, skipping rather than failing on ambiguous evidence. The second review sweep fixed silent under-firing gates, blocking false positives, and artifact miscounting, all locked down with fixture-based regression tests.
The gates
dbt-nothing-builtblocks when the session produced nothing and the task literally names deliverables (orALTIMATE_VALIDATORS_REQUIRE_ARTIFACTS=1is set); a named deliverable already on disk satisfies it. Only models, seeds, and snapshots contribute names, successful seed/snapshot artifacts count as produced, anddata/is no longer treated as a default seed path.dbt-build-greenblocks edited models not covered by a fresh successful build. Known-non-executing commands (dbt compile) are rejected, unrecognized commands trusted, and seed/snapshot/clone artifacts fall through tobuild-unprovenDDL-only coverage. Ephemeral/disabled exemptions stay per-axis and can't be contradicted by source config; dead{% if false %}/{% raw %}arms no longer strip live config; staleness dates from build completion rather than model DDL.dbt-deliverable-namesdiffs literal task deliverable names against project model/seed/snapshot names;dbt-incremental-configflagsmerge/delete+insertwithoutunique_key, missing idempotency guards, and non-determinism in the incremental arm;dbt-dialect-guardflags unguarded warehouse functions in projects already usingtarget.typeguards.unique_key = Noneas absent,and/orpredicates only when no WHERE-like clause exists, idempotency demands read across every task file, column-0 config keys sovars:can't shadow them, Jinja modulo-tolerant guards, identifier-boundedis_incremental()matches, and repository text sanitized before entering synthetic instruction turns.Evidence and rollout
ALTIMATE_VALIDATORS_SHADOW=1first — shadow suppresses only the retry, not the validator or warehouse cost.docs/internal/deterministic-checks-engine-split.mdscopes engine work:dbt-incremental-configduplicates the engine'sdbt_config_lintand should be rewired; onlydbt-dialect-guard's function list warrants reconciling withL033. The dialect-guard branch-semantics gap andscript/upstream/analyze.ts's missingminimatchdep remain recorded follow-ups.Written for commit dd858ef. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation