Remove phantom sql_validate tool and add comprehensive SQL validation tests

[anandgupta42]

Summary

This PR removes references to a non-existent sql_validate tool from agent permissions and prompts, and adds comprehensive end-to-end and adversarial test suites for SQL validation.

Changes Made

Source Code:

• agent.ts: Removed sql_validate: "allow" from analyst agent permissions (the tool doesn't exist; real tool is altimate_core_validate)
• builder.txt & analyst.txt prompts: Removed references to phantom sql_validate tool; updated to reference actual tools (altimate_core_validate, sql_analyze, sql_execute)
• sql-translate SKILL.md: Removed reference to non-existent sql_val tool

Tests Added:

• sql-validation-e2e.test.ts (692 lines): End-to-end tests verifying:

  • Tool names in prompts match registered tools (no sql_validate)
  • Agent permissions reference only real tools
  • altimate_core_validate works via Dispatcher
  • altimate_core_check composite pipeline works
  • sql.analyze composite pipeline works
  • Pre-execution protocol (analyze → validate → classify) works correctly
  • sql-classify correctly gates sql_execute based on query type
  • Analyst and builder agent permissions are consistent

• sql-validation-adversarial.test.ts (980 lines): Adversarial, user-perspective, and stress tests covering:

  • SQL injection bypass attempts (comment injection, string literals, encoding tricks)
  • Whitespace and formatting tricks
  • Multi-statement bypass attempts
  • Edge cases (null, undefined, empty input, very large payloads)
  • User permission flow for read vs. write queries
  • Hard-blocking of destructive operations (DROP DATABASE, TRUNCATE, DROP SCHEMA)
  • Concurrent validation stress tests
  • Error recovery and resilience

Test Plan

All new tests pass via bun test:

• 692-line E2E test suite validates tool consistency and full validation pipeline
• 980-line adversarial test suite validates security gates and user-facing behavior
• Tests confirm that sql_validate references are removed and only real tools are used
• Tests verify that destructive queries are hard-blocked before permission checks
• Stress tests confirm concurrent validation and large payload handling

Checklist

• Tests added (comprehensive E2E and adversarial suites)
• Source code updated (removed phantom tool references)
• Documentation updated (prompts and SKILL.md)

https://claude.ai/code/session_01JzUGJ9qyY5Xgcszvvru7nt

Summary by CodeRabbit

• New Features

  • Introduced /dbt-analyze command for performing downstream impact analysis using dbt lineage metadata.

• Improvements

  • Standardized SQL validation tooling references across agent configurations and system prompts.
  • Updated agent permissions to align with enhanced validation capabilities.

• Tests

  • Added comprehensive end-to-end validation test suite covering complex SQL patterns and edge cases.
  • Introduced adversarial test coverage for SQL safety validation and permission enforcement.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

[coderabbitai]

Warning

Rate limit exceeded

@anandgupta42 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 58 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 6ab5f7c1-0bf9-4939-8d42-aed5308adea5

📥 Commits

Reviewing files that changed from the base of the PR and between 0ab6b17 and cda3ab1.

📒 Files selected for processing (3)

• packages/opencode/test/altimate/sql-validation-adversarial.test.ts
• packages/opencode/test/altimate/sql-validation-e2e.test.ts
• packages/opencode/test/provider/error.test.ts

📝 Walkthrough

Walkthrough

This PR replaces references to the deprecated sql_validate tool with altimate_core_validate across skill documentation, agent permission rules, and AI prompts. Additionally, it adds two comprehensive E2E test suites covering SQL validation, classification, safety checks, and dispatcher integration with extensive adversarial and integration test coverage.

Changes

Cohort / File(s)	Summary
Documentation & Configuration Updates .opencode/skills/sql-translate/SKILL.md, packages/opencode/src/altimate/prompts/analyst.txt, packages/opencode/src/altimate/prompts/builder.txt	Updated all references from deprecated sql_validate to altimate_core_validate across skill documentation and agent prompts. Analyst prompt also renamed /impact-analysis skill command to /dbt-analyze.
Agent Permissions packages/opencode/src/agent/agent.ts	Moved altimate_core_validate from the "Core tools" permission block to the "SQL read tools" allow-list for the analyst agent, maintaining explicit permission while reorganizing rule placement.
SQL Validation Test Suite packages/opencode/test/altimate/sql-validation-adversarial.test.ts	New comprehensive E2E test suite (980 lines) covering adversarial SQL classification bypass attempts, permission flow validation, safety checks, concurrent stress tests, and dialect-specific SQL handling with mocked DuckDB driver.
SQL Validation E2E Integration Tests packages/opencode/test/altimate/sql-validation-e2e.test.ts	New extensive integration test suite (777 lines) validating tool availability, agent prompt consistency, permission rules, dispatcher handler registration, and cross-tool agreement on SQL validation results.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• PR #282 — Overlapping updates to agent permissions, analyst/builder prompts, and SQL validation tooling references (packages/opencode/src/agent/agent.ts and altimate prompt files).

Suggested reviewers

• suryaiyer95

Poem

🐰 A leap through validation tests we bound,
From sql_validate to core_validate sound,
With permissions reorganized just right,
And adversarial challenges burning bright,
The safety checks dance in the digital night! 🌙✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately summarizes the main changes: removing phantom sql_validate tool references and adding comprehensive SQL validation tests.
Description check	✅ Passed	The PR description follows the template with clear Summary, Test Plan, and Checklist sections, detailing all changes made and testing approach.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/review-sql-validation-mKDlf

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 623d1ac and 0ab6b17.

⛔ Files ignored due to path filters (1)

• bun.lock is excluded by !**/*.lock

📒 Files selected for processing (6)

• .opencode/skills/sql-translate/SKILL.md
• packages/opencode/src/agent/agent.ts
• packages/opencode/src/altimate/prompts/analyst.txt
• packages/opencode/src/altimate/prompts/builder.txt
• packages/opencode/test/altimate/sql-validation-adversarial.test.ts
• packages/opencode/test/altimate/sql-validation-e2e.test.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

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()=>{
	constsql="SELECT * FROM users WHERE id = '1' OR '1'='1'"
	constcheckResult=awaitDispatcher.call("altimate_core.check",{ sql })
	expect(checkResult).toHaveProperty("data")
	constdata=checkResult.dataasRecord<string,any>
	expect(data.safety).toBeDefined()
	// Also test is_safe directly
	constsafetyResult=awaitDispatcher.call("altimate_core.is_safe",{ sql })
	expect(safetyResult).toHaveProperty("success")
	test("SQL injection attempt is caught by safety scan",async()=>{
	constsql="SELECT * FROM users WHERE id = '1' OR '1'='1'"
	constcheckResult=awaitDispatcher.call("altimate_core.check",{ sql })
	expect(checkResult).toHaveProperty("data")
	constdata=checkResult.dataasRecord<string,any>
	expect(data.safety).toBeDefined()
	// Also test is_safe directly
	constsafetyResult=awaitDispatcher.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.