From 850a9c5d5633b500d443edcb0981d6a933636b1f Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Tue, 7 Jul 2026 16:54:37 -0700 Subject: [PATCH 1/9] feat: greenfield spec-test synthesis for dbt-pr-review (P0: propose-only) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an auxiliary lane that generates candidate tests for greenfield (git-added) dbt models from their declared intent, raising the no-reference verification ceiling that `missingGrainTestLane` only flagged. Two strictly separated tracks: - Track A (deterministic): materialize declared-but-unenforced dbt constraints. - Track B (LLM, advisory): propose new tests from soft intent. P0 is propose-only: every finding is `suggestion` + `confidence: unknown` under the `altimate.spec_test.proposed` evidence tool, which is excluded from the verdict warning-count — it can never block a PR. Emits a committable `schema.yml` patch as authoring help. - `spec-test-gen.ts`: types, `filterToSpecDerived` anti-fabrication guard, `isBlockEligible`, `runSpecTestGen` transport (inline prompt for P0). - `orchestrate.ts`: `specTestSynthesisLane` gated on added models + injected `generateSpecTests` (pure, like `aiReview`). - `risk-tier.ts`: `spec_tests` in lite/full tiers. - `verdict.ts`: exclude `altimate.spec_test.proposed` from warning accumulation. - `config.ts`: `specTests.execute` shape (P1). `format.ts`: "Proposed tests". - Design specs under `packages/opencode/specs/`. Refs #989 Co-Authored-By: Claude Opus 4.8 --- .../opencode/specs/corrective-app-memory.md | 164 ++++++++ .../specs/greenfield-spec-test-synthesis.md | 268 +++++++++++++ .../opencode/src/altimate/review/config.ts | 6 + .../opencode/src/altimate/review/format.ts | 87 ++++- .../opencode/src/altimate/review/index.ts | 1 + .../src/altimate/review/orchestrate.ts | 272 +++++++++++++ .../opencode/src/altimate/review/risk-tier.ts | 11 +- packages/opencode/src/altimate/review/run.ts | 2 + .../src/altimate/review/spec-test-gen.ts | 359 ++++++++++++++++++ .../opencode/src/altimate/review/verdict.ts | 8 +- .../altimate/review-spec-test-gen.test.ts | 254 +++++++++++++ 11 files changed, 1429 insertions(+), 3 deletions(-) create mode 100644 packages/opencode/specs/corrective-app-memory.md create mode 100644 packages/opencode/specs/greenfield-spec-test-synthesis.md create mode 100644 packages/opencode/src/altimate/review/spec-test-gen.ts create mode 100644 packages/opencode/test/altimate/review-spec-test-gen.test.ts diff --git a/packages/opencode/specs/corrective-app-memory.md b/packages/opencode/specs/corrective-app-memory.md new file mode 100644 index 000000000..bb94c73c1 --- /dev/null +++ b/packages/opencode/specs/corrective-app-memory.md @@ -0,0 +1,164 @@ +# Spec: Corrective, attribute-keyed app-memory + +Status: draft · Owner: anand · 2026-07-07 +Scope: shared substrate; first consumers = `dbt-pr-review` test-acceptance + +reviewer finding-acceptance. Companion to `greenfield-spec-test-synthesis.md` (§P2). + +## 1. Problem + +Two memory needs in the product, both currently unserved or served badly: + +1. **dbt-pr-review learning.** The spec-test-synthesis agent generates tests per + repo. Some a team keeps, some it dismisses as noise. Today that signal + evaporates — the next PR regenerates the same dismissed tests. +2. **altimate-code's own reviewer/skill memory.** Stored today as raw markdown + traces (`MEMORY.md` is already ~34KB and self-warning about size). + +Both are the failure the BAIR "Intelligence is Free" piece names: **file-based +raw-trace memory doesn't scale, and raw traces induce repeated mistakes.** Its +prescription — the thing to build — is *structured, corrective* memory: organize +across attributes, each `*` (universal) or a specific value, and store the +**correction**, never the trace. + +The move that makes this defensible (and not just a cache): store author- +*committed* corrections keyed by the attributes of the situation, retrieve by +most-specific match, and **never let a learned entry silently escalate a block.** + +## 2. Data model + +A memory entry is `(scope, correction, provenance)`. + +```ts +interface MemoryScope { + project: string // tenant / repo — always specific (isolation boundary) + // Each below: a specific value OR "*" (universal). Retrieval matches specificity. + modelLayer?: string | "*" // staging | intermediate | marts | ... + table?: string | "*" + column?: string | "*" + operation?: string | "*" // date | dedup | join | pii | ... + warehouse?: string | "*" + category?: string | "*" // ReviewCategory, or a domain tag + derivedFromKind?: string | "*" // for test-acceptance: contract|grain|range|referential|not_null +} + +interface MemoryEntry { + id: string // fingerprint of the canonicalized scope + directive + scope: MemoryScope + directive: string // the correction: "prefer X" / "do NOT flag Y" / "suppress test kind Z here" + polarity: "prefer" | "suppress" // reinforce vs. suppress the behavior + provenance: { + source: "accepted_pr" | "explicit_dismiss" | "human_rule" + committed: boolean // true only when a human/PR action ratified it + supportCount: number // times observed; merges increment, they don't append + lastSeen: string // ISO; drives decay/expiry + } +} +``` + +Non-negotiables that keep this from becoming raw traces: + +- **Store the directive, not the transcript.** An entry is a single correction, + not a session log. No PR bodies, no diffs, no model output. +- **Merge, don't append.** Two observations with the same canonical scope+directive + update one entry (`supportCount++`, refresh `lastSeen`) — the store does not grow + per event. This is what bounds size where `MEMORY.md` doesn't. +- **Corrective, not descriptive.** "when deduping `int_*` models, dismiss + `referential` tests on unenforced sources" — a rule that changes future + behavior, not a note about the past. + +## 3. Retrieval + +Given a situation described by the same attributes, return the merged set of +applicable entries, **most-specific match wins** on conflict: + +- Score each matching entry by count of non-`*` scope fields that match. +- Higher specificity overrides lower (a `column`-specific `suppress` beats a + `layer`-wide `prefer`). +- Ties → higher `supportCount`, then more recent `lastSeen`. +- Expired/low-support entries (`supportCount < N` past a decay window) are not + returned — a single fluke never becomes a standing rule. + +Retrieval is a pure function of `(scope query, store)` → deterministic, testable. + +## 4. Consumers + +### 4.1 dbt-pr-review test acceptance (the P2 loop) + +- **Write:** when a generated test's proposed patch is merged → `accepted_pr` + `prefer` entry keyed by `(project, modelLayer, derivedFromKind, category)`. + When a proposed test is explicitly dismissed → `explicit_dismiss` `suppress` + entry, same key. +- **Read:** `SpecTestGenInput` gains a `priors: MemoryEntry[]` field. The + generator is told: prefer kinds this repo keeps, avoid kinds it dismisses. + Generation gets sharper per-repo each PR **without** retraining anything. + +### 4.2 Reviewer finding acceptance (generalizes static rubric exclusions) + +- **Write:** a dismissed/muted finding → `suppress` entry keyed by the finding's + `(category, model/column, ruleKey)`. +- **Read:** the orchestrator's post-filter (orchestrate.ts:1336, currently + `exclusionReason` over the static rubric) additionally consults learned + `suppress` entries — turning the hand-written exclusion list into a learned one. + +### 4.3 altimate-code reviewer/skill memory + +Same store replaces the raw-`.md` reviewer memory: corrections keyed by +`(operation, table, column, warehouse)` instead of prose files. The Spider2 +`builder.txt` improvements are already de-facto corrective memory — this promotes +that pattern into the layer itself. + +## 5. Safety: learned memory must not silently escalate + +Mirrors the test-gen honesty rule. A learned entry may: + +- **bias generation** (which tests to propose), and +- **suppress/downgrade** advisory findings (raise precision). + +A learned entry may **NOT**: + +- escalate any finding to blocking, or manufacture a new blocking finding. + +Rationale: `suppress`/`prefer` only ever *reduce* noise or *reweight* proposals — +both fail safe. Escalation off a learned pattern would let accumulated bias +force `REQUEST_CHANGES`, exactly the "livelock / drift" risk. Blocking stays +gated on deterministic engine proof (equivalence, executed author-declared +constraints), never on memory. `provenance.committed` is recorded so a future +governance tier *could* allow human-ratified escalation, but P0 does not. + +## 6. Storage & governance + +- **Per-project, tenant-scoped** (the isolation boundary; `project` is never `*`). +- Backing store: append-and-compact JSONL or a small SQLite table — but the API + is `get(scopeQuery)` / `record(entry)` with merge semantics, so the backend is + swappable. No embeddings needed (structured attribute match, not semantic + search — the article's specific critique of the KG/embedding approach). +- **Reviewable & exportable:** entries are inspectable; `.altimate/review.yml` + can pin (force-keep) or forbid (never-learn) scopes. Memory is a config surface, + not a black box. +- **Decay:** entries below support threshold expire; the store self-prunes. + +## 7. Build phases + +- **P0 — read-path with seeded rules.** The `MemoryEntry` schema + deterministic + `get(scopeQuery)` retrieval + `.altimate/review.yml` hand-authored entries. + Wire into `SpecTestGenInput.priors` and the reviewer post-filter. No learning + yet — proves the retrieval + safety model with static data. +- **P1 — write-path.** Record `accepted_pr` / `explicit_dismiss` entries from PR + merge/dismiss events with merge semantics + decay. +- **P2 — reviewer/skill memory migration.** Move altimate-code's raw-`.md` + reviewer memory onto the same store. + +## 8. Interfaces (summary) + +| File | Change | +|------|--------| +| `corrective-memory.ts` (new, shared) | `MemoryScope`, `MemoryEntry`, `get()`, `record()` with merge/decay | +| `spec-test-gen.ts` | `SpecTestGenInput.priors`; generator prompt consumes them | +| `orchestrate.ts` | post-filter consults learned `suppress` entries | +| `config.ts` | `memory: { pin, forbid }` scopes | + +## 9. Non-goals + +- Not a semantic/embedding memory — deliberately structured attribute match. +- Not cross-tenant — `project` is the hard isolation boundary. +- Not a path to new blocking decisions (§5). diff --git a/packages/opencode/specs/greenfield-spec-test-synthesis.md b/packages/opencode/specs/greenfield-spec-test-synthesis.md new file mode 100644 index 000000000..35c55e764 --- /dev/null +++ b/packages/opencode/specs/greenfield-spec-test-synthesis.md @@ -0,0 +1,268 @@ +# Spec: Auxiliary spec-test synthesis for greenfield dbt models + +Status: draft (rev 2 — Codex review folded in) · Owner: anand · 2026-07-07 +Scope: `packages/opencode/src/altimate/review/*` (dbt-pr-review) + +> **rev 2 changelog (Codex adversarial review, 2026-07-07).** The rev-1 honesty +> guard (a `derivedFrom` tag check) was necessary but **not sufficient** — it +> catches invented refs but not *back-labeling* (assert current output, then tag +> it with a real column). Rev 2 removes LLM trust from the blocking path +> entirely: block-eligible tests are **deterministically materialized from parsed +> declared constraints**, never LLM-authored; everything the LLM proposes is +> advisory and structurally cannot block. See §2, §3.5, §3.6. + +## 1. Problem + +dbt-pr-review has two verification worlds: + +- **Reference-exists** (a `modified` model): base SQL is a trusted reference, so + `semanticChangeLane` → `runner.equivalence(oldSql, newSql)` gives a provable + non-regression claim. This branch works today. +- **Greenfield** (an `added` model): no reference. Today only lint/grade + (`qualityLane`), PII, `dbtConfigLane`, and `missingGrainTestLane` fire — and + the last only *flags* "this new model has no uniqueness test." The reviewer + detects the missing coverage but does nothing about it. + +The BAIR "Intelligence is Free" piece (data systems *by* agents) names the fix: +specs are imperfect, so pair the artifact with an auxiliary agent that generates +test cases to expand the spec. Applied here, for a greenfield model, in two +strictly separated tracks: + +1. **Materialize declared-but-unenforced constraints** (deterministic, no LLM). + The author declared `not_null`/`unique`/`accepted_values`/`relationships` or a + contract column type, but no test actually enforces it. We build the check + from the *parsed* constraint and run it. A failure here is a real, provable + violation of an author-committed constraint → block-eligible. +2. **Propose new tests from soft intent** (LLM, advisory). Column descriptions, + `ref()` edges, naming conventions, PR text. These are hypotheses about intent, + offered as a committable patch. They **never block** and are labeled as + candidates, not verification. + +## 2. The crux: where LLM output is allowed to matter + +An LLM told to "write tests" defaults to a **change-detector** (read current +output, assert it) and will happily back-label it to a real column. A tag check +cannot distinguish that from genuine derivation. So the design does not rely on +one: + +- **Blocking path — no LLM.** Block-eligible tests are materialized by code from + a parsed declared constraint. The constant/column/operator come from the dbt + artifact (`schema.yml`/contract), not from model text. The LLM is not in this + loop, so it cannot manufacture a blocking finding, back-labeled or otherwise. +- **Advisory path — LLM, fenced off.** LLM-proposed tests are tagged + `origin: inferred_context`, emitted at `confidence: "unknown"` under a + dedicated evidence tool, and are **structurally excluded from the verdict** + (§3.5). A back-labeled or wrong LLM test is at worst noise in a proposed patch, + never a block. + +Source taxonomy (drives everything): + +| origin | sources | LLM authors assertion? | block-eligible? | +|--------|---------|------------------------|-----------------| +| `declared_constraint` | schema.yml `not_null`/`unique`/`accepted_values`/`relationships`; enforced contract column type/PK | **No** — materialized from the parsed constraint | Yes (when executed & failed) | +| `inferred_context` | column descriptions, `ref()`/`source()` edges, naming conventions, PR title/body | Yes — proposed test | **Never** | + +Explicitly **not** treated as declared intent (rev-1 error corrected): a `ref()` +edge is not FK existence; `dim_*` is not proof of uniqueness; "the customer's +email" does not imply `not_null`; compiled SQL is context, never an expected +value. All of these are `inferred_context` → advisory only. + +The deterministic guard `filterToSpecDerived` (spec-test-gen.ts) still runs on the +advisory track — it drops proposals with no `derivedFrom`, a disallowed kind, or a +`ref` we did not extract ourselves (anti-fabrication). But it is **not** what +makes the feature sound; the source split + the no-LLM blocking path is. + +## 3. Design + +### 3.1 Flow + +``` +added model file + ── track A (deterministic, block-eligible) ───────────────────── + → runner.declaredConstraints(model) [parsed schema.yml/contract + provenance] + → keep constraints with NO enforcing test + → materialize each into a dbt generic test [code, not LLM] + → runner.runGeneratedTests(...) [P1; null if no warehouse] + → executed & failed → contract_violation finding (block-eligible) + + ── track B (LLM, advisory) ───────────────────────────────────── + → gather inferred_context sources (descriptions, refs, PR intent) + → generateSpecTests(...) [cheap LLM, injected] + → filterToSpecDerived(...) [drop fabricated/untagged] + → emit as proposed-test findings (confidence unknown) + schema.yml patch + → (P1) optionally execute in a sandbox → "candidate fails on current data" + (still advisory — NEVER a contract_violation) +``` + +### 3.2 Types (spec-test-gen.ts) + +```ts +type SpecOrigin = "declared_constraint" | "inferred_context" + +interface SpecSource { + origin: SpecOrigin + kind: "not_null" | "unique" | "accepted_values" | "relationships" + | "column_type" | "schema_desc" | "ref_edge" | "pr_intent" + ref: string // stable id we extracted, e.g. "schema.yml:dim_customer.email" + text?: string // declared value/description + args?: Record // parsed constraint args (accepted_values set, rel target) +} + +interface GeneratedTest { + id: string // deterministic; results key off this, NOT array order (§3.4) + kind: GeneratedTestKind + dbtTest?: { column?: string; test: string; args?: Record } + assertionSql?: string // advisory track only; sandboxed (§3.7) + rationale: string + derivedFrom: SpecSource // origin decides block-eligibility +} +``` + +### 3.3 Runner additions (`ReviewRunner`, orchestrate.ts:114) + +New — do **not** reuse `declaredPrimaryKey()` for escalation (it's a lossy PK +proxy). A typed constraint reader with provenance: + +```ts +/** Parsed, enforce-able constraints declared for a model, with provenance — + * schema.yml tests + enforced contract. Each carries origin + source ref so the + * lane can tell a declared constraint from an inferred one. */ +declaredConstraints?(model: string): Promise + hasEnforcingTest: boolean // is there already a dbt test for this? + uniqueId?: string // manifest node id (provenance) + sourceRef: string // "schema.yml:." etc. +}>> + +/** Execute generated tests. Returns results KEYED BY test id (not array order). + * null when no warehouse/driver (same contract as dataDiff). */ +runGeneratedTests?( + tests: GeneratedTest[], + warehouse?: string, +): Promise | null> +``` + +### 3.4 Result mapping + +`runGeneratedTests` returns a map keyed by `GeneratedTest.id`, so dropped/errored +tests or batched execution can never attach a failure to the wrong test. + +### 3.5 Verdict gating — defense in depth, not lane discipline alone + +Two layers, because `computeIdealVerdict` only sees `severity`+`category` +(verdict.ts:52) and would block on any `critical contract_violation`: + +1. **Lane discipline.** Only track-A (executed + `declared_constraint`) failures + are emitted as `critical contract_violation`. Track-B findings are emitted at + `severity: warning`, `confidence: "unknown"`, evidence tool + `altimate.spec_test.proposed`. +2. **Verdict-layer enforcement (new).** `computeIdealVerdict` is hardened so a + generated-test finding may reach a blocking verdict **only** when its evidence + carries `executed: true` AND `origin: "declared_constraint"`. And the + warning-accumulation count (verdict.ts:63) excludes the + `altimate.spec_test.proposed` tool exactly as it already excludes `ai-review` + — otherwise three failed inferred range tests would block via accumulation + (Codex finding 4). Both rules get their own verdict unit tests. + +Resulting matrix: + +| Situation | Severity | Confidence | Blocks? | +|-----------|----------|------------|---------| +| Track A: declared constraint materialized, **executed & failed** | `critical` (clamped) | `high` | Yes — real `contract_violation`, no LLM in the assertion | +| Track A: no warehouse (couldn't execute) | `suggestion` | `unknown` | No — proposed as an enforcing test to add | +| Track B: LLM proposal, not executed | `suggestion`/`warning` | `unknown` | No (excluded from verdict) | +| Track B: LLM proposal executed & failed (P1 sandbox) | `warning` | `unknown` | No — "candidate fails on current data," never `contract_violation` | +| Any test passed | (no finding) | — | raises the proposed/coverage counters | + +The one blocking path is deterministic end to end: parsed constraint → code-built +assertion → warehouse execution → failure. No back-labeling reaches it because no +LLM text reaches it. + +### 3.6 Envelope + output + +- Add metrics to **`BuildEnvelopeInput`** (not just the zod schema — the summary + is built from findings only, verdict.ts:132): `proposedTests` (track B) and, + for P1, `enforcedConstraints` (track A executed/passed/failed). +- `format.ts` renders **P0 as "Proposed tests"** — authoring help, explicitly not + "verification." "Spec coverage / executed / passed / failed" language is + reserved for P1 track-A execution. Passed + proposed tests are offered as a + committable schema.yml/tests patch; once merged they run in plain dbt CI + forever (the durable spec expansion). + +### 3.7 Execution safety (P1) + +- Track A assertions are code-built dbt generic tests — inherently bounded. +- Any `assertionSql` (advisory track) is treated as untrusted: parse + enforce + single `SELECT` only (no DDL/DML/multi-statement), allowlist referenced + relations to the model + its declared upstreams, cap returned rows, strict + timeout, run under the review warehouse role. Prefer generic dbt test specs + over raw SQL wherever possible. + +## 4. Tiering & cost + +- Gate a `spec_tests` concern into `TIER_LANES.lite`/`.full` (risk-tier.ts:152), + invoked only when the PR **adds** a model. +- Track B runs one cheap-model call per added model (the BAIR premise: auxiliary + per-PR inference is now trivially cheap). +- Track A execution is opt-in via `.altimate/review.yml` `specTests.execute`, + default off (generate-and-propose only) — same posture as `dataDiff`. + +## 5. Failure modes & guards + +1. **Back-labeled change-detector** → cannot reach the block path (no LLM there); + on the advisory track it's at worst a noisy proposal (unknown, non-blocking). +2. **Inferred failure blocks via accumulation** → prevented by excluding + `altimate.spec_test.proposed` from the warning count (§3.5.2) + a verdict test. +3. **Fabricated `critical`** → verdict-layer rule rejects any generated-test + critical lacking `executed:true`+`declared_constraint` (§3.5.2). +4. **Wrong-but-declared constraint fails on valid data** (optional/anonymized/ + late-arriving) → this is a genuine violation of what the author *declared*; the + finding says "your declared `not_null` on `email` fails on 340 rows — enforce + or amend the declaration," which is correct signal, not a false positive. +5. **Unsafe/expensive LLM SQL** → §3.7 sandbox. +6. **Result misattribution** → id-keyed results (§3.4). +7. **Non-determinism across re-reviews** → stable `fingerprint` (finding.ts:107) + keyed `spec_test:`. + +## 6. Build phases + +- **P0 — propose only (no execution). Advisory authoring help, not verification.** + `spec-test-gen.ts` (types + `filterToSpecDerived`), the source-split extraction, + track-B proposal findings (`confidence:"unknown"`) + the schema.yml patch, and + track-A constraints surfaced as **proposed enforcing tests** (also unexecuted → + advisory). Success metric = accepted patches. No blocking, no "coverage" claim. +- **P1 — execution.** `runner.declaredConstraints` + `runGeneratedTests`; track-A + executed failures → the one blocking path; verdict-layer enforcement + tests; + sandbox; `enforcedConstraints` metrics. +- **P2 — feedback loop.** Persist accepted/dismissed proposals as *corrective* + memory (see `corrective-app-memory.md`); feed priors back into track B. + +## 7. Interfaces touched + +| File | Change | +|------|--------| +| `spec-test-gen.ts` (new) | source taxonomy, `GeneratedTest.id`, `filterToSpecDerived`, transport skeleton | +| `orchestrate.ts` | `ReviewRunner.declaredConstraints?` + `runGeneratedTests?`, `OrchestrateInput.generateSpecTests?`, `specTestSynthesisLane` (two tracks), wiring under `spec_tests` | +| `verdict.ts` | verdict-layer enforcement (executed+declared for criticals; exclude `altimate.spec_test.proposed` from warning count); `BuildEnvelopeInput.proposedTests`/`enforcedConstraints` | +| `risk-tier.ts` | `spec_tests` in `lite`/`full` | +| `format.ts` | "Proposed tests" section (P0) + patch block | +| `config.ts` | `specTests: { execute: boolean }` | +| `run.ts` | inject production `generateSpecTests` (cheap model) like `runAiReview` | + +### IP-boundary decision (open) + +`ai-review.ts` keeps its generation prompt + parse/clamp in the compiled core +(`altimate_core.review_ai_*`), TS is transport-only. Track B's prompt should +follow suit → needs a new `altimate_core.review_spec_test_*` method in +altimate-core-internal. **Decision for the user:** (a) add the core method +(honors the IP boundary; cross-repo work) vs. (b) inline a minimal prompt in TS +for P0 speed and move it to core before GA. Track A (deterministic) has no prompt +and no such dependency. + +## 8. Non-goals + +- Not for `modified` models — reference-exists world owns those. +- Not a golden/snapshot generator. +- P0 makes no verification/coverage claim — it authors candidate tests. diff --git a/packages/opencode/src/altimate/review/config.ts b/packages/opencode/src/altimate/review/config.ts index b4f775c2a..e8804fb4f 100644 --- a/packages/opencode/src/altimate/review/config.ts +++ b/packages/opencode/src/altimate/review/config.ts @@ -42,6 +42,12 @@ export const ReviewConfig = z.object({ warehouse: z.string().default(""), }) .default({ enabled: false, warehouse: "" }), + /** Spec-test synthesis. P0 proposes only; execution is reserved for P1. */ + specTests: z + .object({ + execute: z.boolean().default(false), + }) + .default({ execute: false }), /** Rubric overrides, deep-merged onto DEFAULT_RUBRIC. */ rubric: Rubric.partial().default({}), /** Extra path suffixes to exclude from review. */ diff --git a/packages/opencode/src/altimate/review/format.ts b/packages/opencode/src/altimate/review/format.ts index c6bebbc4e..7ff6dad1b 100644 --- a/packages/opencode/src/altimate/review/format.ts +++ b/packages/opencode/src/altimate/review/format.ts @@ -1,3 +1,4 @@ +import YAML from "yaml" import { type Finding, type Severity } from "./finding" import { type VerdictEnvelope } from "./verdict" @@ -34,6 +35,8 @@ export function verdictHeadline(env: VerdictEnvelope): string { /** Full PR/MR summary comment body (markdown), prefixed with the dedup marker. */ export function renderSummary(env: VerdictEnvelope): string { const lines: string[] = [REVIEW_MARKER, "", `## ${verdictHeadline(env)}`, ""] + const proposedTests = env.findings.filter(isProposedTest) + const regularFindings = env.findings.filter((f) => !isProposedTest(f)) if (env.summary.degraded) { lines.push( @@ -46,7 +49,7 @@ export function renderSummary(env: VerdictEnvelope): string { if (!env.findings.length) { lines.push("No issues found in the changed dbt models. 🎉", "") } else { - const grouped = groupBySeverity(env.findings) + const grouped = groupBySeverity(regularFindings) for (const sev of ["critical", "warning", "suggestion"] as const) { const items = grouped[sev] if (!items.length) continue @@ -59,6 +62,7 @@ export function renderSummary(env: VerdictEnvelope): string { } lines.push("") } + if (proposedTests.length) renderProposedTests(lines, proposedTests) } if (env.override) { @@ -111,3 +115,84 @@ function capitalize(s: string): string { function oneLine(s: string): string { return s.replace(/\s*\n\s*/g, " ").trim() } + +function isProposedTest(f: Finding): boolean { + return f.evidence?.tool === "altimate.spec_test.proposed" +} + +function renderProposedTests(lines: string[], findings: Finding[]): void { + lines.push("### Proposed tests", "") + lines.push("Candidate tests to consider adding; this review did not execute them.", "") + for (const f of findings) { + const loc = f.file + (f.startLine ? `:${f.startLine}` : "") + const ref = String((f.evidence?.result as any)?.proposal?.derivedFrom?.ref ?? "") + lines.push( + `- **${f.title}** \n ${oneLine(stripYamlFence(f.body))} \n \`${loc}\`${ref ? ` · \`${ref}\`` : ""}`, + ) + } + const patch = proposedTestsPatch(findings) + if (patch) { + lines.push("", "```yaml", patch, "```") + } + lines.push("") +} + +function stripYamlFence(body: string): string { + return body.replace(/```yaml[\s\S]*?```/g, "").trim() +} + +function yamlFences(body: string): string[] { + return [...body.matchAll(/```yaml\s*([\s\S]*?)```/g)].map((m) => m[1].trim()).filter(Boolean) +} + +function proposedTestsPatch(findings: Finding[]): string { + const merged = new Map() + for (const f of findings) { + for (const yaml of yamlFences(f.body)) { + try { + const doc = YAML.parse(yaml) + for (const rawModel of Array.isArray(doc?.models) ? doc.models : []) { + const modelName = typeof rawModel?.name === "string" ? rawModel.name : "" + if (!modelName) continue + const target = merged.get(modelName) ?? { name: modelName } + mergeTests(target, rawModel) + mergeColumns(target, rawModel) + merged.set(modelName, target) + } + } catch { + // Bodies are generated locally; if one is malformed, omit it from the + // aggregate patch instead of breaking PR summary rendering. + } + } + } + if (!merged.size) return "" + return YAML.stringify({ version: 2, models: [...merged.values()] }).trim() +} + +function mergeTests(target: any, source: any): void { + if (!Array.isArray(source?.tests)) return + target.tests = target.tests ?? [] + for (const test of source.tests) pushUnique(target.tests, test) +} + +function mergeColumns(target: any, source: any): void { + if (!Array.isArray(source?.columns)) return + target.columns = target.columns ?? [] + for (const rawColumn of source.columns) { + const name = typeof rawColumn?.name === "string" ? rawColumn.name : "" + if (!name) continue + let column = target.columns.find((c: any) => c.name === name) + if (!column) { + column = { name } + target.columns.push(column) + } + if (!Array.isArray(rawColumn.tests)) continue + column.tests = column.tests ?? [] + for (const test of rawColumn.tests) pushUnique(column.tests, test) + } +} + +function pushUnique(items: any[], value: any): void { + const key = JSON.stringify(value) + if (!items.some((item) => JSON.stringify(item) === key)) items.push(value) +} diff --git a/packages/opencode/src/altimate/review/index.ts b/packages/opencode/src/altimate/review/index.ts index f668a8879..1ae647ffe 100644 --- a/packages/opencode/src/altimate/review/index.ts +++ b/packages/opencode/src/altimate/review/index.ts @@ -31,3 +31,4 @@ export * from "./schema-context" export * from "./dbt-patterns" export * from "./rule-catalog" export * from "./compiled" +export * from "./spec-test-gen" diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index 68cefafde..5bb0e2b9e 100644 --- a/packages/opencode/src/altimate/review/orchestrate.ts +++ b/packages/opencode/src/altimate/review/orchestrate.ts @@ -1,4 +1,5 @@ import path from "node:path" +import YAML from "yaml" import { type Finding, type ReviewCategory, @@ -15,6 +16,7 @@ import { type ReviewConfig } from "./config" import { type ReviewMode, type VerdictEnvelope, buildEnvelope, signEnvelope } from "./verdict" import { detectModelPatterns, detectSchemaYmlPatterns, splitDiff } from "./dbt-patterns" import { type AiReviewInput } from "./ai-review" +import { filterToSpecDerived, type GeneratedTest, type SpecSource, type SpecTestGenInput } from "./spec-test-gen" /** * The deterministic review recipe. @@ -187,6 +189,11 @@ export interface OrchestrateInput { * deterministic findings as grounding and returns ADVISORY findings only. */ aiReview?: (input: AiReviewInput) => Promise + /** + * Optional spec-test proposal lane. Injected like aiReview so the orchestrator + * stays pure; production wires harness LLM transport, tests pass a fake. + */ + generateSpecTests?: (input: SpecTestGenInput) => Promise /** PR metadata passed to the AI reviewer for intent checking. */ prTitle?: string prBody?: string @@ -754,6 +761,270 @@ async function missingGrainTestLane(ctx: ModelContext, runner: ReviewRunner): Pr ] } +const DECLARED_SPEC_SOURCE_KINDS = new Set(["not_null", "unique", "accepted_values", "relationships"]) + +function record(value: unknown): Record | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) return undefined + return value as Record +} + +function array(value: unknown): unknown[] { + return Array.isArray(value) ? value : [] +} + +function text(value: unknown): string | undefined { + return typeof value === "string" && value.trim() ? value.trim() : undefined +} + +function bool(value: unknown): boolean { + return value === true || value === "true" +} + +function dedupeSpecSources(sources: SpecSource[]): SpecSource[] { + const seen = new Set() + const out: SpecSource[] = [] + for (const source of sources) { + if (seen.has(source.ref)) continue + seen.add(source.ref) + out.push(source) + } + return out +} + +function parseDbtTestSpec(value: unknown): { kind: SpecSource["kind"]; args?: Record } | undefined { + if (typeof value === "string") { + const kind = value.trim() + if (DECLARED_SPEC_SOURCE_KINDS.has(kind)) return { kind: kind as SpecSource["kind"] } + return undefined + } + const obj = record(value) + if (!obj) return undefined + + const named = text(obj.test_name) ?? text(obj.test) + if (named && DECLARED_SPEC_SOURCE_KINDS.has(named)) { + return { + kind: named as SpecSource["kind"], + args: record(obj.arguments) ?? record(obj.args), + } + } + + for (const [key, rawArgs] of Object.entries(obj)) { + if (!DECLARED_SPEC_SOURCE_KINDS.has(key)) continue + return { kind: key as SpecSource["kind"], args: record(rawArgs) } + } + return undefined +} + +function schemaModelSources(model: string, schemaDoc: unknown): SpecSource[] { + const root = record(schemaDoc) + const models = array(root?.models) + const node = models.map(record).find((m) => text(m?.name) === model) + if (!node) return [] + + const out: SpecSource[] = [] + const modelDescription = text(node.description) + if (modelDescription) { + out.push({ + origin: "inferred_context", + kind: "pr_intent", + ref: `schema.yml:${model}:description`, + text: modelDescription, + }) + } + + const contractEnforced = bool(record(node.contract)?.enforced) + for (const rawColumn of array(node.columns)) { + const column = record(rawColumn) + const name = text(column?.name) + if (!column || !name) continue + + const description = text(column.description) + if (description) { + out.push({ + origin: "inferred_context", + kind: "schema_desc", + ref: `schema.yml:${model}.${name}:description`, + text: description, + }) + } + + const dataType = text(column.data_type) + if (contractEnforced && dataType) { + out.push({ + origin: "declared_constraint", + kind: "column_type", + ref: `schema.yml:${model}.${name}:data_type`, + text: dataType, + }) + } + + for (const rawTest of [...array(column.tests), ...array(column.data_tests)]) { + const parsed = parseDbtTestSpec(rawTest) + if (!parsed) continue + out.push({ + origin: "declared_constraint", + kind: parsed.kind, + ref: `schema.yml:${model}.${name}:${parsed.kind}`, + text: parsed.kind, + args: parsed.args, + }) + } + + for (const rawConstraint of array(column.constraints)) { + const constraint = record(rawConstraint) + const type = text(constraint?.type) + if (!type || !DECLARED_SPEC_SOURCE_KINDS.has(type)) continue + out.push({ + origin: "declared_constraint", + kind: type as SpecSource["kind"], + ref: `schema.yml:${model}.${name}:${type}`, + text: type, + args: constraint, + }) + } + } + return out +} + +async function changedSchemaSources(model: string, input: OrchestrateInput): Promise { + if (!input.getContent) return [] + const reviewable = filterChangedFiles(input.changedFiles, input.rubric.exclusions.excludeGlobs) + const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml" && f.status !== "deleted") + const sources: SpecSource[] = [] + for (const file of schemaFiles) { + try { + const raw = await input.getContent(file.path, "new") + if (!raw) continue + sources.push(...schemaModelSources(model, YAML.parse(raw))) + } catch { + // Broken or unavailable schema.yml should not crash an advisory lane. + } + } + return sources +} + +function quotedArgs(text: string): string[] { + return [...text.matchAll(/['"]([^'"]+)['"]/g)].map((m) => m[1]).filter(Boolean) +} + +function sqlIntentSources(sql: string | undefined): SpecSource[] { + if (!sql) return [] + const out: SpecSource[] = [] + for (const match of sql.matchAll(/\bref\s*\(([^)]*)\)/g)) { + const args = quotedArgs(match[1]) + if (!args.length) continue + const name = args.length >= 2 ? `${args[0]}.${args[1]}` : args[0] + out.push({ + origin: "inferred_context", + kind: "ref_edge", + ref: `ref:${name}`, + text: name, + }) + } + for (const match of sql.matchAll(/\bsource\s*\(([^)]*)\)/g)) { + const args = quotedArgs(match[1]) + if (args.length < 2) continue + const name = `${args[0]}.${args[1]}` + out.push({ + origin: "inferred_context", + kind: "ref_edge", + ref: `source:${name}`, + text: name, + }) + } + return out +} + +function prIntentSources(model: string, input: OrchestrateInput): SpecSource[] { + const body = [input.prTitle, input.prBody].map(text).filter(Boolean).join("\n\n") + if (!body) return [] + return [{ origin: "inferred_context", kind: "pr_intent", ref: `pr:${model}:intent`, text: body }] +} + +function dbtTestEntry(test: GeneratedTest): unknown { + const spec = test.dbtTest + if (!spec?.args || Object.keys(spec.args).length === 0) return spec?.test ?? test.kind + return { [spec.test]: spec.args } +} + +function proposedTestYaml(model: string, test: GeneratedTest): string { + if (!test.dbtTest) return (test.assertionSql ?? "").trim() + const entry = dbtTestEntry(test) + const doc = test.dbtTest.column + ? { version: 2, models: [{ name: model, columns: [{ name: test.dbtTest.column, tests: [entry] }] }] } + : { version: 2, models: [{ name: model, tests: [entry] }] } + return YAML.stringify(doc).trim() +} + +function proposedTestBody(test: GeneratedTest, yaml: string): string { + return [ + "Candidate dbt test to consider adding. This review did not execute it and it cannot block the PR.", + "", + `Derived from \`${test.derivedFrom.ref}\`.`, + "", + "```yaml", + yaml, + "```", + "", + test.rationale ? `Rationale: ${test.rationale}` : "", + ] + .filter((line) => line !== "") + .join("\n") +} + +async function specTestSynthesisLane( + ctx: ModelContext, + input: OrchestrateInput, + dialect: string, +): Promise { + if (ctx.file.status !== "added" || !input.generateSpecTests) return [] + const model = modelNameFromPath(ctx.file.path) + const sql = ctx.engineNewSql ?? ctx.newSql ?? "" + const specSources = dedupeSpecSources([ + ...(await changedSchemaSources(model, input)), + ...sqlIntentSources(ctx.newSql), + ...sqlIntentSources(ctx.engineNewSql), + ...prIntentSources(model, input), + ]) + if (!specSources.length) return [] + + let proposed: GeneratedTest[] = [] + try { + proposed = await input.generateSpecTests({ + model, + file: ctx.file.path, + dialect, + compiledSql: sql, + specSources, + upstream: specSources + .filter((s) => s.kind === "ref_edge") + .map((s) => ({ model: s.text ?? s.ref.replace(/^(ref|source):/, ""), columns: [] })), + prTitle: input.prTitle, + prBody: input.prBody, + }) + } catch { + return [] + } + + const { kept } = filterToSpecDerived(proposed, specSources) + return kept.map((test) => { + const yaml = proposedTestYaml(model, test) + const column = test.dbtTest?.column + return makeFinding({ + severity: "suggestion", + category: "test_coverage", + title: `${model}: proposed ${test.kind} test${column ? ` for ${column}` : ""}`, + body: proposedTestBody(test, yaml), + file: ctx.file.path, + model, + column, + confidence: "unknown", + evidence: { tool: "altimate.spec_test.proposed", result: { proposal: test, yaml } }, + ruleKey: "spec_test:" + test.derivedFrom.ref, + }) + }) +} + /** * dbt CONFIG lint — runs the core dbt-config parser (minijinja) over the RAW model * (the `{{ config() }}` block + body) and emits the config/Jinja findings: incremental @@ -1132,6 +1403,7 @@ export async function runReview(input: OrchestrateInput): Promise = { trivial: ["sql_quality", "dbt_patterns"], - lite: ["sql_quality", "lineage_breakage", "semantic_change", "test_coverage", "dbt_patterns", "ai_review"], + lite: [ + "sql_quality", + "lineage_breakage", + "semantic_change", + "test_coverage", + "spec_tests", + "dbt_patterns", + "ai_review", + ], full: [ "sql_quality", "lineage_breakage", @@ -161,6 +169,7 @@ export const TIER_LANES: Record = { "materialization", "warehouse_cost", "test_coverage", + "spec_tests", "idempotency", "dbt_patterns", "ai_review", diff --git a/packages/opencode/src/altimate/review/run.ts b/packages/opencode/src/altimate/review/run.ts index 95b5c1589..65eabd982 100644 --- a/packages/opencode/src/altimate/review/run.ts +++ b/packages/opencode/src/altimate/review/run.ts @@ -8,6 +8,7 @@ import { buildCatalogSchemaContext } from "./schema-context" import { createDispatcherRunner } from "./runner" import { runReview } from "./orchestrate" import { runAiReview } from "./ai-review" +import { runSpecTestGen } from "./spec-test-gen" import type { ReviewMode, VerdictEnvelope } from "./verdict" import type { ChangedFile } from "./diff-filter" @@ -133,6 +134,7 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise modelVersion: opts.modelVersion, coreVersion: opts.coreVersion, aiReview: opts.noAi || config.ai === false ? undefined : runAiReview, + generateSpecTests: opts.noAi || config.ai === false ? undefined : runSpecTestGen, prTitle: opts.prTitle, prBody: opts.prBody, }) diff --git a/packages/opencode/src/altimate/review/spec-test-gen.ts b/packages/opencode/src/altimate/review/spec-test-gen.ts new file mode 100644 index 000000000..6426e2baa --- /dev/null +++ b/packages/opencode/src/altimate/review/spec-test-gen.ts @@ -0,0 +1,359 @@ +// altimate_change - greenfield spec-test synthesis (P0: advisory transport + guard) +import { createHash } from "node:crypto" +import { Provider } from "@/provider/provider" +import { LLM } from "@/session/llm" +import { Agent } from "@/agent/agent" +import { MessageV2 } from "@/session/message-v2" +import { MessageID, SessionID } from "@/session/schema" +import { Log } from "@/util/log" + +/** + * Auxiliary spec-test synthesis for GREENFIELD (git-added) dbt models. + * + * A brand-new model has no base to diff against, so the equivalence / data-diff + * lanes (which own the reference-exists world) can't verify it. This feature + * instead works in TWO strictly separated tracks — see + * specs/greenfield-spec-test-synthesis.md: + * + * Track A (block-eligible, NO LLM): materialize dbt constraints the author + * DECLARED but did not enforce (schema.yml not_null/unique/accepted_values/ + * relationships, contract types). The assertion is built from the PARSED + * constraint, not from model text — no LLM is in this loop, so it cannot be + * tricked into a back-labeled false positive. An executed failure is a real + * contract violation. + * + * Track B (advisory, LLM): propose NEW tests from soft intent (column + * descriptions, ref() edges, PR text). These are hypotheses — emitted at + * confidence "unknown" under a dedicated evidence tool and STRUCTURALLY + * excluded from the verdict. They never block; at worst they are noise in a + * proposed patch. + * + * IP boundary: P0 intentionally inlines a minimal prompt in TS for speed. The + * track-B generation prompt and response parse/clamp should move into compiled + * core (`altimate_core.review_spec_test_*`) before GA. The DETERMINISTIC guard + * below MUST stay in open TS: it is inspectable and unit-tested, never a black box. + */ + +const log = Log.create({ service: "spec-test-gen" }) + +const SPEC_TEST_GEN_TIMEOUT_MS = 60_000 +const MAX_SPEC_SOURCES = 80 + +/** Whether a spec source is an author-DECLARED constraint (machine-readable, + * block-eligible) or merely INFERRED context (advisory only). This split — not + * a tag check — is what keeps the feature honest. */ +export type SpecOrigin = "declared_constraint" | "inferred_context" + +/** + * A source of author intent. `declared_constraint` sources are parsed dbt + * constraints; `inferred_context` sources are soft signals the LLM may propose + * from. Extracted from the dbt project BEFORE generation — the generator never + * reads the model's current output, so a test can only be grounded in stated + * intent, not observed behavior. + */ +export interface SpecSource { + origin: SpecOrigin + kind: + | "not_null" + | "unique" + | "accepted_values" + | "relationships" + | "column_type" // declared_constraint kinds ↑ + | "schema_desc" + | "ref_edge" + | "pr_intent" // inferred_context kinds ↑ + /** Stable identifier of the spec element we extracted, e.g. + * "schema.yml:dim_customer.email" or "ref:stg_customers". Generated tests cite + * this in `derivedFrom.ref`; the guard rejects any citation we did not provide. */ + ref: string + /** The declared text/value/description, when any. */ + text?: string + /** Parsed constraint args (accepted_values set, relationships target). */ + args?: Record +} + +/** Test classes. `not_null`/`unique`/`accepted_values`/`relationships` map to + * track-A declared constraints; `range` is an inferred-context proposal. No + * golden/snapshot class exists — current output is never an expected value. */ +export type GeneratedTestKind = "not_null" | "unique" | "accepted_values" | "relationships" | "range" + +/** Kinds that can derive from a declared constraint (track A, block-eligible when + * executed). `range` and any description-derived test are advisory only. */ +export const DECLARED_CONSTRAINT_KINDS: ReadonlySet = new Set([ + "not_null", + "unique", + "accepted_values", + "relationships", +]) + +/** All kinds a proposal may carry; a test whose `kind` is outside this set is + * dropped by the advisory-track guard. */ +export const ALLOWED_TEST_KINDS: ReadonlySet = new Set([ + ...DECLARED_CONSTRAINT_KINDS, + "range", +]) + +export interface GeneratedTest { + /** Deterministic id. `runGeneratedTests` returns results KEYED BY this id, so a + * dropped/errored/batched test can never attach its result to the wrong test. */ + id: string + kind: GeneratedTestKind + /** Preferred: a dbt schema test spec (persists as YAML). */ + dbtTest?: { column?: string; test: string; args?: Record } + /** Advisory track only, and sandboxed before execution (SELECT-only, allowlisted + * relations, row cap, timeout). Track A never uses raw SQL — it materializes + * bounded dbt generic tests from the parsed constraint. */ + assertionSql?: string + /** Why this test follows from the cited spec element. */ + rationale: string + /** REQUIRED. The intent source this test derives from. `derivedFrom.origin` + * decides block-eligibility: `declared_constraint` (track A) can block when + * executed; `inferred_context` (track B) never blocks. */ + derivedFrom: SpecSource +} + +export interface SpecTestGenInput { + model: string + file: string + dialect: string + /** dbt-compiled SQL — context only; NEVER used to derive an expected value. */ + compiledSql: string + /** Author intent, extracted before generation (both origins). */ + specSources: SpecSource[] + /** Upstream models + their output columns (for referential proposals). */ + upstream: Array<{ model: string; columns: string[] }> + prTitle?: string + prBody?: string + /** Learned per-repo priors (corrective-app-memory.md) — bias only, P2. */ + priors?: Array<{ derivedFromKind: string; polarity: "prefer" | "suppress" }> +} + +/** Per-test execution outcome (P1). 0 violating rows = pass. */ +export interface GeneratedTestResult { + status: "pass" | "fail" | "error" + violatingRows?: number + detail?: string +} + +/** Reason a proposal was rejected by the advisory-track guard. */ +export type GuardDropReason = "no_derived_from" | "kind_not_allowed" | "empty_ref" | "ref_not_provided" + +/** + * ADVISORY-TRACK anti-fabrication guard (deterministic; no LLM trust). + * + * NOTE ON SCOPE: this is NOT what makes the feature sound — the block path is + * kept safe by having no LLM in it at all (track A materializes from parsed + * constraints). This guard hardens the ADVISORY track: it drops LLM proposals + * that are unusable or fabricated, so the proposed-test patch stays grounded. + * + * Keep a proposal ONLY when: + * 1. it carries a `derivedFrom`, + * 2. whose `kind` is allowed, and + * 3. whose `ref` is EXACTLY one of the `providedSources` refs — a spec element + * WE extracted, not a string the model invented (anti-fabrication). + * + * It deliberately does NOT try to prove the assertion follows from the ref — a + * back-labeled test can still pass this. That is acceptable precisely because a + * proposal can never block (verdict excludes the proposed tool); the block path + * does not rely on this function. + * + * Pure and total: same inputs → same partition. Unit-tested. + */ +export function filterToSpecDerived( + tests: GeneratedTest[], + providedSources: SpecSource[], +): { kept: GeneratedTest[]; dropped: Array<{ test: GeneratedTest; reason: GuardDropReason }> } { + const providedRefs = new Set(providedSources.map((s) => s.ref)) + const kept: GeneratedTest[] = [] + const dropped: Array<{ test: GeneratedTest; reason: GuardDropReason }> = [] + for (const t of tests) { + const df = t.derivedFrom + if (!df) { + dropped.push({ test: t, reason: "no_derived_from" }) + continue + } + if (!ALLOWED_TEST_KINDS.has(t.kind)) { + dropped.push({ test: t, reason: "kind_not_allowed" }) + continue + } + if (!df.ref || !df.ref.trim()) { + dropped.push({ test: t, reason: "empty_ref" }) + continue + } + if (!providedRefs.has(df.ref)) { + dropped.push({ test: t, reason: "ref_not_provided" }) + continue + } + kept.push(t) + } + return { kept, dropped } +} + +/** + * Is a generated test eligible to BLOCK a PR (when executed and failed)? + * True only for track-A tests: a declared-constraint origin AND a + * declared-constraint kind. Never true for anything the LLM proposed from soft + * intent. The verdict layer enforces the execution requirement separately. + */ +export function isBlockEligible(test: GeneratedTest): boolean { + return test.derivedFrom.origin === "declared_constraint" && DECLARED_CONSTRAINT_KINDS.has(test.kind) +} + +function stableJson(value: unknown): string { + if (value === null || typeof value !== "object") return JSON.stringify(value) ?? "null" + if (Array.isArray(value)) return "[" + value.map(stableJson).join(",") + "]" + const obj = value as Record + return ( + "{" + + Object.keys(obj) + .sort() + .map((k) => JSON.stringify(k) + ":" + stableJson(obj[k])) + .join(",") + + "}" + ) +} + +function generatedTestId(test: GeneratedTest): string { + const payload = stableJson({ + kind: test.kind, + ref: test.derivedFrom?.ref ?? "", + dbtTest: test.dbtTest ?? null, + assertionSql: test.assertionSql ?? "", + }) + return "gst_" + createHash("sha256").update(payload).digest("hex").slice(0, 16) +} + +function buildSystemPrompt(): string { + return [ + "You propose dbt generic tests for a newly added dbt model.", + "Rules:", + "- Use ONLY the provided specSources. Do not infer expected values from current output or observed data.", + "- Propose dbt generic tests only. Fill dbtTest; do not use assertionSql.", + "- Every proposal must copy one derivedFrom object from specSources exactly, including derivedFrom.ref.", + "- The derivedFrom.ref must be one of the provided refs. Never invent refs.", + "- Allowed kind values: not_null, unique, accepted_values, relationships, range.", + "- Return ONLY a JSON array of GeneratedTest objects. Return [] when there is no grounded proposal.", + ].join("\n") +} + +function buildUserMessage(input: SpecTestGenInput): string { + return JSON.stringify( + { + model: input.model, + file: input.file, + dialect: input.dialect, + specSources: input.specSources.slice(0, MAX_SPEC_SOURCES), + upstream: input.upstream, + }, + null, + 2, + ) +} + +function stripJsonFence(text: string): string { + const trimmed = text.trim() + const fenced = /^```(?:json)?\s*([\s\S]*?)\s*```$/i.exec(trimmed) + return fenced ? fenced[1].trim() : trimmed +} + +function objectOrUndefined(value: unknown): Record | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) return undefined + return value as Record +} + +function normalizeDbtTest(value: unknown): GeneratedTest["dbtTest"] | undefined { + const obj = objectOrUndefined(value) + if (!obj || typeof obj.test !== "string" || !obj.test.trim()) return undefined + const args = objectOrUndefined(obj.args) + return { + column: typeof obj.column === "string" && obj.column.trim() ? obj.column : undefined, + test: obj.test, + args, + } +} + +function parseGeneratedTests(text: string): GeneratedTest[] { + const raw = JSON.parse(stripJsonFence(text)) + if (!Array.isArray(raw)) return [] + const out: GeneratedTest[] = [] + for (const item of raw) { + const obj = objectOrUndefined(item) + if (!obj) continue + const dbtTest = normalizeDbtTest(obj.dbtTest) + if (!dbtTest) continue + const test: GeneratedTest = { + id: "", + kind: String(obj.kind ?? "") as GeneratedTestKind, + dbtTest, + rationale: typeof obj.rationale === "string" ? obj.rationale : "", + derivedFrom: obj.derivedFrom as SpecSource, + } + test.id = generatedTestId(test) + out.push(test) + } + return out +} + +/** + * Run the spec-test proposal lane. Returns proposed dbt generic tests grounded in + * provided `specSources`, or [] if the model/LLM is unavailable or any failure + * occurs. Reviews must never crash because this advisory LLM lane is absent. + */ +export async function runSpecTestGen(input: SpecTestGenInput): Promise { + if (!input.specSources.length) return [] + + const controller = new AbortController() + const timeout = setTimeout(() => controller.abort(), SPEC_TEST_GEN_TIMEOUT_MS) + try { + const defaultModel = await Provider.defaultModel() + const model = await Provider.getModel(defaultModel.providerID, defaultModel.modelID) + const system = buildSystemPrompt() + const agent: Agent.Info = { + name: "dbt-spec-test-generator", + mode: "primary", + hidden: true, + options: {}, + permission: [], + prompt: system, + temperature: 0, + } + const user: MessageV2.User = { + id: MessageID.ascending(), + sessionID: SessionID.descending(), + role: "user", + time: { created: Date.now() }, + agent: agent.name, + model: { providerID: model.providerID, modelID: model.id }, + } + + const stream = await LLM.stream({ + agent, + user, + system: [system], + small: false, + tools: {}, + model, + abort: controller.signal, + sessionID: user.sessionID, + retries: 1, + messages: [{ role: "user", content: buildUserMessage(input) }], + }) + for await (const _ of stream.fullStream) { + // drain to avoid SDK hangs + } + const text = await Promise.resolve(stream.text).catch((err: unknown) => { + log.error("spec-test generation stream failed", { error: err }) + return undefined + }) + if (!text) return [] + + const tests = parseGeneratedTests(text) + const { kept } = filterToSpecDerived(tests, input.specSources) + return kept + } catch (err) { + log.error("spec-test generation failed", { error: err }) + return [] + } finally { + clearTimeout(timeout) + } +} diff --git a/packages/opencode/src/altimate/review/verdict.ts b/packages/opencode/src/altimate/review/verdict.ts index d499695d9..5e3946f6a 100644 --- a/packages/opencode/src/altimate/review/verdict.ts +++ b/packages/opencode/src/altimate/review/verdict.ts @@ -60,8 +60,14 @@ export function computeIdealVerdict(findings: Finding[], rubric: Rubric = DEFAUL // Advisory LLM findings (layer 3) are EXCLUDED: the lane is advisory-only and must // never influence the verdict, so its warnings cannot accumulate into a block // (otherwise a chatty or prompt-injected AI review could force REQUEST_CHANGES). + // P0 spec-test proposals are also advisory-only and unexecuted; exclude their + // evidence tool so a future warning cannot leak into the blocking path. const warningCount = findings.filter( - (f) => f.severity === "warning" && f.confidence !== "unknown" && f.evidence?.tool !== "ai-review", + (f) => + f.severity === "warning" && + f.confidence !== "unknown" && + f.evidence?.tool !== "ai-review" && + f.evidence?.tool !== "altimate.spec_test.proposed", ).length if (warningCount >= rubric.warningPatternThreshold) return "REQUEST_CHANGES" return "COMMENT" diff --git a/packages/opencode/test/altimate/review-spec-test-gen.test.ts b/packages/opencode/test/altimate/review-spec-test-gen.test.ts new file mode 100644 index 000000000..a3fe5fc93 --- /dev/null +++ b/packages/opencode/test/altimate/review-spec-test-gen.test.ts @@ -0,0 +1,254 @@ +import { describe, test, expect } from "bun:test" +import { + DEFAULT_REVIEW_CONFIG, + DEFAULT_RUBRIC, + filterToSpecDerived, + isBlockEligible, + renderSummary, + runReview, + type ChangedFile, + type SpecSource, + type GeneratedTest, + type ReviewRunner, +} from "../../src/altimate/review" + +// A declared-constraint source (track A) and an inferred-context source (track B). +const declared: SpecSource = { + origin: "declared_constraint", + kind: "not_null", + ref: "schema.yml:dim_customer.email", + text: "not_null", +} +const inferred: SpecSource = { + origin: "inferred_context", + kind: "schema_desc", + ref: "desc:dim_customer.discount_pct", + text: "discount percentage 0-100", +} +const providedSources = [declared, inferred] + +function mkTest(over: Partial & { derivedFrom?: GeneratedTest["derivedFrom"] }): GeneratedTest { + return { + id: over.id ?? "t1", + kind: over.kind ?? "not_null", + rationale: "because", + derivedFrom: over.derivedFrom ?? declared, + ...over, + } +} + +describe("filterToSpecDerived (advisory-track anti-fabrication guard)", () => { + test("keeps a proposal citing a provided source ref", () => { + const { kept, dropped } = filterToSpecDerived([mkTest({ derivedFrom: declared })], providedSources) + expect(kept.length).toBe(1) + expect(dropped.length).toBe(0) + }) + + test("drops a proposal with no derivedFrom", () => { + const t = mkTest({}) + // @ts-expect-error deliberately null out the required field to simulate a bad LLM item + t.derivedFrom = undefined + const { kept, dropped } = filterToSpecDerived([t], providedSources) + expect(kept.length).toBe(0) + expect(dropped[0]?.reason).toBe("no_derived_from") + }) + + test("drops a FABRICATED ref the model invented (not in provided sources)", () => { + const fabricated: SpecSource = { origin: "declared_constraint", kind: "not_null", ref: "schema.yml:dim_customer.HALLUCINATED" } + const { kept, dropped } = filterToSpecDerived([mkTest({ derivedFrom: fabricated })], providedSources) + expect(kept.length).toBe(0) + expect(dropped[0]?.reason).toBe("ref_not_provided") + }) + + test("drops a disallowed kind", () => { + const t = mkTest({ derivedFrom: declared }) + // @ts-expect-error out-of-enum kind from a malformed LLM item + t.kind = "golden_snapshot" + const { kept, dropped } = filterToSpecDerived([t], providedSources) + expect(kept.length).toBe(0) + expect(dropped[0]?.reason).toBe("kind_not_allowed") + }) + + test("drops an empty ref", () => { + const empty: SpecSource = { origin: "inferred_context", kind: "schema_desc", ref: " " } + const { kept, dropped } = filterToSpecDerived([mkTest({ derivedFrom: empty })], providedSources) + expect(kept.length).toBe(0) + expect(dropped[0]?.reason).toBe("empty_ref") + }) +}) + +describe("isBlockEligible (only track-A declared constraints can block)", () => { + test("declared_constraint + declared kind → block-eligible", () => { + expect(isBlockEligible(mkTest({ derivedFrom: declared, kind: "not_null" }))).toBe(true) + }) + + test("inferred_context → never block-eligible, even with a declared-style kind", () => { + const backLabeled: SpecSource = { origin: "inferred_context", kind: "not_null", ref: "desc:x" } + expect(isBlockEligible(mkTest({ derivedFrom: backLabeled, kind: "not_null" }))).toBe(false) + }) + + test("range proposal → never block-eligible", () => { + expect(isBlockEligible(mkTest({ derivedFrom: inferred, kind: "range" }))).toBe(false) + }) +}) + +function fakeRunner(): ReviewRunner { + return { + async impact() { + return { hasManifest: true, severity: "SAFE", directCount: 0, transitiveCount: 0, testCount: 0 } + }, + async grade() { + return { grade: "B" } + }, + async check() { + return { issues: [], ran: true } + }, + async equivalence() { + return { decided: false } + }, + async detectPii() { + return { columns: [] } + }, + } as ReviewRunner +} + +const modelSql = "select order_id, status from {{ ref('stg_orders') }}" +const schemaYaml = ` +version: 2 +models: + - name: fct_orders + description: Orders fact model for fulfilled order reporting. + columns: + - name: order_id + description: Stable order identifier. + tests: + - not_null + - name: status + description: Fulfillment state. + tests: + - accepted_values: + values: [placed, shipped] +` + +function content(files: Record) { + return async (file: string, side: "old" | "new") => (side === "new" ? files[file] : undefined) +} + +const changedFiles: ChangedFile[] = [ + { path: "models/marts/fct_orders.sql", status: "added", diff: "+select ...\n" }, + { path: "models/marts/schema.yml", status: "added", diff: "+models:\n" }, +] + +function proposedFrom(source: SpecSource, over: Partial = {}): GeneratedTest { + return { + id: "fake-" + source.ref, + kind: "not_null", + dbtTest: { column: "order_id", test: "not_null" }, + rationale: "The schema declares this expectation.", + derivedFrom: source, + ...over, + } +} + +describe("spec test synthesis lane (P0 propose-only)", () => { + test("an ADDED model yields suggestion proposed-test findings", async () => { + const env = await runReview({ + changedFiles, + config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["spec_tests"], ai: false }, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner: fakeRunner(), + getContent: content({ + "models/marts/fct_orders.sql": modelSql, + "models/marts/schema.yml": schemaYaml, + }), + generateSpecTests: async (input) => { + const source = input.specSources.find((s) => s.ref === "schema.yml:fct_orders.order_id:not_null") + return source ? [proposedFrom(source)] : [] + }, + }) + + const findings = env.findings.filter((f) => f.evidence?.tool === "altimate.spec_test.proposed") + expect(findings.length).toBe(1) + expect(findings[0]).toMatchObject({ + severity: "suggestion", + confidence: "unknown", + category: "test_coverage", + }) + expect(findings[0].body).toContain("```yaml") + expect(findings[0].body).toContain("not_null") + const summary = renderSummary(env) + expect(summary).toContain("### Proposed tests") + expect(summary).toContain("Candidate tests to consider adding") + }) + + test("proposed findings never request changes in comment or gate mode", async () => { + for (const mode of ["comment", "gate"] as const) { + const env = await runReview({ + changedFiles, + config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["spec_tests"], ai: false, mode }, + rubric: DEFAULT_RUBRIC, + mode, + runner: fakeRunner(), + getContent: content({ + "models/marts/fct_orders.sql": modelSql, + "models/marts/schema.yml": schemaYaml, + }), + generateSpecTests: async (input) => + input.specSources.slice(0, 4).map((source, i) => + proposedFrom(source, { + id: `fake-${i}`, + dbtTest: { column: i % 2 === 0 ? "order_id" : "status", test: i % 2 === 0 ? "not_null" : "accepted_values" }, + kind: i % 2 === 0 ? "not_null" : "accepted_values", + }), + ), + }) + expect(env.findings.filter((f) => f.evidence?.tool === "altimate.spec_test.proposed").length).toBeGreaterThan(1) + expect(env.idealVerdict).toBe("COMMENT") + expect(env.verdict).toBe("COMMENT") + } + }) + + test("fabricated-ref proposals are dropped", async () => { + const env = await runReview({ + changedFiles, + config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["spec_tests"], ai: false }, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner: fakeRunner(), + getContent: content({ + "models/marts/fct_orders.sql": modelSql, + "models/marts/schema.yml": schemaYaml, + }), + generateSpecTests: async () => [ + proposedFrom({ + origin: "declared_constraint", + kind: "not_null", + ref: "schema.yml:fct_orders.fabricated:not_null", + text: "not_null", + }), + ], + }) + + expect(env.findings.some((f) => f.evidence?.tool === "altimate.spec_test.proposed")).toBe(false) + }) + + test("a MODIFIED-only PR yields no spec-test findings", async () => { + let calls = 0 + const env = await runReview({ + changedFiles: [{ path: "models/marts/fct_orders.sql", status: "modified", diff: "+select ...\n" }], + config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["spec_tests"], ai: false }, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner: fakeRunner(), + getContent: content({ "models/marts/fct_orders.sql": modelSql }), + generateSpecTests: async () => { + calls++ + return [] + }, + }) + + expect(calls).toBe(0) + expect(env.findings.some((f) => f.evidence?.tool === "altimate.spec_test.proposed")).toBe(false) + }) +}) From 41c586f3d8e5ada121edaf99ae848ba85cd99fc5 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Tue, 7 Jul 2026 17:01:07 -0700 Subject: [PATCH 2/9] feat: [P1] verdict-layer enforcement for spec-test blocking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A spec-test finding may only reach a blocking verdict when it was actually EXECUTED against a constraint the author DECLARED (evidence executed:true + origin:declared_constraint). Advisory/unexecuted/inferred spec-test findings can never force REQUEST_CHANGES, regardless of the severity the lane assigned — defense in depth so the verdict never trusts the lane. Non-spec-test findings are unaffected. Refs #989 Co-Authored-By: Claude Opus 4.8 --- .../opencode/src/altimate/review/verdict.ts | 19 +++++++- .../altimate/review-spec-test-verdict.test.ts | 46 +++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 packages/opencode/test/altimate/review-spec-test-verdict.test.ts diff --git a/packages/opencode/src/altimate/review/verdict.ts b/packages/opencode/src/altimate/review/verdict.ts index 5e3946f6a..a372014f8 100644 --- a/packages/opencode/src/altimate/review/verdict.ts +++ b/packages/opencode/src/altimate/review/verdict.ts @@ -49,10 +49,27 @@ export const VCS_EVENT: Record = { * - any finding at all → COMMENT * - nothing → APPROVE */ +/** + * Defense in depth for the spec-test synthesis lane: a spec-test finding may + * only reach a blocking verdict when it was actually EXECUTED against a + * constraint the author DECLARED (track A). An advisory/unexecuted proposal + * (track B, or any P0 output) can never block, regardless of the severity or + * category the lane assigned it — the verdict does not trust the lane. Findings + * from every other tool are unaffected (return true). + */ +function specTestMayBlock(f: Finding): boolean { + const tool = f.evidence?.tool ?? "" + if (!tool.startsWith("altimate.spec_test")) return true + const result = (f.evidence?.result ?? {}) as Record + return result["executed"] === true && result["origin"] === "declared_constraint" +} + export function computeIdealVerdict(findings: Finding[], rubric: Rubric = DEFAULT_RUBRIC): Verdict { if (findings.length === 0) return "APPROVE" const blockers = blockingCategories(rubric) - const hasBlockingCritical = findings.some((f) => f.severity === "critical" && blockers.has(f.category)) + const hasBlockingCritical = findings.some( + (f) => f.severity === "critical" && blockers.has(f.category) && specTestMayBlock(f), + ) if (hasBlockingCritical) return "REQUEST_CHANGES" // Count only confidently-warned findings toward the risk pattern. Undecidable // ("unknown") warnings — e.g. equivalence that couldn't be proven — must not diff --git a/packages/opencode/test/altimate/review-spec-test-verdict.test.ts b/packages/opencode/test/altimate/review-spec-test-verdict.test.ts new file mode 100644 index 000000000..bc15dff4b --- /dev/null +++ b/packages/opencode/test/altimate/review-spec-test-verdict.test.ts @@ -0,0 +1,46 @@ +import { describe, test, expect } from "bun:test" +import { computeIdealVerdict, makeFinding, type Finding } from "../../src/altimate/review" + +// A critical finding in a blocking category (contract_violation), varying only +// by its evidence tool/result — to prove the verdict-layer enforcement. +function crit(tool: string, result?: Record): Finding { + return makeFinding({ + severity: "critical", + category: "contract_violation", + title: "x", + body: "y", + file: "models/marts/fct_orders.sql", + model: "fct_orders", + confidence: "high", + evidence: { tool, result }, + ruleKey: "t:" + tool, + }) +} + +describe("verdict enforcement — a spec-test finding may only block when executed+declared", () => { + test("normal contract_violation critical still blocks (regression guard)", () => { + expect(computeIdealVerdict([crit("altimate_core.dbt_config")])).toBe("REQUEST_CHANGES") + }) + + test("spec-test critical that was EXECUTED against a DECLARED constraint blocks", () => { + expect( + computeIdealVerdict([crit("altimate.spec_test.executed", { executed: true, origin: "declared_constraint" })]), + ).toBe("REQUEST_CHANGES") + }) + + test("spec-test critical NOT executed does NOT block (downgrades to COMMENT)", () => { + expect( + computeIdealVerdict([crit("altimate.spec_test.executed", { executed: false, origin: "declared_constraint" })]), + ).toBe("COMMENT") + }) + + test("spec-test critical from an INFERRED origin does NOT block", () => { + expect( + computeIdealVerdict([crit("altimate.spec_test.executed", { executed: true, origin: "inferred_context" })]), + ).toBe("COMMENT") + }) + + test("a P0 proposed spec-test critical (advisory) can never block", () => { + expect(computeIdealVerdict([crit("altimate.spec_test.proposed", { proposal: {} })])).toBe("COMMENT") + }) +}) From 649183bc77b08293585cc811f7a947c6ac6104ee Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Tue, 7 Jul 2026 17:22:23 -0700 Subject: [PATCH 3/9] =?UTF-8?q?feat:=20[P1]=20track-A=20execution=20?= =?UTF-8?q?=E2=80=94=20verify=20declared-but-unenforced=20dbt=20constraint?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the deterministic blocking path for greenfield spec-test synthesis: - runner: declaredConstraints(model) reads the manifest for declared not_null/unique/accepted_values/relationships + contract types with provenance and hasEnforcingTest; runGeneratedTests(tests, warehouse?) executes bounded violating-row checks via the dispatcher, id-keyed, null when no warehouse. - orchestrate: materialize only declared-but-UNENFORCED constraints (no LLM) into GeneratedTests; when config.specTests.execute is set and a warehouse is present, execute them. An executed failure emits a critical contract_violation carrying evidence { executed: true, origin: declared_constraint }, the only shape specTestMayBlock lets block. No warehouse / execute off → non-blocking proposals. - Track B (LLM proposals) stays propose-only and is NEVER executed. - verdict: summary.enforcedConstraints (executed/passed/failed); format: Spec coverage. 145 review tests pass; the sole blocking path is deterministic end to end. Refs #989 Co-Authored-By: Claude Opus 4.8 --- .../opencode/src/altimate/review/config.ts | 2 +- .../opencode/src/altimate/review/format.ts | 4 + .../src/altimate/review/orchestrate.ts | 186 ++++++++-- .../opencode/src/altimate/review/runner.ts | 341 +++++++++++++++++- .../src/altimate/review/spec-test-gen.ts | 15 +- .../opencode/src/altimate/review/verdict.ts | 24 +- .../altimate/review-spec-test-exec.test.ts | 197 ++++++++++ 7 files changed, 735 insertions(+), 34 deletions(-) create mode 100644 packages/opencode/test/altimate/review-spec-test-exec.test.ts diff --git a/packages/opencode/src/altimate/review/config.ts b/packages/opencode/src/altimate/review/config.ts index e8804fb4f..5886eae0d 100644 --- a/packages/opencode/src/altimate/review/config.ts +++ b/packages/opencode/src/altimate/review/config.ts @@ -42,7 +42,7 @@ export const ReviewConfig = z.object({ warehouse: z.string().default(""), }) .default({ enabled: false, warehouse: "" }), - /** Spec-test synthesis. P0 proposes only; execution is reserved for P1. */ + /** Spec-test synthesis. Declared-constraint execution is opt-in and off by default. */ specTests: z .object({ execute: z.boolean().default(false), diff --git a/packages/opencode/src/altimate/review/format.ts b/packages/opencode/src/altimate/review/format.ts index 7ff6dad1b..697380d96 100644 --- a/packages/opencode/src/altimate/review/format.ts +++ b/packages/opencode/src/altimate/review/format.ts @@ -45,6 +45,10 @@ export function renderSummary(env: VerdictEnvelope): string { "", ) } + if (env.summary.enforcedConstraints) { + const c = env.summary.enforcedConstraints + lines.push(`**Spec coverage:** ${c.executed} executed / ${c.passed} passed / ${c.failed} failed.`, "") + } if (!env.findings.length) { lines.push("No issues found in the changed dbt models. 🎉", "") diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index 5bb0e2b9e..618940a79 100644 --- a/packages/opencode/src/altimate/review/orchestrate.ts +++ b/packages/opencode/src/altimate/review/orchestrate.ts @@ -1,5 +1,7 @@ import path from "node:path" +import { createHash } from "node:crypto" import YAML from "yaml" +import { Log } from "@/util/log" import { type Finding, type ReviewCategory, @@ -16,7 +18,13 @@ import { type ReviewConfig } from "./config" import { type ReviewMode, type VerdictEnvelope, buildEnvelope, signEnvelope } from "./verdict" import { detectModelPatterns, detectSchemaYmlPatterns, splitDiff } from "./dbt-patterns" import { type AiReviewInput } from "./ai-review" -import { filterToSpecDerived, type GeneratedTest, type SpecSource, type SpecTestGenInput } from "./spec-test-gen" +import { + filterToSpecDerived, + type GeneratedTest, + type GeneratedTestResult, + type SpecSource, + type SpecTestGenInput, +} from "./spec-test-gen" /** * The deterministic review recipe. @@ -62,6 +70,8 @@ const CORE_AST_COVERED: Record = { "timezone-naive-now": /clock.?in.?filter|l049/, } +const log = Log.create({ service: "review-orchestrate" }) + /** * Extract function names a finding references in call form `NAME(` or backticked * `NAME`, lowercased — used to reconcile the regex portability catalog against @@ -112,6 +122,17 @@ export interface CheckResult { ran?: boolean } +export type DeclaredConstraintKind = "not_null" | "unique" | "accepted_values" | "relationships" | "column_type" + +export interface DeclaredConstraint { + kind: DeclaredConstraintKind + column?: string + args?: Record + hasEnforcingTest: boolean + uniqueId?: string + sourceRef: string +} + /** High-level engine surface the orchestrator depends on. */ export interface ReviewRunner { /** True when the configured dbt manifest loaded, independent of model lookup. */ @@ -141,6 +162,13 @@ export interface ReviewRunner { grain?(sql: string): Promise<{ group_by: string[]; dedup_partition: string[] }> /** Declared primary/unique key of a model, from contract or unique test (manifest). */ declaredPrimaryKey?(model: string): Promise + /** Parsed author-declared constraints and whether a dbt test already enforces each one. */ + declaredConstraints?(model: string): Promise + /** Execute generated declared-constraint tests; null when no warehouse/driver is available. */ + runGeneratedTests?( + tests: GeneratedTest[], + warehouse?: string, + ): Promise | null> /** Per-upstream WHERE-filter columns of a model's SQL (cross-model consistency). */ sourceFilters?(sql: string): Promise> /** dbt config/Jinja lint over RAW model SQL (parses `{{ config() }}` in core). */ @@ -972,21 +1000,149 @@ function proposedTestBody(test: GeneratedTest, yaml: string): string { .join("\n") } +interface EnforcedConstraintMetrics { + executed: number + passed: number + failed: number +} + +function stableJson(value: unknown): string { + if (value === null || typeof value !== "object") return JSON.stringify(value) ?? "null" + if (Array.isArray(value)) return "[" + value.map(stableJson).join(",") + "]" + const obj = value as Record + return ( + "{" + + Object.keys(obj) + .sort() + .map((k) => JSON.stringify(k) + ":" + stableJson(obj[k])) + .join(",") + + "}" + ) +} + +function declaredGeneratedTestId(test: Omit): string { + return "gst_declared_" + createHash("sha256").update(stableJson(test)).digest("hex").slice(0, 16) +} + +function materializeDeclaredConstraints(model: string, constraints: DeclaredConstraint[]): GeneratedTest[] { + const tests: GeneratedTest[] = [] + for (const c of constraints) { + if (c.hasEnforcingTest) continue + const args = c.args && Object.keys(c.args).length ? c.args : undefined + const draft: Omit = { + kind: c.kind, + dbtTest: { column: c.column, test: c.kind, args }, + rationale: `The model declares an unenforced ${c.kind} constraint${c.column ? ` on ${c.column}` : ""}.`, + derivedFrom: { + origin: "declared_constraint", + kind: c.kind, + ref: c.sourceRef, + text: c.kind, + args, + }, + } + tests.push({ ...draft, id: declaredGeneratedTestId(draft) }) + } + return tests +} + +function proposedSpecTestFinding(model: string, file: string, test: GeneratedTest): Finding { + const yaml = proposedTestYaml(model, test) + const column = test.dbtTest?.column + return makeFinding({ + severity: "suggestion", + category: "test_coverage", + title: `${model}: proposed ${test.kind} test${column ? ` for ${column}` : ""}`, + body: proposedTestBody(test, yaml), + file, + model, + column, + confidence: "unknown", + evidence: { tool: "altimate.spec_test.proposed", result: { proposal: test, yaml } }, + ruleKey: "spec_test:" + test.derivedFrom.ref, + }) +} + +function executedSpecTestFinding(model: string, file: string, test: GeneratedTest, violatingRows: number): Finding { + const column = test.dbtTest?.column + return makeFinding({ + severity: "critical", + category: "contract_violation", + title: `${model}: declared ${test.kind} constraint failed${column ? ` for ${column}` : ""}`, + body: + `This generated check was built from declared constraint \`${test.derivedFrom.ref}\` and executed against the warehouse. ` + + `It found ${violatingRows} violating row${violatingRows === 1 ? "" : "s"}. Enforce the constraint in dbt or amend the declaration.`, + file, + model, + column, + confidence: "high", + evidence: { + tool: "altimate.spec_test.executed", + result: { executed: true, origin: "declared_constraint", test, violatingRows }, + }, + ruleKey: "spec_test:" + test.derivedFrom.ref, + }) +} + async function specTestSynthesisLane( ctx: ModelContext, input: OrchestrateInput, dialect: string, + enforcedConstraints?: EnforcedConstraintMetrics, ): Promise { - if (ctx.file.status !== "added" || !input.generateSpecTests) return [] + if (ctx.file.status !== "added") return [] const model = modelNameFromPath(ctx.file.path) const sql = ctx.engineNewSql ?? ctx.newSql ?? "" + const findings: Finding[] = [] + + let trackATests: GeneratedTest[] = [] + if (input.runner.declaredConstraints) { + try { + trackATests = materializeDeclaredConstraints(model, await input.runner.declaredConstraints(model)) + } catch { + trackATests = [] + } + } + + if (trackATests.length) { + const shouldExecute = input.config.specTests?.execute === true && !!input.runner.runGeneratedTests + let results: Record | null = null + if (shouldExecute) { + try { + results = await input.runner.runGeneratedTests!(trackATests, input.config.dataDiff?.warehouse || undefined) + } catch { + results = null + } + } + + if (shouldExecute && results) { + for (const test of trackATests) { + const result = results[test.id] + if (!result || result.status === "error") { + log.warn("dropping spec-test execution error", { test: test.id, detail: result?.detail }) + continue + } + enforcedConstraints && enforcedConstraints.executed++ + if (result.status === "pass") { + enforcedConstraints && enforcedConstraints.passed++ + continue + } + enforcedConstraints && enforcedConstraints.failed++ + findings.push(executedSpecTestFinding(model, ctx.file.path, test, result.violatingRows ?? 1)) + } + } else { + findings.push(...trackATests.map((test) => proposedSpecTestFinding(model, ctx.file.path, test))) + } + } + + if (!input.generateSpecTests) return findings const specSources = dedupeSpecSources([ ...(await changedSchemaSources(model, input)), ...sqlIntentSources(ctx.newSql), ...sqlIntentSources(ctx.engineNewSql), ...prIntentSources(model, input), ]) - if (!specSources.length) return [] + if (!specSources.length) return findings let proposed: GeneratedTest[] = [] try { @@ -1003,26 +1159,12 @@ async function specTestSynthesisLane( prBody: input.prBody, }) } catch { - return [] + return findings } const { kept } = filterToSpecDerived(proposed, specSources) - return kept.map((test) => { - const yaml = proposedTestYaml(model, test) - const column = test.dbtTest?.column - return makeFinding({ - severity: "suggestion", - category: "test_coverage", - title: `${model}: proposed ${test.kind} test${column ? ` for ${column}` : ""}`, - body: proposedTestBody(test, yaml), - file: ctx.file.path, - model, - column, - confidence: "unknown", - evidence: { tool: "altimate.spec_test.proposed", result: { proposal: test, yaml } }, - ruleKey: "spec_test:" + test.derivedFrom.ref, - }) - }) + findings.push(...kept.map((test) => proposedSpecTestFinding(model, ctx.file.path, test))) + return findings } /** @@ -1371,6 +1513,7 @@ export async function runReview(input: OrchestrateInput): Promise 0 ? enforcedConstraints : undefined, }) return signEnvelope(envelope) } diff --git a/packages/opencode/src/altimate/review/runner.ts b/packages/opencode/src/altimate/review/runner.ts index e4b5e8f0f..1fd829e0a 100644 --- a/packages/opencode/src/altimate/review/runner.ts +++ b/packages/opencode/src/altimate/review/runner.ts @@ -1,6 +1,8 @@ import { Dispatcher } from "../native" import { parseManifest } from "../native/dbt/manifest" -import type { CheckResult, EquivalenceResult, GradeResult, ImpactResult, ReviewRunner } from "./orchestrate" +import { loadRawManifest } from "../native/dbt/helpers" +import type { CheckResult, DeclaredConstraint, EquivalenceResult, GradeResult, ImpactResult, ReviewRunner } from "./orchestrate" +import type { GeneratedTest, GeneratedTestResult } from "./spec-test-gen" import { buildReviewSchemaContext, type SchemaContext } from "./schema-context" /** @@ -17,6 +19,11 @@ interface ManifestModel { name: string depends_on: string[] path?: string + database?: string + schema_name?: string + relation_name?: string + alias?: string + raw?: any } interface CachedManifest { @@ -24,6 +31,7 @@ interface CachedManifest { byName: Map children: Map // unique_id -> direct child unique_ids testDeps: Map> // model unique_id -> set of test unique_ids depending on it + testNodes: any[] schemaContext?: SchemaContext // model/source columns for equivalence resolution ok: boolean } @@ -67,6 +75,179 @@ function bandConfidence(c: unknown): "high" | "medium" | "low" { return n >= 0.8 ? "high" : n >= 0.5 ? "medium" : "low" } +function boolish(value: unknown): boolean { + return value === true || value === "true" +} + +function normalizeConstraintKind(value: unknown): DeclaredConstraint["kind"] | undefined { + const kind = String(value ?? "") + .trim() + .toLowerCase() + .replace(/[\s-]+/g, "_") + if (kind === "not_null" || kind === "unique" || kind === "accepted_values" || kind === "relationships") + return kind + return undefined +} + +function testKind(node: any): DeclaredConstraint["kind"] | undefined { + const name = String(node?.test_metadata?.name ?? node?.name ?? "") + .trim() + .toLowerCase() + if (name === "unique_combination_of_columns") return "unique" + return normalizeConstraintKind(name) +} + +function testColumn(node: any): string | undefined { + const kwargs = node?.test_metadata?.kwargs ?? {} + const col = kwargs.column_name ?? node?.column_name + return typeof col === "string" && col.trim() ? col.trim() : undefined +} + +function testArgs(node: any): Record | undefined { + const kwargs = node?.test_metadata?.kwargs + if (!kwargs || typeof kwargs !== "object" || Array.isArray(kwargs)) return undefined + const out: Record = {} + for (const [key, value] of Object.entries(kwargs)) { + if (key === "model" || key === "column_name") continue + out[key] = value + } + return Object.keys(out).length ? out : undefined +} + +function constraintKey(kind: DeclaredConstraint["kind"], column?: string): string { + return `${kind}:${(column ?? "").toLowerCase()}` +} + +function sourceRef(model: string, kind: DeclaredConstraint["kind"], column?: string): string { + return column ? `schema.yml:${model}.${column}:${kind}` : `schema.yml:${model}:${kind}` +} + +function sqlIdent(name: string): string { + if (/^[A-Za-z_][A-Za-z0-9_]*$/.test(name)) return name + return `"${name.replace(/"/g, '""')}"` +} + +function sqlTableRef(name: string): string { + if (/["`\[]/.test(name)) return name + return name + .split(".") + .filter(Boolean) + .map(sqlIdent) + .join(".") +} + +function sqlLiteral(value: unknown): string { + if (value === null || value === undefined) return "NULL" + if (typeof value === "number" && Number.isFinite(value)) return String(value) + if (typeof value === "boolean") return value ? "TRUE" : "FALSE" + return "'" + String(value).replace(/'/g, "''") + "'" +} + +function relationForModel(model: ManifestModel): string { + if (model.relation_name) return model.relation_name + const parts = [model.database, model.schema_name, model.alias || model.name].filter((p): p is string => !!p) + return sqlTableRef(parts.join(".")) +} + +function modelNameFromSourceRef(ref: string): string | undefined { + const match = /^schema\.yml:([^.:]+)(?:[.:]|$)/.exec(ref) + return match?.[1] +} + +function columnsForUnique(test: GeneratedTest): string[] { + const args = test.dbtTest?.args ?? {} + const cols = + args["columns"] ?? + args["combination_of_columns"] ?? + args["column_names"] ?? + args["fields"] ?? + (test.dbtTest?.column ? [test.dbtTest.column] : undefined) + return Array.isArray(cols) ? cols.map(String).filter(Boolean) : [] +} + +function relationshipTarget(raw: unknown, manifest: CachedManifest): string | undefined { + if (typeof raw !== "string" || !raw.trim()) return undefined + const ref = /\bref\s*\(\s*['"]([^'"]+)['"]\s*\)/.exec(raw)?.[1] + if (ref) { + const model = manifest.byName.get(ref) + return model ? relationForModel(model) : sqlTableRef(ref) + } + const source = /\bsource\s*\(\s*['"]([^'"]+)['"]\s*,\s*['"]([^'"]+)['"]\s*\)/.exec(raw) + if (source) return sqlTableRef(`${source[1]}.${source[2]}`) + return sqlTableRef(raw) +} + +function buildGeneratedTestSql(test: GeneratedTest, manifest: CachedManifest): { sql?: string; detail?: string } { + const modelName = modelNameFromSourceRef(test.derivedFrom.ref) + const model = modelName ? manifest.byName.get(modelName) : undefined + if (!model) return { detail: `model not found for ${test.derivedFrom.ref}` } + const relation = relationForModel(model) + const column = test.dbtTest?.column + + if (test.kind === "not_null") { + if (!column) return { detail: "not_null test is missing column" } + return { sql: `select count(*) as violating_rows from ${relation} where ${sqlIdent(column)} is null` } + } + + if (test.kind === "unique") { + const columns = columnsForUnique(test) + if (!columns.length) return { detail: "unique test is missing columns" } + const selectCols = columns.map(sqlIdent).join(", ") + const nonNull = columns.map((c) => `${sqlIdent(c)} is not null`).join(" and ") + return { + sql: + `select count(*) as violating_rows from (` + + `select ${selectCols} from ${relation} where ${nonNull} group by ${selectCols} having count(*) > 1` + + `) as altimate_unique_violations`, + } + } + + if (test.kind === "accepted_values") { + if (!column) return { detail: "accepted_values test is missing column" } + const values = test.dbtTest?.args?.["values"] + if (!Array.isArray(values) || values.length === 0) return { detail: "accepted_values test is missing values" } + return { + sql: + `select count(*) as violating_rows from ${relation} ` + + `where ${sqlIdent(column)} is not null and ${sqlIdent(column)} not in (${values.map(sqlLiteral).join(", ")})`, + } + } + + if (test.kind === "relationships") { + if (!column) return { detail: "relationships test is missing column" } + const args = test.dbtTest?.args ?? {} + const target = relationshipTarget(args["to"], manifest) + const field = typeof args["field"] === "string" && args["field"].trim() ? args["field"].trim() : undefined + if (!target || !field) return { detail: "relationships test is missing target relation or field" } + return { + sql: + `select count(*) as violating_rows from ${relation} as child ` + + `left join ${target} as parent on child.${sqlIdent(column)} = parent.${sqlIdent(field)} ` + + `where child.${sqlIdent(column)} is not null and parent.${sqlIdent(field)} is null`, + } + } + + return { detail: `${test.kind} execution is not supported by a row-count assertion` } +} + +function resultCount(res: any): number | undefined { + const row = Array.isArray(res?.rows) ? res.rows[0] : undefined + if (Array.isArray(row)) { + const n = Number(row[0]) + return Number.isFinite(n) ? n : undefined + } + if (row && typeof row === "object") { + const value = row.violating_rows ?? Object.values(row)[0] + const n = Number(value) + return Number.isFinite(n) ? n : undefined + } + return undefined +} + +function isNoWarehouseError(message: string): boolean { + return /no warehouse configured|connection .*not found|warehouse .*not found|driver .*not found|not configured/i.test(message) +} + /** * Map a core lint finding (by rule name / L0xx code) to a review category. * Core's `LintFinding` carries no category, so without this every AST finding @@ -121,13 +302,24 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun // native dispatcher registration path so a core-loading failure // cannot incorrectly downgrade a valid dbt run to lint-only. const res = await parseManifest({ path: opts.manifestPath }) + const rawManifest = loadRawManifest(opts.manifestPath) + const rawNodes = (rawManifest?.nodes ?? {}) as Record const models = new Map() const byName = new Map() const children = new Map() const nodes = [...asArray(res.models), ...asArray((res as any).snapshots)] for (const m of nodes) { - models.set(m.unique_id, m) - byName.set(m.name, m) + const raw = rawNodes[m.unique_id] + const enriched = { + ...m, + database: (m as any).database, + schema_name: (m as any).schema_name, + relation_name: raw?.relation_name, + alias: raw?.alias, + raw, + } + models.set(enriched.unique_id, enriched) + byName.set(enriched.name, enriched) } // Invert depends_on (upstream) into children (downstream). for (const m of nodes) { @@ -139,6 +331,7 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun // Map each model to the SET of tests depending on it (by test // unique_id), so a multi-model test isn't counted more than once. const testDeps = new Map>() + const testNodes = Object.values(rawNodes).filter((n) => n?.resource_type === "test") for (const t of asArray<{ unique_id: string; depends_on: string[] }>(res.tests)) { for (const dep of asArray(t.depends_on)) { if (!testDeps.has(dep)) testDeps.set(dep, new Set()) @@ -152,13 +345,14 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun asArray(res.seeds), asArray((res as any).snapshots), ) - return { models, byName, children, testDeps, schemaContext, ok: models.size > 0 } + return { models, byName, children, testDeps, testNodes, schemaContext, ok: models.size > 0 } } catch { return { models: new Map(), byName: new Map(), children: new Map(), testDeps: new Map(), + testNodes: [], ok: false, } } @@ -362,6 +556,112 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun return Array.isArray(pk) && pk.length ? pk.map((c: string) => String(c)) : undefined }, + async declaredConstraints(model: string): Promise { + try { + const mf = await loadManifest() + const target = mf.byName.get(model) ?? [...mf.models.values()].find((m) => m.name.endsWith(`.${model}`)) + if (!target?.raw) return [] + + const enforcing = new Set() + const testConstraints: DeclaredConstraint[] = [] + for (const test of mf.testNodes) { + const deps = asArray(test?.depends_on?.nodes) + if (test?.attached_node !== target.unique_id && !deps.includes(target.unique_id)) continue + const kind = testKind(test) + if (!kind) continue + const column = testColumn(test) + enforcing.add(constraintKey(kind, column)) + testConstraints.push({ + kind, + column, + args: testArgs(test), + hasEnforcingTest: true, + uniqueId: test.unique_id, + sourceRef: sourceRef(target.name, kind, column), + }) + } + + const out: DeclaredConstraint[] = [...testConstraints] + const seen = new Set(out.map((c) => `${c.kind}:${c.column ?? ""}:${c.uniqueId ?? c.sourceRef}`)) + const add = (c: DeclaredConstraint) => { + const key = `${c.kind}:${c.column ?? ""}:${c.uniqueId ?? c.sourceRef}` + if (seen.has(key)) return + seen.add(key) + out.push(c) + } + + const raw = target.raw + const contract = raw?.config?.contract ?? raw?.contract + if (boolish(contract?.enforced)) { + for (const [key, rawColumn] of Object.entries(raw?.columns ?? {})) { + const column = String(rawColumn?.name ?? key) + const dataType = String(rawColumn?.data_type ?? rawColumn?.type ?? "").trim() + if (dataType) { + const kind: DeclaredConstraint["kind"] = "column_type" + add({ + kind, + column, + args: { data_type: dataType }, + hasEnforcingTest: enforcing.has(constraintKey(kind, column)), + uniqueId: target.unique_id, + sourceRef: sourceRef(target.name, kind, column), + }) + } + for (const rawConstraint of asArray(rawColumn?.constraints)) { + const normalized = String(rawConstraint?.type ?? rawConstraint) + .trim() + .toLowerCase() + .replace(/[\s-]+/g, "_") + const kinds: DeclaredConstraint["kind"][] = + normalized === "primary_key" + ? ["not_null", "unique"] + : normalizeConstraintKind(normalized) + ? [normalizeConstraintKind(normalized)!] + : [] + for (const kind of kinds) { + const args = + kind === "accepted_values" || kind === "relationships" + ? typeof rawConstraint === "object" && rawConstraint + ? rawConstraint + : undefined + : undefined + add({ + kind, + column, + args, + hasEnforcingTest: enforcing.has(constraintKey(kind, column)), + uniqueId: target.unique_id, + sourceRef: sourceRef(target.name, kind, column), + }) + } + } + } + + for (const rawConstraint of asArray(raw?.constraints)) { + const normalized = String(rawConstraint?.type ?? rawConstraint) + .trim() + .toLowerCase() + .replace(/[\s-]+/g, "_") + if (normalized !== "primary_key" && normalized !== "unique") continue + const columns = asArray(rawConstraint?.columns).map(String).filter(Boolean) + if (!columns.length) continue + const kind: DeclaredConstraint["kind"] = "unique" + add({ + kind, + args: { columns }, + hasEnforcingTest: enforcing.has(constraintKey(kind)), + uniqueId: target.unique_id, + sourceRef: sourceRef(target.name, kind), + }) + } + } + + return out + } catch { + return [] + } + }, + async downstreamModels(model: string): Promise> { const m = await loadManifest() const node = m.byName.get(model) @@ -436,6 +736,39 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun } }, + async runGeneratedTests(tests: GeneratedTest[], warehouse?: string): Promise | null> { + if (!tests.length) return {} + const mf = await loadManifest() + const out: Record = {} + for (const test of tests) { + const built = buildGeneratedTestSql(test, mf) + if (!built.sql) { + out[test.id] = { status: "error", detail: built.detail } + continue + } + try { + const res = await Dispatcher.call("sql.execute", { sql: built.sql, warehouse: warehouse || undefined, limit: 1 }) + const error = String((res as any)?.error ?? "") + if (error) { + if (isNoWarehouseError(error)) return null + out[test.id] = { status: "error", detail: error } + continue + } + const violatingRows = resultCount(res) + if (violatingRows === undefined) { + out[test.id] = { status: "error", detail: "could not read violating row count" } + continue + } + out[test.id] = violatingRows === 0 ? { status: "pass", violatingRows } : { status: "fail", violatingRows } + } catch (err) { + const detail = err instanceof Error ? err.message : String(err) + if (isNoWarehouseError(detail)) return null + out[test.id] = { status: "error", detail } + } + } + return out + }, + async columnLineage(sql: string, dialect?: string): Promise> { try { const res = await Dispatcher.call("altimate_core.column_lineage", { diff --git a/packages/opencode/src/altimate/review/spec-test-gen.ts b/packages/opencode/src/altimate/review/spec-test-gen.ts index 6426e2baa..9bbcb7335 100644 --- a/packages/opencode/src/altimate/review/spec-test-gen.ts +++ b/packages/opencode/src/altimate/review/spec-test-gen.ts @@ -72,10 +72,11 @@ export interface SpecSource { args?: Record } -/** Test classes. `not_null`/`unique`/`accepted_values`/`relationships` map to - * track-A declared constraints; `range` is an inferred-context proposal. No - * golden/snapshot class exists — current output is never an expected value. */ -export type GeneratedTestKind = "not_null" | "unique" | "accepted_values" | "relationships" | "range" +/** Test classes. `not_null`/`unique`/`accepted_values`/`relationships`/ + * `column_type` map to track-A declared constraints; `range` is an + * inferred-context proposal. No golden/snapshot class exists — current output + * is never an expected value. */ +export type GeneratedTestKind = "not_null" | "unique" | "accepted_values" | "relationships" | "column_type" | "range" /** Kinds that can derive from a declared constraint (track A, block-eligible when * executed). `range` and any description-derived test are advisory only. */ @@ -84,12 +85,16 @@ export const DECLARED_CONSTRAINT_KINDS: ReadonlySet = new Set "unique", "accepted_values", "relationships", + "column_type", ]) /** All kinds a proposal may carry; a test whose `kind` is outside this set is * dropped by the advisory-track guard. */ export const ALLOWED_TEST_KINDS: ReadonlySet = new Set([ - ...DECLARED_CONSTRAINT_KINDS, + "not_null", + "unique", + "accepted_values", + "relationships", "range", ]) diff --git a/packages/opencode/src/altimate/review/verdict.ts b/packages/opencode/src/altimate/review/verdict.ts index a372014f8..33fb5a867 100644 --- a/packages/opencode/src/altimate/review/verdict.ts +++ b/packages/opencode/src/altimate/review/verdict.ts @@ -122,6 +122,13 @@ export const VerdictEnvelope = z.object({ suggestion: z.number().int().nonnegative(), /** True when the review ran without a manifest/warehouse (lint-only). */ degraded: z.boolean(), + enforcedConstraints: z + .object({ + executed: z.number().int().nonnegative(), + passed: z.number().int().nonnegative(), + failed: z.number().int().nonnegative(), + }) + .optional(), }), engine: EngineVersions, /** Hash of the dbt manifest the verdict was computed against, when present. */ @@ -150,12 +157,23 @@ export interface BuildEnvelopeInput { manifestHash?: string generatedAt?: string degraded?: boolean + enforcedConstraints?: { executed: number; passed: number; failed: number } } -function summarize(findings: Finding[], degraded: boolean): VerdictEnvelope["summary"] { +function summarize( + findings: Finding[], + degraded: boolean, + enforcedConstraints?: BuildEnvelopeInput["enforcedConstraints"], +): VerdictEnvelope["summary"] { const tally: Record = { critical: 0, warning: 0, suggestion: 0 } for (const f of findings) tally[f.severity]++ - return { critical: tally.critical, warning: tally.warning, suggestion: tally.suggestion, degraded } + return { + critical: tally.critical, + warning: tally.warning, + suggestion: tally.suggestion, + degraded, + enforcedConstraints, + } } /** Assemble the verdict envelope (unsigned). Call signEnvelope to sign it. */ @@ -171,7 +189,7 @@ export function buildEnvelope(input: BuildEnvelopeInput): VerdictEnvelope { mode: input.mode, tier: input.tier, findings: input.findings, - summary: summarize(input.findings, degraded), + summary: summarize(input.findings, degraded, input.enforcedConstraints), engine: EngineVersions.parse(input.engine ?? {}), manifestHash: input.manifestHash, generatedAt: input.generatedAt, diff --git a/packages/opencode/test/altimate/review-spec-test-exec.test.ts b/packages/opencode/test/altimate/review-spec-test-exec.test.ts new file mode 100644 index 000000000..7d44bfa45 --- /dev/null +++ b/packages/opencode/test/altimate/review-spec-test-exec.test.ts @@ -0,0 +1,197 @@ +import { describe, test, expect } from "bun:test" +import { + DEFAULT_REVIEW_CONFIG, + DEFAULT_RUBRIC, + runReview, + type ChangedFile, + type DeclaredConstraint, + type GeneratedTest, + type ReviewRunner, +} from "../../src/altimate/review" + +const changedFiles: ChangedFile[] = [ + { path: "models/marts/fct_orders.sql", status: "added", diff: "+select order_id from {{ ref('stg_orders') }}\n" }, +] + +function baseRunner(overrides: Partial = {}): ReviewRunner { + return { + async impact() { + return { hasManifest: true, severity: "SAFE", directCount: 0, transitiveCount: 0, testCount: 0 } + }, + async grade() { + return { grade: "A" } + }, + async check() { + return { issues: [], ran: true } + }, + async equivalence() { + return { decided: false } + }, + async detectPii() { + return { columns: [] } + }, + ...overrides, + } as ReviewRunner +} + +function declared(overrides: Partial = {}): DeclaredConstraint { + return { + kind: "not_null", + column: "order_id", + hasEnforcingTest: false, + sourceRef: "schema.yml:fct_orders.order_id:not_null", + ...overrides, + } +} + +async function reviewWith(runner: ReviewRunner, execute: boolean, generateSpecTests?: () => Promise) { + return runReview({ + changedFiles, + config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["spec_tests"], ai: false, specTests: { execute }, mode: "gate" }, + rubric: DEFAULT_RUBRIC, + mode: "gate", + runner, + getContent: async (file, side) => + side === "new" && file.endsWith(".sql") ? "select order_id from {{ ref('stg_orders') }}" : undefined, + generateSpecTests, + }) +} + +describe("spec test synthesis lane (P1 declared-constraint execution)", () => { + test("failed executed declared not_null emits blocking critical contract_violation", async () => { + const env = await reviewWith( + baseRunner({ + async declaredConstraints() { + return [declared()] + }, + async runGeneratedTests(tests) { + return { [tests[0]!.id]: { status: "fail", violatingRows: 2 } } + }, + }), + true, + ) + + const finding = env.findings.find((f) => f.evidence?.tool === "altimate.spec_test.executed") + expect(finding).toBeTruthy() + expect(finding).toMatchObject({ + severity: "critical", + category: "contract_violation", + confidence: "high", + }) + expect(finding?.evidence?.result).toMatchObject({ + executed: true, + origin: "declared_constraint", + violatingRows: 2, + }) + expect((finding?.evidence?.result as any).test.derivedFrom.origin).toBe("declared_constraint") + expect(env.verdict).toBe("REQUEST_CHANGES") + }) + + test("passed executed declared constraint emits no finding and records coverage", async () => { + const env = await reviewWith( + baseRunner({ + async declaredConstraints() { + return [declared()] + }, + async runGeneratedTests(tests) { + return { [tests[0]!.id]: { status: "pass", violatingRows: 0 } } + }, + }), + true, + ) + + expect(env.findings.some((f) => f.evidence?.tool === "altimate.spec_test.executed")).toBe(false) + expect(env.summary.enforcedConstraints).toEqual({ executed: 1, passed: 1, failed: 0 }) + }) + + test("no warehouse falls back to proposed enforcing test and does not request changes", async () => { + const env = await reviewWith( + baseRunner({ + async declaredConstraints() { + return [declared()] + }, + async runGeneratedTests() { + return null + }, + }), + true, + ) + + const proposed = env.findings.find((f) => f.evidence?.tool === "altimate.spec_test.proposed") + expect(proposed).toMatchObject({ severity: "suggestion", confidence: "unknown", category: "test_coverage" }) + expect(env.verdict).not.toBe("REQUEST_CHANGES") + }) + + test("execute false never calls runGeneratedTests and proposes instead", async () => { + let calls = 0 + const env = await reviewWith( + baseRunner({ + async declaredConstraints() { + return [declared()] + }, + async runGeneratedTests() { + calls++ + return {} + }, + }), + false, + ) + + expect(calls).toBe(0) + expect(env.findings.some((f) => f.evidence?.tool === "altimate.spec_test.proposed")).toBe(true) + }) + + test("track-B proposal is never executed and never critical", async () => { + let calls = 0 + const env = await reviewWith( + baseRunner({ + async declaredConstraints() { + return [] + }, + async runGeneratedTests() { + calls++ + return {} + }, + }), + true, + async () => [ + { + id: "track-b", + kind: "not_null", + dbtTest: { column: "order_id", test: "not_null" }, + rationale: "soft intent", + derivedFrom: { + origin: "inferred_context", + kind: "ref_edge", + ref: "ref:stg_orders", + text: "stg_orders", + }, + }, + ], + ) + + expect(calls).toBe(0) + const proposed = env.findings.find((f) => f.evidence?.tool === "altimate.spec_test.proposed") + expect(proposed?.severity).toBe("suggestion") + expect(env.findings.some((f) => f.severity === "critical")).toBe(false) + }) + + test("already-enforced declared constraint is not materialized", async () => { + let calls = 0 + const env = await reviewWith( + baseRunner({ + async declaredConstraints() { + return [declared({ hasEnforcingTest: true })] + }, + async runGeneratedTests() { + calls++ + return {} + }, + }), + true, + ) + + expect(calls).toBe(0) + expect(env.findings.some((f) => f.evidence?.tool?.startsWith("altimate.spec_test"))).toBe(false) + }) +}) From d5055d1fd349252ad26997046bdef9f8892c55c2 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Tue, 7 Jul 2026 17:46:24 -0700 Subject: [PATCH 4/9] fix: [dbt-pr-review] address bot review findings + sandboxed track-B execution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings (#990): - Include test identity (ref:kind:column) in spec-test ruleKey so distinct proposals from one source no longer collapse in dedupe(). - filterToSpecDerived replaces derivedFrom with the trusted extracted source (matched by ref), so a forged origin/kind can't survive. - Guard requires dbtTest.test === kind (drop test_mismatch), blocking arbitrary macros from reaching the proposed patch. - Build the committable schema.yml patch from evidence.result.yaml, not by reparsing the LLM-influenced finding body. - Don't turn already-enforcing schema.yml tests into proposal sources. - Track A (deterministic) now runs even when AI is disabled: the lane is gated on (generateSpecTests || runner.declaredConstraints), not the LLM injection. Track-B execution + sandbox: - New spec-test-sandbox.ts sanitizeAssertionSql: single SELECT only (state-machine scan), no DDL/DML/side-effects, CTE-aware relation allowlist (model + declared upstreams), wrapped as a bounded count(*). Rejected SQL is dropped, never run. - Executed track-B failures emit warning/unknown under altimate.spec_test.candidate (category test_coverage) — never critical, never blocking (also excluded from the verdict warning-count). Only track-A executed+declared can block. 156 review tests pass; verdict gate specTestMayBlock unchanged. Refs #989 Co-Authored-By: Claude Opus 4.8 --- .../opencode/src/altimate/review/format.ts | 32 ++- .../opencode/src/altimate/review/index.ts | 1 + .../src/altimate/review/orchestrate.ts | 139 +++++++++-- .../opencode/src/altimate/review/runner.ts | 82 ++++++- .../src/altimate/review/spec-test-gen.ts | 18 +- .../src/altimate/review/spec-test-sandbox.ts | 216 ++++++++++++++++++ .../opencode/src/altimate/review/verdict.ts | 7 +- .../altimate/review-spec-test-exec.test.ts | 46 +++- .../altimate/review-spec-test-gen.test.ts | 142 ++++++++++++ .../altimate/review-spec-test-sandbox.test.ts | 32 +++ .../altimate/review-spec-test-verdict.test.ts | 24 ++ 11 files changed, 687 insertions(+), 52 deletions(-) create mode 100644 packages/opencode/src/altimate/review/spec-test-sandbox.ts create mode 100644 packages/opencode/test/altimate/review-spec-test-sandbox.test.ts diff --git a/packages/opencode/src/altimate/review/format.ts b/packages/opencode/src/altimate/review/format.ts index 697380d96..92a315311 100644 --- a/packages/opencode/src/altimate/review/format.ts +++ b/packages/opencode/src/altimate/review/format.ts @@ -145,28 +145,24 @@ function stripYamlFence(body: string): string { return body.replace(/```yaml[\s\S]*?```/g, "").trim() } -function yamlFences(body: string): string[] { - return [...body.matchAll(/```yaml\s*([\s\S]*?)```/g)].map((m) => m[1].trim()).filter(Boolean) -} - function proposedTestsPatch(findings: Finding[]): string { const merged = new Map() for (const f of findings) { - for (const yaml of yamlFences(f.body)) { - try { - const doc = YAML.parse(yaml) - for (const rawModel of Array.isArray(doc?.models) ? doc.models : []) { - const modelName = typeof rawModel?.name === "string" ? rawModel.name : "" - if (!modelName) continue - const target = merged.get(modelName) ?? { name: modelName } - mergeTests(target, rawModel) - mergeColumns(target, rawModel) - merged.set(modelName, target) - } - } catch { - // Bodies are generated locally; if one is malformed, omit it from the - // aggregate patch instead of breaking PR summary rendering. + const yaml = (f.evidence?.result as any)?.yaml + if (typeof yaml !== "string" || !yaml.trim()) continue + try { + const doc = YAML.parse(yaml) + for (const rawModel of Array.isArray(doc?.models) ? doc.models : []) { + const modelName = typeof rawModel?.name === "string" ? rawModel.name : "" + if (!modelName) continue + const target = merged.get(modelName) ?? { name: modelName } + mergeTests(target, rawModel) + mergeColumns(target, rawModel) + merged.set(modelName, target) } + } catch { + // The aggregate patch is built only from locally generated evidence YAML. + // If one snippet is malformed, omit it instead of breaking summary rendering. } } if (!merged.size) return "" diff --git a/packages/opencode/src/altimate/review/index.ts b/packages/opencode/src/altimate/review/index.ts index 1ae647ffe..3dc3bb780 100644 --- a/packages/opencode/src/altimate/review/index.ts +++ b/packages/opencode/src/altimate/review/index.ts @@ -32,3 +32,4 @@ export * from "./dbt-patterns" export * from "./rule-catalog" export * from "./compiled" export * from "./spec-test-gen" +export * from "./spec-test-sandbox" diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index 618940a79..295b41234 100644 --- a/packages/opencode/src/altimate/review/orchestrate.ts +++ b/packages/opencode/src/altimate/review/orchestrate.ts @@ -25,6 +25,7 @@ import { type SpecSource, type SpecTestGenInput, } from "./spec-test-gen" +import { sanitizeAssertionSql } from "./spec-test-sandbox" /** * The deterministic review recipe. @@ -133,6 +134,11 @@ export interface DeclaredConstraint { sourceRef: string } +export interface GeneratedTestExecutionOptions { + model?: string + allowedRelations?: string[] +} + /** High-level engine surface the orchestrator depends on. */ export interface ReviewRunner { /** True when the configured dbt manifest loaded, independent of model lookup. */ @@ -168,6 +174,7 @@ export interface ReviewRunner { runGeneratedTests?( tests: GeneratedTest[], warehouse?: string, + options?: GeneratedTestExecutionOptions, ): Promise | null> /** Per-upstream WHERE-filter columns of a model's SQL (cross-model consistency). */ sourceFilters?(sql: string): Promise> @@ -866,8 +873,22 @@ function schemaModelSources(model: string, schemaDoc: unknown): SpecSource[] { const name = text(column?.name) if (!column || !name) continue + const parsedTests = [...array(column.tests), ...array(column.data_tests)] + .map(parseDbtTestSpec) + .filter((parsed): parsed is { kind: SpecSource["kind"]; args?: Record } => !!parsed) + const rawConstraints = array(column.constraints) + const parsedConstraints = rawConstraints + .map((rawConstraint) => { + const constraint = record(rawConstraint) + const type = text(constraint?.type) + return type && DECLARED_SPEC_SOURCE_KINDS.has(type) ? { type, constraint } : undefined + }) + .filter((parsed): parsed is { type: string; constraint: Record | undefined } => !!parsed) + const dataType = text(column.data_type) + const alreadyEnforced = parsedTests.length > 0 || parsedConstraints.length > 0 || (contractEnforced && !!dataType) + const description = text(column.description) - if (description) { + if (description && !alreadyEnforced) { out.push({ origin: "inferred_context", kind: "schema_desc", @@ -876,7 +897,6 @@ function schemaModelSources(model: string, schemaDoc: unknown): SpecSource[] { }) } - const dataType = text(column.data_type) if (contractEnforced && dataType) { out.push({ origin: "declared_constraint", @@ -886,9 +906,7 @@ function schemaModelSources(model: string, schemaDoc: unknown): SpecSource[] { }) } - for (const rawTest of [...array(column.tests), ...array(column.data_tests)]) { - const parsed = parseDbtTestSpec(rawTest) - if (!parsed) continue + for (const parsed of parsedTests) { out.push({ origin: "declared_constraint", kind: parsed.kind, @@ -898,10 +916,7 @@ function schemaModelSources(model: string, schemaDoc: unknown): SpecSource[] { }) } - for (const rawConstraint of array(column.constraints)) { - const constraint = record(rawConstraint) - const type = text(constraint?.type) - if (!type || !DECLARED_SPEC_SOURCE_KINDS.has(type)) continue + for (const { type, constraint } of parsedConstraints) { out.push({ origin: "declared_constraint", kind: type as SpecSource["kind"], @@ -1000,6 +1015,10 @@ function proposedTestBody(test: GeneratedTest, yaml: string): string { .join("\n") } +function candidateTestBody(test: GeneratedTest): string { + return `candidate test derived from \`${test.derivedFrom.ref}\` fails on current data — confirm whether this is an intended property.` +} + interface EnforcedConstraintMetrics { executed: number passed: number @@ -1046,6 +1065,10 @@ function materializeDeclaredConstraints(model: string, constraints: DeclaredCons return tests } +function specTestRuleKey(test: GeneratedTest): string { + return `spec_test:${test.derivedFrom.ref}:${test.kind}:${test.dbtTest?.column ?? ""}` +} + function proposedSpecTestFinding(model: string, file: string, test: GeneratedTest): Finding { const yaml = proposedTestYaml(model, test) const column = test.dbtTest?.column @@ -1059,7 +1082,7 @@ function proposedSpecTestFinding(model: string, file: string, test: GeneratedTes column, confidence: "unknown", evidence: { tool: "altimate.spec_test.proposed", result: { proposal: test, yaml } }, - ruleKey: "spec_test:" + test.derivedFrom.ref, + ruleKey: specTestRuleKey(test), }) } @@ -1080,10 +1103,57 @@ function executedSpecTestFinding(model: string, file: string, test: GeneratedTes tool: "altimate.spec_test.executed", result: { executed: true, origin: "declared_constraint", test, violatingRows }, }, - ruleKey: "spec_test:" + test.derivedFrom.ref, + ruleKey: specTestRuleKey(test), }) } +function candidateSpecTestFinding(model: string, file: string, test: GeneratedTest, violatingRows: number): Finding { + const column = test.dbtTest?.column + return makeFinding({ + severity: "warning", + category: "test_coverage", + title: `${model}: candidate ${test.kind} test fails${column ? ` for ${column}` : ""}`, + body: candidateTestBody(test), + file, + model, + column, + confidence: "unknown", + evidence: { + tool: "altimate.spec_test.candidate", + result: { executed: true, origin: "inferred_context", test, violatingRows }, + }, + ruleKey: specTestRuleKey(test), + }) +} + +function allowedRelationsForSpecTests(model: string, specSources: SpecSource[]): string[] { + const out = new Set([model]) + for (const source of specSources) { + if (source.kind !== "ref_edge") continue + const name = source.text ?? source.ref.replace(/^(ref|source):/, "") + if (name) out.add(name) + } + return [...out] +} + +function executableTrackBTests(tests: GeneratedTest[], allowedRelations: string[]): GeneratedTest[] { + const out: GeneratedTest[] = [] + for (const test of tests) { + if (test.dbtTest) { + out.push(test) + continue + } + if (!test.assertionSql) continue + const sanitized = sanitizeAssertionSql(test.assertionSql, allowedRelations) + if (!sanitized.ok) { + log.warn("dropping spec-test assertionSql rejected by sandbox", { test: test.id, reason: sanitized.reason }) + continue + } + out.push(test) + } + return out +} + async function specTestSynthesisLane( ctx: ModelContext, input: OrchestrateInput, @@ -1094,6 +1164,7 @@ async function specTestSynthesisLane( const model = modelNameFromPath(ctx.file.path) const sql = ctx.engineNewSql ?? ctx.newSql ?? "" const findings: Finding[] = [] + const trackAAllowedRelations = [model] let trackATests: GeneratedTest[] = [] if (input.runner.declaredConstraints) { @@ -1109,7 +1180,10 @@ async function specTestSynthesisLane( let results: Record | null = null if (shouldExecute) { try { - results = await input.runner.runGeneratedTests!(trackATests, input.config.dataDiff?.warehouse || undefined) + results = await input.runner.runGeneratedTests!(trackATests, input.config.dataDiff?.warehouse || undefined, { + model, + allowedRelations: trackAAllowedRelations, + }) } catch { results = null } @@ -1163,7 +1237,42 @@ async function specTestSynthesisLane( } const { kept } = filterToSpecDerived(proposed, specSources) - findings.push(...kept.map((test) => proposedSpecTestFinding(model, ctx.file.path, test))) + if (!kept.length) return findings + + const shouldExecuteTrackB = input.config.specTests?.execute === true && !!input.runner.runGeneratedTests + if (!shouldExecuteTrackB) { + findings.push(...kept.map((test) => proposedSpecTestFinding(model, ctx.file.path, test))) + return findings + } + + const allowedRelations = allowedRelationsForSpecTests(model, specSources) + const executable = executableTrackBTests(kept, allowedRelations) + if (!executable.length) return findings + + let results: Record | null = null + try { + results = await input.runner.runGeneratedTests!(executable, input.config.dataDiff?.warehouse || undefined, { + model, + allowedRelations, + }) + } catch { + return findings + } + + if (!results) { + findings.push(...executable.map((test) => proposedSpecTestFinding(model, ctx.file.path, test))) + return findings + } + + for (const test of executable) { + const result = results[test.id] + if (!result || result.status === "error") { + log.warn("dropping candidate spec-test execution error", { test: test.id, detail: result?.detail }) + continue + } + if (result.status === "pass") continue + findings.push(candidateSpecTestFinding(model, ctx.file.path, test, result.violatingRows ?? 1)) + } return findings } @@ -1546,7 +1655,9 @@ export async function runReview(input: OrchestrateInput): Promise(v: unknown): T[] { return Array.isArray(v) ? (v as T[]) : [] } @@ -149,6 +160,40 @@ function relationForModel(model: ManifestModel): string { return sqlTableRef(parts.join(".")) } +function findModel(manifest: CachedManifest, model: string | undefined): ManifestModel | undefined { + if (!model) return undefined + return manifest.byName.get(model) ?? [...manifest.models.values()].find((m) => m.name.endsWith(`.${model}`)) +} + +function addRelationVariants(out: Set, relation: string | undefined): void { + if (!relation) return + const trimmed = relation.trim() + if (!trimmed) return + out.add(trimmed) + const parts = trimmed + .replace(/[`"\[\]]/g, "") + .split(".") + .map((p) => p.trim()) + .filter(Boolean) + if (parts.length) out.add(parts[parts.length - 1]!) + if (parts.length >= 2) out.add(parts.slice(-2).join(".")) +} + +function expandedAllowedRelations(manifest: CachedManifest, options?: GeneratedTestExecutionOptions): string[] { + const out = new Set() + const seed = [...(options?.allowedRelations ?? []), options?.model].filter((v): v is string => !!v) + for (const relation of seed) { + addRelationVariants(out, relation) + const model = findModel(manifest, relation) + if (model) { + addRelationVariants(out, model.name) + addRelationVariants(out, model.alias) + addRelationVariants(out, relationForModel(model)) + } + } + return [...out] +} + function modelNameFromSourceRef(ref: string): string | undefined { const match = /^schema\.yml:([^.:]+)(?:[.:]|$)/.exec(ref) return match?.[1] @@ -177,9 +222,18 @@ function relationshipTarget(raw: unknown, manifest: CachedManifest): string | un return sqlTableRef(raw) } -function buildGeneratedTestSql(test: GeneratedTest, manifest: CachedManifest): { sql?: string; detail?: string } { - const modelName = modelNameFromSourceRef(test.derivedFrom.ref) - const model = modelName ? manifest.byName.get(modelName) : undefined +function buildGeneratedTestSql( + test: GeneratedTest, + manifest: CachedManifest, + options?: GeneratedTestExecutionOptions, +): { sql?: string; detail?: string } { + if (!test.dbtTest && test.assertionSql) { + const sanitized = sanitizeAssertionSql(test.assertionSql, expandedAllowedRelations(manifest, options)) + return sanitized.ok ? { sql: sanitized.sql } : { detail: `assertionSql rejected by sandbox: ${sanitized.reason}` } + } + + const modelName = modelNameFromSourceRef(test.derivedFrom.ref) ?? options?.model + const model = findModel(manifest, modelName) if (!model) return { detail: `model not found for ${test.derivedFrom.ref}` } const relation = relationForModel(model) const column = test.dbtTest?.column @@ -248,6 +302,13 @@ function isNoWarehouseError(message: string): boolean { return /no warehouse configured|connection .*not found|warehouse .*not found|driver .*not found|not configured/i.test(message) } +function withTimeout(promise: Promise, ms: number): Promise { + return Promise.race([ + promise, + new Promise((_, reject) => setTimeout(() => reject(new Error(`spec-test SQL timed out after ${ms}ms`)), ms)), + ]) +} + /** * Map a core lint finding (by rule name / L0xx code) to a review category. * Core's `LintFinding` carries no category, so without this every AST finding @@ -736,18 +797,25 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun } }, - async runGeneratedTests(tests: GeneratedTest[], warehouse?: string): Promise | null> { + async runGeneratedTests( + tests: GeneratedTest[], + warehouse?: string, + options?: GeneratedTestExecutionOptions, + ): Promise | null> { if (!tests.length) return {} const mf = await loadManifest() const out: Record = {} for (const test of tests) { - const built = buildGeneratedTestSql(test, mf) + const built = buildGeneratedTestSql(test, mf, options) if (!built.sql) { out[test.id] = { status: "error", detail: built.detail } continue } try { - const res = await Dispatcher.call("sql.execute", { sql: built.sql, warehouse: warehouse || undefined, limit: 1 }) + const res = await withTimeout( + Dispatcher.call("sql.execute", { sql: built.sql, warehouse: warehouse || undefined, limit: 1 }), + SPEC_TEST_SQL_TIMEOUT_MS, + ) const error = String((res as any)?.error ?? "") if (error) { if (isNoWarehouseError(error)) return null diff --git a/packages/opencode/src/altimate/review/spec-test-gen.ts b/packages/opencode/src/altimate/review/spec-test-gen.ts index 9bbcb7335..dc67517d9 100644 --- a/packages/opencode/src/altimate/review/spec-test-gen.ts +++ b/packages/opencode/src/altimate/review/spec-test-gen.ts @@ -141,7 +141,12 @@ export interface GeneratedTestResult { } /** Reason a proposal was rejected by the advisory-track guard. */ -export type GuardDropReason = "no_derived_from" | "kind_not_allowed" | "empty_ref" | "ref_not_provided" +export type GuardDropReason = + | "no_derived_from" + | "kind_not_allowed" + | "empty_ref" + | "ref_not_provided" + | "test_mismatch" /** * ADVISORY-TRACK anti-fabrication guard (deterministic; no LLM trust). @@ -168,7 +173,7 @@ export function filterToSpecDerived( tests: GeneratedTest[], providedSources: SpecSource[], ): { kept: GeneratedTest[]; dropped: Array<{ test: GeneratedTest; reason: GuardDropReason }> } { - const providedRefs = new Set(providedSources.map((s) => s.ref)) + const providedByRef = new Map(providedSources.map((s) => [s.ref, s] as const)) const kept: GeneratedTest[] = [] const dropped: Array<{ test: GeneratedTest; reason: GuardDropReason }> = [] for (const t of tests) { @@ -185,11 +190,16 @@ export function filterToSpecDerived( dropped.push({ test: t, reason: "empty_ref" }) continue } - if (!providedRefs.has(df.ref)) { + const trustedSource = providedByRef.get(df.ref) + if (!trustedSource) { dropped.push({ test: t, reason: "ref_not_provided" }) continue } - kept.push(t) + if (t.dbtTest && t.dbtTest.test !== t.kind) { + dropped.push({ test: t, reason: "test_mismatch" }) + continue + } + kept.push({ ...t, derivedFrom: trustedSource }) } return { kept, dropped } } diff --git a/packages/opencode/src/altimate/review/spec-test-sandbox.ts b/packages/opencode/src/altimate/review/spec-test-sandbox.ts new file mode 100644 index 000000000..d7c5bc3c2 --- /dev/null +++ b/packages/opencode/src/altimate/review/spec-test-sandbox.ts @@ -0,0 +1,216 @@ +const SIDE_EFFECT_RE = + /\b(insert|update|delete|drop|alter|create|truncate|merge|grant|revoke|call|exec|execute|copy|into|vacuum|attach)\b/i + +export type SanitizedAssertionSql = + | { ok: true; sql: string } + | { ok: false; reason: "empty" | "multi_statement" | "side_effect" | "not_select" | "unknown_relation" } + +type ScanState = "normal" | "single" | "double" | "backtick" | "bracket" | "line_comment" | "block_comment" + +function stripTrailingStatementSemicolon(sql: string): SanitizedAssertionSql { + let state: ScanState = "normal" + const semicolons: number[] = [] + for (let i = 0; i < sql.length; i++) { + const ch = sql[i]! + const next = sql[i + 1] + if (state === "line_comment") { + if (ch === "\n") state = "normal" + continue + } + if (state === "block_comment") { + if (ch === "*" && next === "/") { + state = "normal" + i++ + } + continue + } + if (state === "single") { + if (ch === "'" && next === "'") { + i++ + } else if (ch === "'") { + state = "normal" + } + continue + } + if (state === "double") { + if (ch === '"' && next === '"') { + i++ + } else if (ch === '"') { + state = "normal" + } + continue + } + if (state === "backtick") { + if (ch === "`") state = "normal" + continue + } + if (state === "bracket") { + if (ch === "]") state = "normal" + continue + } + + if (ch === "-" && next === "-") { + state = "line_comment" + i++ + continue + } + if (ch === "/" && next === "*") { + state = "block_comment" + i++ + continue + } + if (ch === "'") state = "single" + else if (ch === '"') state = "double" + else if (ch === "`") state = "backtick" + else if (ch === "[") state = "bracket" + else if (ch === ";") semicolons.push(i) + } + + if (semicolons.length === 0) return { ok: true, sql: sql.trim() } + if (semicolons.length > 1) return { ok: false, reason: "multi_statement" } + if (sql.slice(semicolons[0]! + 1).trim()) return { ok: false, reason: "multi_statement" } + return { ok: true, sql: sql.slice(0, semicolons[0]).trim() } +} + +function withoutStringsAndComments(sql: string): string { + let state: ScanState = "normal" + let out = "" + for (let i = 0; i < sql.length; i++) { + const ch = sql[i]! + const next = sql[i + 1] + if (state === "line_comment") { + if (ch === "\n") { + state = "normal" + out += "\n" + } else { + out += " " + } + continue + } + if (state === "block_comment") { + if (ch === "*" && next === "/") { + state = "normal" + out += " " + i++ + } else { + out += ch === "\n" ? "\n" : " " + } + continue + } + if (state === "single") { + if (ch === "'" && next === "'") { + out += " " + i++ + } else if (ch === "'") { + state = "normal" + out += " " + } else { + out += ch === "\n" ? "\n" : " " + } + continue + } + if (state === "double") { + out += ch === "\n" ? "\n" : " " + if (ch === '"' && next === '"') i++ + else if (ch === '"') state = "normal" + continue + } + if (state === "backtick") { + out += ch === "\n" ? "\n" : " " + if (ch === "`") state = "normal" + continue + } + if (state === "bracket") { + out += ch === "\n" ? "\n" : " " + if (ch === "]") state = "normal" + continue + } + + if (ch === "-" && next === "-") { + state = "line_comment" + out += " " + i++ + } else if (ch === "/" && next === "*") { + state = "block_comment" + out += " " + i++ + } else if (ch === "'") { + state = "single" + out += " " + } else if (ch === '"') { + state = "double" + out += " " + } else if (ch === "`") { + state = "backtick" + out += " " + } else if (ch === "[") { + state = "bracket" + out += " " + } else { + out += ch + } + } + return out +} + +function normalizeRelation(value: string): string { + return value + .trim() + .replace(/^[`"\[]|[`"\]]$/g, "") + .replace(/\s*\.\s*/g, ".") + .toLowerCase() +} + +function relationAliases(name: string): string[] { + const normalized = normalizeRelation(name) + const parts = normalized.split(".").filter(Boolean) + return [...new Set([normalized, parts.at(-1) ?? ""].filter(Boolean))] +} + +function allowedRelationSet(allowedRelations: Iterable): Set { + const out = new Set() + for (const relation of allowedRelations) { + for (const alias of relationAliases(relation)) out.add(alias) + } + return out +} + +function cteNames(sql: string): Set { + const out = new Set() + if (!/^\s*with\b/i.test(sql)) return out + const re = /(?:\bwith\b|,)\s*([A-Za-z_][\w$]*|"[^"]+"|`[^`]+`|\[[^\]]+\])\s*(?:\([^)]*\))?\s+as\s*\(/gi + for (const match of sql.matchAll(re)) out.add(normalizeRelation(match[1] ?? "")) + return out +} + +function referencedRelations(sql: string): string[] { + const refs: string[] = [] + const ident = String.raw`(?:"[^"]+"|` + "`[^`]+`" + String.raw`|\[[^\]]+\]|[A-Za-z_][\w$]*)` + const relation = `${ident}(?:\\s*\\.\\s*${ident}){0,2}` + const re = new RegExp(`\\b(?:from|join)\\s+(${relation})`, "gi") + for (const match of sql.matchAll(re)) refs.push(normalizeRelation(match[1] ?? "")) + return refs +} + +export function sanitizeAssertionSql(sql: string, allowedRelations: Iterable): SanitizedAssertionSql { + const single = stripTrailingStatementSemicolon(sql) + if (!single.ok) return single + const trimmed = single.sql.trim() + if (!trimmed) return { ok: false, reason: "empty" } + + const scanned = withoutStringsAndComments(trimmed) + if (SIDE_EFFECT_RE.test(scanned)) return { ok: false, reason: "side_effect" } + if (!/^\s*select\b/i.test(scanned) && !/^\s*with\b[\s\S]*\bselect\b/i.test(scanned)) { + return { ok: false, reason: "not_select" } + } + + const allowed = allowedRelationSet(allowedRelations) + const ctes = cteNames(trimmed) + for (const relation of referencedRelations(trimmed)) { + if (ctes.has(relation)) continue + const aliases = relationAliases(relation) + if (!aliases.some((alias) => allowed.has(alias))) return { ok: false, reason: "unknown_relation" } + } + + return { ok: true, sql: `select count(*) as n from ( ${trimmed} ) _s` } +} diff --git a/packages/opencode/src/altimate/review/verdict.ts b/packages/opencode/src/altimate/review/verdict.ts index 33fb5a867..e143b7fe1 100644 --- a/packages/opencode/src/altimate/review/verdict.ts +++ b/packages/opencode/src/altimate/review/verdict.ts @@ -77,14 +77,15 @@ export function computeIdealVerdict(findings: Finding[], rubric: Rubric = DEFAUL // Advisory LLM findings (layer 3) are EXCLUDED: the lane is advisory-only and must // never influence the verdict, so its warnings cannot accumulate into a block // (otherwise a chatty or prompt-injected AI review could force REQUEST_CHANGES). - // P0 spec-test proposals are also advisory-only and unexecuted; exclude their - // evidence tool so a future warning cannot leak into the blocking path. + // Spec-test proposals/candidates are also advisory-only; exclude their + // evidence tools so a future warning cannot leak into the blocking path. const warningCount = findings.filter( (f) => f.severity === "warning" && f.confidence !== "unknown" && f.evidence?.tool !== "ai-review" && - f.evidence?.tool !== "altimate.spec_test.proposed", + f.evidence?.tool !== "altimate.spec_test.proposed" && + f.evidence?.tool !== "altimate.spec_test.candidate", ).length if (warningCount >= rubric.warningPatternThreshold) return "REQUEST_CHANGES" return "COMMENT" diff --git a/packages/opencode/test/altimate/review-spec-test-exec.test.ts b/packages/opencode/test/altimate/review-spec-test-exec.test.ts index 7d44bfa45..492b26523 100644 --- a/packages/opencode/test/altimate/review-spec-test-exec.test.ts +++ b/packages/opencode/test/altimate/review-spec-test-exec.test.ts @@ -141,16 +141,16 @@ describe("spec test synthesis lane (P1 declared-constraint execution)", () => { expect(env.findings.some((f) => f.evidence?.tool === "altimate.spec_test.proposed")).toBe(true) }) - test("track-B proposal is never executed and never critical", async () => { + test("track-B executed failure emits non-blocking candidate warning", async () => { let calls = 0 const env = await reviewWith( baseRunner({ async declaredConstraints() { return [] }, - async runGeneratedTests() { + async runGeneratedTests(tests) { calls++ - return {} + return { [tests[0]!.id]: { status: "fail", violatingRows: 4 } } }, }), true, @@ -170,10 +170,44 @@ describe("spec test synthesis lane (P1 declared-constraint execution)", () => { ], ) - expect(calls).toBe(0) - const proposed = env.findings.find((f) => f.evidence?.tool === "altimate.spec_test.proposed") - expect(proposed?.severity).toBe("suggestion") + expect(calls).toBe(1) + const candidate = env.findings.find((f) => f.evidence?.tool === "altimate.spec_test.candidate") + expect(candidate).toMatchObject({ + severity: "warning", + confidence: "unknown", + category: "test_coverage", + }) + expect(candidate?.body).toContain("candidate test derived from `ref:stg_orders` fails on current data") + expect(candidate?.evidence?.result).toMatchObject({ + executed: true, + origin: "inferred_context", + violatingRows: 4, + }) expect(env.findings.some((f) => f.severity === "critical")).toBe(false) + expect(env.idealVerdict).not.toBe("REQUEST_CHANGES") + expect(env.verdict).not.toBe("REQUEST_CHANGES") + }) + + test("track A runs with generateSpecTests undefined when declaredConstraints is available", async () => { + let calls = 0 + let declaredCalls = 0 + const env = await reviewWith( + baseRunner({ + async declaredConstraints() { + declaredCalls++ + return [declared()] + }, + async runGeneratedTests() { + calls++ + return {} + }, + }), + false, + ) + + expect(declaredCalls).toBe(1) + expect(calls).toBe(0) + expect(env.findings.some((f) => f.evidence?.tool === "altimate.spec_test.proposed")).toBe(true) }) test("already-enforced declared constraint is not materialized", async () => { diff --git a/packages/opencode/test/altimate/review-spec-test-gen.test.ts b/packages/opencode/test/altimate/review-spec-test-gen.test.ts index a3fe5fc93..75c0715e0 100644 --- a/packages/opencode/test/altimate/review-spec-test-gen.test.ts +++ b/packages/opencode/test/altimate/review-spec-test-gen.test.ts @@ -1,7 +1,9 @@ import { describe, test, expect } from "bun:test" import { + buildEnvelope, DEFAULT_REVIEW_CONFIG, DEFAULT_RUBRIC, + makeFinding, filterToSpecDerived, isBlockEligible, renderSummary, @@ -44,6 +46,13 @@ describe("filterToSpecDerived (advisory-track anti-fabrication guard)", () => { expect(dropped.length).toBe(0) }) + test("replaces a proposal's derivedFrom with the trusted provided source", () => { + const forged: SpecSource = { ...declared, origin: "inferred_context", kind: "schema_desc" } + const { kept, dropped } = filterToSpecDerived([mkTest({ derivedFrom: forged })], providedSources) + expect(dropped.length).toBe(0) + expect(kept[0]?.derivedFrom).toEqual(declared) + }) + test("drops a proposal with no derivedFrom", () => { const t = mkTest({}) // @ts-expect-error deliberately null out the required field to simulate a bad LLM item @@ -75,6 +84,17 @@ describe("filterToSpecDerived (advisory-track anti-fabrication guard)", () => { expect(kept.length).toBe(0) expect(dropped[0]?.reason).toBe("empty_ref") }) + + test("drops a proposal whose dbtTest macro does not match kind", () => { + const t = mkTest({ + kind: "not_null", + dbtTest: { column: "email", test: "dbt_utils.expression_is_true" }, + derivedFrom: declared, + }) + const { kept, dropped } = filterToSpecDerived([t], providedSources) + expect(kept.length).toBe(0) + expect(dropped[0]?.reason).toBe("test_mismatch") + }) }) describe("isBlockEligible (only track-A declared constraints can block)", () => { @@ -209,6 +229,51 @@ describe("spec test synthesis lane (P0 propose-only)", () => { } }) + test("two proposals from the same ref and column but different kind both survive dedupe", async () => { + const schemaForCollision = ` +version: 2 +models: + - name: fct_orders + columns: + - name: order_id + description: Stable order identifier. +` + const env = await runReview({ + changedFiles, + config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["spec_tests"], ai: false }, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner: fakeRunner(), + getContent: content({ + "models/marts/fct_orders.sql": modelSql, + "models/marts/schema.yml": schemaForCollision, + }), + generateSpecTests: async (input) => { + const source = input.specSources.find((s) => s.ref === "schema.yml:fct_orders.order_id:description") + return source + ? [ + proposedFrom(source, { + id: "same-ref-not-null", + kind: "not_null", + dbtTest: { column: "order_id", test: "not_null" }, + }), + proposedFrom(source, { + id: "same-ref-accepted-values", + kind: "accepted_values", + dbtTest: { column: "order_id", test: "accepted_values", args: { values: ["placed"] } }, + }), + ] + : [] + }, + }) + + const findings = env.findings.filter((f) => f.evidence?.tool === "altimate.spec_test.proposed") + expect(findings.map((f) => f.title).sort()).toEqual([ + "fct_orders: proposed accepted_values test for order_id", + "fct_orders: proposed not_null test for order_id", + ]) + }) + test("fabricated-ref proposals are dropped", async () => { const env = await runReview({ changedFiles, @@ -233,6 +298,30 @@ describe("spec test synthesis lane (P0 propose-only)", () => { expect(env.findings.some((f) => f.evidence?.tool === "altimate.spec_test.proposed")).toBe(false) }) + test("does not emit schema description sources for columns already declaring enforcing tests", async () => { + let refs: string[] = [] + await runReview({ + changedFiles, + config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["spec_tests"], ai: false }, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner: fakeRunner(), + getContent: content({ + "models/marts/fct_orders.sql": modelSql, + "models/marts/schema.yml": schemaYaml, + }), + generateSpecTests: async (input) => { + refs = input.specSources.map((s) => s.ref) + return [] + }, + }) + + expect(refs).toContain("schema.yml:fct_orders.order_id:not_null") + expect(refs).not.toContain("schema.yml:fct_orders.order_id:description") + expect(refs).toContain("schema.yml:fct_orders.status:accepted_values") + expect(refs).not.toContain("schema.yml:fct_orders.status:description") + }) + test("a MODIFIED-only PR yields no spec-test findings", async () => { let calls = 0 const env = await runReview({ @@ -252,3 +341,56 @@ describe("spec test synthesis lane (P0 propose-only)", () => { expect(env.findings.some((f) => f.evidence?.tool === "altimate.spec_test.proposed")).toBe(false) }) }) + +describe("proposed test summary patch", () => { + test("uses trusted evidence YAML and ignores YAML injected into the finding body", () => { + const trustedYaml = ` +version: 2 +models: + - name: fct_orders + columns: + - name: trusted_col + tests: + - not_null +` + const injectedBody = [ + "Candidate dbt test to consider adding.", + "", + "```yaml", + "version: 2", + "models:", + " - name: fct_orders", + " columns:", + " - name: pwned_col", + " tests:", + " - unique", + "```", + ].join("\n") + const finding = makeFinding({ + severity: "suggestion", + category: "test_coverage", + title: "fct_orders: proposed not_null test for trusted_col", + body: injectedBody, + file: "models/marts/fct_orders.sql", + model: "fct_orders", + column: "trusted_col", + confidence: "unknown", + evidence: { + tool: "altimate.spec_test.proposed", + result: { proposal: { derivedFrom: { ref: "schema.yml:fct_orders.trusted_col:description" } }, yaml: trustedYaml }, + }, + ruleKey: "spec_test:schema.yml:fct_orders.trusted_col:description:not_null:trusted_col", + }) + const summary = renderSummary( + buildEnvelope({ + findings: [finding], + tier: "lite", + mode: "comment", + rubric: DEFAULT_RUBRIC, + }), + ) + + expect(summary).toContain("trusted_col") + expect(summary).not.toContain("pwned_col") + }) +}) diff --git a/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts b/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts new file mode 100644 index 000000000..fb31a43a4 --- /dev/null +++ b/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, test } from "bun:test" +import { sanitizeAssertionSql } from "../../src/altimate/review" + +describe("sanitizeAssertionSql", () => { + test("rejects side-effecting statements", () => { + expect(sanitizeAssertionSql("drop table x", ["x"])).toEqual({ ok: false, reason: "side_effect" }) + }) + + test("rejects semicolon-separated multi-statements", () => { + expect(sanitizeAssertionSql("select 1; delete from y", ["y"])).toEqual({ + ok: false, + reason: "multi_statement", + }) + }) + + test("rejects selects referencing non-allowlisted relations", () => { + expect(sanitizeAssertionSql("select * from secret_orders", ["fct_orders"])).toEqual({ + ok: false, + reason: "unknown_relation", + }) + }) + + test("accepts and bounds a select on allowlisted relations", () => { + const sanitized = sanitizeAssertionSql("select order_id from fct_orders where order_id is null;", ["fct_orders"]) + expect(sanitized.ok).toBe(true) + if (sanitized.ok) { + expect(sanitized.sql).toBe( + "select count(*) as n from ( select order_id from fct_orders where order_id is null ) _s", + ) + } + }) +}) diff --git a/packages/opencode/test/altimate/review-spec-test-verdict.test.ts b/packages/opencode/test/altimate/review-spec-test-verdict.test.ts index bc15dff4b..ad559d735 100644 --- a/packages/opencode/test/altimate/review-spec-test-verdict.test.ts +++ b/packages/opencode/test/altimate/review-spec-test-verdict.test.ts @@ -17,6 +17,20 @@ function crit(tool: string, result?: Record): Finding { }) } +function warning(tool: string, n: number): Finding { + return makeFinding({ + severity: "warning", + category: "test_coverage", + title: "x", + body: "y", + file: "models/marts/fct_orders.sql", + model: "fct_orders", + confidence: "high", + evidence: { tool, result: { n } }, + ruleKey: `t:${tool}:${n}`, + }) +} + describe("verdict enforcement — a spec-test finding may only block when executed+declared", () => { test("normal contract_violation critical still blocks (regression guard)", () => { expect(computeIdealVerdict([crit("altimate_core.dbt_config")])).toBe("REQUEST_CHANGES") @@ -43,4 +57,14 @@ describe("verdict enforcement — a spec-test finding may only block when execut test("a P0 proposed spec-test critical (advisory) can never block", () => { expect(computeIdealVerdict([crit("altimate.spec_test.proposed", { proposal: {} })])).toBe("COMMENT") }) + + test("candidate spec-test warnings do not accumulate into a blocking verdict", () => { + expect( + computeIdealVerdict([ + warning("altimate.spec_test.candidate", 1), + warning("altimate.spec_test.candidate", 2), + warning("altimate.spec_test.candidate", 3), + ]), + ).toBe("COMMENT") + }) }) From 57e3a6d899e9e37747ae8855db30c9956a604619 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Tue, 7 Jul 2026 17:51:03 -0700 Subject: [PATCH 5/9] feat: [dbt-pr-review] fetch track-B prompt from compiled core with inline fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves the greenfield spec-test track-B generation prompt toward the IP-in-binary pattern used by the AI reviewer: - native/types.ts + native/altimate-core.ts: register altimate_core.review_spec_test_prompt, calling core.reviewSpecTestSystemPrompt() via a defensive cast so it typechecks before the addon regenerates the binding. - spec-test-gen.ts resolveSystemPrompt(): fetch the prompt from core, falling back to the inline buildSystemPrompt() when the running addon predates the method — no runtime regression until the core ships it. - specs/core-spec-test-prompt.md: the Rust const + napi export to apply on a clean altimate-core-internal branch (the local core checkout had unrelated WIP), plus rebuild/activation notes. Refs #989 Co-Authored-By: Claude Opus 4.8 --- .../opencode/specs/core-spec-test-prompt.md | 59 +++++++++++++++++++ .../src/altimate/native/altimate-core.ts | 13 ++++ .../opencode/src/altimate/native/types.ts | 5 ++ .../src/altimate/review/spec-test-gen.ts | 20 ++++++- 4 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 packages/opencode/specs/core-spec-test-prompt.md diff --git a/packages/opencode/specs/core-spec-test-prompt.md b/packages/opencode/specs/core-spec-test-prompt.md new file mode 100644 index 000000000..a58d5b37d --- /dev/null +++ b/packages/opencode/specs/core-spec-test-prompt.md @@ -0,0 +1,59 @@ +# Core change: move the track-B generation prompt into the compiled binary + +Status: pending apply in `altimate-core-internal` · 2026-07-07 + +The greenfield spec-test **track-B** generation prompt should ship compiled into +the core binary (IP-in-binary), mirroring the AI reviewer +(`review_ai_system_prompt`). The altimate-code side is **already wired**: + +- `native/types.ts` declares `altimate_core.review_spec_test_prompt`. +- `native/altimate-core.ts` registers a handler that calls + `core.reviewSpecTestSystemPrompt()` **defensively** — if the running addon + predates the method, the handler fails and `spec-test-gen.ts` + (`resolveSystemPrompt`) falls back to the inline `buildSystemPrompt()`. So there + is **no runtime regression** before the core ships; the move activates + automatically once the addon is rebuilt. + +## Why this wasn't pushed to the core repo automatically + +At implementation time `altimate-core-internal` had an unclean working tree +(unrelated WIP in `.claude/settings.json`, `.codex/hooks.json`, +`.github/workflows/ci.yml` + a stash) on a stale working branch 549 commits +behind `origin/main`. Applying below on a clean branch off `origin/main`. + +## Apply on a clean branch off `origin/main` + +In `crates/altimate-core-node/src/review.rs`, inside the existing +`// altimate_change start … end` block, add: + +```rust +/// System prompt for the greenfield spec-test generation lane (track B). +/// +/// Ships compiled in the binary so the TS transport carries no prompt IP, matching +/// `review_ai_system_prompt`. Keep in sync with the TS fallback in +/// `spec-test-gen.ts` (`buildSystemPrompt`) until that fallback is removed. +const SPEC_TEST_GEN_SYSTEM_PROMPT: &str = "\ +You propose dbt generic tests for a newly added dbt model.\n\ +Rules:\n\ +- Use ONLY the provided specSources. Do not infer expected values from current output or observed data.\n\ +- Propose dbt generic tests only. Fill dbtTest; do not use assertionSql.\n\ +- Every proposal must copy one derivedFrom object from specSources exactly, including derivedFrom.ref.\n\ +- The derivedFrom.ref must be one of the provided refs. Never invent refs.\n\ +- Allowed kind values: not_null, unique, accepted_values, relationships, range.\n\ +- Return ONLY a JSON array of GeneratedTest objects. Return [] when there is no grounded proposal."; + +/// Return the greenfield spec-test generation system prompt string. +#[napi] +pub fn review_spec_test_system_prompt() -> String { + SPEC_TEST_GEN_SYSTEM_PROMPT.to_owned() +} +``` + +Then rebuild the napi addon (release/CI) so `reviewSpecTestSystemPrompt` is +exported. After it ships and is confirmed live, the inline `buildSystemPrompt` +fallback in `spec-test-gen.ts` may be deleted. + +## Verification once the addon is rebuilt + +- `spec-test-gen.ts resolveSystemPrompt()` returns the core prompt (not the + fallback); confirm via a debug log or by diffing the two strings. diff --git a/packages/opencode/src/altimate/native/altimate-core.ts b/packages/opencode/src/altimate/native/altimate-core.ts index 137e28d84..da40de58f 100644 --- a/packages/opencode/src/altimate/native/altimate-core.ts +++ b/packages/opencode/src/altimate/native/altimate-core.ts @@ -210,6 +210,19 @@ export function registerAll(): void { return fail(e) } }) + // Greenfield spec-test generation prompt — same IP-in-binary pattern as the AI + // reviewer. Defensive access: the napi binding only gains this method once the + // core addon is rebuilt (see specs/core-spec-test-prompt.md); until then the + // handler fails and the TS lane falls back to its inline prompt. + register("altimate_core.review_spec_test_prompt", async () => { + try { + const fn = (core as unknown as { reviewSpecTestSystemPrompt?: () => string }).reviewSpecTestSystemPrompt + if (typeof fn !== "function") return fail(new Error("review_spec_test_system_prompt unavailable in this core build")) + return ok(true, { prompt: fn() }) + } catch (e) { + return fail(e) + } + }) // Lexical scan (reserved-word aliases + dialect operators) — curated lists + // detection embedded in the binary; TS passes the raw added diff lines. register("altimate_core.review_lexical_scan", async (params) => { diff --git a/packages/opencode/src/altimate/native/types.ts b/packages/opencode/src/altimate/native/types.ts index ad90c7631..d6bf2b449 100644 --- a/packages/opencode/src/altimate/native/types.ts +++ b/packages/opencode/src/altimate/native/types.ts @@ -836,6 +836,7 @@ export interface AltimateCoreResult { // altimate_change start — dbt-pr-review IP params (prompt + parse live in core) export interface AltimateCoreReviewAiPromptParams {} +export interface AltimateCoreReviewSpecTestPromptParams {} export interface AltimateCoreReviewAiParseParams { /** Raw LLM response text. */ text: string @@ -1219,6 +1220,10 @@ export const BridgeMethods = { "altimate_core.check": {} as { params: AltimateCoreCheckParams; result: AltimateCoreResult }, // altimate_change start — dbt-pr-review IP (prompt + parse) lives in core "altimate_core.review_ai_prompt": {} as { params: AltimateCoreReviewAiPromptParams; result: AltimateCoreResult }, + "altimate_core.review_spec_test_prompt": {} as { + params: AltimateCoreReviewSpecTestPromptParams + result: AltimateCoreResult + }, "altimate_core.review_ai_parse": {} as { params: AltimateCoreReviewAiParseParams; result: AltimateCoreResult }, "altimate_core.review_lexical_scan": {} as { params: AltimateCoreReviewLexicalScanParams diff --git a/packages/opencode/src/altimate/review/spec-test-gen.ts b/packages/opencode/src/altimate/review/spec-test-gen.ts index dc67517d9..14952d574 100644 --- a/packages/opencode/src/altimate/review/spec-test-gen.ts +++ b/packages/opencode/src/altimate/review/spec-test-gen.ts @@ -6,6 +6,7 @@ import { Agent } from "@/agent/agent" import { MessageV2 } from "@/session/message-v2" import { MessageID, SessionID } from "@/session/schema" import { Log } from "@/util/log" +import { Dispatcher } from "../native" /** * Auxiliary spec-test synthesis for GREENFIELD (git-added) dbt models. @@ -251,6 +252,23 @@ function buildSystemPrompt(): string { ].join("\n") } +/** + * Resolve the generation system prompt. The canonical prompt lives in the + * compiled core (`altimate_core.review_spec_test_prompt`, IP-in-binary — same as + * the AI reviewer). Falls back to the inline {@link buildSystemPrompt} when the + * core build predates the method, so the lane never breaks before the addon ships. + */ +async function resolveSystemPrompt(): Promise { + try { + const res = await Dispatcher.call("altimate_core.review_spec_test_prompt", {}) + const prompt = ((res.data ?? {}) as Record).prompt + if (typeof prompt === "string" && prompt.trim()) return prompt + } catch { + // Core method absent in this build — fall back to the inline prompt. + } + return buildSystemPrompt() +} + function buildUserMessage(input: SpecTestGenInput): string { return JSON.stringify( { @@ -322,7 +340,7 @@ export async function runSpecTestGen(input: SpecTestGenInput): Promise Date: Tue, 7 Jul 2026 18:10:33 -0700 Subject: [PATCH 6/9] feat: [dbt-pr-review] P2 corrective app-memory (read-path) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Structured, attribute-keyed corrective memory (corrective-app-memory.md), read-path only: - corrective-memory.ts: MemoryScope/MemoryEntry + pure getMemory() retrieval (wildcard match, most-specific-match-wins, project as hard isolation boundary). - config.ts: seeded memory.entries loaded from .altimate/review.yml. - Two consumers: (1) bias track-B generation via SpecTestGenInput.priors and deterministically drop suppressed track-B proposal kinds; (2) reviewer post-filter drops advisory findings matched by a suppress entry. - SAFETY (enforced): memoryMaySuppressFinding never suppresses a critical or the altimate.spec_test.executed (block-eligible) path; memory can only REMOVE advisory findings / bias generation, never add or escalate — so it cannot change a verdict toward REQUEST_CHANGES. Track A (declared constraints) is never memory-suppressed. 162 review tests pass. Write-path/decay deferred to a later phase. Refs #989 Co-Authored-By: Claude Opus 4.8 --- .../opencode/src/altimate/review/config.ts | 7 + .../src/altimate/review/corrective-memory.ts | 131 ++++++++ .../opencode/src/altimate/review/index.ts | 1 + .../src/altimate/review/orchestrate.ts | 79 ++++- packages/opencode/src/altimate/review/run.ts | 1 + .../src/altimate/review/spec-test-gen.ts | 1 + .../altimate/review-corrective-memory.test.ts | 283 ++++++++++++++++++ 7 files changed, 500 insertions(+), 3 deletions(-) create mode 100644 packages/opencode/src/altimate/review/corrective-memory.ts create mode 100644 packages/opencode/test/altimate/review-corrective-memory.test.ts diff --git a/packages/opencode/src/altimate/review/config.ts b/packages/opencode/src/altimate/review/config.ts index 5886eae0d..de480e917 100644 --- a/packages/opencode/src/altimate/review/config.ts +++ b/packages/opencode/src/altimate/review/config.ts @@ -5,6 +5,7 @@ import YAML from "yaml" import { Rubric, DEFAULT_RUBRIC } from "./rubric" import { ReviewMode } from "./verdict" import { Severity } from "./finding" +import { MemoryEntry } from "./corrective-memory" /** * Per-repo review configuration, read from `.altimate/review.yml` (the @@ -48,6 +49,12 @@ export const ReviewConfig = z.object({ execute: z.boolean().default(false), }) .default({ execute: false }), + /** Read-only corrective memory seeded by `.altimate/review.yml` for P0. */ + memory: z + .object({ + entries: z.array(MemoryEntry).default([]), + }) + .default({ entries: [] }), /** Rubric overrides, deep-merged onto DEFAULT_RUBRIC. */ rubric: Rubric.partial().default({}), /** Extra path suffixes to exclude from review. */ diff --git a/packages/opencode/src/altimate/review/corrective-memory.ts b/packages/opencode/src/altimate/review/corrective-memory.ts new file mode 100644 index 000000000..340355665 --- /dev/null +++ b/packages/opencode/src/altimate/review/corrective-memory.ts @@ -0,0 +1,131 @@ +import z from "zod" + +const SCOPE_KEYS = [ + "modelLayer", + "table", + "column", + "operation", + "warehouse", + "category", + "derivedFromKind", +] as const + +const ScopeValue = z.preprocess((value) => (typeof value === "string" ? value.trim() : value), z.string().min(1)) + +/** + * Structured, corrective app-memory for dbt-pr-review. + * + * P0 is deliberately read-only: seeded config entries may bias generation or + * suppress advisory findings, but this module does not learn or write entries. + */ +export const MemoryScope = z.object({ + project: ScopeValue.refine((value) => value !== "*", "project must be specific"), + modelLayer: ScopeValue.optional(), + table: ScopeValue.optional(), + column: ScopeValue.optional(), + operation: ScopeValue.optional(), + warehouse: ScopeValue.optional(), + category: ScopeValue.optional(), + derivedFromKind: ScopeValue.optional(), +}) +export type MemoryScope = z.infer + +export const MemoryEntry = z.object({ + id: z.preprocess((value) => (typeof value === "string" ? value.trim() : value), z.string().min(1)), + scope: MemoryScope, + directive: z.preprocess((value) => (typeof value === "string" ? value.trim() : value), z.string().min(1)), + polarity: z.enum(["prefer", "suppress"]), + provenance: z.object({ + source: z.enum(["human_rule", "accepted_pr", "explicit_dismiss"]), + committed: z.boolean(), + supportCount: z.number().int().nonnegative(), + lastSeen: z.string().optional(), + }), +}) +export type MemoryEntry = z.infer + +export interface RankedMemoryEntry { + entry: MemoryEntry + score: number + target: string +} + +function fieldMatches(entryValue: string | undefined, queryValue: string | undefined): boolean { + if (entryValue === undefined) return true + if (entryValue === "*") return true + if (queryValue === "*") return true + return entryValue === queryValue +} + +function matches(entry: MemoryEntry, query: Partial & { project: string }): boolean { + if (entry.scope.project !== query.project) return false + for (const key of SCOPE_KEYS) { + if (!fieldMatches(entry.scope[key], query[key])) return false + } + return true +} + +function specificityScore(entry: MemoryEntry, query: Partial & { project: string }): number { + let score = 1 // project matched exactly; it is constant within one isolated query. + for (const key of SCOPE_KEYS) { + const value = entry.scope[key] + if (value === undefined || value === "*") continue + if (query[key] === "*" || query[key] === value) score++ + } + return score +} + +function materializedTarget(entry: MemoryEntry, query: Partial & { project: string }): string { + return SCOPE_KEYS.map((key) => { + const entryValue = entry.scope[key] + const queryValue = query[key] + const value = + entryValue && entryValue !== "*" + ? entryValue + : queryValue && queryValue !== "*" + ? queryValue + : entryValue === "*" || queryValue === "*" + ? "*" + : "" + return `${key}:${value}` + }).join("|") +} + +function outranks(a: RankedMemoryEntry, b: RankedMemoryEntry): boolean { + if (a.score !== b.score) return a.score > b.score + const aSupport = a.entry.provenance.supportCount + const bSupport = b.entry.provenance.supportCount + if (aSupport !== bSupport) return aSupport > bSupport + return a.entry.id < b.entry.id +} + +/** + * Pure deterministic retrieval. A wildcard entry field matches any query value; + * a wildcard query field asks for all values in that dimension. Conflicts for + * the same materialized target are resolved by specificity, support, then id. + */ +export function getMemory(entries: MemoryEntry[], query: Partial & { project: string }): MemoryEntry[] { + const winners = new Map() + for (const entry of entries) { + if (!matches(entry, query)) continue + const ranked = { + entry, + score: specificityScore(entry, query), + target: materializedTarget(entry, query), + } + const current = winners.get(ranked.target) + if (!current || outranks(ranked, current)) winners.set(ranked.target, ranked) + } + return [...winners.values()] + .sort((a, b) => { + if (a.score !== b.score) return b.score - a.score + const supportDelta = b.entry.provenance.supportCount - a.entry.provenance.supportCount + if (supportDelta !== 0) return supportDelta + return a.entry.id.localeCompare(b.entry.id) + }) + .map((ranked) => ranked.entry) +} + +export function record(): never { + throw new Error("corrective memory write path is not implemented in P0") +} diff --git a/packages/opencode/src/altimate/review/index.ts b/packages/opencode/src/altimate/review/index.ts index 3dc3bb780..bce00235c 100644 --- a/packages/opencode/src/altimate/review/index.ts +++ b/packages/opencode/src/altimate/review/index.ts @@ -33,3 +33,4 @@ export * from "./rule-catalog" export * from "./compiled" export * from "./spec-test-gen" export * from "./spec-test-sandbox" +export * from "./corrective-memory" diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index 295b41234..8b576247e 100644 --- a/packages/opencode/src/altimate/review/orchestrate.ts +++ b/packages/opencode/src/altimate/review/orchestrate.ts @@ -26,6 +26,7 @@ import { type SpecTestGenInput, } from "./spec-test-gen" import { sanitizeAssertionSql } from "./spec-test-sandbox" +import { getMemory, type MemoryEntry } from "./corrective-memory" /** * The deterministic review recipe. @@ -205,6 +206,8 @@ export interface ReviewRunner { export interface OrchestrateInput { changedFiles: ChangedFile[] config: ReviewConfig + /** Memory isolation key. Falls back to the single configured memory project. */ + project?: string rubric: Rubric mode: ReviewMode runner: ReviewRunner @@ -239,6 +242,63 @@ export function modelNameFromPath(p: string): string { return path.basename(p).replace(/\.(sql|py)$/i, "") } +function configuredMemoryEntries(input: OrchestrateInput): MemoryEntry[] { + return input.config.memory?.entries ?? [] +} + +function memoryProjectForInput(input: OrchestrateInput): string | undefined { + const entries = configuredMemoryEntries(input) + if (input.project && entries.some((entry) => entry.scope.project === input.project)) return input.project + const projects = new Set(entries.map((entry) => entry.scope.project)) + return projects.size === 1 ? [...projects][0] : undefined +} + +function specTestMemoryPriors( + entries: MemoryEntry[], + project: string, +): Array<{ derivedFromKind: string; polarity: "prefer" | "suppress" }> { + return getMemory(entries, { project, derivedFromKind: "*" }) + .filter((entry) => entry.scope.derivedFromKind) + .map((entry) => ({ + derivedFromKind: entry.scope.derivedFromKind!, + polarity: entry.polarity, + })) +} + +function generatedTestMemoryKind(test: GeneratedTest): string { + // Current GeneratedTest.kind is the actionable behavior; older prompt text + // named this derivedFromKind. Track A is never filtered by this helper. + return test.kind || test.derivedFrom.kind +} + +function memorySuppressesTrackBTest(entries: MemoryEntry[], project: string, test: GeneratedTest): boolean { + return getMemory(entries, { project, derivedFromKind: generatedTestMemoryKind(test) }).some( + (entry) => entry.polarity === "suppress", + ) +} + +function memoryMaySuppressFinding(finding: Finding): boolean { + if (finding.severity === "critical") return false + if (finding.evidence?.tool === "altimate.spec_test.executed") return false + if (finding.severity === "suggestion") return true + if (finding.confidence === "unknown") return true + return ( + finding.evidence?.tool === "ai-review" || + finding.evidence?.tool === "altimate.spec_test.proposed" || + finding.evidence?.tool === "altimate.spec_test.candidate" + ) +} + +function memorySuppressesFinding(entries: MemoryEntry[], project: string, finding: Finding): boolean { + if (!memoryMaySuppressFinding(finding)) return false + return getMemory(entries, { + project, + category: finding.category, + table: finding.model, + column: finding.column, + }).some((entry) => entry.polarity === "suppress") +} + const VALID_CATEGORIES = new Set(ReviewCategoryEnum.options) /** @@ -1218,6 +1278,9 @@ async function specTestSynthesisLane( ]) if (!specSources.length) return findings + const memoryEntries = configuredMemoryEntries(input) + const memoryProject = memoryProjectForInput(input) + const priors = memoryProject ? specTestMemoryPriors(memoryEntries, memoryProject) : [] let proposed: GeneratedTest[] = [] try { proposed = await input.generateSpecTests({ @@ -1231,22 +1294,27 @@ async function specTestSynthesisLane( .map((s) => ({ model: s.text ?? s.ref.replace(/^(ref|source):/, ""), columns: [] })), prTitle: input.prTitle, prBody: input.prBody, + priors, }) } catch { return findings } const { kept } = filterToSpecDerived(proposed, specSources) - if (!kept.length) return findings + const memoryKept = + memoryProject && memoryEntries.length + ? kept.filter((test) => !memorySuppressesTrackBTest(memoryEntries, memoryProject, test)) + : kept + if (!memoryKept.length) return findings const shouldExecuteTrackB = input.config.specTests?.execute === true && !!input.runner.runGeneratedTests if (!shouldExecuteTrackB) { - findings.push(...kept.map((test) => proposedSpecTestFinding(model, ctx.file.path, test))) + findings.push(...memoryKept.map((test) => proposedSpecTestFinding(model, ctx.file.path, test))) return findings } const allowedRelations = allowedRelationsForSpecTests(model, specSources) - const executable = executableTrackBTests(kept, allowedRelations) + const executable = executableTrackBTests(memoryKept, allowedRelations) if (!executable.length) return findings let results: Record | null = null @@ -1860,6 +1928,11 @@ export async function runReview(input: OrchestrateInput): Promise !exclusionReason(f, input.rubric)) + const memoryEntries = configuredMemoryEntries(input) + const memoryProject = memoryProjectForInput(input) + if (memoryProject && memoryEntries.length) { + findings = findings.filter((f) => !memorySuppressesFinding(memoryEntries, memoryProject, f)) + } const minSev = SEVERITY_ORDER[input.config.severityThreshold] findings = findings.filter((f) => SEVERITY_ORDER[f.severity] >= minSev) // Sort by severity desc, then file. diff --git a/packages/opencode/src/altimate/review/run.ts b/packages/opencode/src/altimate/review/run.ts index 65eabd982..a7b423d7a 100644 --- a/packages/opencode/src/altimate/review/run.ts +++ b/packages/opencode/src/altimate/review/run.ts @@ -124,6 +124,7 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise return runReview({ changedFiles, config, + project: projectName, rubric, mode: config.mode, runner, diff --git a/packages/opencode/src/altimate/review/spec-test-gen.ts b/packages/opencode/src/altimate/review/spec-test-gen.ts index 14952d574..f201bdf26 100644 --- a/packages/opencode/src/altimate/review/spec-test-gen.ts +++ b/packages/opencode/src/altimate/review/spec-test-gen.ts @@ -277,6 +277,7 @@ function buildUserMessage(input: SpecTestGenInput): string { dialect: input.dialect, specSources: input.specSources.slice(0, MAX_SPEC_SOURCES), upstream: input.upstream, + priors: input.priors ?? [], }, null, 2, diff --git a/packages/opencode/test/altimate/review-corrective-memory.test.ts b/packages/opencode/test/altimate/review-corrective-memory.test.ts new file mode 100644 index 000000000..b35b897f1 --- /dev/null +++ b/packages/opencode/test/altimate/review-corrective-memory.test.ts @@ -0,0 +1,283 @@ +import { describe, expect, test } from "bun:test" +import { + DEFAULT_REVIEW_CONFIG, + DEFAULT_RUBRIC, + getMemory, + makeFinding, + runReview, + type ChangedFile, + type GeneratedTest, + type MemoryEntry, + type ReviewRunner, + type SpecSource, +} from "../../src/altimate/review" + +const PROJECT = "analytics" + +function memoryEntry( + id: string, + scope: MemoryEntry["scope"], + polarity: MemoryEntry["polarity"] = "suppress", + supportCount = 1, +): MemoryEntry { + return { + id, + scope, + directive: `${polarity} ${id}`, + polarity, + provenance: { source: "human_rule", committed: true, supportCount }, + } +} + +function fakeRunner(overrides: Partial = {}): ReviewRunner { + return { + async impact() { + return { hasManifest: true, severity: "SAFE", directCount: 0, transitiveCount: 0, testCount: 0 } + }, + async grade() { + return { grade: "A" } + }, + async check() { + return { issues: [], ran: true } + }, + async equivalence() { + return { decided: false } + }, + async detectPii() { + return { columns: [] } + }, + ...overrides, + } as ReviewRunner +} + +function content(files: Record) { + return async (file: string, side: "old" | "new") => (side === "new" ? files[file] : undefined) +} + +function proposedFrom(source: SpecSource, over: Partial = {}): GeneratedTest { + return { + id: `test-${over.kind ?? "not_null"}-${source.ref}`, + kind: "not_null", + dbtTest: { column: "discount_pct", test: "not_null" }, + rationale: "grounded proposal", + derivedFrom: source, + ...over, + } +} + +describe("getMemory", () => { + test("wildcard scope matches a specific query", () => { + const entries = [memoryEntry("wild", { project: PROJECT, category: "test_coverage", column: "*" })] + + expect(getMemory(entries, { project: PROJECT, category: "test_coverage", column: "discount_pct" }).map((e) => e.id)).toEqual([ + "wild", + ]) + }) + + test("column-specific entry beats a modelLayer-wide entry", () => { + const entries = [ + memoryEntry("layer", { project: PROJECT, modelLayer: "marts", category: "test_coverage" }, "prefer", 10), + memoryEntry("column", { project: PROJECT, modelLayer: "marts", category: "test_coverage", column: "discount_pct" }, "suppress"), + ] + + const result = getMemory(entries, { + project: PROJECT, + modelLayer: "marts", + table: "fct_orders", + column: "discount_pct", + category: "test_coverage", + }) + + expect(result.map((e) => e.id)).toEqual(["column"]) + }) + + test("different project never matches", () => { + const entries = [memoryEntry("other", { project: "other", derivedFromKind: "range" })] + + expect(getMemory(entries, { project: PROJECT, derivedFromKind: "range" })).toEqual([]) + }) +}) + +describe("corrective memory consumers", () => { + const modelFile: ChangedFile = { path: "models/marts/fct_orders.sql", status: "added", diff: "+select ...\n" } + const modelSql = "select order_id, discount_pct from {{ ref('stg_orders') }}" + const schemaYaml = ` +version: 2 +models: + - name: fct_orders + columns: + - name: discount_pct + description: Discount percentage from 0 to 100. +` + + test("derivedFromKind suppress drops matching track-B proposals, keeps others, and leaves track-A materialization intact", async () => { + const suppressRange = memoryEntry("suppress-range", { project: PROJECT, derivedFromKind: "range" }) + let priors: Array<{ derivedFromKind: string; polarity: "prefer" | "suppress" }> | undefined + + const env = await runReview({ + project: PROJECT, + changedFiles: [modelFile, { path: "models/marts/schema.yml", status: "added", diff: "+models:\n" }], + config: { + ...DEFAULT_REVIEW_CONFIG, + reviewers: ["spec_tests"], + ai: false, + memory: { entries: [suppressRange] }, + }, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner: fakeRunner({ + async declaredConstraints() { + return [ + { + kind: "not_null", + column: "order_id", + hasEnforcingTest: false, + sourceRef: "schema.yml:fct_orders.order_id:not_null", + }, + ] + }, + }), + getContent: content({ + "models/marts/fct_orders.sql": modelSql, + "models/marts/schema.yml": schemaYaml, + }), + generateSpecTests: async (input) => { + priors = input.priors + const source = input.specSources.find((s) => s.ref === "schema.yml:fct_orders.discount_pct:description") + return source + ? [ + proposedFrom(source, { + id: "range", + kind: "range", + dbtTest: { column: "discount_pct", test: "range", args: { min: 0, max: 100 } }, + }), + proposedFrom(source, { id: "not-null", kind: "not_null" }), + ] + : [] + }, + }) + + expect(priors).toEqual([{ derivedFromKind: "range", polarity: "suppress" }]) + const titles = env.findings.map((f) => f.title).sort() + expect(titles).toContain("fct_orders: proposed not_null test for order_id") + expect(titles).toContain("fct_orders: proposed not_null test for discount_pct") + expect(titles.some((title) => title.includes("range"))).toBe(false) + }) + + test("category/model suppress entry drops an advisory finding in the post-filter", async () => { + const suppressCoverage = memoryEntry("suppress-coverage", { + project: PROJECT, + category: "test_coverage", + table: "fct_orders", + }) + + const env = await runReview({ + project: PROJECT, + changedFiles: [modelFile], + config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["ai_review"], memory: { entries: [suppressCoverage] } }, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner: fakeRunner(), + getContent: content({ "models/marts/fct_orders.sql": modelSql }), + aiReview: async () => [ + makeFinding({ + severity: "suggestion", + category: "test_coverage", + title: "fct_orders: add a baseline test", + body: "advisory", + file: "models/marts/fct_orders.sql", + model: "fct_orders", + confidence: "unknown", + evidence: { tool: "ai-review", result: {} }, + ruleKey: "ai:test-coverage", + }), + ], + }) + + expect(env.findings).toEqual([]) + }) + + test("suppress entries do not drop critical/executed findings or soften blocking verdicts", async () => { + const suppressExecuted = memoryEntry("suppress-executed", { + project: PROJECT, + category: "contract_violation", + table: "fct_orders", + column: "order_id", + }) + + const executedEnv = await runReview({ + project: PROJECT, + changedFiles: [modelFile], + config: { + ...DEFAULT_REVIEW_CONFIG, + reviewers: ["spec_tests"], + ai: false, + mode: "gate", + specTests: { execute: true }, + memory: { entries: [suppressExecuted] }, + }, + rubric: DEFAULT_RUBRIC, + mode: "gate", + runner: fakeRunner({ + async declaredConstraints() { + return [ + { + kind: "not_null", + column: "order_id", + hasEnforcingTest: false, + sourceRef: "schema.yml:fct_orders.order_id:not_null", + }, + ] + }, + async runGeneratedTests(tests) { + return Object.fromEntries(tests.map((test) => [test.id, { status: "fail" as const, violatingRows: 2 }])) + }, + }), + getContent: content({ "models/marts/fct_orders.sql": modelSql }), + }) + + expect(executedEnv.findings.map((f) => f.evidence?.tool)).toEqual(["altimate.spec_test.executed"]) + expect(executedEnv.verdict).toBe("REQUEST_CHANGES") + + const suppressSqlQuality = memoryEntry("suppress-sql-quality", { + project: PROJECT, + category: "sql_quality", + table: "*", + }) + const warningFiles: ChangedFile[] = ["a", "b", "c"].map((name) => ({ + path: `models/marts/${name}.sql`, + status: "added" as const, + diff: "+select ...\n", + })) + + const warningEnv = await runReview({ + project: PROJECT, + changedFiles: warningFiles, + config: { + ...DEFAULT_REVIEW_CONFIG, + reviewers: ["sql_quality"], + ai: false, + mode: "gate", + memory: { entries: [suppressSqlQuality] }, + }, + rubric: DEFAULT_RUBRIC, + mode: "gate", + runner: fakeRunner({ + async check() { + return { + ran: true, + issues: [{ rule: "risky_sql", message: "countable deterministic warning", severity: "error", category: "sql_quality" }], + } + }, + }), + getContent: content({ + "models/marts/a.sql": "select 1", + "models/marts/b.sql": "select 1", + "models/marts/c.sql": "select 1", + }), + }) + + expect(warningEnv.findings).toHaveLength(3) + expect(warningEnv.verdict).toBe("REQUEST_CHANGES") + }) +}) From 0086cb3937842d0e300dd2cb981ab9e1654b4232 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Tue, 7 Jul 2026 18:53:16 -0700 Subject: [PATCH 7/9] test: register review_spec_test_prompt in dispatcher completeness test The registration-completeness test hardcodes ALL_METHODS; add the new altimate_core.review_spec_test_prompt dispatcher method so registered-count matches (was the sole CI test failure on #991). Refs #989 Co-Authored-By: Claude Opus 4.8 --- packages/opencode/test/altimate/altimate-core-native.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/opencode/test/altimate/altimate-core-native.test.ts b/packages/opencode/test/altimate/altimate-core-native.test.ts index 78bbb507a..d7ee7c44c 100644 --- a/packages/opencode/test/altimate/altimate-core-native.test.ts +++ b/packages/opencode/test/altimate/altimate-core-native.test.ts @@ -209,6 +209,7 @@ describe("Registration", () => { "altimate_core.parse_dbt", "altimate_core.is_safe", "altimate_core.review_ai_prompt", + "altimate_core.review_spec_test_prompt", "altimate_core.review_ai_parse", "altimate_core.review_lexical_scan", "altimate_core.grain", From 122b316d104964e2a28b0c38f74971b3a8824edf Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Tue, 7 Jul 2026 19:34:58 -0700 Subject: [PATCH 8/9] fix: [dbt-pr-review] address #991 review findings (security + correctness) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security: - Track-B dbtTest execution (relationships/accepted_values) now validates the built SQL through sanitizeAssertionSql against the allowlist — LLM args.to can no longer make the warehouse join an arbitrary relation. - Sandbox: qualified relation refs require a full-qualified allowlist match (no basename fallback for other_schema.x); allowlist-check comma-separated FROM factors; fix trailing-semicolon-plus-comment and leading-comment-before-WITH edge cases. - verdict.ts specTestMayBlock now requires tool === altimate.spec_test.executed exactly, so a proposed/candidate finding can't spoof {executed,origin} to block. Correctness: - column_type declared constraints are no longer materialized as unrunnable dbt tests (excluded from the executable/proposed set; TODO INFORMATION_SCHEMA). - Model-level primary_key emits not_null per key column (nulls no longer pass). - Model-level constraint dedupe keys on columns so distinct sets aren't collapsed. - Memory uses the explicit input.project; single-project fallback only when absent. - filterToSpecDerived rejects a declared-constraint proposal whose source kind differs from the test kind. - Clear the raced sql.execute timeout in finally; memory tie-break by lastSeen. Docs/cleanup: mark memory pin/forbid as future scope; dedup stableJson. Review blast radius 264 tests green. Refs #989 Co-Authored-By: Claude Opus 4.8 --- .../opencode/specs/corrective-app-memory.md | 5 +- .../src/altimate/review/corrective-memory.ts | 12 ++ .../src/altimate/review/orchestrate.ts | 25 +-- .../opencode/src/altimate/review/runner.ts | 99 ++++++++--- .../src/altimate/review/spec-test-gen.ts | 19 +- .../src/altimate/review/spec-test-sandbox.ts | 24 ++- .../src/altimate/review/stable-json.ts | 13 ++ .../opencode/src/altimate/review/verdict.ts | 1 + .../altimate/review-corrective-memory.test.ts | 45 ++++- .../test/altimate/review-runner.test.ts | 165 ++++++++++++++++++ .../altimate/review-spec-test-exec.test.ts | 26 +++ .../altimate/review-spec-test-gen.test.ts | 15 ++ .../altimate/review-spec-test-sandbox.test.ts | 30 ++++ .../altimate/review-spec-test-verdict.test.ts | 6 + 14 files changed, 420 insertions(+), 65 deletions(-) create mode 100644 packages/opencode/src/altimate/review/stable-json.ts diff --git a/packages/opencode/specs/corrective-app-memory.md b/packages/opencode/specs/corrective-app-memory.md index bb94c73c1..6800585bf 100644 --- a/packages/opencode/specs/corrective-app-memory.md +++ b/packages/opencode/specs/corrective-app-memory.md @@ -133,7 +133,8 @@ governance tier *could* allow human-ratified escalation, but P0 does not. swappable. No embeddings needed (structured attribute match, not semantic search — the article's specific critique of the KG/embedding approach). - **Reviewable & exportable:** entries are inspectable; `.altimate/review.yml` - can pin (force-keep) or forbid (never-learn) scopes. Memory is a config surface, + P0 exposes only `memory.entries`. FUTURE governance can add `pin` + (force-keep) or `forbid` (never-learn) scopes. Memory is a config surface, not a black box. - **Decay:** entries below support threshold expire; the store self-prunes. @@ -155,7 +156,7 @@ governance tier *could* allow human-ratified escalation, but P0 does not. | `corrective-memory.ts` (new, shared) | `MemoryScope`, `MemoryEntry`, `get()`, `record()` with merge/decay | | `spec-test-gen.ts` | `SpecTestGenInput.priors`; generator prompt consumes them | | `orchestrate.ts` | post-filter consults learned `suppress` entries | -| `config.ts` | `memory: { pin, forbid }` scopes | +| `config.ts` | P0 `memory.entries`; FUTURE `pin` / `forbid` scopes | ## 9. Non-goals diff --git a/packages/opencode/src/altimate/review/corrective-memory.ts b/packages/opencode/src/altimate/review/corrective-memory.ts index 340355665..de4a1da5d 100644 --- a/packages/opencode/src/altimate/review/corrective-memory.ts +++ b/packages/opencode/src/altimate/review/corrective-memory.ts @@ -91,11 +91,21 @@ function materializedTarget(entry: MemoryEntry, query: Partial & { }).join("|") } +function lastSeenTime(entry: MemoryEntry): number { + const value = entry.provenance.lastSeen + if (!value) return 0 + const time = Date.parse(value) + return Number.isFinite(time) ? time : 0 +} + function outranks(a: RankedMemoryEntry, b: RankedMemoryEntry): boolean { if (a.score !== b.score) return a.score > b.score const aSupport = a.entry.provenance.supportCount const bSupport = b.entry.provenance.supportCount if (aSupport !== bSupport) return aSupport > bSupport + const aLastSeen = lastSeenTime(a.entry) + const bLastSeen = lastSeenTime(b.entry) + if (aLastSeen !== bLastSeen) return aLastSeen > bLastSeen return a.entry.id < b.entry.id } @@ -121,6 +131,8 @@ export function getMemory(entries: MemoryEntry[], query: Partial & if (a.score !== b.score) return b.score - a.score const supportDelta = b.entry.provenance.supportCount - a.entry.provenance.supportCount if (supportDelta !== 0) return supportDelta + const lastSeenDelta = lastSeenTime(b.entry) - lastSeenTime(a.entry) + if (lastSeenDelta !== 0) return lastSeenDelta return a.entry.id.localeCompare(b.entry.id) }) .map((ranked) => ranked.entry) diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index 8b576247e..3dfc9eb2d 100644 --- a/packages/opencode/src/altimate/review/orchestrate.ts +++ b/packages/opencode/src/altimate/review/orchestrate.ts @@ -27,6 +27,7 @@ import { } from "./spec-test-gen" import { sanitizeAssertionSql } from "./spec-test-sandbox" import { getMemory, type MemoryEntry } from "./corrective-memory" +import { stableJson } from "./stable-json" /** * The deterministic review recipe. @@ -247,8 +248,8 @@ function configuredMemoryEntries(input: OrchestrateInput): MemoryEntry[] { } function memoryProjectForInput(input: OrchestrateInput): string | undefined { + if (input.project) return input.project const entries = configuredMemoryEntries(input) - if (input.project && entries.some((entry) => entry.scope.project === input.project)) return input.project const projects = new Set(entries.map((entry) => entry.scope.project)) return projects.size === 1 ? [...projects][0] : undefined } @@ -1085,20 +1086,6 @@ interface EnforcedConstraintMetrics { failed: number } -function stableJson(value: unknown): string { - if (value === null || typeof value !== "object") return JSON.stringify(value) ?? "null" - if (Array.isArray(value)) return "[" + value.map(stableJson).join(",") + "]" - const obj = value as Record - return ( - "{" + - Object.keys(obj) - .sort() - .map((k) => JSON.stringify(k) + ":" + stableJson(obj[k])) - .join(",") + - "}" - ) -} - function declaredGeneratedTestId(test: Omit): string { return "gst_declared_" + createHash("sha256").update(stableJson(test)).digest("hex").slice(0, 16) } @@ -1107,6 +1094,9 @@ function materializeDeclaredConstraints(model: string, constraints: DeclaredCons const tests: GeneratedTest[] = [] for (const c of constraints) { if (c.hasEnforcingTest) continue + // TODO: column-type checks need INFORMATION_SCHEMA support before they can + // be materialized into executable/proposed GeneratedTests. + if (c.kind === "column_type") continue const args = c.args && Object.keys(c.args).length ? c.args : undefined const draft: Omit = { kind: c.kind, @@ -1126,7 +1116,10 @@ function materializeDeclaredConstraints(model: string, constraints: DeclaredCons } function specTestRuleKey(test: GeneratedTest): string { - return `spec_test:${test.derivedFrom.ref}:${test.kind}:${test.dbtTest?.column ?? ""}` + const args = test.dbtTest?.args ?? {} + const columns = args["columns"] ?? args["combination_of_columns"] ?? args["column_names"] ?? args["fields"] + const columnKey = Array.isArray(columns) ? columns.map(String).sort().join(",") : (test.dbtTest?.column ?? "") + return `spec_test:${test.derivedFrom.ref}:${test.kind}:${columnKey}` } function proposedSpecTestFinding(model: string, file: string, test: GeneratedTest): Finding { diff --git a/packages/opencode/src/altimate/review/runner.ts b/packages/opencode/src/altimate/review/runner.ts index cf5163c96..4c03d548b 100644 --- a/packages/opencode/src/altimate/review/runner.ts +++ b/packages/opencode/src/altimate/review/runner.ts @@ -46,6 +46,7 @@ interface CachedManifest { } const SPEC_TEST_SQL_TIMEOUT_MS = 30_000 +const TRACK_B_ACCEPTED_VALUES_LIMIT = 100 function asArray(v: unknown): T[] { return Array.isArray(v) ? (v as T[]) : [] @@ -125,8 +126,21 @@ function testArgs(node: any): Record | undefined { return Object.keys(out).length ? out : undefined } -function constraintKey(kind: DeclaredConstraint["kind"], column?: string): string { - return `${kind}:${(column ?? "").toLowerCase()}` +function constraintColumns(args?: Record): string[] { + const cols = args?.["columns"] ?? args?.["combination_of_columns"] ?? args?.["column_names"] ?? args?.["fields"] + return Array.isArray(cols) ? cols.map(String).filter(Boolean) : [] +} + +function constraintKey(kind: DeclaredConstraint["kind"], column?: string, args?: Record): string { + const columns = constraintColumns(args) + .map((c) => c.toLowerCase()) + .sort() + .join(",") + return `${kind}:${(column ?? "").toLowerCase()}:${columns}` +} + +function declaredConstraintKey(c: DeclaredConstraint): string { + return `${constraintKey(c.kind, c.column, c.args)}:${c.uniqueId ?? c.sourceRef}` } function sourceRef(model: string, kind: DeclaredConstraint["kind"], column?: string): string { @@ -237,10 +251,16 @@ function buildGeneratedTestSql( if (!model) return { detail: `model not found for ${test.derivedFrom.ref}` } const relation = relationForModel(model) const column = test.dbtTest?.column + const trackB = test.derivedFrom.origin === "inferred_context" + const sandboxAssertionSql = (sql: string): { sql?: string; detail?: string } => { + const sanitized = sanitizeAssertionSql(sql, expandedAllowedRelations(manifest, options)) + return sanitized.ok ? { sql: sanitized.sql } : { detail: `dbtTest rejected by sandbox: ${sanitized.reason}` } + } if (test.kind === "not_null") { if (!column) return { detail: "not_null test is missing column" } - return { sql: `select count(*) as violating_rows from ${relation} where ${sqlIdent(column)} is null` } + const assertionSql = `select ${sqlIdent(column)} from ${relation} where ${sqlIdent(column)} is null` + return trackB ? sandboxAssertionSql(assertionSql) : { sql: `select count(*) as violating_rows from (${assertionSql}) _nn` } } if (test.kind === "unique") { @@ -248,23 +268,25 @@ function buildGeneratedTestSql( if (!columns.length) return { detail: "unique test is missing columns" } const selectCols = columns.map(sqlIdent).join(", ") const nonNull = columns.map((c) => `${sqlIdent(c)} is not null`).join(" and ") - return { - sql: - `select count(*) as violating_rows from (` + - `select ${selectCols} from ${relation} where ${nonNull} group by ${selectCols} having count(*) > 1` + - `) as altimate_unique_violations`, - } + const assertionSql = `select ${selectCols} from ${relation} where ${nonNull} group by ${selectCols} having count(*) > 1` + return trackB + ? sandboxAssertionSql(assertionSql) + : { sql: `select count(*) as violating_rows from (${assertionSql}) as altimate_unique_violations` } } if (test.kind === "accepted_values") { if (!column) return { detail: "accepted_values test is missing column" } const values = test.dbtTest?.args?.["values"] if (!Array.isArray(values) || values.length === 0) return { detail: "accepted_values test is missing values" } - return { - sql: - `select count(*) as violating_rows from ${relation} ` + - `where ${sqlIdent(column)} is not null and ${sqlIdent(column)} not in (${values.map(sqlLiteral).join(", ")})`, + if (trackB && values.length > TRACK_B_ACCEPTED_VALUES_LIMIT) { + return { detail: `accepted_values test has too many values (${values.length})` } } + const assertionSql = + `select ${sqlIdent(column)} from ${relation} ` + + `where ${sqlIdent(column)} is not null and ${sqlIdent(column)} not in (${values.map(sqlLiteral).join(", ")})` + return trackB + ? sandboxAssertionSql(assertionSql) + : { sql: `select count(*) as violating_rows from (${assertionSql}) as altimate_accepted_value_violations` } } if (test.kind === "relationships") { @@ -273,12 +295,17 @@ function buildGeneratedTestSql( const target = relationshipTarget(args["to"], manifest) const field = typeof args["field"] === "string" && args["field"].trim() ? args["field"].trim() : undefined if (!target || !field) return { detail: "relationships test is missing target relation or field" } - return { - sql: - `select count(*) as violating_rows from ${relation} as child ` + - `left join ${target} as parent on child.${sqlIdent(column)} = parent.${sqlIdent(field)} ` + - `where child.${sqlIdent(column)} is not null and parent.${sqlIdent(field)} is null`, + if (trackB) { + const targetCheck = sanitizeAssertionSql(`select 1 from ${target}`, expandedAllowedRelations(manifest, options)) + if (!targetCheck.ok) return { detail: `relationships target rejected by sandbox: ${targetCheck.reason}` } } + const assertionSql = + `select child.${sqlIdent(column)} from ${relation} as child ` + + `left join ${target} as parent on child.${sqlIdent(column)} = parent.${sqlIdent(field)} ` + + `where child.${sqlIdent(column)} is not null and parent.${sqlIdent(field)} is null` + return trackB + ? sandboxAssertionSql(assertionSql) + : { sql: `select count(*) as violating_rows from (${assertionSql}) as altimate_relationship_violations` } } return { detail: `${test.kind} execution is not supported by a row-count assertion` } @@ -303,10 +330,15 @@ function isNoWarehouseError(message: string): boolean { } function withTimeout(promise: Promise, ms: number): Promise { + let timeout: ReturnType | undefined return Promise.race([ promise, - new Promise((_, reject) => setTimeout(() => reject(new Error(`spec-test SQL timed out after ${ms}ms`)), ms)), - ]) + new Promise((_, reject) => { + timeout = setTimeout(() => reject(new Error(`spec-test SQL timed out after ${ms}ms`)), ms) + }), + ]).finally(() => { + if (timeout) clearTimeout(timeout) + }) } /** @@ -631,11 +663,12 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun const kind = testKind(test) if (!kind) continue const column = testColumn(test) - enforcing.add(constraintKey(kind, column)) + const args = testArgs(test) + enforcing.add(constraintKey(kind, column, args)) testConstraints.push({ kind, column, - args: testArgs(test), + args, hasEnforcingTest: true, uniqueId: test.unique_id, sourceRef: sourceRef(target.name, kind, column), @@ -643,9 +676,9 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun } const out: DeclaredConstraint[] = [...testConstraints] - const seen = new Set(out.map((c) => `${c.kind}:${c.column ?? ""}:${c.uniqueId ?? c.sourceRef}`)) + const seen = new Set(out.map(declaredConstraintKey)) const add = (c: DeclaredConstraint) => { - const key = `${c.kind}:${c.column ?? ""}:${c.uniqueId ?? c.sourceRef}` + const key = declaredConstraintKey(c) if (seen.has(key)) return seen.add(key) out.push(c) @@ -690,7 +723,7 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun kind, column, args, - hasEnforcingTest: enforcing.has(constraintKey(kind, column)), + hasEnforcingTest: enforcing.has(constraintKey(kind, column, args)), uniqueId: target.unique_id, sourceRef: sourceRef(target.name, kind, column), }) @@ -707,13 +740,25 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun const columns = asArray(rawConstraint?.columns).map(String).filter(Boolean) if (!columns.length) continue const kind: DeclaredConstraint["kind"] = "unique" + const args = { columns } add({ kind, - args: { columns }, - hasEnforcingTest: enforcing.has(constraintKey(kind)), + args, + hasEnforcingTest: enforcing.has(constraintKey(kind, undefined, args)), uniqueId: target.unique_id, sourceRef: sourceRef(target.name, kind), }) + if (normalized === "primary_key") { + for (const column of columns) { + add({ + kind: "not_null", + column, + hasEnforcingTest: enforcing.has(constraintKey("not_null", column)), + uniqueId: target.unique_id, + sourceRef: sourceRef(target.name, "not_null", column), + }) + } + } } } diff --git a/packages/opencode/src/altimate/review/spec-test-gen.ts b/packages/opencode/src/altimate/review/spec-test-gen.ts index f201bdf26..027b7e72e 100644 --- a/packages/opencode/src/altimate/review/spec-test-gen.ts +++ b/packages/opencode/src/altimate/review/spec-test-gen.ts @@ -7,6 +7,7 @@ import { MessageV2 } from "@/session/message-v2" import { MessageID, SessionID } from "@/session/schema" import { Log } from "@/util/log" import { Dispatcher } from "../native" +import { stableJson } from "./stable-json" /** * Auxiliary spec-test synthesis for GREENFIELD (git-added) dbt models. @@ -196,6 +197,10 @@ export function filterToSpecDerived( dropped.push({ test: t, reason: "ref_not_provided" }) continue } + if (trustedSource.origin === "declared_constraint" && trustedSource.kind !== t.kind) { + dropped.push({ test: t, reason: "test_mismatch" }) + continue + } if (t.dbtTest && t.dbtTest.test !== t.kind) { dropped.push({ test: t, reason: "test_mismatch" }) continue @@ -215,20 +220,6 @@ export function isBlockEligible(test: GeneratedTest): boolean { return test.derivedFrom.origin === "declared_constraint" && DECLARED_CONSTRAINT_KINDS.has(test.kind) } -function stableJson(value: unknown): string { - if (value === null || typeof value !== "object") return JSON.stringify(value) ?? "null" - if (Array.isArray(value)) return "[" + value.map(stableJson).join(",") + "]" - const obj = value as Record - return ( - "{" + - Object.keys(obj) - .sort() - .map((k) => JSON.stringify(k) + ":" + stableJson(obj[k])) - .join(",") + - "}" - ) -} - function generatedTestId(test: GeneratedTest): string { const payload = stableJson({ kind: test.kind, diff --git a/packages/opencode/src/altimate/review/spec-test-sandbox.ts b/packages/opencode/src/altimate/review/spec-test-sandbox.ts index d7c5bc3c2..7fa138727 100644 --- a/packages/opencode/src/altimate/review/spec-test-sandbox.ts +++ b/packages/opencode/src/altimate/review/spec-test-sandbox.ts @@ -68,7 +68,9 @@ function stripTrailingStatementSemicolon(sql: string): SanitizedAssertionSql { if (semicolons.length === 0) return { ok: true, sql: sql.trim() } if (semicolons.length > 1) return { ok: false, reason: "multi_statement" } - if (sql.slice(semicolons[0]! + 1).trim()) return { ok: false, reason: "multi_statement" } + if (withoutStringsAndComments(sql.slice(semicolons[0]! + 1)).trim()) { + return { ok: false, reason: "multi_statement" } + } return { ok: true, sql: sql.slice(0, semicolons[0]).trim() } } @@ -164,7 +166,7 @@ function normalizeRelation(value: string): string { function relationAliases(name: string): string[] { const normalized = normalizeRelation(name) const parts = normalized.split(".").filter(Boolean) - return [...new Set([normalized, parts.at(-1) ?? ""].filter(Boolean))] + return [...new Set([normalized, parts.length >= 2 ? parts.slice(-2).join(".") : "", parts.at(-1) ?? ""].filter(Boolean))] } function allowedRelationSet(allowedRelations: Iterable): Set { @@ -189,9 +191,22 @@ function referencedRelations(sql: string): string[] { const relation = `${ident}(?:\\s*\\.\\s*${ident}){0,2}` const re = new RegExp(`\\b(?:from|join)\\s+(${relation})`, "gi") for (const match of sql.matchAll(re)) refs.push(normalizeRelation(match[1] ?? "")) + const factor = `${relation}(?:\\s+(?:as\\s+)?${ident})?` + const commaList = new RegExp(`\\bfrom\\s+(${factor}(?:\\s*,\\s*${factor})+)`, "gi") + for (const match of sql.matchAll(commaList)) { + const group = match[1] ?? "" + const factorRe = new RegExp(`(?:^|,)\\s*(${relation})`, "gi") + for (const factorMatch of group.matchAll(factorRe)) refs.push(normalizeRelation(factorMatch[1] ?? "")) + } return refs } +function relationAllowed(relation: string, allowed: Set): boolean { + const parts = normalizeRelation(relation).split(".").filter(Boolean) + if (parts.length > 1) return allowed.has(parts.join(".")) + return relationAliases(relation).some((alias) => allowed.has(alias)) +} + export function sanitizeAssertionSql(sql: string, allowedRelations: Iterable): SanitizedAssertionSql { const single = stripTrailingStatementSemicolon(sql) if (!single.ok) return single @@ -205,11 +220,10 @@ export function sanitizeAssertionSql(sql: string, allowedRelations: Iterable allowed.has(alias))) return { ok: false, reason: "unknown_relation" } + if (!relationAllowed(relation, allowed)) return { ok: false, reason: "unknown_relation" } } return { ok: true, sql: `select count(*) as n from ( ${trimmed} ) _s` } diff --git a/packages/opencode/src/altimate/review/stable-json.ts b/packages/opencode/src/altimate/review/stable-json.ts new file mode 100644 index 000000000..b42315874 --- /dev/null +++ b/packages/opencode/src/altimate/review/stable-json.ts @@ -0,0 +1,13 @@ +export function stableJson(value: unknown): string { + if (value === null || typeof value !== "object") return JSON.stringify(value) ?? "null" + if (Array.isArray(value)) return "[" + value.map(stableJson).join(",") + "]" + const obj = value as Record + return ( + "{" + + Object.keys(obj) + .sort() + .map((k) => JSON.stringify(k) + ":" + stableJson(obj[k])) + .join(",") + + "}" + ) +} diff --git a/packages/opencode/src/altimate/review/verdict.ts b/packages/opencode/src/altimate/review/verdict.ts index e143b7fe1..251d36a2f 100644 --- a/packages/opencode/src/altimate/review/verdict.ts +++ b/packages/opencode/src/altimate/review/verdict.ts @@ -60,6 +60,7 @@ export const VCS_EVENT: Record = { function specTestMayBlock(f: Finding): boolean { const tool = f.evidence?.tool ?? "" if (!tool.startsWith("altimate.spec_test")) return true + if (tool !== "altimate.spec_test.executed") return false const result = (f.evidence?.result ?? {}) as Record return result["executed"] === true && result["origin"] === "declared_constraint" } diff --git a/packages/opencode/test/altimate/review-corrective-memory.test.ts b/packages/opencode/test/altimate/review-corrective-memory.test.ts index b35b897f1..e3c52e080 100644 --- a/packages/opencode/test/altimate/review-corrective-memory.test.ts +++ b/packages/opencode/test/altimate/review-corrective-memory.test.ts @@ -19,13 +19,14 @@ function memoryEntry( scope: MemoryEntry["scope"], polarity: MemoryEntry["polarity"] = "suppress", supportCount = 1, + lastSeen?: string, ): MemoryEntry { return { id, scope, directive: `${polarity} ${id}`, polarity, - provenance: { source: "human_rule", committed: true, supportCount }, + provenance: { source: "human_rule", committed: true, supportCount, lastSeen }, } } @@ -96,6 +97,15 @@ describe("getMemory", () => { expect(getMemory(entries, { project: PROJECT, derivedFromKind: "range" })).toEqual([]) }) + + test("ties after specificity and supportCount prefer most recent lastSeen before id", () => { + const entries = [ + memoryEntry("a-old", { project: PROJECT, derivedFromKind: "range" }, "suppress", 2, "2026-01-01T00:00:00Z"), + memoryEntry("z-new", { project: PROJECT, derivedFromKind: "range" }, "prefer", 2, "2026-02-01T00:00:00Z"), + ] + + expect(getMemory(entries, { project: PROJECT, derivedFromKind: "range" }).map((e) => e.id)).toEqual(["z-new"]) + }) }) describe("corrective memory consumers", () => { @@ -197,6 +207,39 @@ models: expect(env.findings).toEqual([]) }) + test("explicit input project never falls back to another configured project", async () => { + const suppressOtherProject = memoryEntry("suppress-other", { + project: "other", + category: "test_coverage", + table: "fct_orders", + }) + + const env = await runReview({ + project: PROJECT, + changedFiles: [modelFile], + config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["ai_review"], memory: { entries: [suppressOtherProject] } }, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner: fakeRunner(), + getContent: content({ "models/marts/fct_orders.sql": modelSql }), + aiReview: async () => [ + makeFinding({ + severity: "suggestion", + category: "test_coverage", + title: "fct_orders: keep this project-scoped finding", + body: "advisory", + file: "models/marts/fct_orders.sql", + model: "fct_orders", + confidence: "unknown", + evidence: { tool: "ai-review", result: {} }, + ruleKey: "ai:test-coverage", + }), + ], + }) + + expect(env.findings.map((f) => f.title)).toEqual(["fct_orders: keep this project-scoped finding"]) + }) + test("suppress entries do not drop critical/executed findings or soften blocking verdicts", async () => { const suppressExecuted = memoryEntry("suppress-executed", { project: PROJECT, diff --git a/packages/opencode/test/altimate/review-runner.test.ts b/packages/opencode/test/altimate/review-runner.test.ts index 538b2d763..91304bb95 100644 --- a/packages/opencode/test/altimate/review-runner.test.ts +++ b/packages/opencode/test/altimate/review-runner.test.ts @@ -4,6 +4,7 @@ import path from "node:path" import { createDispatcherRunner } from "../../src/altimate/review/runner" import { Dispatcher } from "../../src/altimate/native" import { tmpdir } from "../fixture/fixture" +import type { GeneratedTest } from "../../src/altimate/review/spec-test-gen" let dispatcherSpy: ReturnType | undefined @@ -162,3 +163,167 @@ describe("runner honors engine `decidable` flag (core 0.5.1)", () => { await tmp[Symbol.asyncDispose]?.() }) }) + +describe("review runner declared constraints", () => { + async function runnerWithNodes(nodes: Record) { + const tmp = await tmpdir() + const manifestPath = path.join(tmp.path, "manifest.json") + writeFileSync( + manifestPath, + JSON.stringify({ + metadata: { adapter_type: "duckdb" }, + nodes, + sources: {}, + }), + ) + return { tmp, runner: createDispatcherRunner({ manifestPath }) } + } + + test("model-level primary_key emits composite unique plus not_null per key column", async () => { + const { tmp, runner } = await runnerWithNodes({ + "model.demo.fct_orders": { + resource_type: "model", + name: "fct_orders", + original_file_path: "models/fct_orders.sql", + config: { materialized: "table", contract: { enforced: true } }, + depends_on: { nodes: [] }, + columns: { + customer_id: { name: "customer_id", data_type: "integer" }, + order_id: { name: "order_id", data_type: "integer" }, + }, + constraints: [{ type: "primary_key", columns: ["customer_id", "order_id"] }], + }, + }) + + const constraints = await runner.declaredConstraints?.("fct_orders") + const executable = constraints?.filter((c) => c.kind !== "column_type") ?? [] + + expect(executable).toEqual( + expect.arrayContaining([ + expect.objectContaining({ kind: "unique", args: { columns: ["customer_id", "order_id"] } }), + expect.objectContaining({ kind: "not_null", column: "customer_id" }), + expect.objectContaining({ kind: "not_null", column: "order_id" }), + ]), + ) + await tmp[Symbol.asyncDispose]?.() + }) + + test("keeps distinct model-level unique column sets", async () => { + const { tmp, runner } = await runnerWithNodes({ + "model.demo.fct_orders": { + resource_type: "model", + name: "fct_orders", + original_file_path: "models/fct_orders.sql", + config: { materialized: "table", contract: { enforced: true } }, + depends_on: { nodes: [] }, + columns: { + customer_id: { name: "customer_id", data_type: "integer" }, + order_id: { name: "order_id", data_type: "integer" }, + }, + constraints: [ + { type: "unique", columns: ["customer_id"] }, + { type: "unique", columns: ["order_id"] }, + ], + }, + }) + + const uniques = (await runner.declaredConstraints?.("fct_orders"))?.filter((c) => c.kind === "unique") ?? [] + + expect(uniques.map((c) => (c.args?.columns as string[]).join(",")).sort()).toEqual(["customer_id", "order_id"]) + await tmp[Symbol.asyncDispose]?.() + }) +}) + +describe("review runner generated spec-test sandbox", () => { + async function runnerWithManifest() { + const tmp = await tmpdir() + const manifestPath = path.join(tmp.path, "manifest.json") + writeFileSync( + manifestPath, + JSON.stringify({ + metadata: { adapter_type: "duckdb" }, + nodes: { + "model.demo.fct_orders": { + resource_type: "model", + name: "fct_orders", + original_file_path: "models/fct_orders.sql", + config: { materialized: "table" }, + depends_on: { nodes: [] }, + columns: { + customer_id: { name: "customer_id", data_type: "integer" }, + status: { name: "status", data_type: "text" }, + }, + }, + "model.demo.dim_customers": { + resource_type: "model", + name: "dim_customers", + original_file_path: "models/dim_customers.sql", + config: { materialized: "table" }, + depends_on: { nodes: [] }, + columns: { customer_id: { name: "customer_id", data_type: "integer" } }, + }, + }, + sources: {}, + }), + ) + return { tmp, runner: createDispatcherRunner({ manifestPath }) } + } + + test("rejects track-B relationships when the target relation is not allowlisted", async () => { + const { tmp, runner } = await runnerWithManifest() + let calls = 0 + dispatcherSpy = spyOn(Dispatcher, "call").mockImplementation((async () => { + calls++ + return { rows: [[0]] } + }) as any) + const test: GeneratedTest = { + id: "rel-track-b", + kind: "relationships", + dbtTest: { + column: "customer_id", + test: "relationships", + args: { to: "ref('dim_customers')", field: "customer_id" }, + }, + rationale: "soft inferred relationship", + derivedFrom: { origin: "inferred_context", kind: "ref_edge", ref: "ref:dim_customers", text: "dim_customers" }, + } + + const results = await runner.runGeneratedTests?.([test], undefined, { + model: "fct_orders", + allowedRelations: ["fct_orders"], + }) + + expect(results?.[test.id]?.status).toBe("error") + expect(results?.[test.id]?.detail).toContain("sandbox") + expect(calls).toBe(0) + await tmp[Symbol.asyncDispose]?.() + }) + + test("rejects track-B accepted_values when the checked relation is not allowlisted", async () => { + const { tmp, runner } = await runnerWithManifest() + let calls = 0 + dispatcherSpy = spyOn(Dispatcher, "call").mockImplementation((async () => { + calls++ + return { rows: [[0]] } + }) as any) + const test: GeneratedTest = { + id: "accepted-track-b", + kind: "accepted_values", + dbtTest: { column: "status", test: "accepted_values", args: { values: ["placed", "shipped"] } }, + rationale: "soft inferred values", + derivedFrom: { + origin: "inferred_context", + kind: "schema_desc", + ref: "schema.yml:fct_orders.status:description", + text: "known status values", + }, + } + + const results = await runner.runGeneratedTests?.([test], undefined, { allowedRelations: ["dim_customers"] }) + + expect(results?.[test.id]?.status).toBe("error") + expect(results?.[test.id]?.detail).toContain("sandbox") + expect(calls).toBe(0) + await tmp[Symbol.asyncDispose]?.() + }) +}) diff --git a/packages/opencode/test/altimate/review-spec-test-exec.test.ts b/packages/opencode/test/altimate/review-spec-test-exec.test.ts index 492b26523..2dcb54814 100644 --- a/packages/opencode/test/altimate/review-spec-test-exec.test.ts +++ b/packages/opencode/test/altimate/review-spec-test-exec.test.ts @@ -228,4 +228,30 @@ describe("spec test synthesis lane (P1 declared-constraint execution)", () => { expect(calls).toBe(0) expect(env.findings.some((f) => f.evidence?.tool?.startsWith("altimate.spec_test"))).toBe(false) }) + + test("declared column_type is kept out of generated tests", async () => { + let calls = 0 + const env = await reviewWith( + baseRunner({ + async declaredConstraints() { + return [ + declared({ + kind: "column_type", + column: "order_id", + args: { data_type: "integer" }, + sourceRef: "schema.yml:fct_orders.order_id:column_type", + }), + ] + }, + async runGeneratedTests() { + calls++ + return {} + }, + }), + true, + ) + + expect(calls).toBe(0) + expect(env.findings.some((f) => f.evidence?.tool?.startsWith("altimate.spec_test"))).toBe(false) + }) }) diff --git a/packages/opencode/test/altimate/review-spec-test-gen.test.ts b/packages/opencode/test/altimate/review-spec-test-gen.test.ts index 75c0715e0..87e6f1267 100644 --- a/packages/opencode/test/altimate/review-spec-test-gen.test.ts +++ b/packages/opencode/test/altimate/review-spec-test-gen.test.ts @@ -95,6 +95,21 @@ describe("filterToSpecDerived (advisory-track anti-fabrication guard)", () => { expect(kept.length).toBe(0) expect(dropped[0]?.reason).toBe("test_mismatch") }) + + test("drops a proposal whose kind does not match a declared source kind", () => { + const { kept, dropped } = filterToSpecDerived( + [ + mkTest({ + kind: "unique", + dbtTest: { column: "email", test: "unique" }, + derivedFrom: declared, + }), + ], + providedSources, + ) + expect(kept.length).toBe(0) + expect(dropped[0]?.reason).toBe("test_mismatch") + }) }) describe("isBlockEligible (only track-A declared constraints can block)", () => { diff --git a/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts b/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts index fb31a43a4..df4450fbe 100644 --- a/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts +++ b/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts @@ -20,6 +20,36 @@ describe("sanitizeAssertionSql", () => { }) }) + test("rejects qualified references that only match an allowlisted basename", () => { + expect(sanitizeAssertionSql("select * from other_schema.fct_orders", ["fct_orders"])).toEqual({ + ok: false, + reason: "unknown_relation", + }) + }) + + test("allows unqualified references to match an allowlisted qualified relation", () => { + expect(sanitizeAssertionSql("select * from fct_orders", ["analytics.fct_orders"]).ok).toBe(true) + }) + + test("checks comma-separated FROM factors against the allowlist", () => { + expect(sanitizeAssertionSql("select * from fct_orders, secret_orders", ["fct_orders"])).toEqual({ + ok: false, + reason: "unknown_relation", + }) + }) + + test("allows trailing semicolon followed only by a comment", () => { + expect(sanitizeAssertionSql("select order_id from fct_orders; -- bounded assertion", ["fct_orders"]).ok).toBe(true) + }) + + test("recognizes a CTE after a leading comment", () => { + const sanitized = sanitizeAssertionSql( + "-- generated candidate\nwith scoped as (select order_id from fct_orders) select order_id from scoped", + ["fct_orders"], + ) + expect(sanitized.ok).toBe(true) + }) + test("accepts and bounds a select on allowlisted relations", () => { const sanitized = sanitizeAssertionSql("select order_id from fct_orders where order_id is null;", ["fct_orders"]) expect(sanitized.ok).toBe(true) diff --git a/packages/opencode/test/altimate/review-spec-test-verdict.test.ts b/packages/opencode/test/altimate/review-spec-test-verdict.test.ts index ad559d735..e173b9e15 100644 --- a/packages/opencode/test/altimate/review-spec-test-verdict.test.ts +++ b/packages/opencode/test/altimate/review-spec-test-verdict.test.ts @@ -58,6 +58,12 @@ describe("verdict enforcement — a spec-test finding may only block when execut expect(computeIdealVerdict([crit("altimate.spec_test.proposed", { proposal: {} })])).toBe("COMMENT") }) + test("spoofed proposed/candidate results with executed+declared still do not block", () => { + const spoofed = { executed: true, origin: "declared_constraint" } + expect(computeIdealVerdict([crit("altimate.spec_test.proposed", spoofed)])).toBe("COMMENT") + expect(computeIdealVerdict([crit("altimate.spec_test.candidate", spoofed)])).toBe("COMMENT") + }) + test("candidate spec-test warnings do not accumulate into a blocking verdict", () => { expect( computeIdealVerdict([ From 73e93fba51cf7e2cf1fa1b5574581d129a1f2f92 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Wed, 8 Jul 2026 17:04:45 -0700 Subject: [PATCH 9/9] fix: [dbt-pr-review] address #991 round-2 re-review findings - Sandbox: extract withoutCommentsAndStringLiterals (preserves quoted identifiers) and use it for BOTH cteNames and referencedRelations, so a FROM/JOIN inside a comment/string no longer false-rejects and quoted CTE/relation names resolve. - P1: declared (track-A) relationship targets are now sandbox-validated against the allowlist unconditionally (not only track-B), and source('s','t') targets resolve via the manifest (skip, not guess, when unresolvable). - constraintKey includes normalized args for accepted_values/relationships so an existing test on the same column no longer masks a different declared constraint. - column_type declared constraints surface as a single non-blocking advisory finding instead of silently disappearing. - Composite/model-level unique renders valid dbt_utils.unique_combination_of_columns YAML (core unique takes column_name); single-column keeps core unique. - Drop range from ALLOWED_TEST_KINDS (no dbt core generic test named range). Review blast radius 281 tests green. Refs #989 Co-Authored-By: Claude Opus 4.8 --- .../src/altimate/review/orchestrate.ts | 76 +++++++- .../opencode/src/altimate/review/runner.ts | 119 ++++++++++-- .../src/altimate/review/spec-test-gen.ts | 10 +- .../src/altimate/review/spec-test-sandbox.ts | 111 +++++++++++- .../test/altimate/review-runner.test.ts | 169 ++++++++++++++++++ .../altimate/review-spec-test-exec.test.ts | 16 +- .../altimate/review-spec-test-gen.test.ts | 45 ++++- .../altimate/review-spec-test-sandbox.test.ts | 12 ++ 8 files changed, 519 insertions(+), 39 deletions(-) diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index 3dfc9eb2d..e8294a41e 100644 --- a/packages/opencode/src/altimate/review/orchestrate.ts +++ b/packages/opencode/src/altimate/review/orchestrate.ts @@ -1086,10 +1086,32 @@ interface EnforcedConstraintMetrics { failed: number } +function constraintColumns(args?: Record): string[] { + const cols = args?.["columns"] ?? args?.["combination_of_columns"] ?? args?.["column_names"] ?? args?.["fields"] + return Array.isArray(cols) ? cols.map(String).filter(Boolean) : [] +} + function declaredGeneratedTestId(test: Omit): string { return "gst_declared_" + createHash("sha256").update(stableJson(test)).digest("hex").slice(0, 16) } +function dbtTestForDeclaredConstraint(c: DeclaredConstraint): GeneratedTest["dbtTest"] { + const args = c.args && Object.keys(c.args).length ? c.args : undefined + if (c.kind === "unique" && !c.column) { + const columns = constraintColumns(args) + if (columns.length > 1) { + return { + test: "dbt_utils.unique_combination_of_columns", + args: { combination_of_columns: columns }, + } + } + if (columns.length === 1) { + return { test: "unique", args: { column_name: columns[0] } } + } + } + return { column: c.column, test: c.kind, args } +} + function materializeDeclaredConstraints(model: string, constraints: DeclaredConstraint[]): GeneratedTest[] { const tests: GeneratedTest[] = [] for (const c of constraints) { @@ -1100,7 +1122,7 @@ function materializeDeclaredConstraints(model: string, constraints: DeclaredCons const args = c.args && Object.keys(c.args).length ? c.args : undefined const draft: Omit = { kind: c.kind, - dbtTest: { column: c.column, test: c.kind, args }, + dbtTest: dbtTestForDeclaredConstraint(c), rationale: `The model declares an unenforced ${c.kind} constraint${c.column ? ` on ${c.column}` : ""}.`, derivedFrom: { origin: "declared_constraint", @@ -1122,6 +1144,39 @@ function specTestRuleKey(test: GeneratedTest): string { return `spec_test:${test.derivedFrom.ref}:${test.kind}:${columnKey}` } +function columnTypeAdvisoryFinding(model: string, file: string, constraints: DeclaredConstraint[]): Finding { + const pairs = constraints + .map((c) => ({ column: c.column ?? "(model)", data_type: String(c.args?.data_type ?? c.args?.["type"] ?? "").trim() })) + .filter((p) => p.data_type) + const body = [ + "The model declares column data types that this review could not verify automatically.", + "", + ...pairs.map((p) => `- \`${p.column}\` -> \`${p.data_type}\``), + "", + "Automated type verification is not yet available for these contract declarations, so no dbt generic YAML patch was generated and nothing was executed.", + ].join("\n") + return makeFinding({ + severity: "suggestion", + category: "test_coverage", + title: `${model}: declared column_type constraints are unverified`, + body, + file, + model, + confidence: "unknown", + evidence: { + tool: "altimate.spec_test.proposed", + result: { + proposal: { + kind: "column_type", + derivedFrom: { origin: "declared_constraint", kind: "column_type", ref: `schema.yml:${model}:column_type` }, + }, + columnTypes: pairs, + }, + }, + ruleKey: `spec_test:${model}:column_type:unverified`, + }) +} + function proposedSpecTestFinding(model: string, file: string, test: GeneratedTest): Finding { const yaml = proposedTestYaml(model, test) const column = test.dbtTest?.column @@ -1217,12 +1272,21 @@ async function specTestSynthesisLane( const model = modelNameFromPath(ctx.file.path) const sql = ctx.engineNewSql ?? ctx.newSql ?? "" const findings: Finding[] = [] - const trackAAllowedRelations = [model] + const specSources = dedupeSpecSources([ + ...(await changedSchemaSources(model, input)), + ...sqlIntentSources(ctx.newSql), + ...sqlIntentSources(ctx.engineNewSql), + ...prIntentSources(model, input), + ]) + const trackAAllowedRelations = allowedRelationsForSpecTests(model, specSources) let trackATests: GeneratedTest[] = [] if (input.runner.declaredConstraints) { try { - trackATests = materializeDeclaredConstraints(model, await input.runner.declaredConstraints(model)) + const constraints = await input.runner.declaredConstraints(model) + const columnTypes = constraints.filter((c) => c.kind === "column_type" && !c.hasEnforcingTest) + if (columnTypes.length) findings.push(columnTypeAdvisoryFinding(model, ctx.file.path, columnTypes)) + trackATests = materializeDeclaredConstraints(model, constraints) } catch { trackATests = [] } @@ -1263,12 +1327,6 @@ async function specTestSynthesisLane( } if (!input.generateSpecTests) return findings - const specSources = dedupeSpecSources([ - ...(await changedSchemaSources(model, input)), - ...sqlIntentSources(ctx.newSql), - ...sqlIntentSources(ctx.engineNewSql), - ...prIntentSources(model, input), - ]) if (!specSources.length) return findings const memoryEntries = configuredMemoryEntries(input) diff --git a/packages/opencode/src/altimate/review/runner.ts b/packages/opencode/src/altimate/review/runner.ts index 4c03d548b..ee595a7d6 100644 --- a/packages/opencode/src/altimate/review/runner.ts +++ b/packages/opencode/src/altimate/review/runner.ts @@ -13,6 +13,7 @@ import type { import type { GeneratedTest, GeneratedTestResult } from "./spec-test-gen" import { sanitizeAssertionSql } from "./spec-test-sandbox" import { buildReviewSchemaContext, type SchemaContext } from "./schema-context" +import { stableJson } from "./stable-json" /** * Production ReviewRunner backed by the native Dispatcher (the Rust core). @@ -35,9 +36,22 @@ interface ManifestModel { raw?: any } +interface ManifestSource { + unique_id: string + name: string + source_name: string + database?: string + schema_name?: string + identifier?: string + relation_name?: string + raw?: any +} + interface CachedManifest { models: Map // unique_id -> model byName: Map + sources: Map // unique_id -> source + sourcesByName: Map // source_name.name -> source children: Map // unique_id -> direct child unique_ids testDeps: Map> // model unique_id -> set of test unique_ids depending on it testNodes: any[] @@ -131,12 +145,32 @@ function constraintColumns(args?: Record): string[] { return Array.isArray(cols) ? cols.map(String).filter(Boolean) : [] } +function normalizedConstraintArgs( + kind: DeclaredConstraint["kind"], + args?: Record, +): Record | undefined { + if (kind !== "accepted_values" && kind !== "relationships") return undefined + const out: Record = {} + for (const [key, value] of Object.entries(args ?? {})) { + if (key === "type" || key === "name") continue + if (key === "values" && Array.isArray(value)) { + out[key] = [...value].map((v) => (typeof v === "string" ? v.trim() : v)).sort((a, b) => String(a).localeCompare(String(b))) + } else if ((key === "to" || key === "field") && typeof value === "string") { + out[key] = value.trim().replace(/\s+/g, " ") + } else { + out[key] = value + } + } + return out +} + function constraintKey(kind: DeclaredConstraint["kind"], column?: string, args?: Record): string { const columns = constraintColumns(args) .map((c) => c.toLowerCase()) .sort() .join(",") - return `${kind}:${(column ?? "").toLowerCase()}:${columns}` + const argKey = normalizedConstraintArgs(kind, args) + return `${kind}:${(column ?? "").toLowerCase()}:${columns}:${argKey ? stableJson(argKey) : ""}` } function declaredConstraintKey(c: DeclaredConstraint): string { @@ -174,11 +208,22 @@ function relationForModel(model: ManifestModel): string { return sqlTableRef(parts.join(".")) } +function relationForSource(source: ManifestSource): string { + if (source.relation_name) return source.relation_name + const parts = [source.database, source.schema_name, source.identifier || source.name].filter((p): p is string => !!p) + return sqlTableRef(parts.join(".")) +} + function findModel(manifest: CachedManifest, model: string | undefined): ManifestModel | undefined { if (!model) return undefined return manifest.byName.get(model) ?? [...manifest.models.values()].find((m) => m.name.endsWith(`.${model}`)) } +function findSource(manifest: CachedManifest, sourceName: string | undefined, tableName: string | undefined): ManifestSource | undefined { + if (!sourceName || !tableName) return undefined + return manifest.sourcesByName.get(`${sourceName}.${tableName}`.toLowerCase()) +} + function addRelationVariants(out: Set, relation: string | undefined): void { if (!relation) return const trimmed = relation.trim() @@ -193,6 +238,27 @@ function addRelationVariants(out: Set, relation: string | undefined): vo if (parts.length >= 2) out.add(parts.slice(-2).join(".")) } +function addAllowedModelDeps(out: Set, manifest: CachedManifest, modelName: string | undefined): void { + const model = findModel(manifest, modelName) + if (!model) return + for (const dep of model.depends_on) { + const depModel = manifest.models.get(dep) + if (depModel) { + addRelationVariants(out, depModel.name) + addRelationVariants(out, depModel.alias) + addRelationVariants(out, relationForModel(depModel)) + continue + } + const depSource = manifest.sources.get(dep) + if (depSource) { + addRelationVariants(out, `${depSource.source_name}.${depSource.name}`) + addRelationVariants(out, depSource.name) + addRelationVariants(out, depSource.identifier) + addRelationVariants(out, relationForSource(depSource)) + } + } +} + function expandedAllowedRelations(manifest: CachedManifest, options?: GeneratedTestExecutionOptions): string[] { const out = new Set() const seed = [...(options?.allowedRelations ?? []), options?.model].filter((v): v is string => !!v) @@ -204,7 +270,13 @@ function expandedAllowedRelations(manifest: CachedManifest, options?: GeneratedT addRelationVariants(out, model.alias) addRelationVariants(out, relationForModel(model)) } + const sourceParts = relation.split(".") + if (sourceParts.length === 2) { + const source = findSource(manifest, sourceParts[0], sourceParts[1]) + if (source) addRelationVariants(out, relationForSource(source)) + } } + addAllowedModelDeps(out, manifest, options?.model) return [...out] } @@ -224,16 +296,21 @@ function columnsForUnique(test: GeneratedTest): string[] { return Array.isArray(cols) ? cols.map(String).filter(Boolean) : [] } -function relationshipTarget(raw: unknown, manifest: CachedManifest): string | undefined { - if (typeof raw !== "string" || !raw.trim()) return undefined +function relationshipTarget(raw: unknown, manifest: CachedManifest): { relation?: string; detail?: string } { + if (typeof raw !== "string" || !raw.trim()) return { detail: "relationships test is missing target relation" } const ref = /\bref\s*\(\s*['"]([^'"]+)['"]\s*\)/.exec(raw)?.[1] if (ref) { const model = manifest.byName.get(ref) - return model ? relationForModel(model) : sqlTableRef(ref) + return { relation: model ? relationForModel(model) : sqlTableRef(ref) } } const source = /\bsource\s*\(\s*['"]([^'"]+)['"]\s*,\s*['"]([^'"]+)['"]\s*\)/.exec(raw) - if (source) return sqlTableRef(`${source[1]}.${source[2]}`) - return sqlTableRef(raw) + if (source) { + const resolved = findSource(manifest, source[1], source[2]) + return resolved + ? { relation: relationForSource(resolved) } + : { detail: `relationships source target ${source[1]}.${source[2]} was not found in the manifest` } + } + return { relation: sqlTableRef(raw) } } function buildGeneratedTestSql( @@ -292,13 +369,12 @@ function buildGeneratedTestSql( if (test.kind === "relationships") { if (!column) return { detail: "relationships test is missing column" } const args = test.dbtTest?.args ?? {} - const target = relationshipTarget(args["to"], manifest) + const resolvedTarget = relationshipTarget(args["to"], manifest) + const target = resolvedTarget.relation const field = typeof args["field"] === "string" && args["field"].trim() ? args["field"].trim() : undefined - if (!target || !field) return { detail: "relationships test is missing target relation or field" } - if (trackB) { - const targetCheck = sanitizeAssertionSql(`select 1 from ${target}`, expandedAllowedRelations(manifest, options)) - if (!targetCheck.ok) return { detail: `relationships target rejected by sandbox: ${targetCheck.reason}` } - } + if (!target || !field) return { detail: resolvedTarget.detail ?? "relationships test is missing target relation or field" } + const targetCheck = sanitizeAssertionSql(`select 1 from ${target}`, expandedAllowedRelations(manifest, options)) + if (!targetCheck.ok) return { detail: `relationships target rejected by sandbox: ${targetCheck.reason}` } const assertionSql = `select child.${sqlIdent(column)} from ${relation} as child ` + `left join ${target} as parent on child.${sqlIdent(column)} = parent.${sqlIdent(field)} ` + @@ -399,6 +475,8 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun const rawNodes = (rawManifest?.nodes ?? {}) as Record const models = new Map() const byName = new Map() + const sources = new Map() + const sourcesByName = new Map() const children = new Map() const nodes = [...asArray(res.models), ...asArray((res as any).snapshots)] for (const m of nodes) { @@ -414,6 +492,19 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun models.set(enriched.unique_id, enriched) byName.set(enriched.name, enriched) } + for (const source of asArray(res.sources)) { + const raw = (rawManifest?.sources ?? {})[source.unique_id] + const enriched = { + ...source, + database: (source as any).database, + schema_name: (source as any).schema_name, + identifier: raw?.identifier, + relation_name: raw?.relation_name, + raw, + } + sources.set(enriched.unique_id, enriched) + sourcesByName.set(`${enriched.source_name}.${enriched.name}`.toLowerCase(), enriched) + } // Invert depends_on (upstream) into children (downstream). for (const m of nodes) { for (const parent of asArray(m.depends_on)) { @@ -438,11 +529,13 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun asArray(res.seeds), asArray((res as any).snapshots), ) - return { models, byName, children, testDeps, testNodes, schemaContext, ok: models.size > 0 } + return { models, byName, sources, sourcesByName, children, testDeps, testNodes, schemaContext, ok: models.size > 0 } } catch { return { models: new Map(), byName: new Map(), + sources: new Map(), + sourcesByName: new Map(), children: new Map(), testDeps: new Map(), testNodes: [], diff --git a/packages/opencode/src/altimate/review/spec-test-gen.ts b/packages/opencode/src/altimate/review/spec-test-gen.ts index 027b7e72e..04a1d9e01 100644 --- a/packages/opencode/src/altimate/review/spec-test-gen.ts +++ b/packages/opencode/src/altimate/review/spec-test-gen.ts @@ -75,13 +75,12 @@ export interface SpecSource { } /** Test classes. `not_null`/`unique`/`accepted_values`/`relationships`/ - * `column_type` map to track-A declared constraints; `range` is an - * inferred-context proposal. No golden/snapshot class exists — current output - * is never an expected value. */ + * `column_type` map to track-A declared constraints. No golden/snapshot class + * exists — current output is never an expected value. */ export type GeneratedTestKind = "not_null" | "unique" | "accepted_values" | "relationships" | "column_type" | "range" /** Kinds that can derive from a declared constraint (track A, block-eligible when - * executed). `range` and any description-derived test are advisory only. */ + * executed). Any description-derived test is advisory only. */ export const DECLARED_CONSTRAINT_KINDS: ReadonlySet = new Set([ "not_null", "unique", @@ -97,7 +96,6 @@ export const ALLOWED_TEST_KINDS: ReadonlySet = new Set= 2 && trimmed[0] === trimmed[trimmed.length - 1] && (trimmed[0] === '"' || trimmed[0] === "`")) { + const quote = trimmed[0] + return trimmed.slice(1, -1).replaceAll(`${quote}${quote}`, quote) + } + return trimmed +} + function normalizeRelation(value: string): string { return value .trim() - .replace(/^[`"\[]|[`"\]]$/g, "") - .replace(/\s*\.\s*/g, ".") + .split(/\s*\.\s*/g) + .map(normalizeIdentifierPart) + .join(".") .toLowerCase() } @@ -213,15 +309,16 @@ export function sanitizeAssertionSql(sql: string, allowedRelations: Iterable { expect(uniques.map((c) => (c.args?.columns as string[]).join(",")).sort()).toEqual(["customer_id", "order_id"]) await tmp[Symbol.asyncDispose]?.() }) + + test("does not mark arg-distinct accepted_values constraints as already enforced", async () => { + const { tmp, runner } = await runnerWithNodes({ + "model.demo.fct_orders": { + resource_type: "model", + name: "fct_orders", + original_file_path: "models/fct_orders.sql", + config: { materialized: "table", contract: { enforced: true } }, + depends_on: { nodes: [] }, + columns: { + status: { + name: "status", + data_type: "text", + constraints: [{ type: "accepted_values", values: ["new"] }], + }, + }, + }, + "test.demo.accepted_values_fct_orders_status": { + resource_type: "test", + unique_id: "test.demo.accepted_values_fct_orders_status", + name: "accepted_values_fct_orders_status", + attached_node: "model.demo.fct_orders", + depends_on: { nodes: ["model.demo.fct_orders"] }, + test_metadata: { + name: "accepted_values", + kwargs: { column_name: "status", values: ["placed"] }, + }, + }, + }) + + const constraints = (await runner.declaredConstraints?.("fct_orders")) ?? [] + const declaredStatus = constraints.find( + (c) => c.kind === "accepted_values" && c.column === "status" && (c.args?.values as string[] | undefined)?.[0] === "new", + ) + + expect(declaredStatus?.hasEnforcingTest).toBe(false) + await tmp[Symbol.asyncDispose]?.() + }) }) describe("review runner generated spec-test sandbox", () => { @@ -269,6 +307,41 @@ describe("review runner generated spec-test sandbox", () => { return { tmp, runner: createDispatcherRunner({ manifestPath }) } } + async function runnerWithSourceManifest() { + const tmp = await tmpdir() + const manifestPath = path.join(tmp.path, "manifest.json") + writeFileSync( + manifestPath, + JSON.stringify({ + metadata: { adapter_type: "duckdb" }, + nodes: { + "model.demo.fct_orders": { + resource_type: "model", + name: "fct_orders", + original_file_path: "models/fct_orders.sql", + config: { materialized: "table" }, + depends_on: { nodes: ["source.demo.raw.orders"] }, + columns: { + customer_id: { name: "customer_id", data_type: "integer" }, + }, + }, + }, + sources: { + "source.demo.raw.orders": { + resource_type: "source", + name: "orders", + source_name: "raw", + database: "analytics", + schema: "raw_schema", + identifier: "orders_raw", + columns: { customer_id: { name: "customer_id", data_type: "integer" } }, + }, + }, + }), + ) + return { tmp, runner: createDispatcherRunner({ manifestPath }) } + } + test("rejects track-B relationships when the target relation is not allowlisted", async () => { const { tmp, runner } = await runnerWithManifest() let calls = 0 @@ -326,4 +399,100 @@ describe("review runner generated spec-test sandbox", () => { expect(calls).toBe(0) await tmp[Symbol.asyncDispose]?.() }) + + test("resolves source() relationship targets to the manifest physical relation", async () => { + const { tmp, runner } = await runnerWithSourceManifest() + let seenSql = "" + dispatcherSpy = spyOn(Dispatcher, "call").mockImplementation((async (_method: string, params: any) => { + seenSql = params.sql + return { rows: [[0]] } + }) as any) + const test: GeneratedTest = { + id: "rel-source", + kind: "relationships", + dbtTest: { + column: "customer_id", + test: "relationships", + args: { to: "source('raw', 'orders')", field: "customer_id" }, + }, + rationale: "declared relationship", + derivedFrom: { + origin: "declared_constraint", + kind: "relationships", + ref: "schema.yml:fct_orders.customer_id:relationships", + }, + } + + const results = await runner.runGeneratedTests?.([test], undefined, { model: "fct_orders" }) + + expect(results?.[test.id]?.status).toBe("pass") + expect(seenSql).toContain("analytics.raw_schema.orders_raw") + expect(seenSql).not.toContain("raw.orders") + await tmp[Symbol.asyncDispose]?.() + }) + + test("skips source() relationship execution when the manifest source cannot be resolved", async () => { + const { tmp, runner } = await runnerWithSourceManifest() + let calls = 0 + dispatcherSpy = spyOn(Dispatcher, "call").mockImplementation((async () => { + calls++ + return { rows: [[0]] } + }) as any) + const test: GeneratedTest = { + id: "rel-missing-source", + kind: "relationships", + dbtTest: { + column: "customer_id", + test: "relationships", + args: { to: "source('missing', 'orders')", field: "customer_id" }, + }, + rationale: "declared relationship", + derivedFrom: { + origin: "declared_constraint", + kind: "relationships", + ref: "schema.yml:fct_orders.customer_id:relationships", + }, + } + + const results = await runner.runGeneratedTests?.([test], undefined, { model: "fct_orders" }) + + expect(results?.[test.id]?.status).toBe("error") + expect(results?.[test.id]?.detail).toContain("not found in the manifest") + expect(calls).toBe(0) + await tmp[Symbol.asyncDispose]?.() + }) + + test("rejects track-A relationships when the target relation is not allowlisted", async () => { + const { tmp, runner } = await runnerWithManifest() + let calls = 0 + dispatcherSpy = spyOn(Dispatcher, "call").mockImplementation((async () => { + calls++ + return { rows: [[0]] } + }) as any) + const test: GeneratedTest = { + id: "rel-track-a-rejected", + kind: "relationships", + dbtTest: { + column: "customer_id", + test: "relationships", + args: { to: "secret_customers", field: "customer_id" }, + }, + rationale: "declared relationship", + derivedFrom: { + origin: "declared_constraint", + kind: "relationships", + ref: "schema.yml:fct_orders.customer_id:relationships", + }, + } + + const results = await runner.runGeneratedTests?.([test], undefined, { + model: "fct_orders", + allowedRelations: ["fct_orders"], + }) + + expect(results?.[test.id]?.status).toBe("error") + expect(results?.[test.id]?.detail).toContain("sandbox") + expect(calls).toBe(0) + await tmp[Symbol.asyncDispose]?.() + }) }) diff --git a/packages/opencode/test/altimate/review-spec-test-exec.test.ts b/packages/opencode/test/altimate/review-spec-test-exec.test.ts index 2dcb54814..e6bab390a 100644 --- a/packages/opencode/test/altimate/review-spec-test-exec.test.ts +++ b/packages/opencode/test/altimate/review-spec-test-exec.test.ts @@ -229,7 +229,7 @@ describe("spec test synthesis lane (P1 declared-constraint execution)", () => { expect(env.findings.some((f) => f.evidence?.tool?.startsWith("altimate.spec_test"))).toBe(false) }) - test("declared column_type is kept out of generated tests", async () => { + test("declared column_type emits a non-blocking advisory but is not executed", async () => { let calls = 0 const env = await reviewWith( baseRunner({ @@ -252,6 +252,18 @@ describe("spec test synthesis lane (P1 declared-constraint execution)", () => { ) expect(calls).toBe(0) - expect(env.findings.some((f) => f.evidence?.tool?.startsWith("altimate.spec_test"))).toBe(false) + const finding = env.findings.find((f) => f.evidence?.tool === "altimate.spec_test.proposed") + expect(finding).toMatchObject({ + severity: "suggestion", + confidence: "unknown", + category: "test_coverage", + }) + expect(finding?.body).toContain("order_id") + expect(finding?.body).toContain("integer") + expect(finding?.body).toContain("Automated type verification is not yet available") + expect(finding?.evidence?.result).toMatchObject({ + columnTypes: [{ column: "order_id", data_type: "integer" }], + }) + expect(env.verdict).not.toBe("REQUEST_CHANGES") }) }) diff --git a/packages/opencode/test/altimate/review-spec-test-gen.test.ts b/packages/opencode/test/altimate/review-spec-test-gen.test.ts index 87e6f1267..d1e0352c1 100644 --- a/packages/opencode/test/altimate/review-spec-test-gen.test.ts +++ b/packages/opencode/test/altimate/review-spec-test-gen.test.ts @@ -122,8 +122,16 @@ describe("isBlockEligible (only track-A declared constraints can block)", () => expect(isBlockEligible(mkTest({ derivedFrom: backLabeled, kind: "not_null" }))).toBe(false) }) - test("range proposal → never block-eligible", () => { - expect(isBlockEligible(mkTest({ derivedFrom: inferred, kind: "range" }))).toBe(false) + test("range proposal is dropped by the guard and never block-eligible", () => { + const test = mkTest({ + derivedFrom: inferred, + kind: "range", + dbtTest: { column: "discount_pct", test: "range", args: { min: 0, max: 100 } }, + }) + const { kept, dropped } = filterToSpecDerived([test], providedSources) + expect(kept.length).toBe(0) + expect(dropped[0]?.reason).toBe("kind_not_allowed") + expect(isBlockEligible(test)).toBe(false) }) }) @@ -217,6 +225,39 @@ describe("spec test synthesis lane (P0 propose-only)", () => { expect(summary).toContain("Candidate tests to consider adding") }) + test("composite declared unique renders dbt_utils.unique_combination_of_columns YAML", async () => { + const env = await runReview({ + changedFiles, + config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["spec_tests"], ai: false, specTests: { execute: false } }, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner: { + ...fakeRunner(), + async declaredConstraints() { + return [ + { + kind: "unique", + args: { columns: ["customer_id", "order_id"] }, + hasEnforcingTest: false, + sourceRef: "schema.yml:fct_orders:unique", + }, + ] + }, + }, + getContent: content({ + "models/marts/fct_orders.sql": modelSql, + "models/marts/schema.yml": schemaYaml, + }), + }) + + const finding = env.findings.find((f) => f.evidence?.tool === "altimate.spec_test.proposed") + const yaml = String((finding?.evidence?.result as any)?.yaml ?? "") + expect(yaml).toContain("dbt_utils.unique_combination_of_columns") + expect(yaml).toContain("combination_of_columns") + expect(yaml).toContain("customer_id") + expect(yaml).toContain("order_id") + }) + test("proposed findings never request changes in comment or gate mode", async () => { for (const mode of ["comment", "gate"] as const) { const env = await runReview({ diff --git a/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts b/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts index df4450fbe..7112bfb4b 100644 --- a/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts +++ b/packages/opencode/test/altimate/review-spec-test-sandbox.test.ts @@ -38,6 +38,18 @@ describe("sanitizeAssertionSql", () => { }) }) + test("ignores relation-looking text inside string literals", () => { + expect(sanitizeAssertionSql("select 'from secret_orders' as note from fct_orders", ["fct_orders"]).ok).toBe(true) + }) + + test("preserves quoted CTE and relation identifiers while scanning references", () => { + const sanitized = sanitizeAssertionSql( + 'with "quoted cte" as (select id from "allowed relation") select id from "quoted cte"', + ["allowed relation"], + ) + expect(sanitized.ok).toBe(true) + }) + test("allows trailing semicolon followed only by a comment", () => { expect(sanitizeAssertionSql("select order_id from fct_orders; -- bounded assertion", ["fct_orders"]).ok).toBe(true) })