Skip to content

Commit 3bb1b6b

Browse files
committed
fix: address coderabbit review on #693
- translateExplainError now catches $N and :name bind errors too, not just bare `?`. Detects a syntax-error keyword plus a bind token delimited by whitespace or quotes, covering PG, Snowflake and Oracle phrasings. Adds "there is no parameter $N" (PostgreSQL). - NO_SCHEMA_NOTE documents the flat table-map shape actually accepted by the tool, not the verbose SchemaDefinition shape. - Regression tests added for $1, :name, and the NO_SCHEMA wording.
1 parent 1412310 commit 3bb1b6b

4 files changed

Lines changed: 65 additions & 6 deletions

File tree

packages/opencode/src/altimate/native/connections/register.ts

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -299,9 +299,26 @@ export function translateExplainError(
299299
return "No warehouses are configured. Run `warehouse_add` to set one up before calling sql_explain."
300300
}
301301

302-
// Unsubstituted-placeholder compilation errors from Snowflake / PG / MySQL.
303-
if (/syntax error.*unexpected\s*\?/i.test(msg) || /syntax error.*position\s+\d+.*\?/i.test(msg)) {
304-
return "SQL compilation error: the query contains an unsubstituted `?` placeholder. sql_explain does not support parameterized queries — inline the literal value before calling."
302+
// Unsubstituted-placeholder compilation errors.
303+
//
304+
// Bind placeholders come in three flavours: positional `?` (MySQL / JDBC),
305+
// numbered `$1`, `$2`, ... (PostgreSQL), and named `:name` (Oracle / SQLite /
306+
// some SQLAlchemy dialects). Drivers phrase the resulting syntax error in
307+
// many different ways — Snowflake says "unexpected ?", PostgreSQL says
308+
// `syntax error at or near "$1"`, Oracle says `ORA-00911: invalid character`,
309+
// etc. Rather than enumerate every phrasing, detect a bind-token next to a
310+
// syntax-error keyword and translate them all to the same guidance.
311+
const isSyntaxError = /syntax error|invalid character|unexpected token/i.test(msg)
312+
// Match a bind token that is delimited by whitespace, start/end, or a quote.
313+
// The negative lookbehind `(?<!\w)` prevents matching `$1` inside identifiers.
314+
const containsBindToken =
315+
/(?<!\w)\?(?!\w)/.test(msg) ||
316+
/(?<!\w)\$\d+/.test(msg) ||
317+
/(?<!\w):[a-zA-Z_]\w*/.test(msg)
318+
// PostgreSQL surfaces bind-param issues with its own distinctive phrasing.
319+
const isPgBindError = /there is no parameter \$\d+/i.test(msg)
320+
if ((isSyntaxError && containsBindToken) || isPgBindError) {
321+
return "SQL compilation error: the query contains an unsubstituted bind placeholder (`?`, `$1`, or `:name`). sql_explain does not support parameterized queries — inline the literal values before calling."
305322
}
306323

307324
// Snowflake-specific: ANALYZE not supported.

packages/opencode/src/altimate/tools/altimate-core-validate.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,15 @@ function classifyValidationError(message: string): string {
7777
// Warning appended when validation runs without a schema. Stored without a
7878
// leading blank so it can be pushed into a line array; a blank line is added
7979
// explicitly at each call site where spacing is desired.
80+
//
81+
// The hint uses the flat table-map shape accepted by the tool (see
82+
// schema-resolver.ts), not the verbose SchemaDefinition form — callers that
83+
// already know the inner Rust struct layout can use either, but the flat form
84+
// is strictly easier to construct inline.
8085
const NO_SCHEMA_NOTE =
8186
"Note: No schema was provided, so table/column existence checks were skipped. " +
8287
"To catch missing tables or columns, run `schema_inspect` on the referenced tables first " +
83-
"or pass `schema_context` inline with {tables: {...}}."
88+
'or pass `schema_context` inline as a table map, for example `{ users: { id: "INTEGER", email: "VARCHAR" } }`.'
8489

8590
function formatValidate(data: Record<string, any>, hasSchema: boolean): string {
8691
if (data.error) return `Error: ${data.error}`

packages/opencode/test/altimate/tools/altimate-core-validate.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,16 @@ describe("formatValidate", () => {
153153
expect(result).toContain("No schema was provided")
154154
expect(result).not.toContain("\n\n\n")
155155
})
156+
157+
test("no-schema note recommends the flat table-map shape, not {tables: {...}}", () => {
158+
// Regression: an earlier draft of the note documented the verbose
159+
// SchemaDefinition shape `{tables: {...}}`, but the tool's examples and
160+
// tests use the flat `{users: {...}}` shape. Using the wrong shape in
161+
// the hint would send callers down a rabbit hole.
162+
const result = formatValidate({ valid: true }, false)
163+
expect(result).not.toContain("{tables:")
164+
expect(result).toMatch(/users.*INTEGER/)
165+
})
156166
})
157167

158168
// ---------------------------------------------------------------------------

packages/opencode/test/altimate/tools/sql-explain.test.ts

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -296,8 +296,35 @@ describe("translateExplainError", () => {
296296
"snowflake",
297297
["snowflake"],
298298
)
299-
expect(result).toContain("unsubstituted `?` placeholder")
300-
expect(result).toContain("inline the literal value")
299+
expect(result).toContain("unsubstituted bind placeholder")
300+
expect(result).toContain("inline the literal values")
301+
})
302+
303+
test("translates `$1` numbered placeholder compile errors (PostgreSQL style)", () => {
304+
const result = translateExplainError(
305+
new Error("syntax error at or near \"$1\" at position 24"),
306+
"postgres",
307+
["postgres"],
308+
)
309+
expect(result).toContain("unsubstituted bind placeholder")
310+
})
311+
312+
test("translates `:name` named placeholder compile errors", () => {
313+
const result = translateExplainError(
314+
new Error("syntax error, unexpected :userid near position 31"),
315+
"oracle",
316+
["oracle"],
317+
)
318+
expect(result).toContain("unsubstituted bind placeholder")
319+
})
320+
321+
test("translates PostgreSQL 'no parameter $N' errors", () => {
322+
const result = translateExplainError(
323+
new Error('there is no parameter $1'),
324+
"postgres",
325+
["postgres"],
326+
)
327+
expect(result).toContain("unsubstituted bind placeholder")
301328
})
302329

303330
test("translates EXPLAIN ANALYZE-not-supported errors", () => {

0 commit comments

Comments
 (0)