Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Contributor
anandgupta42
left a comment
anandgupta42
left a comment
There was a problem hiding this comment.
Detailed Code Review — PR #282
Reviewed the full diff across all 12 changed files. Here are the findings organized by severity.
🔴 Must Fix Before Merge
1. BUG: Analyst bash "*": "deny" overrides ALL allow rules
File: agent.ts — analyst bash permissions (line ~196)
The analyst bash config is:
bash: {
"ls *": "allow", "grep *": "allow", "cat *": "allow",
"head *": "allow", "tail *": "allow", "find *": "allow", "wc *": "allow",
"dbt list *": "allow", "dbt ls *": "allow", "dbt debug *": "allow", "dbt deps *": "allow",
"dbt run *": "deny", "dbt build *": "deny", "dbt seed *": "deny", "dbt docs *": "deny",
"*": "deny",
},fromConfig() converts this to rules in object insertion order. evaluate() uses findLast() (last-match-wins). Since "*": "deny" is the LAST rule, it matches everything and overrides all the allow rules above it. This means ALL bash commands are denied for analyst — ls, cat, grep, find, wc, and dbt list/debug/deps are all blocked.
The existing code comments at line 79 already warn about this: "IMPORTANT: *: ask must come FIRST because evaluation uses last-match-wins."
Fix: Move "*": "deny" to the FIRST position in the bash object.
2. always: [] means "Allow always" does nothing
File: sql-execute.ts:41
await ctx.ask({
permission: "sql_execute_write",
patterns: [args.query.slice(0, 200)],
always: [], // <-- empty
metadata: { queryType },
})Every other tool in the codebase passes always: ["*"] or a meaningful pattern. With always: [], when a user clicks "Allow always" in the TUI, it approves the current request but saves nothing — the next write query prompts again. This makes builder mode unusable for any workflow involving multiple writes.
Fix: If intentional (force per-query approval for safety), add a comment explaining why. If not, change to always: ["*"] or always: [args.query.slice(0, 200)].
3. HARD_DENY returns a result, not an error
File: sql-execute.ts:27-32
if (HARD_DENY.test(stripped)) {
return {
title: "SQL: BLOCKED",
metadata: { rowCount: 0, truncated: false },
output: "DROP DATABASE, DROP SCHEMA, and TRUNCATE are blocked for safety...",
}
}Returning a normal tool result means the LLM sees a "successful" tool call with a text message. It might retry with rephrased SQL or misinterpret the response. Other safety mechanisms in the codebase throw DeniedError which the framework handles as a hard stop.
Fix: Consider throw new PermissionNext.DeniedError(...) instead of returning.
🟡 Should Fix Before Merge
4. sql_execute_write safety denials missing Drop Database * variant
File: agent.ts:125-133
The existing bash safety denials include three case variants: DROP DATABASE *, drop database *, Drop Database *. The new sql_execute_write denials only have UPPER and lower — missing the title-case variant. Low risk since HARD_DENY uses /i flag, but inconsistent with the existing pattern.
5. No integration tests for the new permission
The 47 classifier unit tests are solid. But there are no tests verifying:
sql_execute_write: "deny"actually blocks in analyst modesql_execute_write: "ask"prompts in builder modeHARD_DENYbehavior (returns/throws on DROP DATABASE)- The
always: []behavior when user clicks "Allow always"
6. Documentation not updated (12 files changed, 0 docs updated)
These docs are now incorrect:
| Doc | Issue |
|---|---|
docs/docs/data-engineering/agent-modes.md |
Lists 7 agents — 5 no longer exist |
docs/docs/security-faq.md |
Missing sql_execute_write permission, examples incomplete |
docs/docs/configure/permissions.md |
sql_execute_write not in "All Permissioned Tools" table |
docs/docs/configure/agents.md |
Examples reference deleted agents |
🟢 Nice to Have / Low Priority
7. Classifier edge cases (all safe-direction)
- Semicolons in string literals:
SELECT 'foo;DROP TABLE x' FROM t→ splits incorrectly, classifies as write (false positive — safe direction, causes extra prompt) - Missing write keywords:
COPY FROM(PostgreSQL),VACUUM,ANALYZE,COMMENT ON,EXECUTE TASK(Snowflake) - 200-char query truncation: User sees truncated query in TUI prompt, may not see the dangerous part at the end of a long query
8. Double semicolon splitting
sql-execute.ts splits on ; for HARD_DENY check (line 23), then classifyMulti splits on ; again (line 36). Could unify into a single split.
What's Good
- The classifier design philosophy is correct — conservative (unknown → write, false positives cause prompts not data loss)
- CTE paren-depth counting handles nested parens and multi-CTE queries correctly
- Defense-in-depth is solid — three layers: HARD_DENY (hardcoded) → safety denials (permission system) → per-agent config
- Agent simplification reduces maintenance burden — 5 agents with nearly identical permission lists consolidated into 2
- Test coverage for the classifier is thorough (47 tests covering 16+ SQL keywords, comments, CTEs, multi-statement)
- Cleanup is complete — memory types, verify-restructure, agent tests all properly updated for removed agents
Summary
The PR fills a real safety gap (sql_execute had zero runtime guardrails) and the classifier is well-designed. But the analyst bash *: deny ordering bug (#1) is a showstopper — it breaks all bash commands for analyst mode. Items #2 and #3 are design decisions that should be explicitly called out. Docs need updating before or shortly after merge.
kulvirgit
force-pushed
the
modes
branch
2 times, most recently
from
e2cc93e to
209db7e
Compare
Collaborator
Author
|
Thanks for the thorough review @anandgupta42! All issues addressed: 🔴 Must Fix — All Fixed#1 Bash ordering bug — Fixed. #2 #3 HARD_DENY returns instead of throws — Fixed. Now 🟡 Should Fix — All Fixed#4 Missing title-case variants — Fixed. Added #5 Integration tests — Noted for follow-up. The classifier unit tests cover the detection logic; integration tests for the permission wiring would need the full Instance.provide() setup. #6 Docs — Fixed. Updated 🟢 Nice to Have — Fixed#7 Missing write keywords — Fixed. Added #8 Double splitting — Fixed. Unified into 61 tests now passing (up from 47). |
kulvirgit
force-pushed
the
modes
branch
2 times, most recently
from
f2e816d to
de71ebf
Compare
kulvirgit
force-pushed
the
modes
branch
2 times, most recently
from
1be3f65 to
74dae9b
Compare
arora-saurabh448
pushed a commit
that referenced
this pull request
Mar 19, 2026
… stub web UI - Reduce agent modes from 7 to 3 (builder, analyst, plan) per PR #282 - Add SQL Write Access Control section with query classification table - Add sql_execute_write permission to permissions reference - Update /data to /altimate in Claude Code guide, add /configure-claude setup - Add Codex CLI skill integration and /configure-codex setup - Add /configure-claude and /configure-codex to commands reference - Stub web UI page with Coming Soon notice - Update all cross-references (getting-started, quickstart, index, tui, training, migration) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
Sentry Bot Findings — All Addressed
The SQL classifier has been rewritten to use |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
📝 WalkthroughWalkthroughConsolidates custom agent prompts by removing five modes, adds SQL classification and hard-deny checks for destructive statements, enforces permission prompts for SQL writes, updates agent permissions and tests, and adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SqlExecute as SqlExecuteTool
participant Classify as SQL Classify
participant Context as Permission Context
participant DB as Database
User->>SqlExecute: execute(query)
activate SqlExecute
SqlExecute->>Classify: classifyAndCheck(query)
activate Classify
Classify-->>Classify: parse AST & detect statement types
Classify-->>Classify: detect hard-deny patterns (DROP/TRUNCATE)
Classify-->>SqlExecute: { queryType, blocked }
deactivate Classify
alt blocked == true
SqlExecute-->>User: throw Error (blocked statement)
else if queryType == "write"
SqlExecute->>Context: ask(permission: "sql_execute_write", patterns, metadata)
activate Context
Context-->>SqlExecute: approved / denied
deactivate Context
alt approved
SqlExecute->>DB: execute(query)
DB-->>SqlExecute: result
SqlExecute-->>User: result
else denied
SqlExecute-->>User: permission denied
end
else read query
SqlExecute->>DB: execute(query)
DB-->>SqlExecute: result
SqlExecute-->>User: result
end
deactivate SqlExecute
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/opencode/src/altimate/tools/sql-classify.ts (1)
6-7: Consider using ES module import syntax.The file uses CommonJS
require()while the project is configured as"type": "module". While this works due to TypeScript transpilation, using ES import syntax would be more consistent with the rest of the codebase.Suggested change
-// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -const core: any = require("@altimateai/altimate-core") +// eslint-disable-next-line `@typescript-eslint/no-require-imports`, `@typescript-eslint/no-explicit-any` +import * as core from "@altimateai/altimate-core"If the package doesn't export proper TypeScript types, you can keep the
anycast:import * as coreModule from "@altimateai/altimate-core" const core = coreModule as any🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/sql-classify.ts` around lines 6 - 7, Replace the CommonJS require usage for the core dependency by using ES module import syntax: import the package as a namespace (e.g., coreModule) and then assign it to the existing symbol (core) with an any cast to preserve types (e.g., const core = coreModule as any). Update the top of the file where the current const core = require(...) appears, remove the eslint-disable-if-unneeded, and ensure all subsequent references continue to use core.packages/opencode/test/altimate/tools/sql-classify.test.ts (1)
122-156: Good coverage of blocking behavior inclassifyAndChecktests.The tests verify that destructive operations (
DROP DATABASE,DROP SCHEMA,TRUNCATE) are blocked while regular writes are not.Consider adding edge case tests for potential bypass attempts that the AST parser should handle correctly:
Suggested additional test cases
// Edge cases that AST parsing should handle correctly test("SQL comment containing DROP → not blocked", () => { const r = classifyAndCheck("SELECT 1 -- DROP DATABASE mydb") expect(r.blocked).toBe(false) }) test("string literal containing TRUNCATE → not blocked", () => { const r = classifyAndCheck("SELECT 'TRUNCATE TABLE users' as query") expect(r.blocked).toBe(false) }) test("DROP SCHEMA case variants → blocked", () => { expect(classifyAndCheck("drop schema public").blocked).toBe(true) expect(classifyAndCheck("Drop Schema public").blocked).toBe(true) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/tools/sql-classify.test.ts` around lines 122 - 156, Add unit tests to cover parser edge-cases around comments, string literals, and case-insensitivity for destructive keywords: extend the existing test suite that calls classifyAndCheck to include (1) a test ensuring SQL comments containing "DROP" do not mark the statement blocked, (2) a test ensuring string literals containing "TRUNCATE" do not mark the statement blocked, and (3) tests for lowercase/mixed-case "drop schema" (e.g., "drop schema public" and "Drop Schema public") that still return blocked; place these alongside the other classifyAndCheck tests so the AST-based detection logic is exercised for comments, literals, and case variants.
🤖 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/src/altimate/tools/sql-classify.ts`:
- Around line 42-44: The current blocked computation calls
s.statement_type.toUpperCase() without guarding for null/undefined which can
throw; update the predicate used in the const blocked =
result.statements.some(...) to first verify that s.statement_type is a non-empty
string (e.g., typeof s.statement_type === 'string' and s.statement_type.length >
0) before calling toUpperCase(), and only then check HARD_DENY_TYPES.has(...);
ensure any non-string or missing statement_type simply yields false so those
statements are not treated as denied.
---
Nitpick comments:
In `@packages/opencode/src/altimate/tools/sql-classify.ts`:
- Around line 6-7: Replace the CommonJS require usage for the core dependency by
using ES module import syntax: import the package as a namespace (e.g.,
coreModule) and then assign it to the existing symbol (core) with an any cast to
preserve types (e.g., const core = coreModule as any). Update the top of the
file where the current const core = require(...) appears, remove the
eslint-disable-if-unneeded, and ensure all subsequent references continue to use
core.
In `@packages/opencode/test/altimate/tools/sql-classify.test.ts`:
- Around line 122-156: Add unit tests to cover parser edge-cases around
comments, string literals, and case-insensitivity for destructive keywords:
extend the existing test suite that calls classifyAndCheck to include (1) a test
ensuring SQL comments containing "DROP" do not mark the statement blocked, (2) a
test ensuring string literals containing "TRUNCATE" do not mark the statement
blocked, and (3) tests for lowercase/mixed-case "drop schema" (e.g., "drop
schema public" and "Drop Schema public") that still return blocked; place these
alongside the other classifyAndCheck tests so the AST-based detection logic is
exercised for comments, literals, and case variants.
🪄 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: 58d2e544-b49a-4a08-95d6-46ee87e7a051
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
docs/docs/configure/agents.mddocs/docs/configure/permissions.mdpackages/opencode/package.jsonpackages/opencode/src/agent/agent.tspackages/opencode/src/altimate/prompts/executive.txtpackages/opencode/src/altimate/prompts/migrator.txtpackages/opencode/src/altimate/prompts/researcher.txtpackages/opencode/src/altimate/prompts/trainer.txtpackages/opencode/src/altimate/prompts/validator.txtpackages/opencode/src/altimate/tools/sql-classify.tspackages/opencode/src/altimate/tools/sql-execute.tspackages/opencode/src/memory/types.tspackages/opencode/test/agent/agent.test.tspackages/opencode/test/altimate/tools/sql-classify.test.tsscript/upstream/verify-restructure.ts
💤 Files with no reviewable changes (8)
- packages/opencode/src/altimate/prompts/researcher.txt
- script/upstream/verify-restructure.ts
- packages/opencode/src/altimate/prompts/trainer.txt
- packages/opencode/src/memory/types.ts
- packages/opencode/src/altimate/prompts/validator.txt
- packages/opencode/src/altimate/prompts/executive.txt
- packages/opencode/src/altimate/prompts/migrator.txt
- packages/opencode/test/agent/agent.test.ts
arora-saurabh448
pushed a commit
that referenced
this pull request
Mar 19, 2026
…#284) * docs: update README and docs for v0.4.2 launch - Standardize tool count to 100+ across all docs (was 99+/55+) - Update install command to unscoped `altimate-code` package - Remove stale Python/uv auto-setup claims (all-native TypeScript now) - Update docs badge URL to docs.altimate.sh - Remove altimate-core npm badge from README - Add --yolo flag to CLI reference and builder mode subtext - Add new env vars (YOLO, MEMORY, TRAINING) to CLI docs - Add prompt enhancement keybind (leader+i) to TUI and keybinds docs - Add tool_lookup to tools index - Add built-in skills table (sql-review, schema-migration, pii-audit, etc.) - Add altimate-dbt CLI section to dbt-tools.md - Add Oracle and SQLite to warehouse lists - Update security FAQ: replace Python engine FAQ with native engine, add sensitive_write FAQ - Update telemetry docs to remove Python engine references - Add v0.4.2 to README "What's New" section - Update llms.txt URLs to docs.altimate.sh and bump version to v0.4.2 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: simplify zero-setup messaging in README and quickstart Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: simplify install callout to "zero additional setup" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: trim install callout Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: remove em-dashes, fix pill spacing, simplify /discover duplication - Replace 304 em-dashes across 38 docs files with natural sentence structures (colons, commas, periods, split sentences) to avoid AI-generated content appearance - Fix pill-grid CSS: increase gap/padding, add responsive breakpoints at 768px and 480px for reliable scaling across viewport sizes - Simplify quickstart /discover step to brief description + link to Full Setup; add (Optional) marker to getting-started warehouse step Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: overhaul getting-started pages with comprehensive setup guide Rewrite quickstart as a full Setup page covering warehouse connections, LLM provider switching, agent modes, skills, and permissions. Update overview page with ADE-Bench results (74.4%), fix install command, and change 70+ to 50+ tools. Replace query example with NYC taxi cab analytics prompt. Remove time blocks from step headings and trim redundant sections. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: move CI & Headless under Interfaces, deduplicate from CLI Move CI page from data-engineering/guides to usage/. Remove duplicate non-interactive and tracing sections from CLI page, link to CI instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: simplify agents page, quickstart next-steps, and nav label Remove data-engineering-specific agent table from agents.md (now covered elsewhere), replace grid cards in quickstart with a compact link list, and rename "Complete Setup" → "Setup" in nav. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: sync missing content from v2, rename What's New to Changelog - Rename "What's New" to "Changelog" across docs, nav, and README - Populate changelog with full release history (v0.1.0 through v0.4.9) - Add inline permission examples to security-faq (permission table, JSON configs) - Add Data Engineering agents table and Agent Permissions example to agents page - Add Non-interactive Usage and Tracing sections to CLI docs - Add missing nav entries: Web UI, Claude Code/Codex guides, Memory Tools, Observability (Tracing/Telemetry), Training, and Extend (SDK/Server/Plugins/Ecosystem) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: update tool count from 50+ to 100+ across all docs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: fix broken anchors, tool counts, changelog, nav, and hero copy - Fix broken anchor link to #step-3-configure-your-warehouse-optional - Add inline "Adding Custom Skills" section in skills.md - Fix changelog upgrade command to use unscoped package name - Split merged 0.4.1/0.4.2 changelog into separate sections - Update tool count from 70+ to 100+ in configure/tools pages - Move Guides to bottom of Use section in nav - Change hero tagline to "Open-source data engineering harness." - Simplify install command to just npm install Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: sync agent modes from PR #282, Claude/Codex commands from #235, stub web UI - Reduce agent modes from 7 to 3 (builder, analyst, plan) per PR #282 - Add SQL Write Access Control section with query classification table - Add sql_execute_write permission to permissions reference - Update /data to /altimate in Claude Code guide, add /configure-claude setup - Add Codex CLI skill integration and /configure-codex setup - Add /configure-claude and /configure-codex to commands reference - Stub web UI page with Coming Soon notice - Update all cross-references (getting-started, quickstart, index, tui, training, migration) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: fix stale llms.txt URLs, add v0.5.0 changelog entry - Fix 4 broken URLs in llms.txt (network, telemetry, security-faq, troubleshooting) to match reference/ paths in mkdocs nav - Update llms.txt version from v0.4.2 to v0.5.0 - Add missing v0.5.0 changelog entry with features and fixes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Saurabh Arora <saurabh@altimate.ai> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
suryaiyer95
reviewed
Mar 19, 2026
suryaiyer95
reviewed
Mar 19, 2026
Contributor
suryaiyer95
left a comment
suryaiyer95
left a comment
There was a problem hiding this comment.
Some comments and sentry comments
kulvirgit
force-pushed
the
modes
branch
2 times, most recently
from
55e6413 to
f458936
Compare
kulvirgit
force-pushed
the
modes
branch
2 times, most recently
from
fa0c369 to
2efcff8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/agent/agent.ts (1)
149-176:⚠️ Potential issue | 🟠 MajorThe new SQL write gate only covers builder and analyst.
This block adds
sql_execute_writefor those two modes, butplanstill inherits the wildcard allow fromdefaults. That meanspackages/opencode/src/altimate/tools/sql-execute.ts:16-31will allow INSERT/UPDATE/ALTER in plan mode without the new prompt/deny behavior.🧭 Proposed fix
plan: { name: "plan", description: "Plan mode. Disallows all edit tools.", options: {}, permission: PermissionNext.merge( defaults, PermissionNext.fromConfig({ question: "allow", plan_exit: "allow", + sql_execute_write: "deny", external_directory: { [path.join(Global.Path.data, "plans", "*")]: "allow", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/agent/agent.ts` around lines 149 - 176, The plan role still inherits the wildcard allow from defaults so SQL write operations can run in plan mode; update the plan permission block (the PermissionNext.merge call that constructs the "plan" role) to explicitly set sql_execute_write (e.g., "ask" to match builder) inside PermissionNext.fromConfig so plan no longer inherits an unrestricted write permission and will trigger the same prompt/deny flow as the other roles.
♻️ Duplicate comments (2)
packages/opencode/src/altimate/tools/sql-classify.ts (2)
42-43:⚠️ Potential issue | 🟡 MinorGuard
statement_typebefore callingtoUpperCase().
coreis untyped here, so Line 43 can throw on partial parser output and turn the hard-deny path into an unexpected runtime failure.🛡️ Proposed fix
- const blocked = result.statements.some((s: { statement_type: string }) => - HARD_DENY_TYPES.has(s.statement_type.toUpperCase()), + const blocked = result.statements.some((s: { statement_type?: unknown }) => + typeof s.statement_type === "string" && + HARD_DENY_TYPES.has(s.statement_type.toUpperCase()), )What is the documented/runtime shape of `@altimateai/altimate-core` getStatementTypes().statements, and is statement_type guaranteed to always be a non-empty string?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/sql-classify.ts` around lines 42 - 43, The call to s.statement_type.toUpperCase() can throw if statement_type is missing or not a string; update the blocked computation (where result.statements is iterated) to guard statement_type before calling toUpperCase — e.g., verify typeof s.statement_type === "string" and s.statement_type.length > 0 (or coerce safely) then call toUpperCase() and check HARD_DENY_TYPES.has(...); ensure the logic still marks blocked true when a missing/invalid statement_type should be treated as safe or denied according to desired policy.
38-40:⚠️ Potential issue | 🔴 CriticalFail closed when
getStatementTypes()cannot classify a non-empty query.
classifyAndCheck()currently falls back to"read"whenstatementsorcategoriesare missing. Inpackages/opencode/src/altimate/tools/sql-execute.ts:16-31, only"write"triggerssql_execute_write, so an unsupported-but-valid write can bypass the new approval path.🔒 Proposed fix
export function classifyAndCheck(sql: string): { queryType: "read" | "write"; blocked: boolean } { const result = core.getStatementTypes(sql) - if (!result?.statements?.length) return { queryType: "read", blocked: false } + if (!result?.statements?.length) { + return { queryType: sql.trim() ? "write" : "read", blocked: false } + } const blocked = result.statements.some((s: { statement_type: string }) => HARD_DENY_TYPES.has(s.statement_type.toUpperCase()), ) const categories = result.categories ?? [] + if (!categories.length && sql.trim()) return { queryType: "write", blocked } const queryType = categories.some((c: string) => WRITE_CATEGORIES.has(c)) ? "write" : "read" return { queryType: queryType as "read" | "write", blocked } }What does `@altimateai/altimate-core` getStatementTypes return for unsupported or unparsable SQL, and can it yield empty categories/statements for valid statements such as CALL, EXECUTE IMMEDIATE, or dialect-specific procedural blocks?Also applies to: 46-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/sql-classify.ts` around lines 38 - 40, When core.getStatementTypes(sql) returns no statements or missing categories for a non-empty SQL, treat it as unknown/unsafe and fail closed: in classifyAndCheck(), after calling getStatementTypes() check if sql.trim() is non-empty and (result is falsy OR !result.statements?.length OR !result.statements[0].categories?.length) and in that case return { queryType: "write", blocked: true } instead of defaulting to "read"; update the same guard used later in the function that inspects result.statements/result.statements[0].categories to follow this fail-closed behavior so unsupported/unparsable queries (e.g., CALL/EXECUTE IMMEDIATE/procedural blocks) cannot bypass the write approval path.
🤖 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/src/agent/agent.ts`:
- Around line 124-135: The hardcoded sql_execute_write deny rules are currently
being overridden because userWithSafety is applied before
cfg.agent[key].permission is merged later; to fix, ensure the sql_execute_write
rules are applied last with highest precedence so they cannot be overwritten —
e.g., after merging cfg.agent[key].permission, re-merge or overlay the
sql_execute_write rules into the final permission set (or mark them as immutable
in the permission merge logic) so that sql_execute_write always wins; update the
merge order around userWithSafety, cfg.agent[key].permission and the final
permission assembly (referencing sql_execute_write, userWithSafety, and
cfg.agent[key].permission) to enforce non-overridable denial.
- Around line 193-199: The bash command allowlist in agent.ts currently includes
"dbt deps *", which violates the analyst's read-only contract because it
modifies the workspace; remove the "dbt deps *" entry from the bash object (the
allowlist under the bash key in the agent configuration) so only read-only dbt
commands remain, keeping the existing last-match-wins ordering and other allowed
entries intact.
---
Outside diff comments:
In `@packages/opencode/src/agent/agent.ts`:
- Around line 149-176: The plan role still inherits the wildcard allow from
defaults so SQL write operations can run in plan mode; update the plan
permission block (the PermissionNext.merge call that constructs the "plan" role)
to explicitly set sql_execute_write (e.g., "ask" to match builder) inside
PermissionNext.fromConfig so plan no longer inherits an unrestricted write
permission and will trigger the same prompt/deny flow as the other roles.
---
Duplicate comments:
In `@packages/opencode/src/altimate/tools/sql-classify.ts`:
- Around line 42-43: The call to s.statement_type.toUpperCase() can throw if
statement_type is missing or not a string; update the blocked computation (where
result.statements is iterated) to guard statement_type before calling
toUpperCase — e.g., verify typeof s.statement_type === "string" and
s.statement_type.length > 0 (or coerce safely) then call toUpperCase() and check
HARD_DENY_TYPES.has(...); ensure the logic still marks blocked true when a
missing/invalid statement_type should be treated as safe or denied according to
desired policy.
- Around line 38-40: When core.getStatementTypes(sql) returns no statements or
missing categories for a non-empty SQL, treat it as unknown/unsafe and fail
closed: in classifyAndCheck(), after calling getStatementTypes() check if
sql.trim() is non-empty and (result is falsy OR !result.statements?.length OR
!result.statements[0].categories?.length) and in that case return { queryType:
"write", blocked: true } instead of defaulting to "read"; update the same guard
used later in the function that inspects
result.statements/result.statements[0].categories to follow this fail-closed
behavior so unsupported/unparsable queries (e.g., CALL/EXECUTE
IMMEDIATE/procedural blocks) cannot bypass the write approval path.
🪄 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: 2aaa4a2f-47f0-4179-9e2a-31fdf1affc07
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
docs/docs/configure/agents.mddocs/docs/configure/permissions.mdpackages/opencode/package.jsonpackages/opencode/src/agent/agent.tspackages/opencode/src/altimate/prompts/analyst.txtpackages/opencode/src/altimate/prompts/builder.txtpackages/opencode/src/altimate/prompts/executive.txtpackages/opencode/src/altimate/prompts/migrator.txtpackages/opencode/src/altimate/prompts/researcher.txtpackages/opencode/src/altimate/prompts/trainer.txtpackages/opencode/src/altimate/prompts/validator.txtpackages/opencode/src/altimate/tools/sql-classify.tspackages/opencode/src/altimate/tools/sql-execute.tspackages/opencode/src/memory/types.tspackages/opencode/test/agent/agent.test.tspackages/opencode/test/altimate/tools/sql-classify.test.tsscript/upstream/verify-restructure.ts
💤 Files with no reviewable changes (8)
- script/upstream/verify-restructure.ts
- packages/opencode/src/altimate/prompts/executive.txt
- packages/opencode/src/memory/types.ts
- packages/opencode/src/altimate/prompts/validator.txt
- packages/opencode/src/altimate/prompts/migrator.txt
- packages/opencode/src/altimate/prompts/trainer.txt
- packages/opencode/test/agent/agent.test.ts
- packages/opencode/src/altimate/prompts/researcher.txt
✅ Files skipped from review due to trivial changes (5)
- packages/opencode/src/altimate/prompts/builder.txt
- packages/opencode/package.json
- packages/opencode/src/altimate/prompts/analyst.txt
- packages/opencode/test/altimate/tools/sql-classify.test.ts
- docs/docs/configure/agents.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/src/altimate/tools/sql-execute.ts
- docs/docs/configure/permissions.md
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/opencode/src/altimate/tools/sql-classify.ts (2)
40-51: Good defensive coding with null guards; redundant type cast on line 51.The null checks at lines 42, 45, and 48 provide robust handling of edge cases from
getStatementTypes(). The type castas "read" | "write"on line 51 is redundant since the ternary expression already produces that type.♻️ Minor cleanup
const queryType = categories.some((c: string) => !READ_CATEGORIES.has(c)) ? "write" : "read" - return { queryType: queryType as "read" | "write", blocked } + return { queryType, blocked } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/sql-classify.ts` around lines 40 - 51, The cast "as \"read\" | \"write\"" in classifyAndCheck is redundant because the ternary already yields that union; remove the explicit type assertion on the returned queryType and return the ternary value directly (in function classifyAndCheck, where queryType is computed and returned) so the return line becomes simply returning the ternary result and blocked flag without the unnecessary cast.
9-12:WRITE_CATEGORIESis unused — consider removing.The
WRITE_CATEGORIESset is defined but never referenced. The classification logic at lines 25 and 50 only checks againstREAD_CATEGORIES, treating anything not in the read set as a write (fail-safe approach). This unused constant could be removed to avoid confusion.♻️ Proposed fix
-// Categories from altimate-core that indicate write operations -const WRITE_CATEGORIES = new Set(["dml", "ddl", "dcl", "tcl"]) // Known safe (read-only) categories — "query" = SELECT, "other" = SHOW, EXPLAIN, SET, USE const READ_CATEGORIES = new Set(["query", "other"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/sql-classify.ts` around lines 9 - 12, The WRITE_CATEGORIES constant is defined but never used; remove the unused declaration to avoid confusion and dead code: delete the line declaring WRITE_CATEGORIES (the Set(["dml", "ddl", "dcl", "tcl"])) and any nearby comments solely referring to it, leaving READ_CATEGORIES as the single source of truth used by the classification logic (code that checks READ_CATEGORIES to decide read vs write). Ensure no other references to WRITE_CATEGORIES remain in the file.packages/opencode/test/altimate/tools/sql-classify.test.ts (1)
103-116: Duplicate test: "SHOW TABLES → read" appears twice.The test at lines 105-107 duplicates the test at lines 15-17. Consider removing one to keep the test suite DRY.
♻️ Remove duplicate test
// --- "other" category (metadata ops) → read --- - test("SHOW TABLES → read", () => { - expect(classify("SHOW TABLES")).toBe("read") - }) - test("SET variable → read", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/tools/sql-classify.test.ts` around lines 103 - 116, Remove the duplicated test case for "SHOW TABLES → read" in the test suite: locate the redundant test block that calls expect(classify("SHOW TABLES")).toBe("read") and delete it so only a single "SHOW TABLES → read" test remains; ensure the surrounding describe/test grouping and closing braces around the classify tests remain valid after removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/src/altimate/tools/sql-classify.ts`:
- Around line 40-51: The cast "as \"read\" | \"write\"" in classifyAndCheck is
redundant because the ternary already yields that union; remove the explicit
type assertion on the returned queryType and return the ternary value directly
(in function classifyAndCheck, where queryType is computed and returned) so the
return line becomes simply returning the ternary result and blocked flag without
the unnecessary cast.
- Around line 9-12: The WRITE_CATEGORIES constant is defined but never used;
remove the unused declaration to avoid confusion and dead code: delete the line
declaring WRITE_CATEGORIES (the Set(["dml", "ddl", "dcl", "tcl"])) and any
nearby comments solely referring to it, leaving READ_CATEGORIES as the single
source of truth used by the classification logic (code that checks
READ_CATEGORIES to decide read vs write). Ensure no other references to
WRITE_CATEGORIES remain in the file.
In `@packages/opencode/test/altimate/tools/sql-classify.test.ts`:
- Around line 103-116: Remove the duplicated test case for "SHOW TABLES → read"
in the test suite: locate the redundant test block that calls
expect(classify("SHOW TABLES")).toBe("read") and delete it so only a single
"SHOW TABLES → read" test remains; ensure the surrounding describe/test grouping
and closing braces around the classify tests remain valid after removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5249653-d2be-4ba2-84b0-d48723896ead
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
docs/docs/configure/agents.mdpackages/opencode/package.jsonpackages/opencode/src/agent/agent.tspackages/opencode/src/altimate/prompts/analyst.txtpackages/opencode/src/altimate/prompts/builder.txtpackages/opencode/src/altimate/prompts/executive.txtpackages/opencode/src/altimate/prompts/migrator.txtpackages/opencode/src/altimate/prompts/researcher.txtpackages/opencode/src/altimate/prompts/trainer.txtpackages/opencode/src/altimate/prompts/validator.txtpackages/opencode/src/altimate/tools/sql-classify.tspackages/opencode/src/altimate/tools/sql-execute.tspackages/opencode/src/memory/types.tspackages/opencode/test/agent/agent.test.tspackages/opencode/test/altimate/tools/sql-classify.test.tsscript/upstream/verify-restructure.ts
💤 Files with no reviewable changes (8)
- packages/opencode/src/altimate/prompts/validator.txt
- script/upstream/verify-restructure.ts
- packages/opencode/src/memory/types.ts
- packages/opencode/src/altimate/prompts/migrator.txt
- packages/opencode/src/altimate/prompts/executive.txt
- packages/opencode/src/altimate/prompts/researcher.txt
- packages/opencode/src/altimate/prompts/trainer.txt
- packages/opencode/test/agent/agent.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/opencode/package.json
- packages/opencode/src/altimate/prompts/builder.txt
- packages/opencode/src/altimate/prompts/analyst.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/docs/configure/agents.md
- packages/opencode/src/altimate/tools/sql-execute.ts
kulvirgit
force-pushed
the
modes
branch
2 times, most recently
from
ba69c48 to
7573d2d
Compare
suryaiyer95
approved these changes
Mar 19, 2026
| */ | ||
| export function classifyAndCheck(sql: string): { queryType: "read" | "write"; blocked: boolean } { | ||
| const result = core.getStatementTypes(sql) | ||
| if (!result?.statements?.length) return { queryType: "read", blocked: false } |
There was a problem hiding this comment.
Bug: The function classifyAndCheck defaults to a 'read' query classification when the SQL parser fails, potentially allowing write queries to bypass security checks.
Severity: MEDIUM
Suggested Fix
To ensure fail-safe behavior, modify the condition to treat cases where result?.statements?.length is falsy as a 'write' query. This aligns with the stated intent of treating unknown categories as write operations, preventing potential permission bypasses.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/opencode/src/altimate/tools/sql-classify.ts#L42
Potential issue: In `classifyAndCheck`, if the SQL parser `core.getStatementTypes()`
returns a result with a falsy `statements` property (e.g., null, undefined, or an empty
array), the function defaults to classifying the query as `{ queryType: "read", blocked:
false }`. This creates a fail-open condition. If the parser fails to parse a write query
for any reason (such as encountering unsupported SQL dialect syntax) and returns an
empty `statements` array, that write query would be incorrectly classified as a read
query, bypassing permission checks.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/src/altimate/tools/sql-classify.ts (1)
28-34: Consider removing redundantclassifyMultifunction.
classifyMultisimply delegates toclassifywith no additional logic. The docstring mentions multi-statement handling, but sincegetStatementTypeshandles multi-statement natively (as noted), this wrapper adds no value.If the function exists for API clarity/future extension, consider adding a comment explaining the intent. Otherwise, consumers can call
classifydirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/sql-classify.ts` around lines 28 - 34, The classifyMulti wrapper is redundant since classify already handles multi-statement SQL (via getStatementTypes); either remove the classifyMulti function and update any exports/usages to call classify directly, or if you want to preserve the name for API clarity add a short comment above classifyMulti explaining it is an intentional alias for classify (for future extension) and keep it exported; locate the symbol classifyMulti in sql-classify.ts and update imports/exports/usages accordingly (and run tests to ensure no breakage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/src/altimate/tools/sql-classify.ts`:
- Around line 28-34: The classifyMulti wrapper is redundant since classify
already handles multi-statement SQL (via getStatementTypes); either remove the
classifyMulti function and update any exports/usages to call classify directly,
or if you want to preserve the name for API clarity add a short comment above
classifyMulti explaining it is an intentional alias for classify (for future
extension) and keep it exported; locate the symbol classifyMulti in
sql-classify.ts and update imports/exports/usages accordingly (and run tests to
ensure no breakage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 48aa017d-b906-4ecc-8cad-527b8af0e2ec
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
docs/docs/configure/agents.mdpackages/opencode/package.jsonpackages/opencode/src/agent/agent.tspackages/opencode/src/altimate/prompts/analyst.txtpackages/opencode/src/altimate/prompts/builder.txtpackages/opencode/src/altimate/prompts/executive.txtpackages/opencode/src/altimate/prompts/migrator.txtpackages/opencode/src/altimate/prompts/researcher.txtpackages/opencode/src/altimate/prompts/trainer.txtpackages/opencode/src/altimate/prompts/validator.txtpackages/opencode/src/altimate/tools/sql-classify.tspackages/opencode/src/altimate/tools/sql-execute.tspackages/opencode/src/memory/types.tspackages/opencode/test/agent/agent.test.tspackages/opencode/test/altimate/tools/sql-classify.test.tsscript/upstream/verify-restructure.ts
💤 Files with no reviewable changes (7)
- packages/opencode/src/altimate/prompts/validator.txt
- packages/opencode/src/altimate/prompts/executive.txt
- packages/opencode/src/altimate/prompts/researcher.txt
- script/upstream/verify-restructure.ts
- packages/opencode/src/memory/types.ts
- packages/opencode/src/altimate/prompts/migrator.txt
- packages/opencode/src/altimate/prompts/trainer.txt
✅ Files skipped from review due to trivial changes (3)
- packages/opencode/src/altimate/prompts/analyst.txt
- packages/opencode/package.json
- packages/opencode/src/altimate/prompts/builder.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/opencode/src/altimate/tools/sql-execute.ts
- docs/docs/configure/agents.md
- packages/opencode/test/altimate/tools/sql-classify.test.ts
…ontrol Part A — Simplify modes: - Remove executive, validator, migrator, researcher, trainer agents - Delete 5 unused prompt files - Analyst: truly read-only — unknown bash denied, safe commands (ls/grep/cat/head/tail/find/wc) auto-allowed, dbt read commands allowed, dbt write commands (run/build/seed/docs) denied - Builder: add sql_execute_write: "ask" for SQL mutation approval Part B — SQL write access control: - New sql-classify.ts: regex-based classifier detecting INSERT, UPDATE, DELETE, DROP, CREATE, ALTER, TRUNCATE, MERGE, GRANT, REVOKE, COPY INTO, CALL, EXEC, EXECUTE IMMEDIATE, REPLACE, UPSERT, RENAME - sql-execute.ts: classify before execution, ctx.ask() for writes, hardcoded safety block for DROP DATABASE/SCHEMA/TRUNCATE - Safety denials in agent.ts for sql_execute_write - 42 classifier tests covering read/write/edge cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 19, 2026
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
3 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
sql_execute_writepermissionChanges
agent.ts: Remove 5 agent definitions, update analyst/builder permissionssql-classify.ts: New SQL write classifier (INSERT, UPDATE, DELETE, DROP, CREATE, ALTER, TRUNCATE, MERGE, GRANT, REVOKE, COPY INTO, CALL, EXEC, BEGIN, DECLARE, EXECUTE IMMEDIATE)sql-execute.ts: Add classification + permission check before executionTest plan
Review
Reviewed by 7 AI models (Claude, Gemini, Kimi, Grok, MiniMax, GLM-5, Qwen). Key fixes from review:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests
Chores