fix: address pre-release review findings for v0.5.15#580
Merged
anandgupta42 merged 2 commits intomainfrom Mar 30, 2026
Merged
Conversation
…s, driver tests, docs - Remove duplicate `altimate_change end` markers in `prompt.ts` - Replace `password: "secret"` with `"test-fake-password"` in test fixtures - Fix `classifySkillTrigger()` to return `"unknown"` for unexpected triggers - Remove unused `"schema_not_indexed"` from `feature_suggestion` type - Add null guards in `sql-translate`, `dbt-manifest`, `dbt-lineage`, `column-lineage`, `track-lineage` to prevent literal "undefined" display - Propagate `result.error` to metadata in `dbt-manifest` - Add 24 driver security tests (DuckDB retry, MongoDB blocking, PG/Redshift validation, registry unsupported DB hints) - Document DuckDB concurrent access, MongoDB blocked operators, unsupported databases in `warehouses.md` Closes #579 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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds driver security tests and documentation for warehouse constraints, applies defensive null guards in several formatter tools, narrows a telemetry union and changes a fallback classification, and updates a few test fixtures and comment markers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/opencode/src/altimate/tools/dbt-lineage.ts (1)
25-29:⚠️ Potential issue | 🟡 MinorInconsistent null guards:
titleuses unguarded values.The
formatDbtLineagefunction now correctly guardsresult.model_nameandresult.confidencewith?? "unknown", but thetitleat line 25 still interpolates these values directly. If either is undefined, the title will display "undefined".🛡️ Apply consistent null guards in the title
return { - title: `dbt Lineage: ${result.model_name} [${result.confidence}]`, + title: `dbt Lineage: ${result.model_name ?? "unknown"} [${result.confidence ?? "unknown"}]`, metadata: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/dbt-lineage.ts` around lines 25 - 29, The title interpolation in formatDbtLineage uses unguarded result.model_name and result.confidence and can render "undefined"; update the title construction to use the same null-guards as the metadata (e.g., result.model_name ?? "unknown" and result.confidence ?? "unknown") so the title string and metadata stay consistent (look for formatDbtLineage, the title assignment, and the metadata object to apply the change).packages/opencode/test/telemetry/plan-skill-telemetry.test.ts (2)
13-16:⚠️ Potential issue | 🔴 CriticalTest expectations are stale after implementation change.
With the change to
classifySkillTrigger()returning"unknown"for unrecognized triggers (line 873 in telemetry/index.ts), these assertions will fail:
classifySkillTrigger({})now returns"unknown", not"llm_selected"classifySkillTrigger({ foo: "bar" })now returns"unknown", not"llm_selected"🧪 Proposed fix
- test("returns 'llm_selected' when extra has no trigger field", () => { - expect(Telemetry.classifySkillTrigger({})).toBe("llm_selected") - expect(Telemetry.classifySkillTrigger({ foo: "bar" })).toBe("llm_selected") + test("returns 'unknown' when extra has no trigger field", () => { + expect(Telemetry.classifySkillTrigger({})).toBe("unknown") + expect(Telemetry.classifySkillTrigger({ foo: "bar" })).toBe("unknown") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/telemetry/plan-skill-telemetry.test.ts` around lines 13 - 16, The test expectations are stale because Telemetry.classifySkillTrigger now returns "unknown" for unrecognized or empty triggers; update the assertions in plan-skill-telemetry.test.ts so they expect "unknown" for inputs like {} and { foo: "bar" }, or alternatively provide a recognized trigger field (e.g., { trigger: "llm_selected" }) to assert "llm_selected"; target the calls to Telemetry.classifySkillTrigger in the test and change the expected values accordingly.
30-33:⚠️ Potential issue | 🔴 CriticalTest expectations are stale after implementation change.
With the change to
classifySkillTrigger()returning"unknown"for unrecognized trigger values, these assertions will fail:
classifySkillTrigger({ trigger: "something_else" })now returns"unknown", not"llm_selected"classifySkillTrigger({ trigger: 42 })now returns"unknown", not"llm_selected"🧪 Proposed fix
- test("returns 'llm_selected' for unrecognized trigger values", () => { - expect(Telemetry.classifySkillTrigger({ trigger: "something_else" })).toBe("llm_selected") - expect(Telemetry.classifySkillTrigger({ trigger: 42 })).toBe("llm_selected") + test("returns 'unknown' for unrecognized trigger values", () => { + expect(Telemetry.classifySkillTrigger({ trigger: "something_else" })).toBe("unknown") + expect(Telemetry.classifySkillTrigger({ trigger: 42 })).toBe("unknown") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/telemetry/plan-skill-telemetry.test.ts` around lines 30 - 33, Update the test to match the new behavior of Telemetry.classifySkillTrigger: change the expected return value from "llm_selected" to "unknown" for unrecognized trigger inputs (e.g., Telemetry.classifySkillTrigger({ trigger: "something_else" }) and Telemetry.classifySkillTrigger({ trigger: 42 })). Optionally rename the test description to reflect that unrecognized triggers return "unknown" instead of "llm_selected".packages/opencode/src/altimate/telemetry/index.ts (1)
868-874:⚠️ Potential issue | 🔴 CriticalTest-implementation mismatch will cause test failures.
The change to return
"unknown"for unrecognized triggers is correct per PR objectives, but the tests inplan-skill-telemetry.test.tswere not updated:
- Lines 13-16:
classifySkillTrigger({})andclassifySkillTrigger({ foo: "bar" })now return"unknown", but tests expect"llm_selected".- Lines 30-33:
classifySkillTrigger({ trigger: "something_else" })andclassifySkillTrigger({ trigger: 42 })now return"unknown", but tests expect"llm_selected".🧪 Required test updates in plan-skill-telemetry.test.ts
Update lines 13-16:
test("returns 'llm_selected' when extra has no trigger field", () => { - expect(Telemetry.classifySkillTrigger({})).toBe("llm_selected") - expect(Telemetry.classifySkillTrigger({ foo: "bar" })).toBe("llm_selected") + expect(Telemetry.classifySkillTrigger({})).toBe("unknown") + expect(Telemetry.classifySkillTrigger({ foo: "bar" })).toBe("unknown") })Update lines 30-33:
- test("returns 'llm_selected' for unrecognized trigger values", () => { - expect(Telemetry.classifySkillTrigger({ trigger: "something_else" })).toBe("llm_selected") - expect(Telemetry.classifySkillTrigger({ trigger: 42 })).toBe("llm_selected") + test("returns 'unknown' for unrecognized trigger values", () => { + expect(Telemetry.classifySkillTrigger({ trigger: "something_else" })).toBe("unknown") + expect(Telemetry.classifySkillTrigger({ trigger: 42 })).toBe("unknown") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/telemetry/index.ts` around lines 868 - 874, The tests in plan-skill-telemetry.test.ts still expect "llm_selected" for unrecognized or missing triggers but classifySkillTrigger now returns "unknown" for those cases; update the assertions in that test file to expect "unknown" for calls classifySkillTrigger({}), classifySkillTrigger({ foo: "bar" }), classifySkillTrigger({ trigger: "something_else" }), and classifySkillTrigger({ trigger: 42 }) so the tests match the new behavior of the classifySkillTrigger function.
🧹 Nitpick comments (2)
packages/opencode/src/altimate/tools/dbt-manifest.ts (1)
24-24: Type cast(result as any).errorindicates a missing type definition.The
DbtManifestResultinterface (intypes.ts:197-206) doesn't include anerrorfield, necessitating this cast. Consider updating the interface to includeerror?: stringfor type safety.Additionally, other consumers of
Dispatcher.call("dbt.manifest", ...)(e.g.,impact-analysis.ts:32-43,project-scan.ts:498-505) don't check for this error field and may silently proceed with partial/invalid data.🔧 Update the interface to include the error field
In
packages/opencode/src/altimate/native/types.ts:export interface DbtManifestResult { models: DbtModelInfo[] sources: DbtSourceInfo[] tests: DbtTestInfo[] source_count: number model_count: number test_count: number snapshot_count: number seed_count: number + error?: string }Then in this file, replace the cast:
- ...((result as any).error && { error: (result as any).error }), + ...(result.error && { error: result.error }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/dbt-manifest.ts` at line 24, Add an optional error field to the DbtManifestResult interface (e.g., error?: string) so we don't need to cast (result as any).error; then remove the cast in dbt-manifest.ts and return the error property directly. Also update callers of Dispatcher.call("dbt.manifest", ...) such as any usage in impact-analysis and project-scan to check for result.error before proceeding (handle/throw or bail out if present) so they don't operate on partial/invalid data.packages/drivers/test/driver-security.test.ts (1)
14-16: Consider adding mock cleanup for test isolation.The DuckDB, PostgreSQL, and Redshift test sections use
mock.module()inline within each test but don't callmock.restore()in anafterEachhook. While the MongoDB section properly implements cleanup (lines 253-256), the other sections don't follow this pattern.This could lead to test pollution if Bun's module cache doesn't fully reset between tests or if tests run in parallel in the future.
♻️ Suggested pattern (apply to DuckDB, PostgreSQL, Redshift sections)
describe("DuckDB driver", () => { + afterEach(() => { + mock.restore() + }) + // Test wrapDuckDBError logic inline (it's a closure, so we test via connect behavior) describe("wrapDuckDBError", () => {Similarly for PostgreSQL (after line 370) and Redshift (after line 560).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/drivers/test/driver-security.test.ts` around lines 14 - 16, Add mock cleanup to each test section that calls mock.module(): inside the DuckDB, PostgreSQL, and Redshift test describe blocks (e.g., the "DuckDB driver" / "wrapDuckDBError" scope and the PostgreSQL and Redshift test sections) add an afterEach hook that calls mock.restore() to ensure mocks are reset between tests; place the afterEach alongside existing hooks (mirroring the MongoDB pattern) so mock.module() calls are cleaned up and tests remain isolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/opencode/src/altimate/telemetry/index.ts`:
- Around line 868-874: The tests in plan-skill-telemetry.test.ts still expect
"llm_selected" for unrecognized or missing triggers but classifySkillTrigger now
returns "unknown" for those cases; update the assertions in that test file to
expect "unknown" for calls classifySkillTrigger({}), classifySkillTrigger({ foo:
"bar" }), classifySkillTrigger({ trigger: "something_else" }), and
classifySkillTrigger({ trigger: 42 }) so the tests match the new behavior of the
classifySkillTrigger function.
In `@packages/opencode/src/altimate/tools/dbt-lineage.ts`:
- Around line 25-29: The title interpolation in formatDbtLineage uses unguarded
result.model_name and result.confidence and can render "undefined"; update the
title construction to use the same null-guards as the metadata (e.g.,
result.model_name ?? "unknown" and result.confidence ?? "unknown") so the title
string and metadata stay consistent (look for formatDbtLineage, the title
assignment, and the metadata object to apply the change).
In `@packages/opencode/test/telemetry/plan-skill-telemetry.test.ts`:
- Around line 13-16: The test expectations are stale because
Telemetry.classifySkillTrigger now returns "unknown" for unrecognized or empty
triggers; update the assertions in plan-skill-telemetry.test.ts so they expect
"unknown" for inputs like {} and { foo: "bar" }, or alternatively provide a
recognized trigger field (e.g., { trigger: "llm_selected" }) to assert
"llm_selected"; target the calls to Telemetry.classifySkillTrigger in the test
and change the expected values accordingly.
- Around line 30-33: Update the test to match the new behavior of
Telemetry.classifySkillTrigger: change the expected return value from
"llm_selected" to "unknown" for unrecognized trigger inputs (e.g.,
Telemetry.classifySkillTrigger({ trigger: "something_else" }) and
Telemetry.classifySkillTrigger({ trigger: 42 })). Optionally rename the test
description to reflect that unrecognized triggers return "unknown" instead of
"llm_selected".
---
Nitpick comments:
In `@packages/drivers/test/driver-security.test.ts`:
- Around line 14-16: Add mock cleanup to each test section that calls
mock.module(): inside the DuckDB, PostgreSQL, and Redshift test describe blocks
(e.g., the "DuckDB driver" / "wrapDuckDBError" scope and the PostgreSQL and
Redshift test sections) add an afterEach hook that calls mock.restore() to
ensure mocks are reset between tests; place the afterEach alongside existing
hooks (mirroring the MongoDB pattern) so mock.module() calls are cleaned up and
tests remain isolated.
In `@packages/opencode/src/altimate/tools/dbt-manifest.ts`:
- Line 24: Add an optional error field to the DbtManifestResult interface (e.g.,
error?: string) so we don't need to cast (result as any).error; then remove the
cast in dbt-manifest.ts and return the error property directly. Also update
callers of Dispatcher.call("dbt.manifest", ...) such as any usage in
impact-analysis and project-scan to check for result.error before proceeding
(handle/throw or bail out if present) so they don't operate on partial/invalid
data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d4c1c1f2-0b57-4eaf-adbb-1df149efc8cf
📒 Files selected for processing (11)
docs/docs/configure/warehouses.mdpackages/drivers/test/driver-security.test.tspackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/altimate/tools/altimate-core-column-lineage.tspackages/opencode/src/altimate/tools/altimate-core-track-lineage.tspackages/opencode/src/altimate/tools/dbt-lineage.tspackages/opencode/src/altimate/tools/dbt-manifest.tspackages/opencode/src/altimate/tools/sql-translate.tspackages/opencode/src/session/prompt.tspackages/opencode/test/altimate/feature-discovery-e2e.test.tspackages/opencode/test/telemetry/plan-skill-telemetry.test.ts
💤 Files with no reviewable changes (1)
- packages/opencode/src/session/prompt.ts
…pectations - Add back `// altimate_change end — session start telemetry` and `// altimate_change end — accumulate session metrics` markers (the removed duplicates were actually balancing real start blocks) - Update `classifySkillTrigger` tests to expect `"unknown"` for unrecognized/missing trigger values (matches the behavior fix) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6 tasks
kulvirgit
pushed a commit
that referenced
this pull request
Mar 30, 2026
* fix: address pre-release review findings — dead code, undefined guards, driver tests, docs - Remove duplicate `altimate_change end` markers in `prompt.ts` - Replace `password: "secret"` with `"test-fake-password"` in test fixtures - Fix `classifySkillTrigger()` to return `"unknown"` for unexpected triggers - Remove unused `"schema_not_indexed"` from `feature_suggestion` type - Add null guards in `sql-translate`, `dbt-manifest`, `dbt-lineage`, `column-lineage`, `track-lineage` to prevent literal "undefined" display - Propagate `result.error` to metadata in `dbt-manifest` - Add 24 driver security tests (DuckDB retry, MongoDB blocking, PG/Redshift validation, registry unsupported DB hints) - Document DuckDB concurrent access, MongoDB blocked operators, unsupported databases in `warehouses.md` Closes #579 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: restore missing `altimate_change end` markers and update test expectations - Add back `// altimate_change end — session start telemetry` and `// altimate_change end — accumulate session metrics` markers (the removed duplicates were actually balancing real start blocks) - Update `classifySkillTrigger` tests to expect `"unknown"` for unrecognized/missing trigger values (matches the behavior fix) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8 tasks
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?
Addresses all findings from 5-persona pre-release validation for v0.5.15. Fixes dead code, undefined display bugs, adds missing driver security tests, and fills documentation gaps.
Type of change
Issue for this PR
Closes #579
How did you verify your code works?
bun turbo typecheck)bun run script/upstream/analyze.ts --markers --base main --strict)Checklist
Changes
Code fixes:
altimate_change endmarkers inprompt.ts(lines 812, 962)classifySkillTrigger()to return"unknown"for unexpected triggers (was dead code)"schema_not_indexed"fromfeature_suggestiontelemetry typeUndefined display bugs (7 files):
sql-translate.ts: null guards onsource_dialect/target_dialectdbt-manifest.ts: null guards onmodel.name,source.source_name,source.name+ propagateresult.errorto metadatadbt-lineage.ts: null guards onmodel_name,confidencealtimate-core-column-lineage.ts/altimate-core-track-lineage.ts: null guards onedge.source/edge.targetTest credential cleanup:
password: "secret"→"test-fake-password"in 3 test fixturesNew driver security tests (24 tests):
wrapDuckDBErrorwrapping, lock retry with READ_ONLY fallback, in-memory no-retry$out/$mergewrite stage blocking,$function/$accumulatorJS execution blocking, safe pipeline passthroughstatement_timeoutwith valid/NaN/negative valuesDocumentation:
warehouses.mdwarehouses.md🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes
Tests
Chores