Open
Conversation
Add `dbt_unit_test_gen` tool and `dbt-unit-tests` skill for generating dbt unit tests from manifest + compiled SQL. Engine (`unit-tests.ts`): - Reuses `parseManifest()` for model info, deps, columns, descriptions - Reuses `dbtLineage()` for compiled SQL + column lineage mapping - Reuses `schema.inspect` for warehouse column enrichment - Reuses `sql.optimize` for anti-pattern detection - Keyword-based scenario detection (CASE, JOIN, GROUP BY, division, incremental) - Type-correct mock data generation with null/boundary/happy-path variants - Incremental tests include `input: this` mock for existing table state - Ephemeral deps correctly use `format: sql` even with no known columns - Cross-database support via `database` param in `schema.inspect` - YAML assembly via `yaml` library (not string concatenation) - Deterministic test names (index-based, not `Date.now()`) - Rich `UnitTestContext` with descriptions + lineage for LLM refinement Shared infrastructure: - Extract `helpers.ts` from `lineage.ts` (shared: `loadRawManifest`, `findModel`, `getUniqueId`, `detectDialect`, `buildSchemaContext`) - Manifest cache by path+mtime (avoids re-reading 128MB files) - Add `description` to `DbtModelInfo`/`DbtSourceInfo` - Add `adapter_type` to `DbtManifestResult` Skill (`dbt-unit-tests/SKILL.md`): - 5-phase workflow: Analyze -> Generate -> Refine -> Validate -> Write - Reference guides: YAML spec, edge-case patterns, incremental testing Tests: 32 tests covering manifest parsing, scenario detection, YAML round-trip validation, incremental `this` mock, ephemeral sql format, deterministic naming, descriptions, lineage context, source deps. Co-Authored-By: Claude Opus 4.6 (1M context) <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 to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an automated dbt unit-test generation feature: new generator and tool, manifest helpers and caching, extended manifest parsing/types, dispatcher registration, YAML assembly, docs/skills, CLI reference, and comprehensive tests; the generator analyzes compiled SQL and manifest to scaffold unit-test YAML and return semantic context. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tool as DbtUnitTestGenTool
participant Dispatcher
participant Generator as generateDbtUnitTests
participant Manifest as loadRawManifest
participant Lineage as dbtLineage
participant Inspector as schema.inspect
participant Optimizer as sql.optimize
participant YAML as assembleYaml
rect rgba(200,200,255,0.5)
User->>Tool: invoke(manifest_path, model, dialect?, max_scenarios?)
end
Tool->>Dispatcher: call("dbt.unit_test_gen", params)
Dispatcher->>Generator: invoke(params)
Generator->>Manifest: loadRawManifest(manifest_path)
Manifest-->>Generator: manifest JSON
Generator->>Lineage: dbtLineage(manifest, model)
Lineage-->>Generator: model node & column lineage
par Enrich & Analyze
Generator->>Inspector: schema.inspect(missing columns)
Inspector-->>Generator: column schemas
and Anti-pattern detection
Generator->>Optimizer: sql.optimize(compiled_sql, schema context)
Optimizer-->>Generator: anti-patterns
end
Generator->>Generator: detect scenarios, build mock inputs & expected rows
Generator->>YAML: assembleYaml(modelName, tests)
YAML-->>Generator: YAML string
Generator-->>Dispatcher: DbtUnitTestGenResult
Dispatcher-->>Tool: result
Tool-->>User: formatted report + YAML
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/opencode/test/altimate/dbt-unit-test-gen.test.ts (1)
1-25: Use the sharedtmpdir()fixture instead of manual/tmpbookkeeping.This helper writes directly into
os.tmpdir()and depends onafterEachto clean up. Please switch these tests toawait usingwithtmpdir()so cleanup is automatic and consistent with the rest of the repo.As per coding guidelines, "Use the
tmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup in test files" and "Always useawait usingsyntax withtmpdir()for automatic cleanup when the variable goes out of scope".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/dbt-unit-test-gen.test.ts` around lines 1 - 25, Replace manual temp-file bookkeeping (tmpFiles array, writeTmpManifest helper, and afterEach cleanup) with the shared tmpdir fixture: import { tmpdir } from "fixture/fixture.ts", use "await using dir = await tmpdir()" in tests that need a temp directory, create manifest files inside dir.path (e.g., path.join(dir.path, `manifest-utg-${Date.now()}-...`)), and remove the tmpFiles array, writeTmpManifest global helper, and afterEach cleanup block; keep the rest of test logic but update calls that used writeTmpManifest to write files into the tmpdir instance instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.opencode/skills/dbt-unit-tests/references/incremental-testing.md:
- Around line 3-14: The example uses expression delimiters `{{ if
is_incremental() }}` which is invalid for control flow; update the incremental
example to use Jinja statement tags instead—replace `{{ if is_incremental() }}`
/ `{{ endif }}` with `{% if is_incremental() %}` / `{% endif %}` around the two
code paths (referencing the `is_incremental()` conditional and the two SQL
snippets that reference `ref('stg_orders')` and `this`) so the model example is
syntactically correct and easy to copy.
In @.opencode/skills/dbt-unit-tests/references/unit-test-yaml-spec.md:
- Around line 132-137: The example YAML places "tags" under "config" while the
spec above shows "tags" as a top-level sibling of "config"; update the example
to match the spec by moving "tags" out to the top level (i.e., as a sibling of
"config") or add a short note that both top-level "tags" and "config.tags" forms
are supported and show both examples; reference the "config" and "tags" fields
when making the change so the structure matches the documented schema.
In @.opencode/skills/dbt-unit-tests/SKILL.md:
- Around line 64-70: The fenced code block showing the dbt_unit_test_gen
invocation is unlabeled and triggers markdownlint MD040; update that fenced
block (the block containing dbt_unit_test_gen(...), manifest_path, model,
max_scenarios) to include a language label such as "text" after the opening
triple backticks (e.g., ```text) so the fence is properly labeled.
In `@packages/opencode/src/altimate/native/dbt/unit-tests.ts`:
- Around line 409-411: The current construction of testName using
sanitizeName(...).slice(0,64) can truncate the scenario suffix
(scenario.category and optional _{idx}), causing collisions; change the logic in
the testName creation (and the analogous places around the other occurrence) to
build the suffix first (e.g., `_${scenario.category}` plus `_{idx}` when idx>0),
then truncate the model-derived part so that total length <=64 by taking
modelPart.slice(0, 64 - suffix.length) and finally concatenate modelPart +
suffix before calling sanitizeName (or call sanitizeName per-part as
appropriate) to ensure the scenario suffix is always preserved and names remain
unique.
- Around line 81-119: resolveUpstream currently only maps manifest.models and
manifest.sources and drops upstreamIds like seed.* and snapshot.*; update
resolveUpstream to also detect and include seed and snapshot upstreams (by
checking uid prefixes or looking them up in manifest.seeds/manifest.snapshots)
and push corresponding UpstreamDep entries with resource_type set to "seed" or
"snapshot" (including unique_id, name, schema/database if available, description
and columns if present, or minimal placeholders) so non-model ref() dependencies
are not lost; apply the same change pattern where resolveUpstream is used around
the other occurrence noted (lines ~207-208) to ensure mocks include
seeds/snapshots too.
---
Nitpick comments:
In `@packages/opencode/test/altimate/dbt-unit-test-gen.test.ts`:
- Around line 1-25: Replace manual temp-file bookkeeping (tmpFiles array,
writeTmpManifest helper, and afterEach cleanup) with the shared tmpdir fixture:
import { tmpdir } from "fixture/fixture.ts", use "await using dir = await
tmpdir()" in tests that need a temp directory, create manifest files inside
dir.path (e.g., path.join(dir.path, `manifest-utg-${Date.now()}-...`)), and
remove the tmpFiles array, writeTmpManifest global helper, and afterEach cleanup
block; keep the rest of test logic but update calls that used writeTmpManifest
to write files into the tmpdir instance instead.
🪄 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: 1b7a8d5f-2fcd-4cf8-b4f7-27010fe06437
📒 Files selected for processing (15)
.opencode/skills/dbt-test/SKILL.md.opencode/skills/dbt-unit-tests/SKILL.md.opencode/skills/dbt-unit-tests/references/altimate-dbt-commands.md.opencode/skills/dbt-unit-tests/references/edge-case-patterns.md.opencode/skills/dbt-unit-tests/references/incremental-testing.md.opencode/skills/dbt-unit-tests/references/unit-test-yaml-spec.mdpackages/opencode/src/altimate/native/dbt/helpers.tspackages/opencode/src/altimate/native/dbt/lineage.tspackages/opencode/src/altimate/native/dbt/manifest.tspackages/opencode/src/altimate/native/dbt/register.tspackages/opencode/src/altimate/native/dbt/unit-tests.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/dbt-unit-test-gen.tspackages/opencode/src/tool/registry.tspackages/opencode/test/altimate/dbt-unit-test-gen.test.ts
.opencode/skills/dbt-unit-tests/references/incremental-testing.md
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 15 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/native/dbt/unit-tests.ts">
<violation number="1" location="packages/opencode/src/altimate/native/dbt/unit-tests.ts:208">
P1: `resolveUpstream()` only resolves models and sources, but `model.depends_on` can also contain `seed.*` and `snapshot.*` IDs. Those dependencies are silently dropped from `given`, producing YAML that is missing required mock inputs — `dbt test` will fail for any model that `ref()`s a seed or snapshot. Either resolve these dependency types or return an explicit error/warning.</violation>
<violation number="2" location="packages/opencode/src/altimate/native/dbt/unit-tests.ts:339">
P2: Division-detection regex matches inside string literals, causing false-positive "divide-by-zero" test scenarios. SQL string contents like date formats (`'2024/01/15'`) or path literals (`'a/b'`) contain `word/word` and will trigger the heuristic. Strip quoted strings before applying the regex, or anchor the pattern more tightly to arithmetic contexts (e.g., require the operands to be column-like identifiers or numbers, not inside quotes).</violation>
<violation number="3" location="packages/opencode/src/altimate/native/dbt/unit-tests.ts:409">
P2: The `.slice(0, 64)` truncation can cut off the scenario suffix (e.g., `_edge_case_2`, `_incremental_4`) for long model names, causing different test scenarios to collapse to the same dbt test name. Preserve the suffix by budgeting its length before truncating the model-name portion.</violation>
</file>
<file name=".opencode/skills/dbt-unit-tests/SKILL.md">
<violation number="1" location=".opencode/skills/dbt-unit-tests/SKILL.md:36">
P2: Section header omits Phase 5 ("Write"). The workflow has 5 phases but the header only lists 4, which could mislead the agent into skipping the final Write step.</violation>
</file>
<file name="packages/opencode/src/altimate/native/dbt/helpers.ts">
<violation number="1" location="packages/opencode/src/altimate/native/dbt/helpers.ts:46">
P2: Key-based lookup in `findModel` skips the `resource_type === "model"` check that the name-based fallback enforces. If a non-model unique_id is passed, the function returns a non-model node, violating its documented contract.</violation>
</file>
<file name=".opencode/skills/dbt-unit-tests/references/incremental-testing.md">
<violation number="1" location=".opencode/skills/dbt-unit-tests/references/incremental-testing.md:3">
P2: Incorrect Jinja delimiter: `{{ if ... }}` should be `{% if ... %}`. The `{{ }}` syntax outputs expression values; `{% %}` is for control-flow statements.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
.opencode/skills/dbt-unit-tests/references/incremental-testing.md
Outdated
Show resolved
Hide resolved
Address review comments from CodeRabbit and cubic on PR #674. Bug fixes: - `resolveUpstream` now resolves seed.* and snapshot.* deps in addition to models and sources. Previously silently dropped, causing `dbt test` to fail for any model ref()ing a seed or snapshot. - Test name truncation no longer cuts off scenario suffix. Budget suffix length first, then truncate the model-name portion, so scenario names like `_null_handling_2` are always preserved even for long model names. - `findModel`/`getUniqueId` in helpers.ts now validate `resource_type === "model"` on the key lookup path, not just the name fallback. Prevents returning non-model nodes by unique_id. - Division detection regex now strips string literals AND comments before matching, so `'2024/01/15'` no longer triggers a false-positive boundary scenario. Documentation fixes: - `incremental-testing.md`: fix Jinja syntax — `{{ if is_incremental() }}` is invalid; use `{% if is_incremental() %}` for control flow. - `SKILL.md`: workflow header now shows all 5 phases (Analyze -> Generate -> Refine -> Validate -> Write). - `SKILL.md`: add language label to fenced code block for markdownlint. - `unit-test-yaml-spec.md`: show both top-level `tags` and nested `config.tags` forms explicitly. Infrastructure: - Add `seeds` and `snapshots` arrays to `DbtManifestResult` (previously only counts were returned). `parseManifest` now extracts full seed and snapshot info using the same shape as `DbtModelInfo`. - Tests migrate to shared `tmpdir()` fixture from `test/fixture/fixture.ts` for automatic cleanup (per project coding guidelines). - Wrap `DbtUnitTestGenTool` import/registration in `registry.ts` with `altimate_change` markers (upstream-shared file). New tests (4): - seed deps resolve via ref() not source() - snapshot deps resolve via ref() not source() - long model names preserve scenario suffix (no truncation collision) - division in string literals does not trigger boundary scenario Full suite: 2676 pass, 0 fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/opencode/src/altimate/native/dbt/unit-tests.ts (1)
215-218:⚠️ Potential issue | 🟠 MajorFail fast when an upstream dependency ID cannot be resolved.
resolveUpstream()still drops unknown IDs silently. Ifmodel.depends_oncontains anything outside the parsed model/source/seed/snapshot sets, this path returns success with fewergiveninputs than the model actually references, anddependency_countbecomes misleading.💡 Minimal guard
const upstreamDeps = resolveUpstream( model.depends_on, manifest.models, manifest.sources, manifest.seeds, manifest.snapshots, ) + const unresolvedDeps = model.depends_on.filter( + (id) => !upstreamDeps.some((dep) => dep.unique_id === id), + ) + if (unresolvedDeps.length > 0) { + return failResult( + model.name, + `Unsupported or unresolved upstream dependencies: ${unresolvedDeps.join(", ")}`, + ) + } const materialized = model.materialized || "view"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/native/dbt/unit-tests.ts` around lines 215 - 218, resolveUpstream currently drops unknown dependency IDs silently causing misleading dependency_count; after calling resolveUpstream(model.depends_on, manifest.models, manifest.sources, manifest.seeds, manifest.snapshots) verify that every ID in model.depends_on was actually resolved and fail-fast if any are missing. Either update resolveUpstream to throw on unresolved IDs or, immediately after the call, compare the set/length of resolved upstream IDs to model.depends_on and throw an error (including the missing IDs) so dependency_count and given inputs accurately reflect the model's declared deps.
🧹 Nitpick comments (1)
packages/opencode/test/altimate/dbt-unit-test-gen.test.ts (1)
13-23: Useawait using tmpdir()here instead of manual hook-based disposal.The shared
tmpplus explicitafterEachcleanup bypasses the repo’s required temp-dir pattern and makes cleanup depend on hook ordering instead of scope.As per coding guidelines, "Use the
tmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup in test files" and "Always useawait usingsyntax withtmpdir()for automatic cleanup when the variable goes out of scope".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/dbt-unit-test-gen.test.ts` around lines 13 - 23, Replace the shared tmp + manual disposal pattern with scope-based automatic cleanup: remove the top-level tmp variable and the beforeEach/afterEach tmp setup/teardown that call tmpdir() and Symbol.asyncDispose, and instead use "await using tmpdir()" in the test scope where a temp directory is needed; keep resetting manifestCounter in beforeEach if required. Locate references to tmp, the beforeEach/afterEach blocks, and the tmpdir() call in this file and refactor tests to obtain temp dirs via await using tmpdir() so disposal is automatic when the variable goes out of scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/native/dbt/helpers.ts`:
- Around line 24-39: The parsed manifest is still produced twice because
parseManifest() bypasses the new _manifestCache used by loadRawManifest();
update parseManifest(manifestPath) to call loadRawManifest(manifestPath) and use
its returned parsed JSON when available (falling back to existing parsing logic
only if loadRawManifest returns null), and ensure generateDbtUnitTests() and any
callers (e.g., dbtLineage()) use parseManifest(manifestPath) so the single
cached parse from _manifestCache is reused across the flow.
In `@packages/opencode/src/altimate/native/dbt/unit-tests.ts`:
- Around line 159-163: The SchemaInspectParams interface in types.ts is missing
an optional database field while dispatcherCall("schema.inspect", ...) passes
database; update the SchemaInspectParams type to include database?: string so
the bridge contract matches callers (used by the schema.inspect dispatcher and
the unit test calls that pass dep.database), and ensure any
consumers/implementations that construct or validate SchemaInspectParams accept
this optional field.
---
Duplicate comments:
In `@packages/opencode/src/altimate/native/dbt/unit-tests.ts`:
- Around line 215-218: resolveUpstream currently drops unknown dependency IDs
silently causing misleading dependency_count; after calling
resolveUpstream(model.depends_on, manifest.models, manifest.sources,
manifest.seeds, manifest.snapshots) verify that every ID in model.depends_on was
actually resolved and fail-fast if any are missing. Either update
resolveUpstream to throw on unresolved IDs or, immediately after the call,
compare the set/length of resolved upstream IDs to model.depends_on and throw an
error (including the missing IDs) so dependency_count and given inputs
accurately reflect the model's declared deps.
---
Nitpick comments:
In `@packages/opencode/test/altimate/dbt-unit-test-gen.test.ts`:
- Around line 13-23: Replace the shared tmp + manual disposal pattern with
scope-based automatic cleanup: remove the top-level tmp variable and the
beforeEach/afterEach tmp setup/teardown that call tmpdir() and
Symbol.asyncDispose, and instead use "await using tmpdir()" in the test scope
where a temp directory is needed; keep resetting manifestCounter in beforeEach
if required. Locate references to tmp, the beforeEach/afterEach blocks, and the
tmpdir() call in this file and refactor tests to obtain temp dirs via await
using tmpdir() so disposal is automatic when the variable goes out of scope.
🪄 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: ef52cb31-c89b-4e9e-87cc-529d789e21d4
📒 Files selected for processing (9)
.opencode/skills/dbt-unit-tests/SKILL.md.opencode/skills/dbt-unit-tests/references/incremental-testing.md.opencode/skills/dbt-unit-tests/references/unit-test-yaml-spec.mdpackages/opencode/src/altimate/native/dbt/helpers.tspackages/opencode/src/altimate/native/dbt/manifest.tspackages/opencode/src/altimate/native/dbt/unit-tests.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/tool/registry.tspackages/opencode/test/altimate/dbt-unit-test-gen.test.ts
✅ Files skipped from review due to trivial changes (4)
- packages/opencode/src/tool/registry.ts
- .opencode/skills/dbt-unit-tests/references/incremental-testing.md
- .opencode/skills/dbt-unit-tests/references/unit-test-yaml-spec.md
- .opencode/skills/dbt-unit-tests/SKILL.md
Comment on lines
+24
to
+39
| export function loadRawManifest(manifestPath: string): any | null { | ||
| if (!fs.existsSync(manifestPath)) return null | ||
| const resolved = fs.realpathSync(manifestPath) | ||
| const mtimeMs = fs.statSync(resolved).mtimeMs | ||
|
|
||
| if (_manifestCache && _manifestCache.path === resolved && _manifestCache.mtimeMs === mtimeMs) { | ||
| return _manifestCache.data | ||
| } | ||
|
|
||
| const raw = fs.readFileSync(resolved, "utf-8") | ||
| const parsed = JSON.parse(raw) | ||
| if (typeof parsed !== "object" || parsed === null) { | ||
| throw new Error("Manifest is not a JSON object") | ||
| } | ||
| _manifestCache = { path: resolved, mtimeMs, data: parsed } | ||
| return parsed |
There was a problem hiding this comment.
Wire parseManifest() through this cache too.
generateDbtUnitTests() still parses the manifest via parseManifest() first and then calls dbtLineage(), so the manifest is still read/parsed once on the uncached path and then again on the cached one. On large manifests, this defeats the main performance win the new helper is trying to provide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/altimate/native/dbt/helpers.ts` around lines 24 - 39,
The parsed manifest is still produced twice because parseManifest() bypasses the
new _manifestCache used by loadRawManifest(); update parseManifest(manifestPath)
to call loadRawManifest(manifestPath) and use its returned parsed JSON when
available (falling back to existing parsing logic only if loadRawManifest
returns null), and ensure generateDbtUnitTests() and any callers (e.g.,
dbtLineage()) use parseManifest(manifestPath) so the single cached parse from
_manifestCache is reused across the flow.
anandgupta42
changed the title
Follow-up fixes for additional CodeRabbit comments on PR #674. Performance: - `parseManifest()` now uses `loadRawManifest()` from helpers.ts, so it goes through the path+mtime cache. Previously, `generateDbtUnitTests` called `parseManifest()` (which parsed the manifest directly) and then `dbtLineage()` (which went through the cache) — meaning large 128MB manifests were still parsed twice on the first call. Now both paths hit the same cache; second call is a no-op. Type correctness: - Add `database?: string` to `SchemaInspectParams` interface. All call sites in `unit-tests.ts` pass it for cross-database support, but the interface didn't declare it. Wiring through to individual drivers is a separate refactor (the `Connector.describeTable` signature would need to change across all 10+ drivers); documented in the handler. Robustness: - `generateDbtUnitTests` now warns when any upstream dep in `model.depends_on` cannot be resolved by `resolveUpstream`. This catches future dbt resource types (e.g., `semantic_model.*`) and manifest inconsistencies that would otherwise silently drop deps from the generated `given` block. New test: verifies unresolvable deps (e.g., `semantic_model.*`) produce a warning instead of silently being dropped. Full suite: 2677 pass, 0 fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update project docs to surface the new dbt unit test generation feature. - `docs/docs/data-engineering/tools/dbt-tools.md`: add `dbt_unit_test_gen` tool reference with parameters, scenario categories, dialect coverage, and example output. Add `/dbt-unit-tests` skill entry with workflow summary. - `docs/docs/data-engineering/tools/index.md`: bump dbt Tools row to "3 tools + 6 skills". - `docs/docs/configure/skills.md`: add `/dbt-unit-tests` to the skill reference table. - `packages/opencode/src/altimate/prompts/builder.txt`: add skill row to the dbt development table so the builder agent knows when to invoke `/dbt-unit-tests` vs `/dbt-test` (schema vs unit tests). - `CHANGELOG.md`: add Unreleased section documenting the feature, manifest parse cache, `description`/`adapter_type` additions. No functional changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/native/dbt/unit-tests.ts`:
- Around line 433-497: The mapping currently slices scenarios before building
tests so an incremental scenario can be dropped; update buildTests so that the
incremental scenario is always present: after computing the sliced list
(scenarios.slice(0, maxScenarios)) ensure any scenario with scenario.category
=== "incremental" appears in the list (if missing, append it or replace the last
entry) before mapping to tests; reference the local symbols scenarios,
maxScenarios, and the incremental check (scenario.category === "incremental")
and preserve the existing logic that pushes the mock input "this" into
test.given for incremental cases.
- Around line 460-467: The ephemeral SQL generation in the isEphemeral branch
builds column aliases using unquoted identifiers (`AS ${k}`), which breaks for
reserved words or names with special/mixed-case characters; update the sql
assembly in the isEphemeral return (the sql property created from rows.map and
Object.entries) to quote column identifiers using double quotes (DBT/SQL
standard) and ensure any internal double quotes in the column name are escaped
(replace " with ""), e.g., produce AS "columnName" for each key; reference the
same sql construction and formatSqlLiteral helper when making this change.
- Around line 364-365: The division detection regex in the if condition
(currently /\b\w+\s*\/\s*\w+/.test(cleaned)) is too narrow; update that check to
detect divisions between identifiers, literals, stars, function calls, and
CAST/COALESCE expressions so SQL like SUM(amount) / COUNT(*), CAST(a AS INT) /
CAST(b AS INT), and COALESCE(x,0) / COALESCE(y,1) are caught. Replace the regex
used on cleaned with a broader pattern that matches left and right operands
consisting of identifiers, *, numeric literals, parenthesized function calls, or
CAST/COALESCE forms (for example use a single regex that allows \w+\s*\([^)]*\),
CAST\([^)]*\), COALESCE\([^)]*\), \* or \w+ on both sides with \s*\/\s* between
them) and keep pushing the same scenarios.push({ category: "edge_case",
description: "Verify divide-by-zero protection", mockStyle: "boundary",
rowCount: 2 }) when it matches.
In `@packages/opencode/test/altimate/dbt-unit-test-gen.test.ts`:
- Around line 13-29: The test currently uses suite-level tmp and manifestCounter
with beforeEach/afterEach and manual disposal; instead, remove the global tmp
and manifestCounter and change tests to create per-test temp dirs with "await
using tmp = await tmpdir()" inside each test, then call a local helper (or
modify writeTmpManifest) to take tmp.path (or the tmp object) and produce
manifest files using that path; delete the beforeEach/afterEach hooks and update
any references to the globals (tmp, manifestCounter) to the per-test tmp and a
local counter or unique filename generation within the test so cleanup is
automatic when tmp goes out of scope.
🪄 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: bf942bd7-a4b7-4be3-be61-c55bbf971d5c
📒 Files selected for processing (5)
packages/opencode/src/altimate/native/connections/register.tspackages/opencode/src/altimate/native/dbt/manifest.tspackages/opencode/src/altimate/native/dbt/unit-tests.tspackages/opencode/src/altimate/native/types.tspackages/opencode/test/altimate/dbt-unit-test-gen.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/opencode/src/altimate/native/connections/register.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/src/altimate/native/dbt/manifest.ts
- packages/opencode/src/altimate/native/types.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/data-engineering/tools/dbt-tools.md`:
- Around line 104-118: Update the example unit test "test_fct_orders_happy_path"
under unit_tests for model "fct_orders" to correct the arithmetic in the expect
block: for the given input ref('stg_orders') where order_id 1 has quantity: 3
and unit_price: 100, change the expected row for order_id: 1 from order_total:
100 to order_total: 300 so the expect.rows reflect 3 × 100 = 300.
- Line 174: The code fence ending at the snippet that contains the command
string "You: /dbt-unit-tests fct_orders" is missing a language specifier; update
the triple-backtick fence to include a language (e.g., add ```text or ```bash)
so the block becomes a fenced code block with a language identifier around the
"You: /dbt-unit-tests fct_orders" line.
- Line 79: The code block containing the example command starting with
"dbt_unit_test_gen --manifest_path target/manifest.json --model fct_orders
--max_scenarios 5" is missing a language specifier; change the opening fence
from ``` to ```text so the block is annotated (e.g., replace the opening triple
backticks for that code fence with "```text") to satisfy rendering and linting.
🪄 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: 815543d4-643c-4d98-bda5-62d8c5e38775
📒 Files selected for processing (5)
CHANGELOG.mddocs/docs/configure/skills.mddocs/docs/data-engineering/tools/dbt-tools.mddocs/docs/data-engineering/tools/index.mdpackages/opencode/src/altimate/prompts/builder.txt
✅ Files skipped from review due to trivial changes (2)
- docs/docs/data-engineering/tools/index.md
- CHANGELOG.md
There was a problem hiding this comment.
1 issue found across 5 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="docs/docs/data-engineering/tools/dbt-tools.md">
<violation number="1" location="docs/docs/data-engineering/tools/dbt-tools.md:115">
P2: Example expected value is incorrect: `order_total` for `order_id: 1` should be `300` (3 × 100), not `100`. A wrong expected result in documentation will confuse users trying to understand the tool's output.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
PR #674 follow-up review comments + CI failure on main branch. Review fixes: - **Division detection regex broadened** — previously `\b\w+\s*/\s*\w+` only caught bare identifiers like `a / b`. Now matches common SQL patterns: `SUM(amount) / COUNT(*)`, `CAST(a AS INT) / CAST(b AS INT)`, `COALESCE(x, 0) / COALESCE(y, 1)`, `a.col / b.col`, and parenthesized expressions. Still excludes `/*` (block comment) and `//`. - **Incremental scenario preserved across max_scenarios cap** — `buildTests()` sliced `scenarios` before processing, so SQL with enough non-incremental triggers (JOIN + CASE + division + happy_path) would push the incremental test out of the default `max_scenarios = 3` window, dropping the `input: this` mock entirely. Now the incremental scenario is always kept in the capped window (replaces the last non-happy-path scenario if missing). - **Ephemeral SQL column aliases now quoted** — the generated ephemeral `format: sql` block used raw identifiers (`AS order_id`), which breaks for reserved keywords (`select`, `order`, `group`), mixed case, or special characters. Now uses ANSI-standard double-quoted identifiers (`AS "order_id"`) via a new `quoteIdent()` helper that also escapes embedded double quotes. - **Tests use `await using tmpdir()` per-test** — replaced suite-level `beforeEach`/`afterEach` hooks and shared `tmp`/`manifestCounter` globals with per-test `await using tmp = await tmpdir()`. Each test now owns its own disposable tmpdir with automatic cleanup when the scope ends. Matches project coding guideline: "Always use `await using` syntax with `tmpdir()` for automatic cleanup when the variable goes out of scope." CI fix: - **Flaky `oauth-browser.test.ts`** — the "BrowserOpenFailed event is NOT published when open() succeeds" test used a fixed 600ms sleep that wasn't enough on slow CI runners to let the authenticate() flow reach the mocked `open()` call. Now polls for `openCalledWith` to be defined (5s budget) before the 500ms detection window assertion. Test passes locally in 2s, no more flaky CI failures. Full suite: 2677 pass, 0 fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR #674 review fixes for dbt-tools.md documentation. - **Fix arithmetic error in example output** — the expected `order_total` for `order_id: 1` was `100`, but with `quantity: 3` and `unit_price: 100` the correct value is `3 × 100 = 300`. Flagged by both CodeRabbit (Critical) and cubic (P2). A wrong expected result in docs would mislead users about what the tool generates. - **Add `text` language specifier to two unlabeled code fences** — tool invocation example (line 79) and skill invocation example (line 174). Satisfies markdownlint MD040. No code changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/native/dbt/unit-tests.ts`:
- Line 183: Clamp and coerce params.max_scenarios into a positive integer before
using it for slicing: when computing maxScenarios (currently assigned from
params.max_scenarios ?? DEFAULT_MAX_SCENARIOS) ensure you parse/round it to an
integer and clamp to a minimum of 1 (e.g., Math.max(1, Math.floor(...))) so that
later slice(0, maxScenarios) cannot receive 0 or negative values and produce
empty/incorrect test sets.
🪄 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: 71fa30ee-ed7b-4bfe-8777-1e86ee3a5d93
📒 Files selected for processing (3)
packages/opencode/src/altimate/native/dbt/unit-tests.tspackages/opencode/test/altimate/dbt-unit-test-gen.test.tspackages/opencode/test/mcp/oauth-browser.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/test/altimate/dbt-unit-test-gen.test.ts
| ): Promise<DbtUnitTestGenResult> { | ||
| const warnings: string[] = [] | ||
| const antiPatterns: string[] = [] | ||
| const maxScenarios = params.max_scenarios ?? DEFAULT_MAX_SCENARIOS |
There was a problem hiding this comment.
Clamp max_scenarios to a positive integer before scenario capping.
max_scenarios is used as-is; 0/negative values make slice(0, maxScenarios) behave unexpectedly and can produce empty/incorrect test sets.
🛠️ Suggested fix
- const maxScenarios = params.max_scenarios ?? DEFAULT_MAX_SCENARIOS
+ const requestedMaxScenarios = params.max_scenarios ?? DEFAULT_MAX_SCENARIOS
+ const maxScenarios =
+ Number.isInteger(requestedMaxScenarios) && requestedMaxScenarios > 0
+ ? requestedMaxScenarios
+ : DEFAULT_MAX_SCENARIOS
+ if (params.max_scenarios !== undefined && maxScenarios !== params.max_scenarios) {
+ warnings.push(
+ `Invalid max_scenarios '${params.max_scenarios}'. Using default ${DEFAULT_MAX_SCENARIOS}.`,
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const maxScenarios = params.max_scenarios ?? DEFAULT_MAX_SCENARIOS | |
| const requestedMaxScenarios = params.max_scenarios ?? DEFAULT_MAX_SCENARIOS | |
| const maxScenarios = | |
| Number.isInteger(requestedMaxScenarios) && requestedMaxScenarios > 0 | |
| ? requestedMaxScenarios | |
| : DEFAULT_MAX_SCENARIOS | |
| if (params.max_scenarios !== undefined && maxScenarios !== params.max_scenarios) { | |
| warnings.push( | |
| `Invalid max_scenarios '${params.max_scenarios}'. Using default ${DEFAULT_MAX_SCENARIOS}.`, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/altimate/native/dbt/unit-tests.ts` at line 183, Clamp
and coerce params.max_scenarios into a positive integer before using it for
slicing: when computing maxScenarios (currently assigned from
params.max_scenarios ?? DEFAULT_MAX_SCENARIOS) ensure you parse/round it to an
integer and clamp to a minimum of 1 (e.g., Math.max(1, Math.floor(...))) so that
later slice(0, maxScenarios) cannot receive 0 or negative values and produce
empty/incorrect test sets.
There was a problem hiding this comment.
1 issue found across 4 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/native/dbt/unit-tests.ts">
<violation number="1" location="packages/opencode/src/altimate/native/dbt/unit-tests.ts:450">
P2: When `maxScenarios` is 1 (user-supplied `max_scenarios`), `capped` only contains the happy-path scenario. The replacement `capped[capped.length - 1] = incremental` overwrites it, violating the stated invariant that happy-path is always kept. Guard this to only replace when there's a non-happy-path slot available.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| if (incremental && !capped.includes(incremental)) { | ||
| // Replace the last non-happy-path scenario with the incremental one. | ||
| // Happy path is always first and must be kept. | ||
| capped[capped.length - 1] = incremental |
There was a problem hiding this comment.
P2: When maxScenarios is 1 (user-supplied max_scenarios), capped only contains the happy-path scenario. The replacement capped[capped.length - 1] = incremental overwrites it, violating the stated invariant that happy-path is always kept. Guard this to only replace when there's a non-happy-path slot 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/native/dbt/unit-tests.ts, line 450:
<comment>When `maxScenarios` is 1 (user-supplied `max_scenarios`), `capped` only contains the happy-path scenario. The replacement `capped[capped.length - 1] = incremental` overwrites it, violating the stated invariant that happy-path is always kept. Guard this to only replace when there's a non-happy-path slot available.</comment>
<file context>
@@ -430,7 +438,19 @@ function buildTests(
+ if (incremental && !capped.includes(incremental)) {
+ // Replace the last non-happy-path scenario with the incremental one.
+ // Happy path is always first and must be kept.
+ capped[capped.length - 1] = incremental
+ }
+
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds a
dbt_unit_test_gentool anddbt-unit-testsskill that generates dbt unit tests from manifest + compiled SQL.Engine (
packages/opencode/src/altimate/native/dbt/unit-tests.ts):parseManifest(),dbtLineage(),schema.inspect,sql.optimize— no reinvented wheelsinput: thismock for existing table stateformat: sqleven with no known columnsdatabaseparam inschema.inspectyamllibrary (not string concatenation)UnitTestContext(descriptions, lineage, compiled SQL) for LLM refinementShared infra (
helpers.ts):lineage.ts:loadRawManifest,findModel,getUniqueId,detectDialect,buildSchemaContextdescriptiontoDbtModelInfo/DbtSourceInfo,adapter_typetoDbtManifestResultSkill (
.opencode/skills/dbt-unit-tests/):Type of change
Issue for this PR
Closes #673
How did you verify your code works?
thismock, ephemeral sql format, deterministic naming, descriptions, lineage context, source depsChecklist
Summary by cubic
Adds automated dbt unit test generation via
dbt_unit_test_genand/dbt-unit-teststo meet AI-673. Faster and more reliable with better edge-case handling and manifest caching.New Features
dbt_unit_test_genbuilds unit test YAML from manifest + compiled SQL with deterministic names./dbt-unit-testsruns analyze → generate → refine → validate → write with scenario detection and type-correct mocks (incl. null/boundary and incrementalinput: this).format: sql, cross-database via dialect detect andschema.inspect; outputs YAML with rich context. Docs updated (dbt-tools.mdfixes arithmetic example and adds code fence labels) and builder prompts updated.Bug Fixes
seed.*andsnapshot.*deps viaref(); warn on unresolvabledepends_onentries.loadRawManifest()cache; stricter model lookups (resource_type === "model");SchemaInspectParamsadds optionaldatabase(driver limitation noted).max_scenarioscaps results.format: sqlaliases are now quoted to handle reserved/mixed-case identifiers.Written for commit ad962f6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests