feat: verified query optimization via altimate_core_rewrite verify_equivalence mode#918
Conversation
… rewrites Composes altimate_core.rewrite + altimate_core.equivalence into one gated primitive: a rewrite is reported VERIFIED only when equivalence affirmatively returns equivalent===true; everything else (incl. no-schema) is returned but labeled 'review before applying'. The core mechanic for one-click verified query optimization. 15 tests (gate logic + adversarial strict-true + real-engine). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
Multi-Persona Review — Verdict: ship-with-notes
This PR introduces a well-designed, safety-critical tool for verified SQL optimization with strong test coverage and alignment with industry best practices. Two high-confidence bugs were identified: missing type validation for equivalence responses and potential deduplication issues with case-sensitive identifiers. A CLAUDE.md compliance gap exists. All findings are non-critical and fixable without blocking deployment.
14/14 agents completed · 220s · 3 findings (0 critical, 1 high, 1 medium)
High
- [code-reviewer, web-researcher] The equivalence check does not validate that 'equivalent' is a boolean before comparison, risking misclassification of malformed responses from the engine as 'not equivalent' instead of 'invalid input'. →
packages/opencode/src/altimate/tools/altimate-verified-optimize.ts:45- 💡 Add explicit type validation: if typeof ed.equivalent !== 'boolean', treat as unverified with reason 'equivalence engine returned invalid response'.
Medium
- [code-reviewer] SQL normalization in deduplication may incorrectly merge distinct rewrites if the dialect supports case-sensitive identifiers or quoted identifiers. →
packages/opencode/src/altimate/tools/altimate-verified-optimize.ts:72- 💡 Clarify in comments that normalization assumes case-insensitive semantics, or restrict normalization to non-quoted parts.
Low
- [code-reviewer] New tool lacks a CLAUDE.md file documenting safety guarantees, failure modes, and behavior, violating project conventions for trust-gating tools. →
packages/opencode/src/altimate/tools/altimate-verified-optimize.ts:1- 💡 Create a CLAUDE.md file in the same directory detailing conservative gate logic, error handling, and safety properties.
Multi-Persona Review · vllm:qwen3-next-80b (waves) + vllm-fallback (synth) ·
| const rwError = rw.error ?? data.error | ||
| if (rwError) { | ||
| return { | ||
| title: "Verified Optimize: ERROR", |
There was a problem hiding this comment.
[HIGH · code-reviewer, web-researcher] The equivalence check does not validate that 'equivalent' is a boolean before comparison, risking misclassification of malformed responses from the engine as 'not equivalent' instead of 'invalid input'.
💡 Suggestion: Add explicit type validation: if typeof ed.equivalent !== 'boolean', treat as unverified with reason 'equivalence engine returned invalid response'.
Confidence: 95/100
| }) | ||
|
|
||
| if (!candidates.length) { | ||
| return { |
There was a problem hiding this comment.
[MEDIUM · code-reviewer] SQL normalization in deduplication may incorrectly merge distinct rewrites if the dialect supports case-sensitive identifiers or quoted identifiers.
💡 Suggestion: Clarify in comments that normalization assumes case-insensitive semantics, or restrict normalization to non-quoted parts.
Confidence: 80/100
| @@ -0,0 +1,159 @@ | |||
| import z from "zod" | |||
There was a problem hiding this comment.
[LOW · code-reviewer] New tool lacks a CLAUDE.md file documenting safety guarantees, failure modes, and behavior, violating project conventions for trust-gating tools.
💡 Suggestion: Create a CLAUDE.md file in the same directory detailing conservative gate logic, error handling, and safety properties.
Confidence: 75/100
…fy_equivalence)
Addresses review feedback: a standalone `altimate_verified_optimize` tool
overlapped with `sql_optimize` and `altimate_core_rewrite`, risking agent
tool-selection confusion (three near-identical "optimize a query" tools).
Instead, verification is now a MODE of the existing rewrite tool:
- `altimate_core_rewrite` gains `verify_equivalence?: boolean` (default false →
existing behavior unchanged). When true, each proposed rewrite is checked
against the original via `altimate_core.equivalence`; only rewrites that
return strict `equivalent === true` are labeled verified-safe, the rest
"review before applying." Conservative gate (no truthy coercion), per-candidate
try/catch, array-guarded differences, null-guarded responses, and specific
unverified reasons (missing-field / non-boolean / not-equivalent).
- Remove the standalone `altimate_verified_optimize` tool + its registration.
- `query-optimize` skill now calls `altimate_core_rewrite` with
`verify_equivalence: true`, folding its old manual rewrite→equivalence steps
(3 + 6) into one verified call.
Tests (18) moved to `altimate-core-rewrite-verify.test.ts`, exercising the verify
mode: gate logic, adversarial strict-true (rejects "true"/1/{}/undefined/null +
missing-field/non-boolean reasons), partial-failure resilience, and a real-engine
integration test.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
anandgupta42
changed the title
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. |
…ry later release) (#923) The "docs point at the action patch" test derived `version` from the CHANGELOG's top entry and asserted it equals "0.8.5". After 0.8.6 shipped, the top entry moved to 0.8.6, so the assertion permanently failed — broken on main, blocking every PR (seen on #918, #854). Only the 0.8.5 gate had this pattern. This gate is specific to the 0.8.5 release (docs reference the patched @v0.8.5, not the broken @v0.8.4). Pin to the constant "0.8.5" and assert that changelog entry EXISTS rather than that it is the latest. Docs assertions unchanged (and passing). Closes 922 Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
|
Warning Review limit reached
More reviews will be available in 5 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces verified query optimization by adding a ChangesVerified Query Rewrite Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/altimate/tools/altimate-core-rewrite.ts`:
- Around line 74-91: The deduplication currently lowercases SQL via the norm
helper, which can collapse case-sensitive differences; update norm to only
normalize whitespace/trim (remove .toLowerCase()), and ensure the seen set and
candidate filter use that case-preserving norm (references: norm, args.sql,
seen, raw, candidates, suggestions) so duplicates are based on
whitespace-normalized-but-case-preserved SQL strings.
- Around line 28-35: The current check uses the truthiness of "error" and misses
failure payloads like error: "" — update the handling in the
altimate-core-rewrite flow to explicitly check for a failure flag (e.g., if
result?.success === false || data?.success === false) alongside the existing
error variable (result, data, error) and return the error-shaped response (title
"Rewrite: ERROR", metadata with success: false and counts 0, and output using
the error string) whenever success === false, even if error is an empty string;
keep the existing fallback truthy error branch for backward compatibility.
🪄 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: 576dbe94-a94e-4bf0-bb5f-456736c68071
📒 Files selected for processing (3)
.opencode/skills/query-optimize/SKILL.mdpackages/opencode/src/altimate/tools/altimate-core-rewrite.tspackages/opencode/test/altimate/altimate-core-rewrite-verify.test.ts
…rsona)
Two real findings on altimate-core-rewrite.ts (the current verify-mode file;
most other review comments targeted the now-deleted standalone tool):
- MAJOR (coderabbit): handle `success === false` explicitly. An
`{ success: false, error: "" }` rewrite payload has a falsy error string and
fell through to the success path, misreporting results. Now fails on an
explicit `success === false` regardless of error string.
- MINOR (coderabbit + multi-persona): dedup/no-op normalization lowercased the
full SQL, which could collapse rewrites differing only by a case-sensitive
string literal / quoted identifier into one — dropping a valid candidate before
verification. Normalize whitespace only; no case-folding.
Already-addressed findings (reviewers were looking at the deleted standalone
tool): per-candidate try/catch (equivalence throw fails closed per candidate, not
the whole tool) and strict boolean validation of `equivalent` (=== true + a
typeof-based "non-boolean" reason) are both present in the current code.
Tests (20): + success:false-with-empty-error regression, + case-sensitive-literal
dedup preservation.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Addressed the review against the current file ( Fixed (real findings on current code)
Already addressed (these targeted the deleted standalone
Noted, not changed
Tests now 20/20 (was 18); typecheck clean. |
2 participants
What does this PR do?
Adds verified query optimization as a mode of the existing
altimate_core_rewritetool (rather than a separate, overlapping tool).altimate_core_rewritegains averify_equivalenceoption. Whentrue, each proposed rewrite is checked against the original viaaltimate_core.equivalence, and only rewrites that return strictequivalent === trueare labeled verified-safe to apply — the rest are returned but labeled "review before applying." The gate is conservative: it never promises "safe" unless equivalence proves it, so a verified rewrite can be applied without changing results.Also wires the
query-optimizeskill to callaltimate_core_rewritewithverify_equivalence: true, folding its old manual rewrite→equivalence steps into one verified call.Type of change
Issue for this PR
Closes #917
How did you verify your code works?
test/altimate/altimate-core-rewrite-verify.test.ts) exercising verify mode:equivalent === true— rejects"true",1,{},undefined,null, with specific reasons (missing-field / non-boolean / not-equivalent); fails safe when the equivalence handler throws (partial-failure resilience)verify_equivalencedefaults to false → existingaltimate_core_rewritebehavior is unchanged (regression-checked againsttool-formatters+tool-error-propagation-complete).tsgotypecheck clean; marker guard clean.Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation