fix: harden sql_explain and altimate_core_validate input handling#692
fix: harden sql_explain and altimate_core_validate input handling#692anandgupta42 wants to merge 2 commits intomainfrom
Conversation
…andling
`sql_explain` and `altimate_core_validate` produced high failure rates and
unhelpful errors because their input validation did not match the parameter
contracts they advertised.
**`sql_explain`**
- Reject empty, whitespace-only, and placeholder-only SQL (`?`, `:var`, `$1`)
before hitting the warehouse. Verbatim driver errors like "SQL compilation
error: syntax error line 1 at position 8 unexpected ?." are unrecoverable
for LLM callers — catch the common cases early with actionable guidance.
- Reject empty / placeholder warehouse names (`?`, `:wh`, `""`) with a
pointer to `warehouse_list` rather than letting the Registry emit
"Connection ? not found. Available: (none)".
- Introduce `buildExplainPlan()` that returns `{prefix, actuallyAnalyzed}`
so the handler can pick dialect-correct syntax AND report the true plan
mode. Fixes the previous lie where Snowflake users who asked for ANALYZE
saw `analyzed: true` in metadata even though we silently sent plain
`EXPLAIN USING TEXT`.
- Warehouse coverage is now explicit: Snowflake (`EXPLAIN USING TEXT`),
PostgreSQL (`EXPLAIN (ANALYZE, BUFFERS)`), Redshift (plain `EXPLAIN` —
does NOT support ANALYZE), MySQL / MariaDB / DuckDB (`EXPLAIN ANALYZE`),
Databricks / Spark (`EXPLAIN FORMATTED`), ClickHouse (`EXPLAIN`).
BigQuery, Oracle, and SQL Server return an empty prefix and a clear
"not supported via statement prefix — this warehouse needs a different
plan mechanism" error instead of issuing a broken statement.
- New `translateExplainError()` helper rewrites common driver errors into
actionable messages: connection-not-found becomes "Available warehouses:
a, b, c", unsubstituted-`?` compile errors become "inline the literal
value", permission denials call out role grants, etc.
- Intentional non-change: we do NOT scan mid-query for stray `?` because
PostgreSQL JSONB uses `?`, `?|`, `?&` as legitimate operators. Only the
bare-placeholder check runs, which has no false positives.
**`altimate_core_validate`**
- Remove the hard-gate that returned "No schema provided..." before the
engine ran. The parameter schema already declared `schema_path` and
`schema_context` as optional, and the Rust core (`altimate-core`) has
accepted empty schemas for a long time via the existing `schemaOrEmpty`
helper — the hard-gate was gratuitous and every call without schema
failed identically with no recovery path.
- When schema is absent, the engine still runs so syntax / dialect findings
come through, and the tool surfaces a clear warning in the output and
`has_schema: false` in metadata so callers can distinguish full validation
from schema-less runs. Title becomes `Validate: VALID (no schema)` for
unambiguous signaling.
- Errors thrown by the engine are now classified as `error_class:
engine_failure` to keep them distinct from user-input problems.
**Tests**
- New `test/altimate/tools/sql-explain.test.ts`: input validation (empty,
placeholder, short), warehouse-name validation, full `buildExplainPlan`
matrix (including the Redshift / BigQuery / Oracle regressions above),
error translation paths, and Tool.execute integration with a mocked
dispatcher.
- New `test/altimate/tools/altimate-core-validate.test.ts`: schema
resolution (none / `schema_context` / `schema_path` / empty object),
validation error classification, output formatting, and a regression
guard asserting the dispatcher is called even when no schema is given.
- Updated `test/altimate/tool-error-propagation.test.ts` to reflect the
new contract: the validate tool now runs the engine without schema and
returns `success: true` with a warning, instead of early-returning an
error. The regression fixture keeps telemetry coverage.
Closes #691
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughAdds dialect-aware EXPLAIN planning and error translation for sql.explain, strengthens sql_explain input validation, and makes altimate_core_validate run without a schema while marking schema-dependent checks as skipped; includes new unit tests and exposes internal helpers for testing. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Tool
participant Validator as Input Validator
participant Dispatcher as Dispatcher
participant Registry as Registry/Warehouse
participant ErrorTrans as Error Translator
Note over Client,ErrorTrans: sql_explain flow
Client->>Validator: validateSqlInput(sql) / validateWarehouseName(warehouse)
alt Invalid input
Validator-->>Client: INVALID INPUT (input_validation, analyzed:false)
else Valid input
Validator->>Dispatcher: call sql.explain(params)
Dispatcher->>Registry: buildExplainPlan(warehouse_type, analyze)
alt Unsupported prefix (empty)
Registry-->>Dispatcher: prefix = ""
Dispatcher-->>Client: unsupported mechanism error (analyzed:false)
else Supported prefix
Registry->>Registry: execute `${plan.prefix} ${sql}`
alt Success
Registry-->>Dispatcher: plan text, metadata
Dispatcher-->>Client: success (analyzed = plan.actuallyAnalyzed)
else Failure
Registry-->>Dispatcher: raw error
Dispatcher->>ErrorTrans: translateExplainError(raw, warehouseName, available)
ErrorTrans-->>Dispatcher: actionable message
Dispatcher-->>Client: error (includes warehouse_type)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/tools/altimate-core-validate.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/altimate-core-validate.ts:91">
P3: The `schemaNote` starts with `\n\n`, which works for direct concatenation in the valid case but produces an extra blank line when pushed into `lines` and joined with `\n`. Strip the leading newlines when appending to the array.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Cubic flagged `altimate_core_validate`'s no-schema note: when the note was pushed into the `lines` array, its leading `\n\n` combined with the `\n` join produced three consecutive newlines in the rendered output. Fixed by storing the note without the leading blanks and inserting an explicit empty line at each call site. Also: - Extracted the warning to a module-level `NO_SCHEMA_NOTE` constant so the text lives in one place. - Added regression tests that assert the rendered output never contains three consecutive newlines for either the valid or the failure path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Superseded by #693 (same branch, squashed commits + shorter description). Closing this one. |
1 participant
Summary
Two tools produced high failure rates because their runtime behaviour did not match the parameter contracts they advertised. Both fixes live entirely in TypeScript under packages/opencode/src/altimate; the Rust altimate-core engine does not need changes (its validate already accepts empty schemas, and sql_explain never calls into Rust).
What changed
sql_explain
?, :var, $1, @name) before the call reaches the warehouse. Also reject empty or placeholder warehouse names with a pointer to warehouse_list. These now return a clear input_validation error envelope instead of surfacing verbatim driver errors.altimate_core_validate
Type of change
Issue for this PR
Closes #691
How did you verify your code works?
New and updated unit tests under packages/opencode/test/altimate/tools cover input validation, warehouse-name validation, the full buildExplainPlan warehouse matrix including Snowflake, Redshift, BigQuery, Oracle, and SQL Server regressions, translateExplainError paths, SqlExplainTool.execute integration with a mocked dispatcher, classifyValidationError precedence, extractValidationErrors, formatValidate (including regression guards that assert no triple-newline in the rendered output), and AltimateCoreValidateTool.execute paths for no schema, inline schema_context, schema_path, empty schema_context, invalid findings classification, and dispatcher throws. The existing tool-error-propagation regression test was updated to reflect the new contract.
Local verification: marker check passes, typecheck passes with zero errors, full altimate suite reports 2745 passing and 0 failing (up from 2739 on main).
Checklist