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) + }) +})