Remove phantom sql_validate tool and add comprehensive SQL validation tests#352
Open
anandgupta42 wants to merge 6 commits intomainfrom
Open
Remove phantom sql_validate tool and add comprehensive SQL validation tests#352anandgupta42 wants to merge 6 commits intomainfrom
anandgupta42 wants to merge 6 commits intomainfrom
Conversation
…core_validate` tool The prompts (builder.txt, analyst.txt), agent permissions (agent.ts), and the sql-translate skill all referenced `sql_validate` — a tool that was never registered. The actual tool is `altimate_core_validate`. This caused the pre-execution validation protocol to silently fail when the LLM tried to call a non-existent tool. Also removed duplicate `altimate_core_validate` entry in analyst agent permissions that resulted from the fix. https://claude.ai/code/session_01JzUGJ9qyY5Xgcszvvru7nt
52 tests covering: - Tool name consistency: prompts/skills reference `altimate_core_validate` not phantom `sql_validate` - Agent permissions: analyst/builder allow correct tool names - altimate_core.validate: valid SQL, malformed SQL, schema contexts - altimate_core.check: composite pipeline (validate + lint + safety) - sql.analyze: lint + semantics + safety detection - Pre-execution protocol: analyze → validate → classify sequence - sql-classify: read/write/destructive query gating - Dispatcher registration: all SQL validation methods exist - Skill files: no phantom tool references - Cross-cutting: validate + check + analyze agree on same SQL https://claude.ai/code/session_01JzUGJ9qyY5Xgcszvvru7nt
…QL validation Adds 72 new tests covering: - Adversarial sql-classify bypass attempts (comment injection, string literals, unicode, encoding tricks, whitespace, multi-statement, CTE bypass, edge inputs) - User-perspective sql_execute tool tests with mocked ctx.ask (permission flow for read/write/blocked queries, error messages, query truncation) - E2E realistic scenarios (analyst queries, migration DDL, DROP blocking, complex CTEs, anti-pattern detection, SQL injection, schema context) - Error recovery (malformed SQL, empty input, null schema, large schema) - Stress tests (20 concurrent validates, 200 rapid classifies, mixed pipelines) - Dialect-specific SQL (Snowflake QUALIFY, BigQuery STRUCT, PG RETURNING, CTAS) - Translate and optimize pipeline integration https://claude.ai/code/session_01JzUGJ9qyY5Xgcszvvru7nt
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis PR replaces references to the deprecated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…ompt The analyst prompt referenced a non-existent `/impact-analysis` skill. The actual skill is `/dbt-analyze` (which already handles downstream impact analysis using lineage + manifest). This matches how the builder prompt already references it. Also adds tests to catch prompt-to-skill reference mismatches: - Analyst "Skills Available" section only lists existing skill directories - Analyst and builder prompts don't reference phantom /impact-analysis - Builder prompt correctly references /dbt-analyze https://claude.ai/code/session_01JzUGJ9qyY5Xgcszvvru7nt
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/test/altimate/sql-validation-adversarial.test.ts`:
- Around line 650-660: The test calls a non-registered method name
"altimate_core.is_safe" causing a handler-not-found error; change that call to
the registered dispatcher method "altimate_core.safety" (same pattern as the
earlier suite) so the safety scan is invoked correctly, i.e., update the
Dispatcher.call invocation that uses "altimate_core.is_safe" to use
"altimate_core.safety" and keep the existing assertions (e.g., checking the
returned success/data properties) unchanged.
- Around line 950-978: The tests call Dispatcher.call("sql.translate") with the
wrong field names and assert the wrong response shape; update calls to use
source_dialect and target_dialect (instead of from_dialect/to_dialect) and when
inspecting the response use translated.data?.translated_sql (not
translated.data?.sql), then pass that translated_sql string into
classifyAndCheck; update both occurrences around the
Dispatcher.call("sql.translate") usages and the variable translated before
classifyAndCheck so the tests exercise the real translation path and follow-up
classification.
In `@packages/opencode/test/altimate/sql-validation-e2e.test.ts`:
- Around line 364-375: The test currently computes selectStarIssue from the
Dispatcher.call("sql.analyze") result but never asserts it; update the "SELECT *
triggers lint finding" test to explicitly assert that selectStarIssue exists
(truthy) and optionally that selectStarIssue.type === "lint" and its message
contains "SELECT *" or "select_star" to ensure the lint rule is being reported;
locate the computation of selectStarIssue in the test and add the explicit
expect checks for presence and expected properties.
- Around line 608-610: The repoRoot resolution uses process.cwd() and ../..
which breaks when tests are run from the monorepo root; change the resolution to
derive the repo root from the test file location using __dirname (e.g. replace
path.resolve(process.cwd(), "../..") with path.resolve(__dirname, "../../.."))
so repoRoot is stable regardless of working directory; update the same pattern
around the other occurrences referenced (the code that sets repoRoot in the
"Skill files reference real tools" describe block and the related lines
~640-646).
🪄 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: 7dd12505-d886-49e6-bb9e-a72616583a8c
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.opencode/skills/sql-translate/SKILL.mdpackages/opencode/src/agent/agent.tspackages/opencode/src/altimate/prompts/analyst.txtpackages/opencode/src/altimate/prompts/builder.txtpackages/opencode/test/altimate/sql-validation-adversarial.test.tspackages/opencode/test/altimate/sql-validation-e2e.test.ts
Comment on lines
+650
to
+660
| test("SQL injection attempt is caught by safety scan", async () => { | ||
| const sql = "SELECT * FROM users WHERE id = '1' OR '1'='1'" | ||
|
|
||
| const checkResult = await Dispatcher.call("altimate_core.check", { sql }) | ||
| expect(checkResult).toHaveProperty("data") | ||
| const data = checkResult.data as Record<string, any> | ||
| expect(data.safety).toBeDefined() | ||
|
|
||
| // Also test is_safe directly | ||
| const safetyResult = await Dispatcher.call("altimate_core.is_safe", { sql }) | ||
| expect(safetyResult).toHaveProperty("success") |
There was a problem hiding this comment.
Call the registered safety method name here.
This suite already validates altimate_core.safety as the registered dispatcher method in packages/opencode/test/altimate/sql-validation-e2e.test.ts. If altimate_core.is_safe is not an alias, Line 659 throws No native handler... instead of testing the safety scan.
Suggested fix
- const safetyResult = await Dispatcher.call("altimate_core.is_safe", { sql })
+ const safetyResult = await Dispatcher.call("altimate_core.safety", { sql })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("SQL injection attempt is caught by safety scan", async () => { | |
| const sql = "SELECT * FROM users WHERE id = '1' OR '1'='1'" | |
| const checkResult = await Dispatcher.call("altimate_core.check", { sql }) | |
| expect(checkResult).toHaveProperty("data") | |
| const data = checkResult.data as Record<string, any> | |
| expect(data.safety).toBeDefined() | |
| // Also test is_safe directly | |
| const safetyResult = await Dispatcher.call("altimate_core.is_safe", { sql }) | |
| expect(safetyResult).toHaveProperty("success") | |
| test("SQL injection attempt is caught by safety scan", async () => { | |
| const sql = "SELECT * FROM users WHERE id = '1' OR '1'='1'" | |
| const checkResult = await Dispatcher.call("altimate_core.check", { sql }) | |
| expect(checkResult).toHaveProperty("data") | |
| const data = checkResult.data as Record<string, any> | |
| expect(data.safety).toBeDefined() | |
| // Also test is_safe directly | |
| const safetyResult = await Dispatcher.call("altimate_core.safety", { sql }) | |
| expect(safetyResult).toHaveProperty("success") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/sql-validation-adversarial.test.ts` around
lines 650 - 660, The test calls a non-registered method name
"altimate_core.is_safe" causing a handler-not-found error; change that call to
the registered dispatcher method "altimate_core.safety" (same pattern as the
earlier suite) so the safety scan is invoked correctly, i.e., update the
Dispatcher.call invocation that uses "altimate_core.is_safe" to use
"altimate_core.safety" and keep the existing assertions (e.g., checking the
returned success/data properties) unchanged.
Comment on lines
+950
to
+978
| test("sql.translate is callable and returns result shape", async () => { | ||
| const r = await Dispatcher.call("sql.translate", { | ||
| sql: "SELECT IFNULL(name, 'unknown') FROM users", | ||
| from_dialect: "snowflake", | ||
| to_dialect: "postgres", | ||
| }) | ||
| expect(r).toHaveProperty("success") | ||
| }) | ||
|
|
||
| test("sql.optimize is callable and returns suggestions", async () => { | ||
| const r = await Dispatcher.call("sql.optimize", { | ||
| sql: "SELECT * FROM users WHERE id IN (SELECT user_id FROM orders)", | ||
| }) | ||
| expect(r).toHaveProperty("success") | ||
| }) | ||
|
|
||
| test("translated SQL maintains correct classification", async () => { | ||
| // A SELECT stays a SELECT after translation | ||
| const translated = await Dispatcher.call("sql.translate", { | ||
| sql: "SELECT NVL(name, 'unknown') FROM users LIMIT 10", | ||
| from_dialect: "snowflake", | ||
| to_dialect: "postgres", | ||
| }) | ||
|
|
||
| if (translated.success && translated.data?.sql) { | ||
| const { queryType, blocked } = classifyAndCheck(translated.data.sql as string) | ||
| expect(queryType).toBe("read") | ||
| expect(blocked).toBe(false) | ||
| } |
There was a problem hiding this comment.
Match the sql.translate contract that the dispatcher actually exposes.
packages/opencode/src/altimate/native/sql/register.ts:99-129 reads source_dialect/target_dialect and returns translated_sql. Lines 953-954 and 970-971 send different field names, and Line 974 looks for translated.data?.sql, so these tests can pass without ever exercising a successful translation or the follow-up classification.
Suggested fix
const r = await Dispatcher.call("sql.translate", {
sql: "SELECT IFNULL(name, 'unknown') FROM users",
- from_dialect: "snowflake",
- to_dialect: "postgres",
+ source_dialect: "snowflake",
+ target_dialect: "postgres",
})
- expect(r).toHaveProperty("success")
+ expect(r.success).toBe(true)
+ expect(r.translated_sql).toBeTruthy()
@@
const translated = await Dispatcher.call("sql.translate", {
sql: "SELECT NVL(name, 'unknown') FROM users LIMIT 10",
- from_dialect: "snowflake",
- to_dialect: "postgres",
+ source_dialect: "snowflake",
+ target_dialect: "postgres",
})
-
- if (translated.success && translated.data?.sql) {
- const { queryType, blocked } = classifyAndCheck(translated.data.sql as string)
- expect(queryType).toBe("read")
- expect(blocked).toBe(false)
- }
+ expect(translated.success).toBe(true)
+ const { queryType, blocked } = classifyAndCheck(translated.translated_sql)
+ expect(queryType).toBe("read")
+ expect(blocked).toBe(false)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("sql.translate is callable and returns result shape", async () => { | |
| const r = await Dispatcher.call("sql.translate", { | |
| sql: "SELECT IFNULL(name, 'unknown') FROM users", | |
| from_dialect: "snowflake", | |
| to_dialect: "postgres", | |
| }) | |
| expect(r).toHaveProperty("success") | |
| }) | |
| test("sql.optimize is callable and returns suggestions", async () => { | |
| const r = await Dispatcher.call("sql.optimize", { | |
| sql: "SELECT * FROM users WHERE id IN (SELECT user_id FROM orders)", | |
| }) | |
| expect(r).toHaveProperty("success") | |
| }) | |
| test("translated SQL maintains correct classification", async () => { | |
| // A SELECT stays a SELECT after translation | |
| const translated = await Dispatcher.call("sql.translate", { | |
| sql: "SELECT NVL(name, 'unknown') FROM users LIMIT 10", | |
| from_dialect: "snowflake", | |
| to_dialect: "postgres", | |
| }) | |
| if (translated.success && translated.data?.sql) { | |
| const { queryType, blocked } = classifyAndCheck(translated.data.sql as string) | |
| expect(queryType).toBe("read") | |
| expect(blocked).toBe(false) | |
| } | |
| test("sql.translate is callable and returns result shape", async () => { | |
| const r = await Dispatcher.call("sql.translate", { | |
| sql: "SELECT IFNULL(name, 'unknown') FROM users", | |
| source_dialect: "snowflake", | |
| target_dialect: "postgres", | |
| }) | |
| expect(r.success).toBe(true) | |
| expect(r.translated_sql).toBeTruthy() | |
| }) | |
| test("sql.optimize is callable and returns suggestions", async () => { | |
| const r = await Dispatcher.call("sql.optimize", { | |
| sql: "SELECT * FROM users WHERE id IN (SELECT user_id FROM orders)", | |
| }) | |
| expect(r).toHaveProperty("success") | |
| }) | |
| test("translated SQL maintains correct classification", async () => { | |
| // A SELECT stays a SELECT after translation | |
| const translated = await Dispatcher.call("sql.translate", { | |
| sql: "SELECT NVL(name, 'unknown') FROM users LIMIT 10", | |
| source_dialect: "snowflake", | |
| target_dialect: "postgres", | |
| }) | |
| expect(translated.success).toBe(true) | |
| const { queryType, blocked } = classifyAndCheck(translated.translated_sql) | |
| expect(queryType).toBe("read") | |
| expect(blocked).toBe(false) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/sql-validation-adversarial.test.ts` around
lines 950 - 978, The tests call Dispatcher.call("sql.translate") with the wrong
field names and assert the wrong response shape; update calls to use
source_dialect and target_dialect (instead of from_dialect/to_dialect) and when
inspecting the response use translated.data?.translated_sql (not
translated.data?.sql), then pass that translated_sql string into
classifyAndCheck; update both occurrences around the
Dispatcher.call("sql.translate") usages and the variable translated before
classifyAndCheck so the tests exercise the real translation path and follow-up
classification.
Comment on lines
+364
to
+375
| test("SELECT * triggers lint finding", async () => { | ||
| const result = await Dispatcher.call("sql.analyze", { | ||
| sql: "SELECT * FROM users", | ||
| }) | ||
| expect(result).toHaveProperty("issues") | ||
| // SELECT * should be caught by lint | ||
| const selectStarIssue = result.issues.find( | ||
| (i: any) => i.type === "lint" && (i.message?.includes("SELECT *") || i.message?.includes("select_star") || i.message?.toLowerCase?.().includes("star")), | ||
| ) | ||
| // At minimum, issues array should exist and analyzer should not crash | ||
| expect(Array.isArray(result.issues)).toBe(true) | ||
| }) |
There was a problem hiding this comment.
Assert the SELECT * finding explicitly.
Line 370 computes selectStarIssue, but the test never checks it. As written, this still passes if sql.analyze stops flagging SELECT *, so the coverage here is mostly “no crash” rather than lint validation.
Suggested tightening
const selectStarIssue = result.issues.find(
(i: any) => i.type === "lint" && (i.message?.includes("SELECT *") || i.message?.includes("select_star") || i.message?.toLowerCase?.().includes("star")),
)
- // At minimum, issues array should exist and analyzer should not crash
expect(Array.isArray(result.issues)).toBe(true)
+ expect(selectStarIssue).toBeDefined()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("SELECT * triggers lint finding", async () => { | |
| const result = await Dispatcher.call("sql.analyze", { | |
| sql: "SELECT * FROM users", | |
| }) | |
| expect(result).toHaveProperty("issues") | |
| // SELECT * should be caught by lint | |
| const selectStarIssue = result.issues.find( | |
| (i: any) => i.type === "lint" && (i.message?.includes("SELECT *") || i.message?.includes("select_star") || i.message?.toLowerCase?.().includes("star")), | |
| ) | |
| // At minimum, issues array should exist and analyzer should not crash | |
| expect(Array.isArray(result.issues)).toBe(true) | |
| }) | |
| test("SELECT * triggers lint finding", async () => { | |
| const result = await Dispatcher.call("sql.analyze", { | |
| sql: "SELECT * FROM users", | |
| }) | |
| expect(result).toHaveProperty("issues") | |
| // SELECT * should be caught by lint | |
| const selectStarIssue = result.issues.find( | |
| (i: any) => i.type === "lint" && (i.message?.includes("SELECT *") || i.message?.includes("select_star") || i.message?.toLowerCase?.().includes("star")), | |
| ) | |
| expect(selectStarIssue).toBeDefined() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/sql-validation-e2e.test.ts` around lines 364
- 375, The test currently computes selectStarIssue from the
Dispatcher.call("sql.analyze") result but never asserts it; update the "SELECT *
triggers lint finding" test to explicitly assert that selectStarIssue exists
(truthy) and optionally that selectStarIssue.type === "lint" and its message
contains "SELECT *" or "select_star" to ensure the lint rule is being reported;
locate the computation of selectStarIssue in the test and add the explicit
expect checks for presence and expected properties.
Comment on lines
+608
to
+610
| describe("Skill files reference real tools", () => { | ||
| // Skills live at the repo root, not packages/opencode | ||
| const repoRoot = path.resolve(process.cwd(), "../..") |
There was a problem hiding this comment.
Resolve the repo root from the test file, not process.cwd().
Line 610 and Line 641 only work when bun test is launched from packages/opencode. Running the same suite from the monorepo root makes ../.. point outside the repo, so these file-based checks fail for the wrong reason.
Suggested fix
- const repoRoot = path.resolve(process.cwd(), "../..")
+ const repoRoot = path.resolve(import.meta.dir, "../../../..")Also applies to: 640-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/sql-validation-e2e.test.ts` around lines 608
- 610, The repoRoot resolution uses process.cwd() and ../.. which breaks when
tests are run from the monorepo root; change the resolution to derive the repo
root from the test file location using __dirname (e.g. replace
path.resolve(process.cwd(), "../..") with path.resolve(__dirname, "../../.."))
so repoRoot is stable regardless of working directory; update the same pattern
around the other occurrences referenced (the code that sets repoRoot in the
"Skill files reference real tools" describe block and the related lines
~640-646).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR removes references to a non-existent
sql_validatetool from agent permissions and prompts, and adds comprehensive end-to-end and adversarial test suites for SQL validation.Changes Made
Source Code:
sql_validate: "allow"from analyst agent permissions (the tool doesn't exist; real tool isaltimate_core_validate)sql_validatetool; updated to reference actual tools (altimate_core_validate,sql_analyze,sql_execute)sql_valtoolTests Added:
sql-validation-e2e.test.ts (692 lines): End-to-end tests verifying:
sql_validate)altimate_core_validateworks via Dispatcheraltimate_core_checkcomposite pipeline workssql.analyzecomposite pipeline workssql-classifycorrectly gatessql_executebased on query typesql-validation-adversarial.test.ts (980 lines): Adversarial, user-perspective, and stress tests covering:
Test Plan
All new tests pass via
bun test:sql_validatereferences are removed and only real tools are usedChecklist
https://claude.ai/code/session_01JzUGJ9qyY5Xgcszvvru7nt
Summary by CodeRabbit
New Features
/dbt-analyzecommand for performing downstream impact analysis using dbt lineage metadata.Improvements
Tests