fix: pass warehouse dialect to altimate-core tools instead of hardcoding snowflake#550
fix: pass warehouse dialect to altimate-core tools instead of hardcoding snowflake#550VJ-yadav wants to merge 5 commits intoAltimateAI:mainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
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:
📝 Walkthrough🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
🤖 Behavioral Analysis — 4 Finding(s)🟡 Warnings (3)
🔵 Nits (1)
Analysis run | Powered by QA Autopilot |
Auto-fixed 4 finding(s)
|
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/tools/altimate-core-correct.ts`:
- Around line 22-27: The six native handlers drop the dialect despite it being
passed in params; update altimate_core.validate, altimate_core.check,
altimate_core.fix, altimate_core.semantics, altimate_core.equivalence, and
altimate_core.correct so their core calls include the dialect (use
params.dialect || undefined) when invoking core.validate, core.fix,
core.checkSemantics, core.checkEquivalence, and core.correct respectively;
ensure each call signature matches the core function's dialect parameter
position and propagate params.dialect in the same pattern used by other handlers
(e.g., formatSql, extractMetadata, compareQueries).
🪄 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: 46aff016-36ac-4072-a604-a460e3a35cc1
📒 Files selected for processing (11)
packages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/altimate-core-check.tspackages/opencode/src/altimate/tools/altimate-core-correct.tspackages/opencode/src/altimate/tools/altimate-core-equivalence.tspackages/opencode/src/altimate/tools/altimate-core-fix.tspackages/opencode/src/altimate/tools/altimate-core-policy.tspackages/opencode/src/altimate/tools/altimate-core-semantics.tspackages/opencode/src/altimate/tools/altimate-core-validate.tspackages/opencode/src/altimate/tools/impact-analysis.tspackages/opencode/test/altimate/e2e-tool-errors.tspackages/opencode/test/altimate/tool-error-propagation.test.ts
VJ-yadav
force-pushed
the
fix/hardcoded-snowflake-dialect
branch
from
3ff930e to
6edf2e5
Compare
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
cc @VJ-yadav |
Review — Ready for mergeScope: Fixes #455 — replaces hardcoded Analysis
Files changed: 13 files, +108 −46 CI status (commit
Recommendation cc @anandgupta42 — ready for |
anandgupta42
force-pushed
the
fix/hardcoded-snowflake-dialect
branch
from
aaa1816 to
4ceea83
Compare
Code review (Gemini 3.1 Pro) caught a missed handler in the dialect-threading migration: 'altimate_core.policy' (altimate-core.ts:200) called schemaOrEmpty() without params.dialect, while the 6 other policy- family handlers (validate, check, fix, semantics, equivalence, correct, explain) all pass it through. AltimateCorePolicyParams already declares 'dialect?: string' in types.ts and the tool definition plumbs it into Dispatcher.call(), so the param was reaching the handler but being silently discarded at the schema resolution step — meaning the fallback empty schema was always built with the default (Snowflake) dialect instead of the user-selected one, defeating the purpose of PR AltimateAI#550 for policy checks specifically. 1-line fix brings the policy handler into line with its 6 siblings.
Consensus code-review applied — fix pushedRan a 4-participant consensus review (Claude + GPT 5.4 + Gemini 3.1 Pro + self-review). Real bug found and fixedCRITICAL — The dialect parameter was correctly plumbed through the tool → Dispatcher → type definition, but the native handler at Effect: policy checks with a user-provided dialect would silently build the fallback empty schema with the default (Snowflake) dialect instead of the user-selected one — defeating the purpose of this PR for policy checks specifically. 1-line fix brings the policy handler into line with its siblings. Scope discussionGemini also flagged that the PR doesn't migrate dialect into the ~20 OTHER altimate_core handlers (rewrite, testgen, grade, prune_schema, etc.). That's intentionally out of scope — issue #455 explicitly lists the 8 tools this PR targets. The remaining handlers can get dialect as a follow-up if needed; no silent-fallback bug exists for tools that aren't updated (they don't accept dialect at the tool level yet). Test coverage29 existing altimate-core-native tests continue to pass. No new tests added because Pushed as |
|
Hi @anandgupta42 — you mentioned this is ready for merge after the consensus review + policy handler fix (d2ad6ac). There are merge conflicts now though — I'll rebase and resolve. Once CI is green again, ready to merge. |
Code review (Gemini 3.1 Pro) caught a missed handler in the dialect-threading migration: 'altimate_core.policy' (altimate-core.ts:200) called schemaOrEmpty() without params.dialect, while the 6 other policy- family handlers (validate, check, fix, semantics, equivalence, correct, explain) all pass it through. AltimateCorePolicyParams already declares 'dialect?: string' in types.ts and the tool definition plumbs it into Dispatcher.call(), so the param was reaching the handler but being silently discarded at the schema resolution step — meaning the fallback empty schema was always built with the default (Snowflake) dialect instead of the user-selected one, defeating the purpose of PR AltimateAI#550 for policy checks specifically. 1-line fix brings the policy handler into line with its 6 siblings.
VJ-yadav
force-pushed
the
fix/hardcoded-snowflake-dialect
branch
from
0ae637d to
758b8e5
Compare
…ing snowflake Add optional `dialect` parameter (default: "snowflake") to all 8 altimate-core tools that were missing it: - altimate-core-validate, fix, correct, semantics, equivalence, policy, check, and impact-analysis Each tool now: 1. Accepts `dialect` via its zod parameter schema 2. Passes it through to the Dispatcher call 3. Includes it in telemetry metadata Follows the pattern established in sql-analyze.ts, sql-optimize.ts, and schema-diff.ts. Type interfaces updated in types.ts. Tests updated to pass dialect explicitly. Fixes AltimateAI#455 Co-Authored-By: Vijay Yadav <vijay@studentsucceed.com>
Address behavioral analysis findings: the NO SCHEMA early returns in validate, semantics, and equivalence tools, plus the NO MANIFEST and MODEL NOT FOUND early returns in impact-analysis, were missing the dialect field in their metadata. All return paths now consistently include dialect for telemetry. Co-Authored-By: Vijay Yadav <vijay@studentsucceed.com>
The dialect parameter was passed through the tool layer but dropped at the schema resolution step. Six native handlers (validate, check, fix, semantics, equivalence, correct) called schemaOrEmpty() without dialect, so the fallback empty schema always used the default dialect. Fix: add optional dialect param to schemaOrEmpty() and pass it to Schema.fromDdl(). Update all six handlers to forward params.dialect. Co-Authored-By: Vijay Yadav <vjyadav194@gmail.com>
Code review (Gemini 3.1 Pro) caught a missed handler in the dialect-threading migration: 'altimate_core.policy' (altimate-core.ts:200) called schemaOrEmpty() without params.dialect, while the 6 other policy- family handlers (validate, check, fix, semantics, equivalence, correct, explain) all pass it through. AltimateCorePolicyParams already declares 'dialect?: string' in types.ts and the tool definition plumbs it into Dispatcher.call(), so the param was reaching the handler but being silently discarded at the schema resolution step — meaning the fallback empty schema was always built with the default (Snowflake) dialect instead of the user-selected one, defeating the purpose of PR AltimateAI#550 for policy checks specifically. 1-line fix brings the policy handler into line with its 6 siblings.
The upstream altimate-core-validate tests (PR AltimateAI#693) call execute() without dialect, but our PR made dialect a required field in the zod output type. Add explicit dialect: 'snowflake' to all test calls. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
VJ-yadav
force-pushed
the
fix/hardcoded-snowflake-dialect
branch
from
758b8e5 to
82af0f7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/test/altimate/tools/altimate-core-validate.test.ts (2)
305-317: Consider verifying dialect is passed to Dispatcher.This test already spies on
Dispatcher.call— adding an assertion on the call arguments would validate that the dialect is correctly propagated to the backend.♻️ Proposed enhancement
const tool = await AltimateCoreValidateTool.init() await tool.execute({ sql: "SELECT 1", dialect: "snowflake" }, ctx as any) expect(spy).toHaveBeenCalledTimes(1) + expect(spy.mock.calls[0][0]).toBe("altimate_core.validate") + expect(spy.mock.calls[0][1]).toMatchObject({ dialect: "snowflake" }) spy.mockRestore()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/tools/altimate-core-validate.test.ts` around lines 305 - 317, Add an assertion to the existing spy on Dispatcher.call to verify that the dialect argument is forwarded to the dispatcher: after calling AltimateCoreValidateTool.init() and tool.execute(...), inspect the spy's most recent call (Dispatcher.call) and assert that the payload includes dialect: "snowflake" (or that the second/appropriate arg contains the dialect), then restore the spy; this change touches the test around the spyOn(Dispatcher, "call") block in altimate-core-validate.test.ts to ensure dialect propagation.
172-178: Consider adding test coverage for a non-snowflake dialect.All test cases use
dialect: "snowflake". Since the PR enables multi-dialect support, adding at least one test with a different dialect (e.g.,"postgres") would provide automated regression protection and verify the dialect parameter is correctly passed through the entire flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/tools/altimate-core-validate.test.ts` around lines 172 - 178, Add a new unit test that mirrors the existing AltimateCoreValidateTool.execute tests but uses a non-snowflake dialect (e.g., "postgres") to verify dialect propagation; specifically, create a test that instantiates AltimateCoreValidateTool.execute (or the same test helper used in this file), sets dialect: "postgres" in the input, spies on the dispatcherSpy/spyOn target used by other tests, invokes the tool, and asserts the dispatcher was called with the expected payload containing dialect: "postgres" (and any downstream fields that should reflect the dialect) to ensure the dialect parameter is passed through the flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/test/altimate/tools/altimate-core-validate.test.ts`:
- Around line 305-317: Add an assertion to the existing spy on Dispatcher.call
to verify that the dialect argument is forwarded to the dispatcher: after
calling AltimateCoreValidateTool.init() and tool.execute(...), inspect the spy's
most recent call (Dispatcher.call) and assert that the payload includes dialect:
"snowflake" (or that the second/appropriate arg contains the dialect), then
restore the spy; this change touches the test around the spyOn(Dispatcher,
"call") block in altimate-core-validate.test.ts to ensure dialect propagation.
- Around line 172-178: Add a new unit test that mirrors the existing
AltimateCoreValidateTool.execute tests but uses a non-snowflake dialect (e.g.,
"postgres") to verify dialect propagation; specifically, create a test that
instantiates AltimateCoreValidateTool.execute (or the same test helper used in
this file), sets dialect: "postgres" in the input, spies on the
dispatcherSpy/spyOn target used by other tests, invokes the tool, and asserts
the dispatcher was called with the expected payload containing dialect:
"postgres" (and any downstream fields that should reflect the dialect) to ensure
the dialect parameter is passed through the flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: da3518f0-c951-4ae3-aa1a-9ebde771109a
📒 Files selected for processing (14)
packages/opencode/src/altimate/native/altimate-core.tspackages/opencode/src/altimate/native/schema-resolver.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/altimate-core-check.tspackages/opencode/src/altimate/tools/altimate-core-correct.tspackages/opencode/src/altimate/tools/altimate-core-equivalence.tspackages/opencode/src/altimate/tools/altimate-core-fix.tspackages/opencode/src/altimate/tools/altimate-core-policy.tspackages/opencode/src/altimate/tools/altimate-core-semantics.tspackages/opencode/src/altimate/tools/altimate-core-validate.tspackages/opencode/src/altimate/tools/impact-analysis.tspackages/opencode/test/altimate/e2e-tool-errors.tspackages/opencode/test/altimate/tool-error-propagation.test.tspackages/opencode/test/altimate/tools/altimate-core-validate.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/opencode/src/altimate/native/altimate-core.ts
- packages/opencode/src/altimate/native/types.ts
- packages/opencode/test/altimate/tool-error-propagation.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/opencode/src/altimate/tools/altimate-core-validate.ts
- packages/opencode/src/altimate/tools/altimate-core-correct.ts
- packages/opencode/src/altimate/native/schema-resolver.ts
- packages/opencode/src/altimate/tools/altimate-core-semantics.ts
- packages/opencode/src/altimate/tools/altimate-core-check.ts
- packages/opencode/src/altimate/tools/altimate-core-policy.ts
- packages/opencode/test/altimate/e2e-tool-errors.ts
3 participants
Summary
Fixes #455 — Adds optional
dialectparameter (default:"snowflake") to all 8 altimate-core tools that were hardcoding it:altimate-core-validate.ts,fix.ts,correct.ts,semantics.ts,equivalence.ts,policy.ts,check.tsimpact-analysis.tsEach tool now accepts
dialectvia zod schema, passes it to the Dispatcher call, and includes it in telemetry metadata. Follows the pattern already established insql-analyze.ts,sql-optimize.ts, andschema-diff.ts.Type interfaces updated in
types.ts. Tests updated to pass dialect explicitly.Test Plan
bun turbo typecheck— 5/5 packages passbun test test/altimate/schema-finops-dbt.test.ts— 56 passbun test test/altimate/tool-error-propagation.test.ts— 22 passdialect: "postgres"and verify it reaches telemetryChecklist
Summary by CodeRabbit
New Features
Tests