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}
|
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 (13)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Tool as Tool (caller)
participant Dispatcher as Dispatcher
participant Native as Native Handler
participant Resolver as schemaOrEmpty
participant Core as Altimate Core
Tool->>Dispatcher: call("altimate_core.*", { sql, schema_path, schema_context, dialect })
Dispatcher->>Native: invoke handler with params (including dialect)
Native->>Resolver: schemaOrEmpty(schema_path, schema_context, dialect)
alt schema resolved
Resolver->>Native: Schema (from resolveSchema)
else fallback used
Resolver->>Resolver: Schema.fromDdl(..., dialect)
Resolver->>Native: fallback Schema
end
Native->>Core: core.*(sql, schema, ...)
Core-->>Native: result
Native-->>Dispatcher: wrapped result (includes metadata.dialect)
Dispatcher-->>Tool: response (includes metadata.dialect)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 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 |
…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>
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 |
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