Skip to content

Commit 0c3c316

Browse files
anandgupta42claude
andcommitted
fix(agent): address PR #1092 review findings from Bugbot and CodeRabbit
- `sql_explain` analyze-safety guard hardened: string literals are masked BEFORE comments (comments-first was bypassable via quoted `/*`..`*/` markers smuggling DML), multi-statement payloads rejected (non-trailing `;`), and dollar-quoted strings fail closed; adversarial tests added - rewrite verify gate tightened from `decidable !== false` to `decidable === true` — the field is required since altimate-core@0.5.1, so a missing value is a malformed response and fails closed - `sql_diff` now forwards `schema_context`/`dialect` to the native handler (equivalence never ran before — no schema ever reached it) and reports "not assessed" instead of "not proven" when no schema is supplied - warehouse-write boundary wording aligned: the agent description and prompt now state that the direct SQL write tool is denied non-overridably while dbt builds run only as user-approved shell commands against a dev target; a new test documents the boundary under permissive global + per-agent bash overrides (builds allowed, DDL and sql_execute_write still denied) - live eval: tmpdir cleanup via `await using tmpdir()` on all exit paths; nonzero CLI exit now fails the eval - docs: https links, one-line reference labels (MD039), Databricks liquid-clustering citations [14]->[21], and a status note on the catalog research doc recording the security review's four design requirements for the future SaaS distribution (not built in this PR) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 19faea5 commit 0c3c316

13 files changed

Lines changed: 155 additions & 47 deletions

docs/internal/2026-08-12-agent-catalog-scaling-research.md

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,22 @@
11
# Scaling Agent Catalogs Without Losing TUI Speed
22

3+
> **Status: research record, not an implementation spec.** The SaaS distribution
4+
> design sketched below (control plane, signed bundles, capability tiers) is NOT
5+
> built; nothing in this document changes runtime behavior. Security review
6+
> flagged four design requirements that any implementation must resolve before
7+
> shipping, tracked here as design inputs:
8+
> 1. cache entries partitioned by full authorization scope + policy version, with
9+
> defined offline/revocation behavior including active sessions;
10+
> 2. warehouse-write denial for optimizer-class agents expressed as a
11+
> non-overridable deny state, not an approval tier (the shipped `dbt-optimizer`
12+
> already enforces this in `agent.ts``sql_execute_write` deny is re-applied
13+
> after all config merges);
14+
> 3. promotion to primary restricted to `mode: both` definitions with
15+
> immutable-manifest + session-policy validation, never a safety label alone;
16+
> 4. a non-self-referential signature envelope (canonical serialization, signature
17+
> excluded from signed bytes, algorithm + key ID) and redacted, allowlisted
18+
> audit payloads.
19+
320
## Executive Summary
421

522
- **Hybrid Selection**: Claude Code, OpenCode, Cursor, Codex, and Cline all expose a user-facing agent that can delegate bounded work to specialists, while also offering explicit ways to invoke or configure specialists. Claude Code uses description-based routing plus explicit `@` selection, and OpenCode uses automatic invocation plus `@` mentions. [16] [16] [5] [5] -> Keep automatic delegation, but always provide an explicit picker and mention syntax.
@@ -192,24 +209,20 @@ On distribution, use the same boundary that the strongest platforms expose: huma
192209
12. *Configure the sandboxed Bash tool*. https://code.claude.com/docs/en/sandboxing
193210
13. *Application card: GitHub Copilot Agents*. https://docs.github.com/en/copilot/responsible-use/agents
194211
14. *Agent Server*. https://docs.langchain.com/langsmith/agent-server
195-
15. *Create and distribute a plugin marketplace*. http://code.claude.com/docs/en/plugin-marketplaces
212+
15. *Create and distribute a plugin marketplace*. https://code.claude.com/docs/en/plugin-marketplaces
196213
16. *Create custom subagents*. https://code.claude.com/docs/en/sub-agents
197-
17. *Discover and install prebuilt plugins through marketplaces*. http://code.claude.com/docs/en/discover-plugins
214+
17. *Discover and install prebuilt plugins through marketplaces*. https://code.claude.com/docs/en/discover-plugins
198215
18. *Preparing to use custom agents in your organization*. https://docs.github.com/en/copilot/how-tos/administer-copilot/manage-for-organization/prepare-for-custom-agents
199216
19. *Creating and using custom agents for GitHub Copilot CLI*. https://docs.github.com/en/copilot/how-tos/copilot-cli/customize-copilot/create-custom-agents-for-cli
200-
20. [
201-
Subagents | ChatGPT Learn
202-
](https://learn.chatgpt.com/docs/agent-configuration/subagents)
217+
20. *Subagents | ChatGPT Learn*. https://learn.chatgpt.com/docs/agent-configuration/subagents
203218
21. *feat: subagents · Issue #431 · charmbracelet/crush · GitHub*. https://github.com/charmbracelet/crush/issues/431
204219
22. *Aider - AI Pair Programming in Your Terminal*. https://aider.chat/
205220
23. *Run agents in parallel*. https://code.claude.com/docs/en/agents
206-
24. [
207-
Advanced Configuration | ChatGPT Learn
208-
](https://learn.chatgpt.com/docs/config-file/config-advanced)
221+
24. *Advanced Configuration | ChatGPT Learn*. https://learn.chatgpt.com/docs/config-file/config-advanced
209222
25. *Recipe Reference Guide | goose | Your open source AI agent*. https://goose-docs.ai/docs/guides/recipes/recipe-reference/
210223
26. *Manage multiple agents with agent view*. https://code.claude.com/docs/en/agent-view
211224
27. *Subagents*. https://docs.cline.bot/features/subagents
212-
28. *Using Agent in CLI | Cursor Docs*. http://cursor.com/docs/cli/using
225+
28. *Using Agent in CLI | Cursor Docs*. https://cursor.com/docs/cli/using
213226
29. *Subagents | Cursor Docs*. https://cursor.com/docs/subagents
214227
30. *Reusable Recipes | goose | Your open source AI agent*. https://goose-docs.ai/docs/guides/recipes/session-recipes
215228
31. *Using Commands*. https://docs.cline.bot/core-workflows/using-commands
@@ -225,17 +238,16 @@ On distribution, use the same boundary that the strongest platforms expose: huma
225238
41. *Tools*. https://opencode.ai/docs/tools/
226239
42. *Chat modes*. https://aider.chat/docs/usage/modes.html
227240
43. *Plugins*. https://opencode.ai/docs/plugins/
228-
44. [Chat Participant API | Visual Studio Code Extension
229-
API](https://code.visualstudio.com/api/extension-guides/ai/chat)
230-
45. *http://cursor.com/docs*. http://cursor.com/docs
241+
44. *Chat Participant API | Visual Studio Code Extension API*. https://code.visualstudio.com/api/extension-guides/ai/chat
242+
45. *https://cursor.com/docs*. https://cursor.com/docs
231243
46. *Agent Skills | Cursor Docs*. https://cursor.com/docs/skills
232244
47. *Agent Skills*. https://opencode.ai/docs/skills/
233245
48. *Aider Documentation*. https://aider.chat/docs/
234246
49. *Workspace agents | OpenAI*. https://openai.com/academy/workspace-agents/
235247
50. *Plugins | Cursor Docs*. https://cursor.com/docs/plugins
236248
51. *Separating code reasoning and editing*. https://aider.chat/2024/09/26/architect.html
237249
52. *Set up Claude Code for your organization*. https://code.claude.com/docs/en/admin-setup
238-
53. *Enterprise | Cursor Docs*. http://cursor.com/docs/enterprise
250+
53. *Enterprise | Cursor Docs*. https://cursor.com/docs/enterprise
239251
54. *Configure server-managed settings*. https://code.claude.com/docs/en/server-managed-settings
240252
55. *About GitHub Copilot cloud agent*. https://docs.github.com/copilot/concepts/agents/cloud-agent/about-cloud-agent
241253
56. *Managing GPT access in Enterprise and Edu workspaces | OpenAI Help Center*. https://help.openai.com/en/articles/8555535-managing-gpt-access-in-enterprise-and-edu-workspaces

docs/internal/2026-08-12-dbt-optimization-taxonomy-research.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ The key implementation rule is to recommend a strategy together with its failure
6565

6666
## 3 Warehouse Physical Design and Storage Taxonomy
6767

68-
Physical design should be inferred from recurring filters, join keys, partition coverage, and query profiles. It should not be generated from column names alone. Snowflake's micro-partition metadata supports runtime pruning [33], while Databricks liquid clustering is documented as a data-layout technique that replaces traditional partitioning and ZORDER [14]. Those mechanisms are similar in purpose but not interchangeable.
68+
Physical design should be inferred from recurring filters, join keys, partition coverage, and query profiles. It should not be generated from column names alone. Snowflake's micro-partition metadata supports runtime pruning [33], while Databricks liquid clustering is documented as a data-layout technique that replaces traditional partitioning and ZORDER [21]. Those mechanisms are similar in purpose but not interchangeable.
6969

7070
| Engine and use-case | Detection signal and evidence needed | Proposed fix | Impact | Risk and preconditions | Agent confidence |
7171
|---|---|---|---|---|---|
@@ -78,7 +78,7 @@ Physical design should be inferred from recurring filters, join keys, partition
7878
| BigQuery partition pruning defeated | Partition filter is wrapped in a function, uses a mismatched type, or is applied after a broad subquery | Use direct partition-column predicates and push them to the scan | C, R | Correctness depends on timezone and inclusive boundary rules | Medium |
7979
| BigQuery required partition filter missing | Large partitioned table is queried without a bounded partition predicate | Set `require_partition_filter`; add model/source conventions and tests | C, Q | It can break legitimate unbounded jobs and BI queries, so propose with affected-consumer list | Medium |
8080
| BigQuery clustering missing or wrong | Repeated filters after partition pruning still scan many blocks; common equality/range columns are visible in query history | Add up to the engine-supported clustering columns in predicate/join order; validate bytes and slot time | C, R | Clustering is not a substitute for partitioning; high-cardinality or unstable keys may not help | Medium |
81-
| Databricks liquid clustering candidate | Delta table has recurring filters on dimensions, poor data skipping, and frequent schema/query evolution | Use liquid clustering on observed keys; run/monitor `OPTIMIZE` according to platform policy | C, R | Liquid clustering replaces partitioning/ZORDER and is not a drop-in change for all runtimes [14]; maintenance consumes compute | Low to medium |
81+
| Databricks liquid clustering candidate | Delta table has recurring filters on dimensions, poor data skipping, and frequent schema/query evolution | Use liquid clustering on observed keys; run/monitor `OPTIMIZE` according to platform policy | C, R | Liquid clustering replaces partitioning/ZORDER and is not a drop-in change for all runtimes [21]; maintenance consumes compute | Low to medium |
8282
| Databricks ZORDER candidate on legacy layout | Legacy Delta table, repeated selective predicates, data-skipping stats show poor locality | ZORDER on a small set of high-value columns, or migrate to liquid clustering | C, R | ZORDER can be inferior to liquid clustering on supported runtimes; avoid piling both without evidence | Medium |
8383
| Databricks partition explosion | Too many tiny partitions, small files, high task overhead, low rows per file | Remove or coarsen partitions; use liquid clustering or compaction | C, R | File layout and streaming latency can change | Medium |
8484
| Redshift sort key missing or misordered | Poor sort order, high unsorted region, scans on a recurring time/join predicate, EXPLAIN shows broad scan | Add/reorder sort key; choose compound/interleaved/automatic design appropriate to workload | C, R | Sort maintenance, vacuum, load order, and workload mix matter; use EXPLAIN and history | Medium |

packages/opencode/src/agent/agent.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ export const layer = Layer.effect(
383383
"dbt-optimizer": {
384384
name: "dbt-optimizer",
385385
description:
386-
"Scan a dbt project for fixable issues — performance, materialization, repeated logic, missing tests/docs — and propose targeted fixes with cost and impact reporting. File edits and shell commands prompt for approval; warehouse writes are denied.",
386+
"Scan a dbt project for fixable issues — performance, materialization, repeated logic, missing tests/docs — and propose targeted fixes with cost and impact reporting. File edits and shell commands prompt for approval; the direct SQL write tool is denied non-overridably, and dbt builds against a dev target run only as user-approved shell commands.",
387387
prompt: PROMPT_OPTIMIZER,
388388
options: {},
389389
permission: Permission.merge(

packages/opencode/src/altimate/native/types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,8 @@ export interface SqlDiffParams {
726726
context_lines?: number
727727
/** Optional parsing-dialect hint forwarded to the equivalence engine. */
728728
dialect?: string
729+
/** Inline schema for the equivalence check; without it the handler skips equivalence entirely. */
730+
schema_context?: Record<string, unknown>
729731
}
730732

731733
export interface SqlDiffResult {

packages/opencode/src/altimate/prompts/dbt-optimizer.txt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@ fixes with cost and impact reporting so the user can prioritize. Your outputs ar
88
- **Pull requests** — for the fixes the user applies
99

1010
Your permissions enforce your phases: file edits and shell commands prompt the user
11-
for approval by default, and warehouse writes are denied outright.
11+
for approval by default, and the direct SQL write tool (`sql_execute_write`) is
12+
denied outright. dbt builds DO mutate the warehouse — that is why they run only as
13+
shell commands the user approves, against a dev target (see Build safety).
1214

1315
## Trust boundary
1416

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -123,11 +123,13 @@ async function verifyRewrites(
123123
schema_context: args.schema_context,
124124
})) as { error?: string; data?: Record<string, any> } | null
125125
const ed = (eq?.data ?? {}) as Record<string, any>
126-
// The gate requires BOTH `equivalent === true` AND a decidable result. The
127-
// engine can return { equivalent: true, decidable: false } when its heuristic
128-
// comparator could not actually decide — that is an unproven rewrite, and
129-
// reporting it as verified would be a false safety claim.
130-
if (ed.equivalent === true && ed.decidable !== false) {
126+
// The gate requires BOTH `equivalent === true` AND `decidable === true`.
127+
// The engine can return { equivalent: true, decidable: false } when its
128+
// heuristic comparator could not actually decide — that is an unproven
129+
// rewrite, and reporting it as verified would be a false safety claim.
130+
// `decidable` is a required field since altimate-core@0.5.1, so a missing
131+
// value means a malformed response and also fails closed.
132+
if (ed.equivalent === true && ed.decidable === true) {
131133
verified.push({ sql: c.sql, rule: c.rule, confidence: ed.confidence })
132134
} else {
133135
// Derive a specific reason. The gate stays strict (only `=== true` verifies);
@@ -138,6 +140,8 @@ async function verifyRewrites(
138140
reason = String(eq?.error ?? ed.error)
139141
} else if (ed.equivalent === true && ed.decidable === false) {
140142
reason = "equivalence undecidable — the engine could not prove the rewrite; validate with a data-diff"
143+
} else if (ed.equivalent === true && ed.decidable !== true) {
144+
reason = "missing or invalid 'decidable' field in equivalence response"
141145
} else if (!("equivalent" in ed)) {
142146
reason = "missing 'equivalent' field in equivalence response"
143147
} else if (typeof ed.equivalent !== "boolean") {

packages/opencode/src/altimate/tools/sql-diff.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,22 +4,31 @@ import { Dispatcher } from "../native"
44

55
export const SqlDiffTool = Tool.define("sql_diff", {
66
description:
7-
"Compare two SQL queries and show the differences. Returns a line diff plus a semantic-equivalence assessment. Useful for reviewing suggested changes before applying them.",
7+
"Compare two SQL queries and show the differences. Returns a line diff, plus a semantic-equivalence assessment when schema_context is provided (equivalence needs schema to resolve table/column references). Useful for reviewing suggested changes before applying them.",
88
parameters: z.object({
99
original: z.string().describe("The original SQL"),
1010
modified: z.string().describe("The modified SQL"),
1111
context_lines: z.number().optional().default(3).describe("Number of context lines around changes"),
12+
schema_context: z
13+
.record(z.string(), z.any())
14+
.optional()
15+
.describe("Inline schema definition — required for the semantic-equivalence assessment"),
16+
dialect: z.string().optional().describe("SQL dialect hint for equivalence (e.g. snowflake, bigquery)"),
1217
}),
1318
async execute(args, ctx) {
1419
try {
1520
// Native handler contract (sql/register.ts): { success, diff, equivalent,
1621
// equivalence_confidence, differences, error? }. The previous wrapper read
1722
// fields the handler never returns (has_changes/unified_diff/similarity),
18-
// so every comparison fell into the "no changes" branch.
23+
// so every comparison fell into the "no changes" branch. The handler only
24+
// runs the equivalence check when schema_context is present.
25+
const hasSchema = !!(args.schema_context && Object.keys(args.schema_context).length > 0)
1926
const result = (await Dispatcher.call("sql.diff", {
2027
original: args.original,
2128
modified: args.modified,
2229
context_lines: args.context_lines,
30+
schema_context: args.schema_context,
31+
dialect: args.dialect,
2332
})) as Record<string, any>
2433

2534
if (result.success === false) {
@@ -34,8 +43,11 @@ export const SqlDiffTool = Tool.define("sql_diff", {
3443
const diffText = typeof result.diff === "string" ? result.diff : ""
3544
const changeCount = diffText.length ? diffText.split("\n").filter((l) => /^[+-]/.test(l)).length : 0
3645
const differences: any[] = Array.isArray(result.differences) ? result.differences : []
37-
const equivalenceLine =
38-
result.equivalent === true
46+
// Without a schema the handler never runs the equivalence check — saying
47+
// "not proven" there would misrepresent an unassessed comparison.
48+
const equivalenceLine = !hasSchema
49+
? "Semantic equivalence: not assessed (pass schema_context to enable)"
50+
: result.equivalent === true
3951
? `Semantic equivalence: equivalent (confidence ${result.equivalence_confidence ?? "unknown"})`
4052
: "Semantic equivalence: not proven"
4153

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

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -83,11 +83,26 @@ function validateWarehouseName(warehouse: string | undefined): string | null {
8383
*/
8484
function validateAnalyzeSafety(sql: string, analyze: boolean | undefined): string | null {
8585
if (!analyze) return null
86+
const blocked =
87+
"analyze:true runs EXPLAIN ANALYZE, which actually executes the statement — it is only allowed for a single plain SELECT query. " +
88+
"Re-run with analyze:false for an estimated plan."
89+
// Dollar-quoted strings (PostgreSQL $tag$...$tag$) can hide arbitrary text from
90+
// the masking below; fail closed rather than tokenize them.
91+
if (/\$[a-zA-Z_]*\$/.test(sql)) return blocked
92+
// Mask string literals BEFORE removing comments. Comments-first is bypassable:
93+
// in `SELECT '/*'; DELETE FROM t; SELECT '*/'` the comment regex would swallow
94+
// the DELETE because the `/*` and `*/` live inside string literals. Masking
95+
// errs toward false positives (a mangled comment can only ADD leftover text),
96+
// which fails closed — the caller just falls back to analyze:false.
8697
const stripped = sql
98+
.replace(/'(?:[^']|'')*'/g, "''")
99+
.replace(/"(?:[^"]|"")*"/g, '""')
87100
.replace(/--[^\n]*/g, " ")
88101
.replace(/\/\*[\s\S]*?\*\//g, " ")
89-
.replace(/'(?:[^']|'')*'/g, "''")
90102
.trim()
103+
// Exactly one executable statement: any semicolon other than a trailing one
104+
// means a multi-statement payload.
105+
if (stripped.replace(/;\s*$/, "").includes(";")) return blocked
91106
const readOnlyStart = /^(select|with|show|describe|desc|values|table)\b/i.test(stripped)
92107
// Data-modifying CTEs (`WITH x AS (...) INSERT INTO ...`) and similar mean a
93108
// read-only prefix is not enough — reject write keywords anywhere.
@@ -97,12 +112,7 @@ function validateAnalyzeSafety(sql: string, analyze: boolean | undefined): strin
97112
/\b(insert|update|delete|merge|truncate|drop|alter|create|replace|grant|revoke|copy|call|vacuum|set|into)\b/i.test(
98113
stripped,
99114
)
100-
if (!readOnlyStart || hasWriteKeyword) {
101-
return (
102-
"analyze:true runs EXPLAIN ANALYZE, which actually executes the statement — it is only allowed for plain SELECT queries. " +
103-
"Re-run with analyze:false for an estimated plan."
104-
)
105-
}
115+
if (!readOnlyStart || hasWriteKeyword) return blocked
106116
return null
107117
}
108118

packages/opencode/test/agent/agent.test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,32 @@ it.instance(
172172
},
173173
)
174174

175+
it.instance(
176+
"dbt-optimizer bash boundary: user bash overrides relax builds, never DDL or SQL writes",
177+
() =>
178+
Effect.gen(function* () {
179+
const optimizer = yield* load((svc) => svc.get("dbt-optimizer"))
180+
// DOCUMENTED BOUNDARY: `bash` is "ask" by default; a user who explicitly
181+
// sets bash overrides accepts that dbt builds (which mutate a dev target)
182+
// run without a prompt. That is the user's choice — but the invariants
183+
// hold regardless: destructive DDL through bash stays denied, and the
184+
// direct SQL write tool stays denied.
185+
expect(
186+
Permission.evaluate("bash", "altimate-dbt build --model fct_orders", optimizer!.permission).action,
187+
).toBe("allow")
188+
expect(Permission.evaluate("bash", "DROP DATABASE prod", optimizer!.permission).action).toBe("deny")
189+
expect(evalPerm(optimizer, "sql_execute_write")).toBe("deny")
190+
}),
191+
{
192+
config: {
193+
permission: { bash: "allow" },
194+
agent: {
195+
"dbt-optimizer": { permission: { bash: "allow" } },
196+
},
197+
},
198+
},
199+
)
200+
175201
it.instance("sensitive_write guard actually fires (not neutralized by *: allow)", () =>
176202
Effect.gen(function* () {
177203
// The #209 sensitive-write guard asks for the "sensitive_write" permission. It must NOT

0 commit comments

Comments
 (0)