Skip to content

Commit b4bea7f

Browse files
anandgupta42claude
anandgupta42
and
claude
committed
fix: address review on verify_equivalence mode (coderabbit + multi-persona)
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]>
1 parent 01d65e3 commit b4bea7f

2 files changed

Lines changed: 37 additions & 3 deletions

File tree

‎packages/opencode/src/altimate/tools/altimate-core-rewrite.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,12 @@ export const AltimateCoreRewriteTool = Tool.define("altimate_core_rewrite", {
2525
schema_context: args.schema_context,
2626
})) as { success?: boolean; error?: string; data?: Record<string, any> } | null
2727
const data = (result?.data ?? {}) as Record<string, any>
28-
const error = result?.error ?? data.error
29-
if (error) {
28+
// Treat an explicit `success === false` as a failure even when the error
29+
// string is empty/absent — otherwise an `{ success: false, error: "" }`
30+
// payload would fall through to the success path and misreport results.
31+
const failed = result?.success === false || !!(result?.error ?? data.error)
32+
if (failed) {
33+
const error = result?.error ?? data.error ?? "rewrite failed"
3034
return {
3135
title: "Rewrite: ERROR",
3236
metadata: { success: false, rewrite_count: 0, verified_count: 0, unverified_count: 0, error },
@@ -71,7 +75,12 @@ async function verifyRewrites(
7175
data: Record<string, any>,
7276
) {
7377
const hasSchema = !!(args.schema_path || (args.schema_context && Object.keys(args.schema_context).length > 0))
74-
const norm = (s: string) => s.replace(/\s+/g, " ").trim().toLowerCase()
78+
// Normalize whitespace only for dedup / no-op detection — do NOT case-fold:
79+
// lowercasing would collapse rewrites that differ only by a case-sensitive
80+
// string literal or quoted identifier (distinct queries) into one, dropping a
81+
// valid candidate before it's verified. Equivalence itself is checked by the
82+
// engine, not by this string compare.
83+
const norm = (s: string) => s.replace(/\s+/g, " ").trim()
7584

7685
// Collect candidate rewrites (whole-query + per-suggestion), drop blanks/no-ops/dupes.
7786
const suggestions: any[] = data.suggestions ?? data.rewrites ?? []

‎packages/opencode/test/altimate/altimate-core-rewrite-verify.test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,31 @@ describe("verified-optimize — gate logic (mocked engine)", () => {
130130
expect(String(r.output)).toContain("engine down")
131131
})
132132

133+
test("success:false with EMPTY error -> still treated as failure (no fall-through)", async () => {
134+
// Regression: a `{ success: false, error: "" }` payload must not fall through
135+
// to the success path and misreport rewrites.
136+
Dispatcher.register("altimate_core.rewrite" as any, async () => ({ success: false, error: "" }))
137+
const r = await runTool({ sql: ORIGINAL, schema_context: SCHEMA })
138+
expect(r.metadata.success).toBe(false)
139+
expect(r.metadata.verified_count).toBe(0)
140+
expect(String(r.output)).toContain("Failed to generate rewrites")
141+
})
142+
143+
test("dedup preserves case-sensitive literals (does NOT case-fold)", async () => {
144+
// Two rewrites differing only by a string-literal case are DISTINCT and must
145+
// both be verified — not collapsed into one by case-folding.
146+
const rwA = "SELECT id FROM t WHERE name = 'A'"
147+
const rwB = "SELECT id FROM t WHERE name = 'a'"
148+
Dispatcher.register("altimate_core.rewrite" as any, async () => ({
149+
success: true,
150+
data: { suggestions: [{ rewritten_sql: rwA }, { rewritten_sql: rwB }] },
151+
}))
152+
Dispatcher.register("altimate_core.equivalence" as any, async () => ({ success: true, data: { equivalent: true } }))
153+
const r = await runTool({ sql: "SELECT id FROM t", schema_context: SCHEMA })
154+
// Both candidates survive dedup and get verified (2, not 1).
155+
expect(r.metadata.verified_count).toBe(2)
156+
})
157+
133158
test("duplicate candidates are de-duplicated", async () => {
134159
Dispatcher.register("altimate_core.rewrite" as any, async () => ({
135160
success: true,

0 commit comments

Comments
 (0)