feat: greenfield spec-test synthesis for dbt-pr-review (P0: propose-only)#990
feat: greenfield spec-test synthesis for dbt-pr-review (P0: propose-only)#990anandgupta42 wants to merge 1 commit into
Conversation
…nly) 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 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThis PR adds greenfield spec-test synthesis for dbt PR review: a new ChangesGreenfield spec-test synthesis
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant runReview
participant Orchestrator
participant specTestSynthesisLane
participant runSpecTestGen
participant LLM
participant Findings
runReview->>Orchestrator: run(input with generateSpecTests)
Orchestrator->>Orchestrator: lanes.has("spec_tests")?
Orchestrator->>specTestSynthesisLane: run for added model
specTestSynthesisLane->>specTestSynthesisLane: extract SpecSource from schema.yml
specTestSynthesisLane->>runSpecTestGen: generateSpecTests(input)
runSpecTestGen->>LLM: stream prompt
LLM-->>runSpecTestGen: candidate tests JSON
runSpecTestGen->>runSpecTestGen: filterToSpecDerived
runSpecTestGen-->>specTestSynthesisLane: GeneratedTest[]
specTestSynthesisLane->>Findings: emit suggestion-level findings
Findings-->>Orchestrator: proposed test findings
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
packages/opencode/src/altimate/review/format.ts (1)
148-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the YAML merge helpers instead of using
any.The proposed-test patch helpers (
proposedTestsPatch,mergeTests,mergeColumns,pushUnique) are entirelyany-typed, which eliminates compile-time safety if the YAML document shape drifts. Consider defining a lightweight interface for the parsed YAML model/column structure to catch mismatches early.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/review/format.ts` around lines 148 - 198, The YAML merge helpers in proposedTestsPatch, mergeTests, mergeColumns, and pushUnique are using any throughout, which removes type safety for the parsed document shape. Introduce lightweight local interfaces for the parsed model and column structures, then update these helpers to use those types instead of any so the YAML parsing and merge logic in format.ts is compile-time checked.packages/opencode/test/altimate/review-spec-test-gen.test.ts (1)
153-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an assertion verifying the aggregated YAML patch appears in the rendered summary.
The e2e test at lines 180-182 checks that the summary contains the "### Proposed tests" section header but doesn't verify that the aggregated
yamlpatch (the committableschema.ymloutput fromproposedTestsPatch) is present. Adding an assertion likeexpect(summary).toContain("version: 2")orexpect(summary).toContain("not_null")within a yaml fence would close the loop on the formatting layer's patch aggregation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/altimate/review-spec-test-gen.test.ts` around lines 153 - 253, The summary assertion in the spec test synthesis suite only checks for the “### Proposed tests” section, but it does not verify that the aggregated YAML patch from proposedTestsPatch is actually rendered. Update the relevant test in review-spec-test-gen.test.ts, using renderSummary(env) and the existing spec_tests flow, to assert the summary includes the committable schema.yml YAML content (for example the version header or not_null content inside the fenced YAML block) so the formatting and patch aggregation path is covered end to end.packages/opencode/src/altimate/review/orchestrate.ts (3)
991-1007: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent failure on
generateSpecTestserrors.Unlike
runSpecTestGen's internal catches (whichlog.error), this catch swallows any error from the injectedgenerateSpecTestscall with no logging — a persistently failing LLM lane (bad model config, quota, etc.) would be invisible.♻️ Proposed fix
} catch { - return [] + log.error("spec-test synthesis lane failed", { model, file: ctx.file.path }) + return [] }(requires importing/creating a
Loginstance in this file, e.g.Log.create({ service: "orchestrate" }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/review/orchestrate.ts` around lines 991 - 1007, The generateSpecTests call in orchestrate.ts is swallowing all errors silently, so failing LLM/spec generation is invisible. Update the try/catch around input.generateSpecTests in the orchestration flow to log the caught error before returning an empty array, using a Log instance (for example, a module-level logger created for orchestrate) and keeping the existing control flow intact. Use the generateSpecTests and runSpecTestGen paths as the reference points for where the logging should be added.
818-887: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winModel-level dbt constraints aren't extracted.
schemaModelSourcesonly inspectscolumn.constraints/column.tests/column.data_tests. dbt also supports model-level constraints (e.g., compositeprimary_key, table-levelcheck), which are silently missed here, undercounting declared-but-unenforced constraints available as spec sources for Track B.♻️ Proposed addition
const contractEnforced = bool(record(node.contract)?.enforced) + for (const rawConstraint of array(node.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}:${type}`, + text: type, + args: constraint, + }) + } for (const rawColumn of array(node.columns)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/review/orchestrate.ts` around lines 818 - 887, schemaModelSources only extracts column-level tests and constraints, so model-level dbt constraints are missed. Update schemaModelSources to also inspect the model node itself for table-level/model-level constraints (for example primary_key and check definitions) and add them to the returned SpecSource[] using the same declared_constraint mapping approach. Use the existing schema.yml:${model} ref pattern and keep the logic alongside the current node.columns traversal so the function captures both column and model constraints.
889-904: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSchema.yml is re-fetched and re-parsed per added model.
changedSchemaSourcesre-reads and re-parses every changedschema.ymlviagetContent+YAML.parsefor each added model in the PR. For a PR adding several models across shared schema files, this repeats the same I/O and parse work redundantly.♻️ Proposed caching
-async function changedSchemaSources(model: string, input: OrchestrateInput): Promise<SpecSource[]> { +async function changedSchemaSources( + model: string, + input: OrchestrateInput, + parsedCache: Map<string, unknown>, +): Promise<SpecSource[]> { 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))) + if (!parsedCache.has(file.path)) { + const raw = await input.getContent(file.path, "new") + parsedCache.set(file.path, raw ? YAML.parse(raw) : undefined) + } + const doc = parsedCache.get(file.path) + if (doc !== undefined) sources.push(...schemaModelSources(model, doc)) } catch { // Broken or unavailable schema.yml should not crash an advisory lane. } } return sources }Pass a shared
parsedCachefromrunReviewinto eachspecTestSynthesisLane(ctx, input, dialect)call.Also applies to: 975-989
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/review/orchestrate.ts` around lines 889 - 904, The schema parsing work in changedSchemaSources is being repeated for every added model, causing the same schema.yml files to be re-fetched and re-parsed multiple times. Update runReview to create and pass a shared parsedCache through specTestSynthesisLane(ctx, input, dialect), then have changedSchemaSources reuse that cache instead of calling input.getContent and YAML.parse repeatedly for the same file. Keep the fix centered around changedSchemaSources and the specTestSynthesisLane call path so shared schema files are parsed once per PR.packages/opencode/src/altimate/review/spec-test-gen.ts (1)
264-295: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
dbtTest.testisn't validated againstkind.
normalizeDbtTestonly checks thattestis a non-empty string, andfilterToSpecDerivedonly validateskindagainstALLOWED_TEST_KINDS— nothing checks thatdbtTest.testactually corresponds tokind. An LLM proposal could carrykind: "not_null"(passes the guard) whiledbtTest.testnames an unrelated/arbitrary generic test, which then gets rendered verbatim into the committable schema.yml patch (proposedTestYamlin orchestrate.ts).♻️ Proposed tightening
+const KIND_TO_DBT_TEST_NAME: Record<GeneratedTestKind, string> = { + not_null: "not_null", + unique: "unique", + accepted_values: "accepted_values", + relationships: "relationships", + range: "", // advisory-only; no canonical generic test name +} + export function filterToSpecDerived( tests: GeneratedTest[], providedSources: SpecSource[], ): { kept: GeneratedTest[]; dropped: Array<{ test: GeneratedTest; reason: GuardDropReason }> } { ... + const canonical = KIND_TO_DBT_TEST_NAME[t.kind] + if (canonical && t.dbtTest && t.dbtTest.test !== canonical) { + dropped.push({ test: t, reason: "kind_not_allowed" }) + continue + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/review/spec-test-gen.ts` around lines 264 - 295, The generated dbt test payload currently accepts any non-empty dbtTest.test string, so validate that it matches the selected kind before keeping the test. Update normalizeDbtTest and/or parseGeneratedTests to cross-check dbtTest.test against the allowed test name for the GeneratedTestKind used in filterToSpecDerived, and drop or reject mismatches so arbitrary test names cannot flow into proposedTestYaml. Keep the existing symbols normalizeDbtTest, parseGeneratedTests, and GeneratedTestKind as the main integration points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/src/altimate/review/format.ts`:
- Around line 148-198: The YAML merge helpers in proposedTestsPatch, mergeTests,
mergeColumns, and pushUnique are using any throughout, which removes type safety
for the parsed document shape. Introduce lightweight local interfaces for the
parsed model and column structures, then update these helpers to use those types
instead of any so the YAML parsing and merge logic in format.ts is compile-time
checked.
In `@packages/opencode/src/altimate/review/orchestrate.ts`:
- Around line 991-1007: The generateSpecTests call in orchestrate.ts is
swallowing all errors silently, so failing LLM/spec generation is invisible.
Update the try/catch around input.generateSpecTests in the orchestration flow to
log the caught error before returning an empty array, using a Log instance (for
example, a module-level logger created for orchestrate) and keeping the existing
control flow intact. Use the generateSpecTests and runSpecTestGen paths as the
reference points for where the logging should be added.
- Around line 818-887: schemaModelSources only extracts column-level tests and
constraints, so model-level dbt constraints are missed. Update
schemaModelSources to also inspect the model node itself for
table-level/model-level constraints (for example primary_key and check
definitions) and add them to the returned SpecSource[] using the same
declared_constraint mapping approach. Use the existing schema.yml:${model} ref
pattern and keep the logic alongside the current node.columns traversal so the
function captures both column and model constraints.
- Around line 889-904: The schema parsing work in changedSchemaSources is being
repeated for every added model, causing the same schema.yml files to be
re-fetched and re-parsed multiple times. Update runReview to create and pass a
shared parsedCache through specTestSynthesisLane(ctx, input, dialect), then have
changedSchemaSources reuse that cache instead of calling input.getContent and
YAML.parse repeatedly for the same file. Keep the fix centered around
changedSchemaSources and the specTestSynthesisLane call path so shared schema
files are parsed once per PR.
In `@packages/opencode/src/altimate/review/spec-test-gen.ts`:
- Around line 264-295: The generated dbt test payload currently accepts any
non-empty dbtTest.test string, so validate that it matches the selected kind
before keeping the test. Update normalizeDbtTest and/or parseGeneratedTests to
cross-check dbtTest.test against the allowed test name for the GeneratedTestKind
used in filterToSpecDerived, and drop or reject mismatches so arbitrary test
names cannot flow into proposedTestYaml. Keep the existing symbols
normalizeDbtTest, parseGeneratedTests, and GeneratedTestKind as the main
integration points.
In `@packages/opencode/test/altimate/review-spec-test-gen.test.ts`:
- Around line 153-253: The summary assertion in the spec test synthesis suite
only checks for the “### Proposed tests” section, but it does not verify that
the aggregated YAML patch from proposedTestsPatch is actually rendered. Update
the relevant test in review-spec-test-gen.test.ts, using renderSummary(env) and
the existing spec_tests flow, to assert the summary includes the committable
schema.yml YAML content (for example the version header or not_null content
inside the fenced YAML block) so the formatting and patch aggregation path is
covered end to end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca950d82-bd97-49e1-b057-066e663ba885
📒 Files selected for processing (11)
packages/opencode/specs/corrective-app-memory.mdpackages/opencode/specs/greenfield-spec-test-synthesis.mdpackages/opencode/src/altimate/review/config.tspackages/opencode/src/altimate/review/format.tspackages/opencode/src/altimate/review/index.tspackages/opencode/src/altimate/review/orchestrate.tspackages/opencode/src/altimate/review/risk-tier.tspackages/opencode/src/altimate/review/run.tspackages/opencode/src/altimate/review/spec-test-gen.tspackages/opencode/src/altimate/review/verdict.tspackages/opencode/test/altimate/review-spec-test-gen.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 850a9c5d56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }) | ||
| } | ||
|
|
||
| for (const rawTest of [...array(column.tests), ...array(column.data_tests)]) { |
There was a problem hiding this comment.
When an added model already includes schema tests in the same PR, this loop turns those existing tests/data_tests entries into spec sources for the proposal LLM, so the review can emit a patch suggesting the author add not_null/accepted_values tests that are already present in schema.yml. For greenfield models where the author correctly added tests alongside the SQL, this creates duplicate proposed-test noise instead of only suggesting missing coverage; filter out already-enforcing tests or keep them out of the proposal sources.
Useful? React with 👍 / 👎.
| column, | ||
| confidence: "unknown", | ||
| evidence: { tool: "altimate.spec_test.proposed", result: { proposal: test, yaml } }, | ||
| ruleKey: "spec_test:" + test.derivedFrom.ref, |
There was a problem hiding this comment.
For multiple proposals derived from the same source ref and column, this rule key makes all of them share the same finding fingerprint because dedupe() keys on category/file/model/column/ruleKey. A single column description or PR-intent source can legitimately yield more than one candidate test, such as not_null plus range, but only one survives the final dedupe; include test.id, kind, or the dbt test spec in the rule key so distinct proposals are not silently dropped.
Useful? React with 👍 / 👎.
| function proposedTestsPatch(findings: Finding[]): string { | ||
| const merged = new Map<string, any>() | ||
| for (const f of findings) { | ||
| for (const yaml of yamlFences(f.body)) { |
There was a problem hiding this comment.
When the LLM-controlled rationale contains its own fenced yaml block, scanning every YAML fence in f.body lets that extra block be parsed and merged into the aggregate proposed-tests patch without passing filterToSpecDerived. The scenario only requires a generated rationale like yaml\nmodels: ...\n, because proposedTestBody appends the rationale after the trusted snippet; use evidence.result.yaml or otherwise select only the locally generated YAML instead of reparsing the whole markdown body.
Useful? React with 👍 / 👎.
| if (!ALLOWED_TEST_KINDS.has(t.kind)) { | ||
| dropped.push({ test: t, reason: "kind_not_allowed" }) | ||
| continue |
There was a problem hiding this comment.
When a generated item uses an allowed kind but sets dbtTest.test to a different or arbitrary generic test macro, this guard still keeps it because it only validates t.kind and the cited ref; proposedTestYaml later writes dbtTest.test directly into the suggested schema patch. That lets malformed or prompt-injected LLM output propose tests outside the advertised allowlist, so the guard should require dbtTest.test to match the allowed kind/test mapping before accepting the proposal.
Useful? React with 👍 / 👎.
| tests: GeneratedTest[], | ||
| providedSources: SpecSource[], | ||
| ): { kept: GeneratedTest[]; dropped: Array<{ test: GeneratedTest; reason: GuardDropReason }> } { | ||
| const providedRefs = new Set(providedSources.map((s) => s.ref)) |
There was a problem hiding this comment.
When a proposal cites a real ref but changes the accompanying derivedFrom.origin or kind, this check still accepts the LLM-supplied object because providedRefs tracks only strings. The kept proposal then carries forged source metadata in evidence, and helpers like isBlockEligible() can misclassify an inferred-context proposal as a declared constraint; compare against the full provided source or replace t.derivedFrom with the trusted extracted source before returning it.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/review/run.ts">
<violation number="1" location="packages/opencode/src/altimate/review/run.ts:137">
P2: The `generateSpecTests` lane is gated on the same `noAi || config.ai === false` check as the AI reviewer, but Track A of spec-test generation is purely deterministic (parsed dbt constraints → test proposals, no LLM). When AI is disabled, only Track B (LLM-based inferred_context proposals) should be skipped; Track A should still run so deterministic constraint-materialization tests are available.</violation>
</file>
<file name="packages/opencode/src/altimate/review/format.ts">
<violation number="1" location="packages/opencode/src/altimate/review/format.ts:196">
P3: The aggregate schema.yml patch can include duplicate candidate tests when equivalent YAML mappings arrive with different key order. Using an order-stable key for parsed YAML values would keep the proposed patch idempotent.</violation>
</file>
<file name="packages/opencode/src/altimate/review/spec-test-gen.ts">
<violation number="1" location="packages/opencode/src/altimate/review/spec-test-gen.ts:351">
P2: Spec-test proposals can pass the anti-fabrication guard while citing a source the LLM was never given when `specSources` exceeds `MAX_SPEC_SOURCES`. The guard should validate against the same sliced source list used in `buildUserMessage` so omitted refs are not accepted as grounded.</violation>
</file>
<file name="packages/opencode/src/altimate/review/orchestrate.ts">
<violation number="1" location="packages/opencode/src/altimate/review/orchestrate.ts:764">
P2: Greenfield models with declared `primary_key` or `foreign_key` contract constraints will not get spec-test proposals from those declarations because the source-kind allowlist omits dbt's contract constraint names. Consider accepting `primary_key`/`foreign_key` here and mapping them to the appropriate proposed `unique`/`relationships` tests.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| modelVersion: opts.modelVersion, | ||
| coreVersion: opts.coreVersion, | ||
| aiReview: opts.noAi || config.ai === false ? undefined : runAiReview, | ||
| generateSpecTests: opts.noAi || config.ai === false ? undefined : runSpecTestGen, |
There was a problem hiding this comment.
P2: The generateSpecTests lane is gated on the same noAi || config.ai === false check as the AI reviewer, but Track A of spec-test generation is purely deterministic (parsed dbt constraints → test proposals, no LLM). When AI is disabled, only Track B (LLM-based inferred_context proposals) should be skipped; Track A should still run so deterministic constraint-materialization tests are available.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/review/run.ts, line 137:
<comment>The `generateSpecTests` lane is gated on the same `noAi || config.ai === false` check as the AI reviewer, but Track A of spec-test generation is purely deterministic (parsed dbt constraints → test proposals, no LLM). When AI is disabled, only Track B (LLM-based inferred_context proposals) should be skipped; Track A should still run so deterministic constraint-materialization tests are available.</comment>
<file context>
@@ -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,
</file context>
| if (!text) return [] | ||
|
|
||
| const tests = parseGeneratedTests(text) | ||
| const { kept } = filterToSpecDerived(tests, input.specSources) |
There was a problem hiding this comment.
P2: Spec-test proposals can pass the anti-fabrication guard while citing a source the LLM was never given when specSources exceeds MAX_SPEC_SOURCES. The guard should validate against the same sliced source list used in buildUserMessage so omitted refs are not accepted as grounded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/review/spec-test-gen.ts, line 351:
<comment>Spec-test proposals can pass the anti-fabrication guard while citing a source the LLM was never given when `specSources` exceeds `MAX_SPEC_SOURCES`. The guard should validate against the same sliced source list used in `buildUserMessage` so omitted refs are not accepted as grounded.</comment>
<file context>
@@ -0,0 +1,359 @@
+ if (!text) return []
+
+ const tests = parseGeneratedTests(text)
+ const { kept } = filterToSpecDerived(tests, input.specSources)
+ return kept
+ } catch (err) {
</file context>
| const { kept } = filterToSpecDerived(tests, input.specSources) | |
| const { kept } = filterToSpecDerived(tests, input.specSources.slice(0, MAX_SPEC_SOURCES)) |
| ] | ||
| } | ||
|
|
||
| const DECLARED_SPEC_SOURCE_KINDS = new Set(["not_null", "unique", "accepted_values", "relationships"]) |
There was a problem hiding this comment.
P2: Greenfield models with declared primary_key or foreign_key contract constraints will not get spec-test proposals from those declarations because the source-kind allowlist omits dbt's contract constraint names. Consider accepting primary_key/foreign_key here and mapping them to the appropriate proposed unique/relationships tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/review/orchestrate.ts, line 764:
<comment>Greenfield models with declared `primary_key` or `foreign_key` contract constraints will not get spec-test proposals from those declarations because the source-kind allowlist omits dbt's contract constraint names. Consider accepting `primary_key`/`foreign_key` here and mapping them to the appropriate proposed `unique`/`relationships` tests.</comment>
<file context>
@@ -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<string, unknown> | undefined {
</file context>
| } | ||
|
|
||
| function pushUnique(items: any[], value: any): void { | ||
| const key = JSON.stringify(value) |
There was a problem hiding this comment.
P3: The aggregate schema.yml patch can include duplicate candidate tests when equivalent YAML mappings arrive with different key order. Using an order-stable key for parsed YAML values would keep the proposed patch idempotent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/review/format.ts, line 196:
<comment>The aggregate schema.yml patch can include duplicate candidate tests when equivalent YAML mappings arrive with different key order. Using an order-stable key for parsed YAML values would keep the proposed patch idempotent.</comment>
<file context>
@@ -111,3 +115,84 @@ function capitalize(s: string): string {
+}
+
+function pushUnique(items: any[], value: any): void {
+ const key = JSON.stringify(value)
+ if (!items.some((item) => JSON.stringify(item) === key)) items.push(value)
+}
</file context>
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 8 finding(s)
- 8 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/altimate/review/format.ts (L195-L198)
[🟠 MEDIUM] Using JSON.stringify to determine object uniqueness is fragile as it is sensitive to object key ordering (e.g., {"a": 1, "b": 2} vs {"b": 2, "a": 1}). Furthermore, repeatedly stringifying item inside the .some loop introduces unnecessary computational overhead.
Consider using a deep equality function (like isDeepEqual from remeda which seems available in the project) to ensure robust and efficient deduplication.
Suggested change:
function pushUnique(items: any[], value: any): void {
// If `isDeepEqual` from "remeda" is available:
// if (!items.some((item) => isDeepEqual(item, value))) items.push(value)
const key = JSON.stringify(value)
if (!items.some((item) => JSON.stringify(item) === key)) items.push(value)
}
2. packages/opencode/src/altimate/review/format.ts (L154-L161)
[🟠 MEDIUM] When merging parsed YAML content, the current logic strictly constructs new objects and copies only the name, tests, and columns properties, effectively ignoring other valid DBT properties (like description, data_type, or meta).
If the proposed test YAML snippets include other valid properties, they will be silently excluded from the final aggregated patch. Consider preserving the rest of the properties by spreading the original object, e.g., const target = merged.get(modelName) ?? { ...rawModel, tests: [], columns: [] } (and similar in mergeColumns).
Suggested change:
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) ?? { ...rawModel, tests: [], columns: [] }
mergeTests(target, rawModel)
mergeColumns(target, rawModel)
merged.set(modelName, target)
}
3. packages/opencode/src/altimate/review/format.ts (L184-L188)
[🟠 MEDIUM] Similar to the models merging logic, constructing a new object with only the name property drops other valid column-level properties (e.g., description, data_type) present in the proposed tests.
Consider spreading the rawColumn properties when creating a new column entry.
Suggested change:
let column = target.columns.find((c: any) => c.name === name)
if (!column) {
column = { ...rawColumn, tests: [] }
target.columns.push(column)
}
4. packages/opencode/src/altimate/review/format.ts (L172-L176)
[🔵 LOW] Widespread use of the any type (e.g., target: any, source: any, Map<string, any>) bypasses static type checking, violating TypeScript best practices.
This obscures the expected data shape for YAML parsing results and model structures, reducing maintainability and increasing the risk of runtime errors if schemas change. Consider defining and using explicit interfaces (e.g., for DBT models and columns) instead of relying on any.
5. packages/opencode/src/altimate/review/orchestrate.ts (L910-L913)
[🟠 MEDIUM] The regular expressions /\\bref\\s*\\(([^)]*)\\)/g and /\\bsource\\s*\\(([^)]*)\\)/g will match ref() and source() calls even if they are inside single-line (--) or block (/* ... */) SQL comments. This could result in hallucinated spec tests based on commented-out code. Consider stripping comments before running the regex matches.
You could define a local helper or import one if available:
function stripComments(s: string): string {
return s.replace(/\/\*[\s\S]*?\*\//g, " ").replace(/--[^\n]*/g, " ")
}6. packages/opencode/src/altimate/review/orchestrate.ts (L896-L901)
[🔵 LOW] Swallowing the YAML.parse exception completely makes debugging difficult when users expect tests to be generated but they aren't (due to a malformed schema.yml).
Consider capturing the error and logging it with structured logging (e.g., log.warn, ensuring you don't use console.* to avoid TUI corruption).
} catch (err) {
log.warn(`Failed to parse schema.yml from ${file.path}`, { error: err })
// Broken or unavailable schema.yml should not crash an advisory lane.
}7. packages/opencode/src/altimate/review/spec-test-gen.ts (L162-L190)
[🔴 HIGH] High Severity: LLM Hallucination bypasses block eligibility check
The parseGeneratedTests function directly casts and trusts the derivedFrom object from the LLM's JSON output. In filterToSpecDerived, you only check if the ref exists in providedSources, but you don't replace the LLM's object with the trusted source.
If the LLM hallucinates and sets derivedFrom.origin to "declared_constraint" for an inferred context source, isBlockEligible will evaluate to true. This bypasses the strict architectural separation between Track A and Track B, allowing an advisory-only LLM proposal to wrongly block a PR.
To fix this, overwrite the LLM's derivedFrom object with the trusted SpecSource from providedSources.
export function filterToSpecDerived(
tests: GeneratedTest[],
providedSources: SpecSource[],
): { kept: GeneratedTest[]; dropped: Array<{ test: GeneratedTest; reason: GuardDropReason }> } {
const providedMap = new Map(providedSources.map((s) => [s.ref, s]))
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
}
const trustedSource = providedMap.get(df.ref)
if (!trustedSource) {
dropped.push({ test: t, reason: "ref_not_provided" })
continue
}
t.derivedFrom = trustedSource
kept.push(t)
}
return { kept, dropped }
}8. packages/opencode/src/altimate/review/spec-test-gen.ts (L253-L257)
[🟠 MEDIUM] Medium Severity: Strict Regex for JSON Extraction
The regex /^```(?:json)?\s*([\s\S]*?)\s*```$/i uses strict start (^) and end ($) boundary anchors. If the LLM output contains any conversational text before or after the markdown JSON block (e.g., "Here are the tests:\n```json..."), the regex will fail to match.
This forces JSON.parse to evaluate the raw text, resulting in a SyntaxError that silently fails the entire generation process. Removing the anchors ensures the regex is resilient to LLM chatter.
Suggested change:
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 pushUnique(items: any[], value: any): void { | ||
| const key = JSON.stringify(value) | ||
| if (!items.some((item) => JSON.stringify(item) === key)) items.push(value) | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] Using JSON.stringify to determine object uniqueness is fragile as it is sensitive to object key ordering (e.g., {"a": 1, "b": 2} vs {"b": 2, "a": 1}). Furthermore, repeatedly stringifying item inside the .some loop introduces unnecessary computational overhead.
Consider using a deep equality function (like isDeepEqual from remeda which seems available in the project) to ensure robust and efficient deduplication.
Suggested change:
| function pushUnique(items: any[], value: any): void { | |
| const key = JSON.stringify(value) | |
| if (!items.some((item) => JSON.stringify(item) === key)) items.push(value) | |
| } | |
| function pushUnique(items: any[], value: any): void { | |
| // If `isDeepEqual` from "remeda" is available: | |
| // if (!items.some((item) => isDeepEqual(item, value))) items.push(value) | |
| const key = JSON.stringify(value) | |
| if (!items.some((item) => JSON.stringify(item) === key)) items.push(value) | |
| } |
| 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) | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] When merging parsed YAML content, the current logic strictly constructs new objects and copies only the name, tests, and columns properties, effectively ignoring other valid DBT properties (like description, data_type, or meta).
If the proposed test YAML snippets include other valid properties, they will be silently excluded from the final aggregated patch. Consider preserving the rest of the properties by spreading the original object, e.g., const target = merged.get(modelName) ?? { ...rawModel, tests: [], columns: [] } (and similar in mergeColumns).
Suggested change:
| 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) | |
| } | |
| 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) ?? { ...rawModel, tests: [], columns: [] } | |
| mergeTests(target, rawModel) | |
| mergeColumns(target, rawModel) | |
| merged.set(modelName, target) | |
| } |
| let column = target.columns.find((c: any) => c.name === name) | ||
| if (!column) { | ||
| column = { name } | ||
| target.columns.push(column) | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] Similar to the models merging logic, constructing a new object with only the name property drops other valid column-level properties (e.g., description, data_type) present in the proposed tests.
Consider spreading the rawColumn properties when creating a new column entry.
Suggested change:
| let column = target.columns.find((c: any) => c.name === name) | |
| if (!column) { | |
| column = { name } | |
| target.columns.push(column) | |
| } | |
| let column = target.columns.find((c: any) => c.name === name) | |
| if (!column) { | |
| column = { ...rawColumn, tests: [] } | |
| target.columns.push(column) | |
| } |
| 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) | ||
| } |
There was a problem hiding this comment.
[🔵 LOW] Widespread use of the any type (e.g., target: any, source: any, Map<string, any>) bypasses static type checking, violating TypeScript best practices.
This obscures the expected data shape for YAML parsing results and model structures, reducing maintainability and increasing the risk of runtime errors if schemas change. Consider defining and using explicit interfaces (e.g., for DBT models and columns) instead of relying on any.
| function sqlIntentSources(sql: string | undefined): SpecSource[] { | ||
| if (!sql) return [] | ||
| const out: SpecSource[] = [] | ||
| for (const match of sql.matchAll(/\bref\s*\(([^)]*)\)/g)) { |
There was a problem hiding this comment.
[🟠 MEDIUM] The regular expressions /\\bref\\s*\\(([^)]*)\\)/g and /\\bsource\\s*\\(([^)]*)\\)/g will match ref() and source() calls even if they are inside single-line (--) or block (/* ... */) SQL comments. This could result in hallucinated spec tests based on commented-out code. Consider stripping comments before running the regex matches.
You could define a local helper or import one if available:
function stripComments(s: string): string {
return s.replace(/\/\*[\s\S]*?\*\//g, " ").replace(/--[^\n]*/g, " ")
}| 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. | ||
| } |
There was a problem hiding this comment.
[🔵 LOW] Swallowing the YAML.parse exception completely makes debugging difficult when users expect tests to be generated but they aren't (due to a malformed schema.yml).
Consider capturing the error and logging it with structured logging (e.g., log.warn, ensuring you don't use console.* to avoid TUI corruption).
} catch (err) {
log.warn(`Failed to parse schema.yml from ${file.path}`, { error: err })
// Broken or unavailable schema.yml should not crash an advisory lane.
}| 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 } | ||
| } |
There was a problem hiding this comment.
[🔴 HIGH] High Severity: LLM Hallucination bypasses block eligibility check
The parseGeneratedTests function directly casts and trusts the derivedFrom object from the LLM's JSON output. In filterToSpecDerived, you only check if the ref exists in providedSources, but you don't replace the LLM's object with the trusted source.
If the LLM hallucinates and sets derivedFrom.origin to "declared_constraint" for an inferred context source, isBlockEligible will evaluate to true. This bypasses the strict architectural separation between Track A and Track B, allowing an advisory-only LLM proposal to wrongly block a PR.
To fix this, overwrite the LLM's derivedFrom object with the trusted SpecSource from providedSources.
export function filterToSpecDerived(
tests: GeneratedTest[],
providedSources: SpecSource[],
): { kept: GeneratedTest[]; dropped: Array<{ test: GeneratedTest; reason: GuardDropReason }> } {
const providedMap = new Map(providedSources.map((s) => [s.ref, s]))
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
}
const trustedSource = providedMap.get(df.ref)
if (!trustedSource) {
dropped.push({ test: t, reason: "ref_not_provided" })
continue
}
t.derivedFrom = trustedSource
kept.push(t)
}
return { kept, dropped }
}| 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 | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] Medium Severity: Strict Regex for JSON Extraction
The regex /^```(?:json)?\s*([\s\S]*?)\s*```$/i uses strict start (^) and end ($) boundary anchors. If the LLM output contains any conversational text before or after the markdown JSON block (e.g., "Here are the tests:\n```json..."), the regex will fail to match.
This forces JSON.parse to evaluate the raw text, resulting in a SyntaxError that silently fails the entire generation process. Removing the anchors ensures the regex is resilient to LLM chatter.
Suggested change:
| 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 stripJsonFence(text: string): string { | |
| const trimmed = text.trim() | |
| const fenced = /```(?:json)?\s*([\s\S]*?)\s*```/i.exec(trimmed) | |
| return fenced ? fenced[1].trim() : trimmed | |
| } |
🤖 Code Review — OpenCodeReview (Gemini) — 8 finding(s)
All findings (full text)1.
|
|
Superseded by #991, which consolidates the propose-only P0 with track-A execution, the Track-B SQL sandbox, the compiled-core prompt move, P2 corrective app-memory, and all the bot-review fixes from this PR. Closing in favor of reviewing the full feature in one place. |
2 participants
PINEAPPLE
Summary
Adds greenfield spec-test synthesis to
dbt-pr-review(P0: propose-only). Closes #989.For a brand-new (git-added) dbt model — which has no base to diff against, so the equivalence/data-diff lanes can't verify it — an auxiliary lane generates candidate tests from the model's declared intent and surfaces them as a committable
schema.ymlpatch. This raises the no-reference verification ceiling thatmissingGrainTestLanepreviously only flagged.Two strictly separated tracks (design:
packages/opencode/specs/greenfield-spec-test-synthesis.md):schema.ymlnot_null/unique/accepted_values/relationships, contract types) — no LLM in the (future) blocking path, so it can't be tricked into a back-labeled false positive.ref()edges, PR text).P0 is propose-only and cannot block a PR: every finding is emitted at
severity: suggestion+confidence: unknownunder thealtimate.spec_test.proposedevidence tool, which is excluded from the verdict warning-count (mirrors theai-reviewexclusion). Execution + the deterministic blocking path (Track A) land in P1.Test Plan
review.test.ts,review-dbt-patterns.test.ts, and the newreview-spec-test-gen.test.ts— covering added-only behavior, non-blocking verdict in bothcommentandgatemode, fabricated-ref dropping (filterToSpecDerived), and modified-only silence.verdict.tsexcludes the proposed evidence tool from the warning-count so proposals can never reachREQUEST_CHANGES.bun run script/upstream/analyze.ts --markers --base main --strict— clean.Checklist
packages/opencode/specs/)Summary by cubic
Adds propose‑only greenfield spec‑test synthesis to
dbt-pr-reviewto generate grounded candidate dbt tests for newly added models and output a committableschema.ymlpatch without affecting blocking verdicts. Also wires verdict guards and a newspec_testslane into lite/full tiers.spec_testslane runs only for added models; two tracks: deterministic from declared constraints and advisory from inferred context; emits suggestions only when AI is enabled.altimate.spec_test.proposedwithconfidence: unknown.altimate.spec_test.proposedfrom warning accumulation to ensure proposals never block.specTests.execute(defaultfalse); execution comes later in P1.generateSpecTests; guardfilterToSpecDeriveddrops ungrounded/fabricated refs; deterministic test IDs prevent misattribution.spec_testsinliteandfull.Written for commit 850a9c5. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests