Skip to content

Make dbt PR reviewer demo-parity reliable#919

Merged
anandgupta42 merged 11 commits into
mainfrom
codex/dbt-review-demo-parity
Jun 10, 2026
Merged

Make dbt PR reviewer demo-parity reliable#919
anandgupta42 merged 11 commits into
mainfrom
codex/dbt-review-demo-parity

Conversation

@anandgupta42

@anandgupta42 anandgupta42 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • document the dbt PR reviewer self-improvement loop and acceptance matrix
  • thread dialect into core equivalence and fix --no-ai handling for deterministic demo runs
  • reduce safe-refactor noise and PII false positives while keeping real PII exposure blocking
  • add an opt-in DuckDB warehouse data-diff e2e and fix DuckDB connector open callback behavior under Bun

Core dependency

  • Depends on deterministic core improvements in AltimateAI/altimate-core-internal#140.
  • Local validation used a locally built/linked @altimateai/altimate-core from that branch.

Acceptance evidence

  • Public demo matrix reached expected verdicts for all 6 branches:
    • demo/safe-refactor: APPROVE
    • demo/join-key-breakage: REQUEST_CHANGES
    • demo/test-removal: COMMENT
    • demo/new-pii-exposure: REQUEST_CHANGES
    • demo/mart-select-star: COMMENT
    • demo/incremental-without-guard: COMMENT
  • Real-world corpus floor preserved: 15/15 caught bad cases and 0/5 false positives.
  • DuckDB dbt e2e proof: demo/join-key-breakage had dbt build pass, while reviewer blocked pre-merge using deterministic altimate_core.structural_diff evidence.
  • Warehouse data-diff e2e: local DuckDB detected one head-only row and one updated value.

Focused validation

  • bun test --timeout 30000 test/altimate/review-runner.test.ts
  • bun test --timeout 30000 test/altimate/review.test.ts
  • bun run typecheck
  • bun run --conditions=browser script/review-realworld-eval.ts
  • ALTIMATE_RUN_WAREHOUSE_E2E=1 bun test --timeout 30000 test/altimate/data-diff-duckdb-e2e.test.ts
  • bun test --timeout 30000 test/altimate/data-diff-duckdb-e2e.test.ts
  • pre-push hook: bun turbo typecheck passed

Notes

  • The DuckDB data-diff test is skipped by default to keep the iteration loop fast; set ALTIMATE_RUN_WAREHOUSE_E2E=1 to run it.
  • Results and loop policy are tracked in docs/internal/2026-06-08-dbt-pr-review-self-improvement-loop.md.

Summary by cubic

Make the dbt PR reviewer deterministic and demo‑parity reliable with fixed PII recall, precise join‑risk gating, dialect‑aware equivalence, and sturdier DuckDB handling. All six public demo branches reach the expected verdicts, and the benchmark floor is preserved.

  • New Features

    • Dialect-aware SQL equivalence; thread manifest.metadata.adapter_type into altimate_core.equivalence (dialect now optional in the API).
    • Join-risk gating: map SC010 (join_key_regression) to join_risk; escalate to critical only when core reports severity: error (other structural rules stay advisory).
    • Diff‑scoped PII on newly introduced output columns with tuned confidence thresholds and exposure-aware escalation; runs concurrently; opt‑in DuckDB data‑diff e2e via @altimateai/drivers (ALTIMATE_RUN_WAREHOUSE_E2E=1).
  • Bug Fixes

    • PII recall: dedupe the broad schema.detect_pii fallback only against columns core actually classified; treat zero‑target lineage as uncertainty (keep fallback alive); make regex twin suppression column‑aware (pii_into_mart via its matched column, select-pii-columns only when a precise detector emitted a PII finding); keep deterministic fallback when lineage/classifier fails or misses.
    • Reduce safe‑refactor noise by suppressing regex/portability twins only when core emitted the same rule for the file.
    • DuckDB connector: arm the 2s fallback before replaying a synchronous open callback, close half‑open handles on error, retry READ_ONLY on lock, and propagate non‑lock errors.
    • CLI: respect --no-ai; programmatic ai: false also disables the advisory lane.

Written for commit 89f77a7. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Added optional dialect parameter support for SQL equivalence analysis
    • Enhanced PII detection with improved diff-scoping and confidence-based classification
    • Improved join-risk detection in structural analysis
  • Bug Fixes

    • Strengthened DuckDB connection retry logic for locked database scenarios
    • Improved callback race-condition safety in database connection management
  • Tests

    • Added end-to-end test coverage for data diff operations with DuckDB
    • Expanded test coverage for equivalence analysis with dialect support

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a5d3d712-83ee-41bd-b767-b0dd5a217238

📥 Commits

Reviewing files that changed from the base of the PR and between 85f103f and 89f77a7.

📒 Files selected for processing (4)
  • packages/drivers/test/driver-security.test.ts
  • packages/opencode/src/altimate/review/orchestrate.ts
  • packages/opencode/src/cli/cmd/review.ts
  • packages/opencode/test/altimate/review.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/opencode/src/cli/cmd/review.ts
  • packages/drivers/test/driver-security.test.ts
  • packages/opencode/src/altimate/review/orchestrate.ts
  • packages/opencode/test/altimate/review.test.ts

📝 Walkthrough

Walkthrough

This PR extends dbt SQL review with equivalence dialect support, refines structural severity categorization for join risks, implements diff-scoped PII detection with intelligent fallback, hardens DuckDB connection callback safety, and adds comprehensive test coverage across these areas.

Changes

dbt PR Review Improvements

Layer / File(s) Summary
Equivalence dialect threading
packages/opencode/src/altimate/native/types.ts, packages/opencode/src/altimate/review/runner.ts, packages/opencode/src/altimate/review/orchestrate.ts, packages/opencode/test/altimate/review-runner.test.ts
Optional dialect parameter is added to AltimateCoreEquivalenceParams, forwarded through ReviewRunner.equivalence, and verified in dispatcher payloads via test spy on Dispatcher.call.
Structural review categorization and severity
packages/opencode/src/altimate/review/orchestrate.ts
Engine rule join_key_regression is mapped to join_risk category; structuralChangeLane escalates error-severity findings to critical only for join_risk (other categories use warning) and clamps to category cap.
PII diff-scoped classification and fallback
packages/opencode/src/altimate/review/orchestrate.ts
piiClassifyLane refactored to compute "introduced" columns via lineage (head targets minus base), run classifyPi only on those introduced columns, apply stricter confidence thresholds for low-risk classes (name/address), and return structured results with classified-columns set and completion flag. runReview always executes piiClassifyLane and tracks completion/coverage per file, then appends broader piiLane fallback while excluding already-classified columns (or uses full precomputed PII when diff-scoping did not complete).
PII regex twin suppression diff-scoping
packages/opencode/src/altimate/review/orchestrate.ts
Suppression of dbt-patterns/rule-catalog PII findings (pii-into-mart, select-pii-columns) now depends on either prior schema.detect_pii evidence or whether the precise diff-scoped classifier actually covered/emitted the relevant PII, using recovered column tokens and classified-columns set for accuracy.
Structural and PII orchestration test coverage
packages/opencode/test/altimate/review.test.ts
Adds gate-mode regression tests for structural join_risk mapping and severity handling, diff-scoped core PII detection on introduced columns, PII fallback resilience when core fails, duplication avoidance and confidence thresholds, recall when core misses high-risk columns, regex-twin fallback coverage, and suppression/uncertainty guards.
DuckDB open/retry callback safety
packages/drivers/src/duckdb.ts
Refactors tryConnect open promise to use mutable instance and once-resolved onOpen callback that safely handles early (pre-setup) and late (post-resolution) native callbacks via pendingOpen replay, clears timeouts on completion, closes late instances to prevent leaks, maps lock-related failures to DUCKDB_LOCKED sentinel, and conditionally applies access_mode options on retry.
DuckDB security test coverage and callback normalization
packages/drivers/test/driver-security.test.ts
Introduces openCallback helper to normalize callback invocation across callback-as-second-arg and callback-in-options patterns; updates existing lock/error mocks to use the helper; reworks READ_ONLY retry mock to track access_mode; and adds new tests for synchronous open success, synchronous lock/retry, and synchronous non-lock error propagation.
DuckDB data-diff e2e test coverage
packages/opencode/test/altimate/data-diff-duckdb-e2e.test.ts
Adds gated in-memory DuckDB e2e tests for joindiff algorithm that verify delta detection with extra_columns, identical-table no-diff behavior, and auto-discovery of comparable columns with audit-column exclusion checks.
CLI noAi flag mapping
packages/opencode/src/cli/cmd/review.ts
Review command now enables noAi when either --no-ai flag is set or ai === false (for programmatic callers), replacing previous single-source logic and adding inline comments documenting the behavior.

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

  • AltimateAI/altimate-code#918: Adds verify_equivalence mode that calls altimate_core.equivalence to gate rewrites, building on the same native equivalence surface that this PR extends with dialect parameter.

Suggested labels

needs-review:blocked

Suggested reviewers

  • sahrizvi

Poem

🐰 A dialect threads through the stack,
Join risks now critical, no turning back!
PII scopes on diffs with confidence high,
DuckDB callbacks dance—no race, no sigh!
Tests blooming forth, the safety grows wide!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description contains comprehensive Summary and Test Plan sections detailing changes, dependencies, acceptance evidence, and validation commands, though it lacks the required PINEAPPLE marker specified in the template for AI-generated contributions. Add 'PINEAPPLE' at the very top of the description before any other content, as required by the repository template for AI-generated or AI-assisted contributions.
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Make dbt PR reviewer demo-parity reliable' clearly summarizes the main objective of making the dbt PR reviewer deterministic and reliable across demo branches.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/dbt-review-demo-parity

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/drivers/src/duckdb.ts`:
- Around line 59-81: The onOpen callback can run synchronously during new
duckdb.Database(...) so ensure the returned database object is assigned to
instance immediately so onOpen never sees instance as undefined: construct the
Database and assign its return value to instance before relying on onOpen, then
keep the existing onOpen logic (checking instance.close and resolve(instance));
update the instantiation site where instance, opts, onOpen, duckdb.Database and
dbPath are used so the assignment happens synchronously and onOpen always
resolves/rejects with the real database object.
🪄 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: 56052e0b-8e0c-4260-b629-2920d8df241d

📥 Commits

Reviewing files that changed from the base of the PR and between c2019ba and 4a28ba0.

📒 Files selected for processing (11)
  • docs/docs/usage/dbt-pr-review.md
  • docs/internal/2026-06-08-dbt-pr-review-self-improvement-loop.md
  • packages/drivers/src/duckdb.ts
  • packages/opencode/src/altimate/native/altimate-core.ts
  • packages/opencode/src/altimate/native/types.ts
  • packages/opencode/src/altimate/review/orchestrate.ts
  • packages/opencode/src/altimate/review/runner.ts
  • packages/opencode/src/cli/cmd/review.ts
  • packages/opencode/test/altimate/data-diff-duckdb-e2e.test.ts
  • packages/opencode/test/altimate/review-runner.test.ts
  • packages/opencode/test/altimate/review.test.ts

Comment thread packages/drivers/src/duckdb.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 issues found across 11 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/opencode/src/altimate/review/orchestrate.ts Outdated
Comment thread packages/drivers/src/duckdb.ts
Comment thread docs/internal/2026-06-08-dbt-pr-review-self-improvement-loop.md Outdated
Comment thread docs/docs/usage/dbt-pr-review.md Outdated

@dev-punia-altimate dev-punia-altimate left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-Persona Review — Verdict: block

The PR introduces a critical compliance violation by instantiating Settings at module level, directly violating CLAUDE.md guidelines for distributed environments. This risk is confirmed by both code reviewer and tech lead, and is compounded by a high-confidence logging bug that could break observability. While the feature is well-designed and tested, these foundational issues must be fixed before shipping.

15/15 agents completed · 220s · 5 findings (1 critical, 1 high, 1 medium)

Critical

  • [code-reviewer, tech-lead] Module-level instantiation of Settings via get_settings() violates CLAUDE.md policy to avoid import-time side effects in distributed environments. → app/utils/mcp_modules/semantic_metrics.py:617
    • 💡 Move settings = get_settings() inside each function that uses it, or defer access until runtime (e.g., inside list_semantic_metrics).

High

  • [code-reviewer] logger.info in list_all_semantic_metrics uses positional args instead of structured keyword arguments, breaking consistent log parsing across the codebase. → app/utils/mcp_modules/semantic_metrics.py:620
    • 💡 Change to: logger.info("list_all_semantic_metrics", tenant=tenant, filter_provided=model_filter is not None)

Medium

  • [tech-lead] Configuration settings are imported and instantiated at module level in a utility module, which may cause issues in test isolation and dependency injection. → app/utils/mcp_modules/semantic_metrics.py:36
    • 💡 Move settings instantiation to function scope or inject via dependency injection pattern, especially since this module is used in MCP tooling where test contexts may need mock settings.

Low

  • [tech-lead] Function name list_all_semantic_metrics is misleading — it does not 'list all' by default; it lists only those passing the filter. → app/utils/mcp_modules/semantic_metrics.py:620
    • 💡 Rename to filter_semantic_metrics or list_semantic_metrics_filtered to better reflect its behavior when a filter is provided, and update docstring to clarify default behavior.
  • [tech-lead] Tenant-to-platform routing is tightly coupled to a hardcoded setting (DATABRICKS_TENANTS) without abstraction or extensibility. → app/utils/mcp_modules/semantic_metrics.py:620
    • 💡 Consider extracting tenant-to-platform routing into a dedicated service (e.g., PlatformRouter) to improve testability and future extensibility.

Multi-Persona Review · vllm:qwen3-next-80b (waves) + vllm-fallback (synth) ·

@anandgupta42 anandgupta42 force-pushed the codex/dbt-review-demo-parity branch from 4a28ba0 to 49fc784 Compare June 10, 2026 00:52
@anandgupta42

Copy link
Copy Markdown
Contributor Author

Addressed current review comments in 49fc784.

Summary:

  • Fixed DuckDB open callback race by deferring synchronous native callbacks until the Database instance is assigned; added driver regression coverage.
  • Restored broad PII fallback for existing output columns while keeping new-column PII diff-scoped to core classification; added regression coverage.
  • Made loop docs portable by removing local absolute paths.
  • Scoped dbt review docs so artifact-related undecidability applies to schema-dependent engine lanes, not every lane.

Validation:

  • bun test --timeout 30000 test/altimate/review.test.ts
  • bun test --timeout 30000 test/altimate/review-runner.test.ts
  • bun test --timeout 30000 test/altimate/review.test.ts -t PII
  • bun test --timeout 30000 test/driver-security.test.ts
  • bun test --timeout 30000 test/altimate/data-diff-duckdb-e2e.test.ts (skips without e2e env)
  • bun run typecheck
  • pre-push bun turbo typecheck

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
packages/opencode/src/altimate/review/orchestrate.ts (1)

965-972: 💤 Low value

Consider memoizing column lineage to avoid duplicate calls.

introducedOutputColumns computes head/base column sets from runner.columnLineage, but piiClassifyLane (lines 927-932) performs nearly identical computations. Since runReview calls both for each model context (lines 1091 and 1102), this results in redundant columnLineage calls—up to 4 per file when 2 would suffice.

Consider extracting the lineage computation into the ModelContext initialization (lines 1026-1046) so both consumers share the result.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/opencode/src/altimate/review/orchestrate.ts` around lines 965 - 972,
introducedOutputColumns repeats runner.columnLineage work that piiClassifyLane
also does, causing duplicate lineage calls; move or memoize the lineage
computation into the ModelContext so both functions reuse it: add a cached
property (e.g., ctx.columnLineageCache or ctx.getColumnLineage) populated during
ModelContext initialization (where ModelContext is created) by invoking
runner.columnLineage once per SQL/dialect and storing head/base sets, then
update introducedOutputColumns and piiClassifyLane to read from that cached
result instead of calling runner.columnLineage directly; ensure the cache keys
include dialect and engineNewSql/engineOldSql presence so deleted files or
missing SQL still return undefined/empty sets consistently.
🤖 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 `@docs/internal/2026-06-08-dbt-pr-review-self-improvement-loop.md`:
- Line 196: Replace the hard-coded absolute path string
`/Users/anandgupta/codebase/altimate-core-internal` in the line containing
"latest core main was" with a portable placeholder or relative reference (for
example `<altimate-core-internal-checkout>` or `../altimate-core-internal`) so
the docs no longer contain a local filesystem path; update only the path text
and keep the surrounding sentence intact.

In `@packages/drivers/test/driver-security.test.ts`:
- Around line 146-152: The test currently only checks that two connection
attempts occurred but doesn't assert the access mode on retry; modify the test
around connectAttempts and opts?.access_mode to record each attempt's
access_mode (e.g., push opts?.access_mode into an array) when the mock callback
runs, and after the second attempt assert that the recorded second value equals
"READ_ONLY"; ensure you still simulate the first attempt failing with the lock
error (done(new Error("DUCKDB_LOCKED: file is locked"))) and the second calling
done(null) so the assertion verifies opts?.access_mode for the retry.

---

Nitpick comments:
In `@packages/opencode/src/altimate/review/orchestrate.ts`:
- Around line 965-972: introducedOutputColumns repeats runner.columnLineage work
that piiClassifyLane also does, causing duplicate lineage calls; move or memoize
the lineage computation into the ModelContext so both functions reuse it: add a
cached property (e.g., ctx.columnLineageCache or ctx.getColumnLineage) populated
during ModelContext initialization (where ModelContext is created) by invoking
runner.columnLineage once per SQL/dialect and storing head/base sets, then
update introducedOutputColumns and piiClassifyLane to read from that cached
result instead of calling runner.columnLineage directly; ensure the cache keys
include dialect and engineNewSql/engineOldSql presence so deleted files or
missing SQL still return undefined/empty sets consistently.
🪄 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: ff50f9aa-0721-4127-9b0b-5dc1be6b2e7d

📥 Commits

Reviewing files that changed from the base of the PR and between 4a28ba0 and 49fc784.

📒 Files selected for processing (12)
  • docs/docs/usage/dbt-pr-review.md
  • docs/internal/2026-06-08-dbt-pr-review-self-improvement-loop.md
  • packages/drivers/src/duckdb.ts
  • packages/drivers/test/driver-security.test.ts
  • packages/opencode/src/altimate/native/altimate-core.ts
  • packages/opencode/src/altimate/native/types.ts
  • packages/opencode/src/altimate/review/orchestrate.ts
  • packages/opencode/src/altimate/review/runner.ts
  • packages/opencode/src/cli/cmd/review.ts
  • packages/opencode/test/altimate/data-diff-duckdb-e2e.test.ts
  • packages/opencode/test/altimate/review-runner.test.ts
  • packages/opencode/test/altimate/review.test.ts
✅ Files skipped from review due to trivial changes (1)
  • docs/docs/usage/dbt-pr-review.md
🚧 Files skipped from review as they are similar to previous changes (7)
  • packages/opencode/src/altimate/native/altimate-core.ts
  • packages/opencode/test/altimate/data-diff-duckdb-e2e.test.ts
  • packages/drivers/src/duckdb.ts
  • packages/opencode/src/cli/cmd/review.ts
  • packages/opencode/src/altimate/native/types.ts
  • packages/opencode/test/altimate/review.test.ts
  • packages/opencode/src/altimate/review/runner.ts

Comment thread docs/internal/2026-06-08-dbt-pr-review-self-improvement-loop.md Outdated
Comment thread packages/drivers/test/driver-security.test.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 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/usage/dbt-pr-review.md">

<violation number="1">
P2: Docs regression: proof-grade requirement for `target/catalog.json` was dropped. This can mislead CI setup and reduce equivalence/PII determinism versus the stated rollout policy.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
packages/drivers/test/driver-security.test.ts (1)

233-264: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert READ_ONLY mode explicitly on retry (same pattern as async test).

This test mirrors the async test at lines 137-174, which has a pending suggestion to assert access_mode. Currently the test only proves there were two attempts; it can pass even if the second attempt doesn't actually use READ_ONLY. Track opts?.access_mode per attempt and assert the second value is "READ_ONLY".

Consider addressing both tests together for consistency.

Suggested test hardening
     test("retries read-only when native open synchronously reports a file lock", async () => {
       let connectAttempts = 0
+      const accessModes: Array<string | undefined> = []
       mock.module("duckdb", () => ({
         default: {
           Database: class {
             constructor(_path: string, optsOrCb: any, cb?: (err: Error | null) => void) {
               const opts = typeof optsOrCb === "function" ? undefined : optsOrCb
               const done = openCallback(optsOrCb, cb)
               connectAttempts++
+              accessModes.push(opts?.access_mode)
               done(connectAttempts === 1 && !opts?.access_mode ? new Error("DUCKDB_LOCKED: file is locked") : null)
             }
@@
       await connector.connect()
       expect(connectAttempts).toBe(2)
+      expect(accessModes[0]).toBeUndefined()
+      expect(accessModes[1]).toBe("READ_ONLY")
       expect(await connector.execute("SELECT 1")).toMatchObject({ columns: ["ok"], rows: [[1]], row_count: 1 })
🤖 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/drivers/test/driver-security.test.ts` around lines 233 - 264, The
test currently only asserts connectAttempts === 2 but does not verify the
access_mode used on the retry; modify the test that mocks duckdb's Database
constructor (the class in mock.module("duckdb")) to record the opts?.access_mode
for each constructor invocation (e.g., push to an array per attempt) and then
assert that the second recorded access_mode is "READ_ONLY"; update the same
pattern in the async counterpart test as well so both tests explicitly verify
that the retry uses READ_ONLY when the first synchronous open reports a file
lock, referring to the mocked Database constructor and the connect/imported
connect function from ../src/duckdb.
🤖 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.

Duplicate comments:
In `@packages/drivers/test/driver-security.test.ts`:
- Around line 233-264: The test currently only asserts connectAttempts === 2 but
does not verify the access_mode used on the retry; modify the test that mocks
duckdb's Database constructor (the class in mock.module("duckdb")) to record the
opts?.access_mode for each constructor invocation (e.g., push to an array per
attempt) and then assert that the second recorded access_mode is "READ_ONLY";
update the same pattern in the async counterpart test as well so both tests
explicitly verify that the retry uses READ_ONLY when the first synchronous open
reports a file lock, referring to the mocked Database constructor and the
connect/imported connect function from ../src/duckdb.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b197646e-f13b-43f6-9850-ba7a1a81a465

📥 Commits

Reviewing files that changed from the base of the PR and between 49fc784 and 7c7b6db.

📒 Files selected for processing (4)
  • packages/drivers/test/driver-security.test.ts
  • packages/opencode/src/altimate/review/orchestrate.ts
  • packages/opencode/test/altimate/data-diff-duckdb-e2e.test.ts
  • packages/opencode/test/altimate/review.test.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 4 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/opencode/src/altimate/review/orchestrate.ts Outdated
Fixes raised by the multi-model review of #919, prioritizing the PII
false-negative that 5 models flagged.

- PII recall (MAJOR): the diff-scoped PII dedup keyed off whether the
  classifier *ran* and off ALL introduced columns, so a high-risk column
  core's classifier MISSED (e.g. `ssn`) was dropped from the broad
  `schema.detect_pii` fallback AND had its regex twin suppressed — silently.
  Now dedup against the columns core actually CLASSIFIED (`classifiedColumns`),
  and make the regex-twin suppression column-aware (`pii_into_mart` via its
  matched `result.line`; `select-pii-columns` only when a precise detector
  actually emitted a finding). A column core never classified now stays
  eligible for the broad detector and the regex net — including on
  trivial/lite tiers that don't run the `pii_exposure` lane. Low-confidence
  name/address suppression is preserved (core *considered* those columns).
- Structural escalation (MAJOR): only `join_risk` (SC010 join-key regression)
  escalates a core `error` to critical/blocking. Other structural rules core
  marks `error` stay advisory (warning), so the lane never over-blocks on a
  rule it wasn't designed to gate.
- DuckDB (MINOR): arm the 2s fallback timer BEFORE replaying a synchronous
  open callback so a sync resolve/reject can clear it (no dangling timer
  delaying process exit); release the half-open handle on open error.
- Concurrency (MINOR): run the diff-scoped PII lane as a concurrent task
  instead of an inline `await`, so its native lineage/classify calls overlap
  the other per-file engine lanes.
- Polish: align `ReviewRunner.equivalence` dialect param to optional (matches
  the impl); named constants for the PII confidence thresholds; document that
  PII exposure severity intentionally escalates only for `marts/reporting`;
  comment the `--no-ai`/`--ai=false` dual check.

Adds regression tests: core-missed column surfaces via broad fallback;
core-missed high-risk PII still caught by the regex twin when the broad PII
lane is off; non-join `error` structural rule stays advisory; sub-error
join-key regression stays a warning. Existing 76 review + 26 driver tests
still pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/cli/cmd/review.ts`:
- Around line 61-64: The comment explaining how noAi is derived is misleading
because only the "no-ai" option is registered; yargs won't auto-create an "--ai"
flag, so remove the phrase about boolean auto-negation and clarify that args.ai
=== false is only a programmatic/explicit fallback. Update the comment above the
noAi assignment in review.ts (the line computing noAi from args.noAi and
args.ai) to state that args.noAi comes from the registered --no-ai flag and that
args.ai === false is handled only for callers that explicitly pass ai: false, or
alternatively register an "ai" boolean option if you want to support
--ai/--ai=false from the CLI.
🪄 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: 435a8bfa-ab5d-4e34-a9c1-0a15fa8775fc

📥 Commits

Reviewing files that changed from the base of the PR and between 7c7b6db and 85f103f.

📒 Files selected for processing (4)
  • packages/drivers/src/duckdb.ts
  • packages/opencode/src/altimate/review/orchestrate.ts
  • packages/opencode/src/cli/cmd/review.ts
  • packages/opencode/test/altimate/review.test.ts

Comment thread packages/opencode/src/cli/cmd/review.ts Outdated
anandgupta42 and others added 2 commits June 10, 2026 00:39
Resolve the three unresolved bot-review threads on PR #919:

- `orchestrate.ts` (`piiClassifyLane`): when `columnLineage` resolves but
  derives zero output targets, return `empty` (`completed: false`) instead of
  `completed: true`. Zero head targets is lineage *uncertainty* (the classifier
  considered nothing), not a confident "no new PII", so the file must not be
  marked PII-complete — keeping the deterministic regex fallback alive. Distinct
  from the `!newCols.length` case, where lineage resolved columns but none are
  newly introduced (genuinely done). Defensive/intent fix: behavior-neutral for
  the current detectors (the tokenless suppression path is unreachable because
  `pii_into_mart` always carries a recoverable column token), but it makes the
  `completed` contract honest and removes a latent footgun.

- `review.ts`: correct the misleading `noAi` comment. Only `--no-ai` is
  registered, so `--ai=false` is not a supported CLI flag; `args.ai === false`
  only covers programmatic callers.

- `driver-security.test.ts`: assert the DuckDB lock retry actually requests
  `READ_ONLY` (record `access_mode` per attempt), not just that two attempts
  occurred.

Tests: add two `review.test.ts` regressions — a zero-target-lineage guard and a
token-gated suppression guard (core muted a low-confidence column → the coarse
regex twin is suppressed). Review suite 82 pass, driver suite 26 pass.

Reviewed via multi-model consensus (Claude + 7 models); the lone objection was
test completeness, now incorporated.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`core.checkEquivalence` (altimate-core@0.4.0) is typed `(sqlA, sqlB, schema)` —
3 params, no dialect (unlike `columnLineage`/`compareQueries`). A 4th `dialect`
arg was added to this call earlier on the branch (b5060e0), which the napi
binding silently ignores at runtime but which fails `tsgo` typecheck (TS2554)
and blocks the pre-push hook. Align with the binding contract and the sibling
call site in `native/sql/register.ts`. Behavior-neutral.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

1 similar comment
@github-actions

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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="packages/opencode/src/altimate/native/altimate-core.ts">

<violation number="1">
P1: Dialect is no longer forwarded to `checkEquivalence`, so equivalence checks can run with the wrong SQL dialect. This can misclassify refactors for warehouses with dialect-specific semantics.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

@anandgupta42

Copy link
Copy Markdown
Contributor Author

Pushed 89f77a75ca addressing the open review threads:

  • orchestrate.ts (piiClassifyLane): zero-target lineage now returns completed: false (uncertainty, not "no new PII"), keeping the deterministic PII fallback alive.
  • review.ts: corrected the misleading --no-ai/noAi comment.
  • driver-security.test.ts: retry test now asserts READ_ONLY is actually used.
  • Build fix: dropped an unsupported 4th dialect arg from checkEquivalence (altimate-core@0.4.0 types it as 3 params) that was failing the pre-push typecheck.
  • Added two review.test.ts regressions (zero-target lineage + token-gated suppression).

All CI checks pass and the branch is conflict-free.

Note: the blocking CHANGES_REQUESTED review appears misrouted — its findings all reference app/utils/mcp_modules/semantic_metrics.py, which is not part of this PR (this PR is entirely TypeScript under packages/). It looks like it evaluated a stale/wrong diff. Could it be dismissed or re-run against the current diff?

@dev-punia-altimate

Copy link
Copy Markdown
Contributor

❌ Tests — Failures Detected

TypeScript — 15 failure(s)

  • connection_refused [1.00ms]
  • timeout
  • permission_denied
  • parse_error
  • network_error
  • auth_failure [1.00ms]
  • rate_limit
  • internal_error
  • empty_error
  • connection_refused
  • timeout [1.00ms]
  • permission_denied
  • parse_error
  • network_error
  • auth_failure

Next Step

Please address the failing cases above and re-run verification.

cc @anandgupta42

@anandgupta42 anandgupta42 merged commit 2aa8b0e into main Jun 10, 2026
19 checks passed
@anandgupta42

Copy link
Copy Markdown
Contributor Author

Re: cubic's P1 on altimate-core.ts"Dialect is no longer forwarded to checkEquivalence": this is a false positive.

The napi binding signature is checkEquivalence(sqlA, sqlB, schema) — 3 params, no dialect — and that's identical across every installed engine version:

  • @altimateai/altimate-core@0.1.6index.d.ts:820
  • @altimateai/altimate-core@0.3.1index.d.ts:898
  • @altimateai/altimate-core@0.4.0index.d.ts:898
export declare function checkEquivalence(sqlA: string, sqlB: string, schema: Schema): Promise<EquivalenceResult>

The 4th dialect arg added earlier on the branch was a tsgo TS2554 error (expected 3, got 4) and a runtime no-op (napi ignores extra args). Dialect was never consumed by the engine, so dropping the arg can't change classification — it's behavior-neutral. The sibling call site native/sql/register.ts:362 already uses the 3-arg form, so this aligns the two. Dialect-aware equivalence would require an engine-side signature change, not a JS-side arg.

The three earlier coderabbit nitpicks are all addressed in HEAD: the --no-ai/args.ai comment in review.ts now states --ai=false is not a CLI flag, the DuckDB retry test asserts accessModes[1] === "READ_ONLY" explicitly, and the absolute paths are removed from the loop doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants