feat: greenfield spec-test synthesis for dbt-pr-review (P0–P2)#991
feat: greenfield spec-test synthesis for dbt-pr-review (P0–P2)#991anandgupta42 wants to merge 9 commits 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>
A spec-test finding may only reach a blocking verdict when it was actually EXECUTED against a constraint the author DECLARED (evidence executed:true + origin:declared_constraint). Advisory/unexecuted/inferred spec-test findings can never force REQUEST_CHANGES, regardless of the severity the lane assigned — defense in depth so the verdict never trusts the lane. Non-spec-test findings are unaffected. Refs #989 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…straints
Adds the deterministic blocking path for greenfield spec-test synthesis:
- runner: declaredConstraints(model) reads the manifest for declared
not_null/unique/accepted_values/relationships + contract types with
provenance and hasEnforcingTest; runGeneratedTests(tests, warehouse?) executes
bounded violating-row checks via the dispatcher, id-keyed, null when no warehouse.
- orchestrate: materialize only declared-but-UNENFORCED constraints (no LLM) into
GeneratedTests; when config.specTests.execute is set and a warehouse is present,
execute them. An executed failure emits a critical contract_violation carrying
evidence { executed: true, origin: declared_constraint }, the only shape
specTestMayBlock lets block. No warehouse / execute off → non-blocking proposals.
- Track B (LLM proposals) stays propose-only and is NEVER executed.
- verdict: summary.enforcedConstraints (executed/passed/failed); format: Spec coverage.
145 review tests pass; the sole blocking path is deterministic end to end.
Refs #989
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…execution Review findings (#990): - Include test identity (ref:kind:column) in spec-test ruleKey so distinct proposals from one source no longer collapse in dedupe(). - filterToSpecDerived replaces derivedFrom with the trusted extracted source (matched by ref), so a forged origin/kind can't survive. - Guard requires dbtTest.test === kind (drop test_mismatch), blocking arbitrary macros from reaching the proposed patch. - Build the committable schema.yml patch from evidence.result.yaml, not by reparsing the LLM-influenced finding body. - Don't turn already-enforcing schema.yml tests into proposal sources. - Track A (deterministic) now runs even when AI is disabled: the lane is gated on (generateSpecTests || runner.declaredConstraints), not the LLM injection. Track-B execution + sandbox: - New spec-test-sandbox.ts sanitizeAssertionSql: single SELECT only (state-machine scan), no DDL/DML/side-effects, CTE-aware relation allowlist (model + declared upstreams), wrapped as a bounded count(*). Rejected SQL is dropped, never run. - Executed track-B failures emit warning/unknown under altimate.spec_test.candidate (category test_coverage) — never critical, never blocking (also excluded from the verdict warning-count). Only track-A executed+declared can block. 156 review tests pass; verdict gate specTestMayBlock unchanged. Refs #989 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…line fallback Moves the greenfield spec-test track-B generation prompt toward the IP-in-binary pattern used by the AI reviewer: - native/types.ts + native/altimate-core.ts: register altimate_core.review_spec_test_prompt, calling core.reviewSpecTestSystemPrompt() via a defensive cast so it typechecks before the addon regenerates the binding. - spec-test-gen.ts resolveSystemPrompt(): fetch the prompt from core, falling back to the inline buildSystemPrompt() when the running addon predates the method — no runtime regression until the core ships it. - specs/core-spec-test-prompt.md: the Rust const + napi export to apply on a clean altimate-core-internal branch (the local core checkout had unrelated WIP), plus rebuild/activation notes. Refs #989 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Structured, attribute-keyed corrective memory (corrective-app-memory.md), read-path only: - corrective-memory.ts: MemoryScope/MemoryEntry + pure getMemory() retrieval (wildcard match, most-specific-match-wins, project as hard isolation boundary). - config.ts: seeded memory.entries loaded from .altimate/review.yml. - Two consumers: (1) bias track-B generation via SpecTestGenInput.priors and deterministically drop suppressed track-B proposal kinds; (2) reviewer post-filter drops advisory findings matched by a suppress entry. - SAFETY (enforced): memoryMaySuppressFinding never suppresses a critical or the altimate.spec_test.executed (block-eligible) path; memory can only REMOVE advisory findings / bias generation, never add or escalate — so it cannot change a verdict toward REQUEST_CHANGES. Track A (declared constraints) is never memory-suppressed. 162 review tests pass. Write-path/decay deferred to a later phase. Refs #989 Co-Authored-By: Claude Opus 4.8 <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.
📝 WalkthroughWalkthroughAdds greenfield spec-test generation and execution for dbt reviews, with core prompt bridging, corrective-memory suppression, verdict gating, summary rendering, and tests covering the new flows. ChangesSpec-test synthesis and corrective memory
Sequence Diagram(s)sequenceDiagram
participant runReview
participant runSpecTestGen
participant altimate_core_review_spec_test_prompt
participant LLM
runReview->>runSpecTestGen: generateSpecTests(specSources)
runSpecTestGen->>altimate_core_review_spec_test_prompt: resolveSystemPrompt()
altimate_core_review_spec_test_prompt-->>runSpecTestGen: prompt or fallback
runSpecTestGen->>LLM: stream request
LLM-->>runSpecTestGen: JSON proposed tests
runSpecTestGen->>runSpecTestGen: filterToSpecDerived(tests)
runSpecTestGen-->>runReview: GeneratedTest[]
sequenceDiagram
participant specTestSynthesisLane
participant ReviewRunner
participant sanitizeAssertionSql
participant getMemory
participant buildEnvelope
specTestSynthesisLane->>ReviewRunner: declaredConstraints(model)
ReviewRunner-->>specTestSynthesisLane: DeclaredConstraint[]
specTestSynthesisLane->>ReviewRunner: runGeneratedTests(tests, warehouse, options)
ReviewRunner-->>specTestSynthesisLane: pass/fail results
specTestSynthesisLane->>sanitizeAssertionSql: validate assertionSql
sanitizeAssertionSql-->>specTestSynthesisLane: sanitized SQL or reject
specTestSynthesisLane->>getMemory: getMemory(entries, query)
getMemory-->>specTestSynthesisLane: suppression entries
specTestSynthesisLane->>buildEnvelope: findings + enforcedConstraints
buildEnvelope-->>specTestSynthesisLane: VerdictEnvelope
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
packages/opencode/test/altimate/review-spec-test-sandbox.test.ts (1)
4-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd CTE/WITH and comment-stripping coverage for the SQL sandbox.
The four tests cover the core rejection paths (side-effect, multi-statement, unknown relation) and a basic acceptance case. However, the upstream
sanitizeAssertionSqlexplicitly handles CTE/WITH queries (/^\s*with\b[\s\S]*\bselect\b/i) and strips strings/comments viawithoutStringsAndCommentsbefore side-effect detection — neither path is exercised by these tests. Since this is a security-critical function guarding Track B execution, adding coverage for these cases would strengthen confidence.🧪 Suggested additional sandbox tests
test("accepts and bounds a select on allowlisted relations", () => { const sanitized = sanitizeAssertionSql("select order_id from fct_orders where order_id is null;", ["fct_orders"]) expect(sanitized.ok).toBe(true) if (sanitized.ok) { expect(sanitized.sql).toBe( "select count(*) as n from ( select order_id from fct_orders where order_id is null ) _s", ) } }) + + test("accepts a WITH/CTE query referencing allowlisted relations", () => { + const sanitized = sanitizeAssertionSql( + "with cte as (select order_id from fct_orders) select * from cte where order_id is null", + ["fct_orders"], + ) + expect(sanitized.ok).toBe(true) + if (sanitized.ok) { + expect(sanitized.sql).toContain("count(*) as n") + } + }) + + test("does not flag side-effect keywords inside string literals", () => { + const sanitized = sanitizeAssertionSql( + "select 'drop table evil' as msg from fct_orders", + ["fct_orders"], + ) + expect(sanitized.ok).toBe(true) + }) + + test("does not flag side-effect keywords inside comments", () => { + const sanitized = sanitizeAssertionSql( + "select order_id from fct_orders /* drop table evil */", + ["fct_orders"], + ) + expect(sanitized.ok).toBe(true) + }) })🤖 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-sandbox.test.ts` around lines 4 - 32, Add coverage in sanitizeAssertionSql for the missing CTE/WITH and comment-stripping paths. Extend the review-spec-test-sandbox tests to verify that a WITH query containing a SELECT is accepted and sanitized correctly, and that SQL comments are ignored by the side-effect detection in withoutStringsAndComments. Keep the existing sanitizeAssertionSql assertions pattern and place the new cases alongside the current rejection/acceptance tests.
🤖 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.
Inline comments:
In `@packages/opencode/src/altimate/review/orchestrate.ts`:
- Around line 1106-1123: materializeDeclaredConstraints is currently emitting
DeclaredConstraintKind.column_type as an executable dbt test, but the
runner-side SQL generation path does not support that kind and drops the
resulting error in execute mode. Update materializeDeclaredConstraints (and the
corresponding generated-test SQL/rendering path it feeds) to either add explicit
column_type support end-to-end or skip materializing column_type into
GeneratedTest until it can be represented safely. Make sure the handling is
consistent with the other declared constraint kinds so unsupported tests are not
silently lost.
- Around line 249-253: The project selection in memoryProjectForInput is
incorrectly falling back to the only configured project even when input.project
is explicitly set to a different repo. Update memoryProjectForInput so that when
input.project exists it always returns that value as the query key, and only use
the single configured project fallback when input.project is absent; keep the
existing configuredMemoryEntries and scope.project checks to locate the affected
logic.
In `@packages/opencode/src/altimate/review/runner.ts`:
- Around line 660-669: The declaredConstraints()/buildGeneratedTestSql() flow is
emitting `column_type` as an executable constraint even though
`buildGeneratedTestSql()` does not support it, which causes contract-type
violations to be dropped instead of enforced. Update the `runner.ts` handling
around `declaredConstraints()` and `buildGeneratedTestSql()` so `column_type` is
either fully implemented as an executable SQL test or explicitly excluded from
the set of constraints that are treated as executable. Make sure the
`DeclaredConstraint["kind"]` branch for `column_type` stays aligned with
whatever `buildGeneratedTestSql()` can actually generate.
- Around line 701-716: The constraint normalization in runner.ts currently
treats table-level primary_key the same as unique, so null key values can still
slip through. Update the constraint handling loop in the constraint collection
logic to detect primary_key separately from unique, and when a primary_key is
found, add the existing unique declaration plus not_null checks for each column
in the key. Use the existing helpers around add(), constraintKey(), and
sourceRef() so the new not_null entries are generated alongside the primary_key
constraint.
- Around line 305-309: The withTimeout helper leaves the fallback setTimeout
running even when the raced promise settles first, which can keep the CLI alive
unnecessarily. Update withTimeout in runner.ts to retain the timer handle, and
clear it as soon as the input promise settles (success or failure) while
preserving the existing timeout error behavior from the Promise.race path.
- Around line 270-280: The relationships branch in runner.ts is building
executable SQL directly from dbtTest args["to"], which can bypass
sanitizeAssertionSql and allow unsafe generated relation targets to reach
sql.execute when specTests.execute is enabled. Update the logic in relationships
handling and relationshipTarget usage so the target relation is validated and
sanitized against the manifest before being interpolated, rejecting anything
outside the expected relation format or any quoted/multi-statement input. Keep
the fix localized to the test SQL construction path in runner.ts, especially the
relationships test branch and relationshipTarget resolution.
In `@packages/opencode/src/altimate/review/spec-test-sandbox.ts`:
- Around line 211-212: The relation allowlist check in
relationAliases/relationAllows currently permits basename fallback, so qualified
references can match too loosely. Update the sandbox validation in
spec-test-sandbox.ts so exact allowlist matching is required for qualified
relation references, while still allowing unqualified basenames only when the
reference itself is unqualified. Use the existing relationAliases and
allowed-set check to distinguish fully qualified names from basename aliases and
reject cases like other_schema.fct_orders unless that exact qualified form is
explicitly allowlisted.
In `@packages/opencode/src/altimate/review/verdict.ts`:
- Around line 60-65: The blocking gate in specTestMayBlock should only accept
findings from the exact executed spec-test tool, not any altimate.spec_test*
prefix. Update the check in specTestMayBlock to require the tool name to be
altimate.spec_test.executed explicitly before allowing the executed/origin
result fields to block, so malformed proposed or candidate findings cannot spoof
the result object. Keep the existing result validation in place, but tighten the
tool identity check used by the Finding evidence.
---
Nitpick comments:
In `@packages/opencode/test/altimate/review-spec-test-sandbox.test.ts`:
- Around line 4-32: Add coverage in sanitizeAssertionSql for the missing
CTE/WITH and comment-stripping paths. Extend the review-spec-test-sandbox tests
to verify that a WITH query containing a SELECT is accepted and sanitized
correctly, and that SQL comments are ignored by the side-effect detection in
withoutStringsAndComments. Keep the existing sanitizeAssertionSql assertions
pattern and place the new cases alongside the current rejection/acceptance
tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 19e062d7-f949-4fe0-8624-6539080daf60
📒 Files selected for processing (21)
packages/opencode/specs/core-spec-test-prompt.mdpackages/opencode/specs/corrective-app-memory.mdpackages/opencode/specs/greenfield-spec-test-synthesis.mdpackages/opencode/src/altimate/native/altimate-core.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/review/config.tspackages/opencode/src/altimate/review/corrective-memory.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/runner.tspackages/opencode/src/altimate/review/spec-test-gen.tspackages/opencode/src/altimate/review/spec-test-sandbox.tspackages/opencode/src/altimate/review/verdict.tspackages/opencode/test/altimate/review-corrective-memory.test.tspackages/opencode/test/altimate/review-spec-test-exec.test.tspackages/opencode/test/altimate/review-spec-test-gen.test.tspackages/opencode/test/altimate/review-spec-test-sandbox.test.tspackages/opencode/test/altimate/review-spec-test-verdict.test.ts
| if (dataType) { | ||
| const kind: DeclaredConstraint["kind"] = "column_type" | ||
| add({ | ||
| kind, | ||
| column, | ||
| args: { data_type: dataType }, | ||
| hasEnforcingTest: enforcing.has(constraintKey(kind, column)), | ||
| uniqueId: target.unique_id, | ||
| sourceRef: sourceRef(target.name, kind, column), | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Implement column_type execution or exclude it from executable constraints.
declaredConstraints() emits column_type, but buildGeneratedTestSql() always returns unsupported. Executed contract-type violations will become errors and get dropped instead of enforcing Track A.
Also applies to: 284-284
🤖 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/runner.ts` around lines 660 - 669, The
declaredConstraints()/buildGeneratedTestSql() flow is emitting `column_type` as
an executable constraint even though `buildGeneratedTestSql()` does not support
it, which causes contract-type violations to be dropped instead of enforced.
Update the `runner.ts` handling around `declaredConstraints()` and
`buildGeneratedTestSql()` so `column_type` is either fully implemented as an
executable SQL test or explicitly excluded from the set of constraints that are
treated as executable. Make sure the `DeclaredConstraint["kind"]` branch for
`column_type` stays aligned with whatever `buildGeneratedTestSql()` can actually
generate.
![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: 416bab7b8b
ℹ️ 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".
| if (test.kind === "relationships") { | ||
| if (!column) return { detail: "relationships test is missing column" } | ||
| const args = test.dbtTest?.args ?? {} | ||
| const target = relationshipTarget(args["to"], manifest) |
There was a problem hiding this comment.
When specTests.execute is enabled, a Track-B LLM proposal can return a relationships dbtTest with args.to set to any relation; this line turns that untrusted value into a JOIN target without checking options.allowedRelations (unlike the assertionSql sandbox). A malformed or prompt-injected proposal such as to: "sensitive_schema.users" would make the review warehouse query an arbitrary relation instead of only the changed model/upstreams.
Useful? React with 👍 / 👎.
| function withTimeout<T>(promise: Promise<T>, ms: number): Promise<T> { | ||
| return Promise.race([ | ||
| promise, | ||
| new Promise<T>((_, reject) => setTimeout(() => reject(new Error(`spec-test SQL timed out after ${ms}ms`)), ms)), | ||
| ]) |
There was a problem hiding this comment.
For fast sql.execute calls, the timeout created here is never cleared after Promise.race resolves, so every generated test leaves a live 30s timer behind. In the specTests.execute path this keeps the CLI/CI process alive until the last stale timer fires even though the review work is complete; store the timer id and clear it in a finally/settled handler.
Useful? React with 👍 / 👎.
| const kind: DeclaredConstraint["kind"] = "column_type" | ||
| add({ | ||
| kind, | ||
| column, | ||
| args: { data_type: dataType }, | ||
| hasEnforcingTest: enforcing.has(constraintKey(kind, column)), |
There was a problem hiding this comment.
For a new model with contract.enforced: true, this records every declared data_type as an unenforced column_type constraint, but the later materialization emits a dbt test named column_type and runGeneratedTests does not support executing it. With the default non-executing mode the summary proposes invalid YAML for every typed column; with execution enabled these checks are all dropped as errors, so contract type coverage is noisy rather than enforceable.
Useful? React with 👍 / 👎.
| if (normalized !== "primary_key" && normalized !== "unique") continue | ||
| const columns = asArray(rawConstraint?.columns).map(String).filter(Boolean) | ||
| if (!columns.length) continue | ||
| const kind: DeclaredConstraint["kind"] = "unique" | ||
| add({ |
There was a problem hiding this comment.
When a dbt contract declares a model-level primary_key, this branch collapses it to only a unique generated test. The generated unique SQL filters out null key values, so with specTests.execute a new model can contain nulls in a declared primary key and still be counted as passed instead of producing the block-eligible contract violation.
Useful? React with 👍 / 👎.
| const out: DeclaredConstraint[] = [...testConstraints] | ||
| const seen = new Set(out.map((c) => `${c.kind}:${c.column ?? ""}:${c.uniqueId ?? c.sourceRef}`)) | ||
| const add = (c: DeclaredConstraint) => { | ||
| const key = `${c.kind}:${c.column ?? ""}:${c.uniqueId ?? c.sourceRef}` |
There was a problem hiding this comment.
When a model declares more than one model-level unique/primary_key constraint, the constraints added below all have no column and reuse the model's uniqueId, so this dedupe key treats distinct column sets as the same constraint and drops every one after the first. In that schema, only one declared constraint is proposed/executed and the rest are silently skipped.
Useful? React with 👍 / 👎.
| function relationAliases(name: string): string[] { | ||
| const normalized = normalizeRelation(name) | ||
| const parts = normalized.split(".").filter(Boolean) | ||
| return [...new Set([normalized, parts.at(-1) ?? ""].filter(Boolean))] |
There was a problem hiding this comment.
For Track-B assertionSql, the sandbox allowlist accepts any relation whose final segment matches an allowed model name because relationAliases() adds parts.at(-1). In a multi-schema warehouse, an LLM-proposed assertion like select * from prod_sensitive.fct_orders passes when only fct_orders was allowlisted, so the review can read an unintended relation that merely shares the table basename.
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.
14 issues found across 21 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
…ness)
Security:
- Track-B dbtTest execution (relationships/accepted_values) now validates the
built SQL through sanitizeAssertionSql against the allowlist — LLM args.to can
no longer make the warehouse join an arbitrary relation.
- Sandbox: qualified relation refs require a full-qualified allowlist match (no
basename fallback for other_schema.x); allowlist-check comma-separated FROM
factors; fix trailing-semicolon-plus-comment and leading-comment-before-WITH
edge cases.
- verdict.ts specTestMayBlock now requires tool === altimate.spec_test.executed
exactly, so a proposed/candidate finding can't spoof {executed,origin} to block.
Correctness:
- column_type declared constraints are no longer materialized as unrunnable dbt
tests (excluded from the executable/proposed set; TODO INFORMATION_SCHEMA).
- Model-level primary_key emits not_null per key column (nulls no longer pass).
- Model-level constraint dedupe keys on columns so distinct sets aren't collapsed.
- Memory uses the explicit input.project; single-project fallback only when absent.
- filterToSpecDerived rejects a declared-constraint proposal whose source kind
differs from the test kind.
- Clear the raced sql.execute timeout in finally; memory tie-break by lastSeen.
Docs/cleanup: mark memory pin/forbid as future scope; dedup stableJson.
Review blast radius 264 tests green.
Refs #989
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/test/altimate/review-runner.test.ts (1)
166-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
await usingwithtmpdir()instead of manual disposal.Both
runnerWithNodesandrunnerWithManifestuseconst tmp = await tmpdir()and rely on manualawait tmp[Symbol.asyncDispose]?.()at the end of each test. If an assertion fails before that line, the temp directory leaks. Per coding guidelines,await usingsyntax is required for automatic cleanup.The helpers can be restructured to spread
tmpinto the returned object (preserving[Symbol.asyncDispose]), then useawait usingat call sites:♻️ Proposed refactor for helpers and test call sites
async function runnerWithNodes(nodes: Record<string, any>) { - const tmp = await tmpdir() + const tmp = await tmpdir() const manifestPath = path.join(tmp.path, "manifest.json") writeFileSync( manifestPath, JSON.stringify({ metadata: { adapter_type: "duckdb" }, nodes, sources: {}, }), ) - return { tmp, runner: createDispatcherRunner({ manifestPath }) } + return { ...tmp, runner: createDispatcherRunner({ manifestPath }) } }async function runnerWithManifest() { - const tmp = await tmpdir() + const tmp = await tmpdir() const manifestPath = path.join(tmp.path, "manifest.json") writeFileSync( manifestPath, JSON.stringify({ metadata: { adapter_type: "duckdb" }, nodes: { ... }, sources: {}, }), ) - return { tmp, runner: createDispatcherRunner({ manifestPath }) } + return { ...tmp, runner: createDispatcherRunner({ manifestPath }) } }Then update each test to use
await usingand remove manual disposal:test("model-level primary_key emits composite unique plus not_null per key column", async () => { - const { tmp, runner } = await runnerWithNodes({ + await using ctx = await runnerWithNodes({ "model.demo.fct_orders": { ... }, }) + const { runner } = ctx ... - await tmp[Symbol.asyncDispose]?.() })test("rejects track-B relationships when the target relation is not allowlisted", async () => { - const { tmp, runner } = await runnerWithManifest() + await using ctx = await runnerWithManifest() + const { runner } = ctx ... - await tmp[Symbol.asyncDispose]?.() })Apply the same pattern to all four tests in both suites.
Based on learnings, for brand-new test files under
packages/opencode/test/altimate/, useawait using tmp = await tmpdir()with per-test scoping. As per coding guidelines, always useawait usingsyntax withtmpdir()for automatic cleanup when the variable goes out of scope.Also applies to: 238-270
🤖 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-runner.test.ts` around lines 166 - 180, The test helpers in review-runner.test.ts still manage tmpdir cleanup manually, which can leak temp directories if a test fails early. Update the runnerWithNodes and runnerWithManifest helpers to work with await using by preserving the async disposable tmp object when returning it, then change each test that calls tmpdir() to declare the temp directory with await using at the call site and remove the explicit Symbol.asyncDispose cleanup. Apply the same pattern consistently across the review runner test suites so tmpdir usage is always automatically cleaned up.Sources: Coding guidelines, Learnings
🤖 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.
Inline comments:
In `@packages/opencode/src/altimate/review/spec-test-sandbox.ts`:
- Around line 224-226: The relation extraction in referencedRelations(trimmed)
is picking up FROM/JOIN tokens inside comments and string literals, which can
incorrectly trigger unknown_relation. Update the parsing path in the sandbox
assertion check to scan a masked SQL form that blanks comments and string
literals while preserving quoted identifiers, then run relation extraction
against that sanitized text. Keep the ctes and relationAllowed checks in the
same flow so only real table references are validated.
---
Nitpick comments:
In `@packages/opencode/test/altimate/review-runner.test.ts`:
- Around line 166-180: The test helpers in review-runner.test.ts still manage
tmpdir cleanup manually, which can leak temp directories if a test fails early.
Update the runnerWithNodes and runnerWithManifest helpers to work with await
using by preserving the async disposable tmp object when returning it, then
change each test that calls tmpdir() to declare the temp directory with await
using at the call site and remove the explicit Symbol.asyncDispose cleanup.
Apply the same pattern consistently across the review runner test suites so
tmpdir usage is always automatically cleaned up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43fc890e-1b65-4caf-8903-b121a4ef5ef3
📒 Files selected for processing (14)
packages/opencode/specs/corrective-app-memory.mdpackages/opencode/src/altimate/review/corrective-memory.tspackages/opencode/src/altimate/review/orchestrate.tspackages/opencode/src/altimate/review/runner.tspackages/opencode/src/altimate/review/spec-test-gen.tspackages/opencode/src/altimate/review/spec-test-sandbox.tspackages/opencode/src/altimate/review/stable-json.tspackages/opencode/src/altimate/review/verdict.tspackages/opencode/test/altimate/review-corrective-memory.test.tspackages/opencode/test/altimate/review-runner.test.tspackages/opencode/test/altimate/review-spec-test-exec.test.tspackages/opencode/test/altimate/review-spec-test-gen.test.tspackages/opencode/test/altimate/review-spec-test-sandbox.test.tspackages/opencode/test/altimate/review-spec-test-verdict.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/opencode/specs/corrective-app-memory.md
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/opencode/test/altimate/review-spec-test-sandbox.test.ts
- packages/opencode/test/altimate/review-spec-test-verdict.test.ts
- packages/opencode/test/altimate/review-corrective-memory.test.ts
- packages/opencode/src/altimate/review/corrective-memory.ts
- packages/opencode/src/altimate/review/verdict.ts
- packages/opencode/test/altimate/review-spec-test-exec.test.ts
- packages/opencode/src/altimate/review/spec-test-gen.ts
- packages/opencode/test/altimate/review-spec-test-gen.test.ts
- packages/opencode/src/altimate/review/runner.ts
- packages/opencode/src/altimate/review/orchestrate.ts
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 122b316d10
ℹ️ 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".
| kind: c.kind, | ||
| dbtTest: { column: c.column, test: c.kind, args }, |
There was a problem hiding this comment.
When declaredConstraints passes a model-level unique/primary_key, c.column is undefined and args contains columns, so the default/no-warehouse path renders a schema patch like models: [{ tests: [{ unique: { columns: [...] } }] }]. The built-in dbt unique test expects a single column_name rather than a columns list, so users applying the suggested patch for composite keys get a test that fails to compile instead of enforcing the declared key; materialize these as an actual multi-column generic test or avoid proposing YAML for them.
Useful? React with 👍 / 👎.
| const columns = constraintColumns(args) | ||
| .map((c) => c.toLowerCase()) | ||
| .sort() | ||
| .join(",") | ||
| return `${kind}:${(column ?? "").toLowerCase()}:${columns}` |
There was a problem hiding this comment.
For argument-bearing constraints, this key ignores everything except the column set, so an existing accepted_values/relationships test on the same column marks a different declared constraint as hasEnforcingTest. In that case Track A never materializes or executes the declared values/to/field check, allowing a contract violation to be missed even with specTests.execute enabled; include the normalized argument payload in the key for these kinds.
Useful? React with 👍 / 👎.
| return model ? relationForModel(model) : sqlTableRef(ref) | ||
| } | ||
| const source = /\bsource\s*\(\s*['"]([^'"]+)['"]\s*,\s*['"]([^'"]+)['"]\s*\)/.exec(raw) | ||
| if (source) return sqlTableRef(`${source[1]}.${source[2]}`) |
There was a problem hiding this comment.
When a declared/proposed relationships test uses to: source('src','tbl'), this branch turns the macro arguments into the physical relation src.tbl. In dbt the first argument is the source name, not necessarily the database/schema/identifier, so projects with source('stripe','payments') backed by e.g. raw.stripe_payments will execute the generated relationship check against the wrong relation or error and then be dropped, leaving source relationships unverified under specTests.execute.
Useful? React with 👍 / 👎.
| "unique", | ||
| "accepted_values", | ||
| "relationships", | ||
| "range", |
There was a problem hiding this comment.
By admitting range here, a Track-B response like dbtTest: { test: 'range' } passes the guard and is rendered verbatim into the proposed schema YAML, but dbt core has no built-in generic test named range and runGeneratedTests also reports it as unsupported. With the default propose-only mode this produces a non-compiling patch for users to apply; use a supported package test name or keep this kind out of emitted dbt tests.
Useful? React with 👍 / 👎.
| if (trackB) { | ||
| const targetCheck = sanitizeAssertionSql(`select 1 from ${target}`, expandedAllowedRelations(manifest, options)) | ||
| if (!targetCheck.ok) return { detail: `relationships target rejected by sandbox: ${targetCheck.reason}` } |
There was a problem hiding this comment.
With specTests.execute enabled, a PR-controlled schema can declare an unenforced relationships constraint whose to points at any relation, and Track A skips the sandbox because this check only runs for trackB. Fresh evidence: the current code validates LLM-inferred relationship targets but still executes declared-constraint joins directly, so a new model can make CI query an unrelated sensitive table under the review warehouse role.
Useful? React with 👍 / 👎.
- Sandbox: extract withoutCommentsAndStringLiterals (preserves quoted identifiers)
and use it for BOTH cteNames and referencedRelations, so a FROM/JOIN inside a
comment/string no longer false-rejects and quoted CTE/relation names resolve.
- P1: declared (track-A) relationship targets are now sandbox-validated against
the allowlist unconditionally (not only track-B), and source('s','t') targets
resolve via the manifest (skip, not guess, when unresolvable).
- constraintKey includes normalized args for accepted_values/relationships so an
existing test on the same column no longer masks a different declared constraint.
- column_type declared constraints surface as a single non-blocking advisory
finding instead of silently disappearing.
- Composite/model-level unique renders valid dbt_utils.unique_combination_of_columns
YAML (core unique takes column_name); single-column keeps core unique.
- Drop range from ALLOWED_TEST_KINDS (no dbt core generic test named range).
Review blast radius 281 tests green.
Refs #989
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/test/altimate/review-runner.test.ts (1)
310-343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
await usingfor temp directory cleanup in newly added tests.The
runnerWithSourceManifesthelper returnstmpfor manual disposal, but if any assertion throws beforeawait tmp[Symbol.asyncDispose]?.()executes, the temp directory leaks. Usingawait usingensures cleanup on scope exit regardless of assertion outcomes. Based on learnings, newly added code underpackages/opencode/test/altimate/should useawait using tmp = await tmpdir()with per-test scoping.♻️ Refactor helper to enable `await using`
async function runnerWithSourceManifest() { - const tmp = await tmpdir() + await using tmp = await tmpdir() const manifestPath = path.join(tmp.path, "manifest.json") writeFileSync( manifestPath, JSON.stringify({ metadata: { adapter_type: "duckdb" }, nodes: { /* ... */ }, sources: { /* ... */ }, }), ) - return { tmp, runner: createDispatcherRunner({ manifestPath }) } + return { runner: createDispatcherRunner({ manifestPath }) } }Then in each test:
- const { tmp, runner } = await runnerWithSourceManifest() + await using tmp = await tmpdir() + const { runner } = await runnerWithSourceManifest(tmp) // ... assertions ... - await tmp[Symbol.asyncDispose]?.()Alternatively, inline the helper into each test so
await usingbinds directly.🤖 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-runner.test.ts` around lines 310 - 343, The new temp-directory helper in runnerWithSourceManifest should be updated to avoid manual disposal and ensure cleanup even if a test assertion fails. Refactor the helper so tmpdir() is bound with await using at the per-test scope, or inline the setup into each test so the temporary directory is automatically disposed when the scope exits. Keep the change localized to runnerWithSourceManifest and the nearby review-runner.test.ts test setup.Source: Learnings
🤖 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/test/altimate/review-runner.test.ts`:
- Around line 310-343: The new temp-directory helper in runnerWithSourceManifest
should be updated to avoid manual disposal and ensure cleanup even if a test
assertion fails. Refactor the helper so tmpdir() is bound with await using at
the per-test scope, or inline the setup into each test so the temporary
directory is automatically disposed when the scope exits. Keep the change
localized to runnerWithSourceManifest and the nearby review-runner.test.ts test
setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 30d1be6d-8a73-4477-b5ab-77880f4aef75
📒 Files selected for processing (8)
packages/opencode/src/altimate/review/orchestrate.tspackages/opencode/src/altimate/review/runner.tspackages/opencode/src/altimate/review/spec-test-gen.tspackages/opencode/src/altimate/review/spec-test-sandbox.tspackages/opencode/test/altimate/review-runner.test.tspackages/opencode/test/altimate/review-spec-test-exec.test.tspackages/opencode/test/altimate/review-spec-test-gen.test.tspackages/opencode/test/altimate/review-spec-test-sandbox.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/opencode/test/altimate/review-spec-test-sandbox.test.ts
- packages/opencode/src/altimate/review/spec-test-sandbox.ts
- packages/opencode/src/altimate/review/spec-test-gen.ts
- packages/opencode/src/altimate/review/orchestrate.ts
- packages/opencode/test/altimate/review-spec-test-gen.test.ts
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
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/orchestrate.ts">
<violation number="1" location="packages/opencode/src/altimate/review/orchestrate.ts:1109">
P2: Single-column model-level `unique` constraints are not executed: this branch stores the column only in `args.column_name`, but the executor does not read that key. Representing it as a column-level unique test keeps Track A executable and produces the expected YAML.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| } | ||
| if (columns.length === 1) { | ||
| return { test: "unique", args: { column_name: columns[0] } } |
There was a problem hiding this comment.
P2: Single-column model-level unique constraints are not executed: this branch stores the column only in args.column_name, but the executor does not read that key. Representing it as a column-level unique test keeps Track A executable and produces the expected YAML.
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 1109:
<comment>Single-column model-level `unique` constraints are not executed: this branch stores the column only in `args.column_name`, but the executor does not read that key. Representing it as a column-level unique test keeps Track A executable and produces the expected YAML.</comment>
<file context>
@@ -1086,10 +1086,32 @@ interface EnforcedConstraintMetrics {
+ }
+ }
+ if (columns.length === 1) {
+ return { test: "unique", args: { column_name: columns[0] } }
+ }
+ }
</file context>
| return { test: "unique", args: { column_name: columns[0] } } | |
| return { column: columns[0], test: "unique" } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73e93fba51
ℹ️ 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".
| const ref = /\bref\s*\(\s*['"]([^'"]+)['"]\s*\)/.exec(raw)?.[1] | ||
| if (ref) { | ||
| const model = manifest.byName.get(ref) | ||
| return { relation: model ? relationForModel(model) : sqlTableRef(ref) } |
There was a problem hiding this comment.
For declared relationships tests that point to a package model with dbt's valid ref('package', 'model') form, this regex only recognizes single-argument refs, so the value falls through to sqlTableRef(raw) and is later rejected by the sandbox as an unknown relation. With specTests.execute enabled, those declared relationship constraints are recorded as execution errors and dropped, so cross-package foreign-key violations are never checked; parse the two-argument form and resolve the second argument against the manifest/package target instead of treating the macro text as a table name.
Useful? React with 👍 / 👎.
| text: name, | ||
| }) | ||
| } | ||
| for (const match of sql.matchAll(/\bsource\s*\(([^)]*)\)/g)) { |
There was a problem hiding this comment.
When specTests.execute is enabled, specTestSynthesisLane feeds refs collected by this raw regex into allowedRelationsForSpecTests, and those allowed relations are passed to generated-test execution. Because the scan does not strip SQL comments or string literals, a PR can add a harmless-looking comment such as -- source('prod_sensitive','users'), causing the Track-B prompt and sandbox allowlist to treat that unrelated relation as an upstream and allowing a generated relationships/assertion test to query it under the review warehouse role. Only collect refs from parsed/model dependencies or strip comments/strings before deriving the execution allowlist.
Useful? React with 👍 / 👎.
| log.warn("dropping candidate spec-test execution error", { test: test.id, detail: result?.detail }) | ||
| continue | ||
| } | ||
| if (result.status === "pass") continue |
There was a problem hiding this comment.
When specTests.execute is enabled and a Track-B generated test passes current data, this branch drops it without emitting the proposed YAML patch. In projects that enable execution to validate candidates before review, the useful passing proposals disappear entirely and only failing hypotheses surface as warnings; keep the proposal finding for passing advisory tests, optionally annotated as executed/passed.
Useful? React with 👍 / 👎.
| for (const parsed of parsedTests) { | ||
| out.push({ | ||
| origin: "declared_constraint", | ||
| kind: parsed.kind, | ||
| ref: `schema.yml:${model}.${name}:${parsed.kind}`, |
There was a problem hiding this comment.
For a new model whose changed schema.yml already contains column tests, this loop adds those existing tests as spec sources for Track B, so the generator can return the same not_null/accepted_values test and the summary renders a duplicate YAML patch. Since entries from column.tests/data_tests are already dbt tests, they should not be fed back as candidate tests to add again; reserve proposal sources for unenforced constraints or soft intent.
Useful? React with 👍 / 👎.
| if (columns.length === 1) { | ||
| return { test: "unique", args: { column_name: columns[0] } } | ||
| } |
There was a problem hiding this comment.
For a model-level unique/primary_key constraint with exactly one column, this branch materializes args.column_name but leaves dbtTest.column unset. The execution path's columnsForUnique() does not read column_name, so with specTests.execute enabled the generated check errors as missing columns and is dropped, letting duplicate values in a declared single-column key pass unreported.
Useful? React with 👍 / 👎.
| for (const source of specSources) { | ||
| if (source.kind !== "ref_edge") continue | ||
| const name = source.text ?? source.ref.replace(/^(ref|source):/, "") | ||
| if (name) out.add(name) |
There was a problem hiding this comment.
For Track-A declared relationships constraints, the sandbox allowlist is built only from ref_edge sources found in the model SQL, not from the relationship constraint's own to target. A normal foreign-key declaration to ref('dim_customers') on a fact model that does not select from that dimension will therefore fail the target sandbox check and be dropped as an execution error, so the declared relationship is never verified; include the resolved declared target in the allowlist after manifest validation.
Useful? React with 👍 / 👎.
1 participant
PINEAPPLE
Summary
Greenfield spec-test synthesis for
dbt-pr-review— the full feature (P0→P2). Closes #989. Supersedes #990 (the propose-only P0 slice, folded in here with the review-comment fixes).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 tests from the model's declared intent and (optionally) executes them, raising the no-reference verification ceiling that
missingGrainTestLanepreviously only flagged. Design:packages/opencode/specs/greenfield-spec-test-synthesis.md.Two strictly separated tracks:
not_null/unique/accepted_values/relationships, contract types). The assertion is built from the parsed constraint — no LLM in the blocking path. Executed against the warehouse behindspecTests.execute; an executed failure is a realcontract_violation.ref()edges, PR text). Non-blocking; optionally executed under a strict SQL sandbox and surfaced ascandidatefindings that can never block.Defense in depth:
verdict.ts specTestMayBlocklets a spec-test finding block only with evidence{executed:true, origin:"declared_constraint"}; advisory tools are excluded from the warning-count.Also included:
spec-test-sandbox.ts): single-SELECT-only state-machine scan, no DDL/DML/multi-statement, CTE-aware relation allowlist, boundedcount(*).review_spec_test_prompt) with an inline fallback (Rust const inspecs/core-spec-test-prompt.md; addon rebuild activates it, no regression before then).Test Plan
candidatenon-blocking, and corrective-memory retrieval + suppression safety.criticalor the executed track-A path.bun run script/upstream/analyze.ts --markers --base main --strict— clean. TypeScript CI was green on feat: greenfield spec-test synthesis for dbt-pr-review (P0: propose-only) #990.Note: local
turbo typecheckreports 3 errors in untouchedpackages/drivers/src/sqlserver.ts— they surface only when the optional@azure/identitypeer is installed locally; CI omits it and passed.Checklist
packages/opencode/specs/)Summary by cubic
Adds greenfield spec-test synthesis for
dbt-pr-reviewto validate brand-new dbt models by generating tests from declared intent and, optionally, executing deterministic constraints. Tightens blocking rules and hardens the SQL sandbox, with better dedupe and relationship validation. Closes #989.New Features
not_null,unique,accepted_values,relationships, contract types). WhenspecTests.execute: trueand a warehouse is available, executes and reportscontract_violationwith evidence{ executed: true, origin: "declared_constraint" }. Relationship targets are sandbox‑validated;source('s','t')targets resolve via the manifest (skip when unresolvable).candidatefindings that never block and are excluded from warning counts.altimate.spec_test.executedwith{ executed: true, origin: "declared_constraint" }. Advisory/unexecuted outputs cannot block.count(*); validates built SQL for track‑B dbtTests and track‑A relationships; requires fully qualified allowlist matches, handles comma‑separated FROM factors, and preserves quoted identifiers while stripping comments/strings.memory.entriesto bias generation and suppress advisory‑only findings; never suppresses critical or Track A paths.altimate_core.review_spec_test_promptin the dispatcher.spec_testslane in lite and full reviews.column_typedeclared constraints now surface as a single non‑blocking advisory; modelprimary_keyemitsnot_nullper key column; improved constraint dedupe with normalized args foraccepted_values/relationships; composite/model‑leveluniquerendersdbt_utils.unique_combination_of_columnsYAML (single‑column uses coreunique); stricter source‑kind matching; dropped unsupportedrange; stable timeout handling; memory uses explicitprojectwithlastSeentiebreak.Migration
specTests.execute: truein.altimate/review.ymland ensure a warehouse is configured; without a warehouse, proposals are shown but not run.memory.entriesto suppress known‑noisy advisory proposals.Written for commit 73e93fb. Summary will update on new commits.
Summary by CodeRabbit
specTests/memoryto review configuration.