Repository navigation
feat: model-free tool CLI and MCP server for external coding agents - #1609
Conversation
Do not merge: experiment for evaluating direct altimate-code tools in Claude Code. - Bring in `altimate-code mcp serve` and the workspace tools from the MCP draft branches - `tool list|schema|run|context|daemon`: run the same adapter as `mcp serve` without a model - `mcp serve` default set adds impact, lineage, PII, data-diff, equivalence and translation tools; `--all-tools` exposes the full allow-list - Long-running per-project process keeps connections warm and releases DuckDB/SQLite file locks between calls - `--connections` / `ALTIMATE_TOOL_CONNECTIONS`, `--args -` from stdin, `tool schema` for one tool - DROP DATABASE, DROP SCHEMA and TRUNCATE stay blocked even with `--allow-write` - `link --claude-md` writes CLI routing guidance for clients without MCP Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
Merge Risk:
|
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Do not merge: experiment. - `tool list|schema|run|context|daemon` enable workspace tools automatically when the project is linked; `--no-workspace-tools` disables them - `tool context` lists the linked workspace, its skills and how to use the workspace tools - Daemon identity and spawn arguments follow the effective workspace-tools value Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
…ills Do not merge: experiment. - Every workspace skill sync also writes the skills into `<project>/.claude/skills/`, so linking or refreshing a project puts them on disk before a Claude Code session starts - Copies follow the Claude Code plugin's rules: frontmatter reduced to `name` and `description`, command blocks neutralised, hook and plugin component paths skipped, user-written skills never touched, each copy marked and git-ignored - Unlink and the feature-off path remove the copies; projects without `.claude` are left alone; `ALTIMATE_CLAUDE_SKILL_SYNC=0` disables it - A mirror failure never changes the sync result - Tests: `claude-skill-mirror.test.ts`, mirror wiring in `skill-sync.test.ts` Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
# Conflicts: # packages/opencode/src/altimate/api/client.ts
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
tool CLI and MCP server on one adaptertool CLI and MCP server for external coding agents
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
…ll mirror - daemon: release file locks when the last request finishes; serialize bind and stale-socket recovery with a lock; clients never unlink sockets; refuse a symlinked or foreign-owned socket directory - mcp: warehouse_remove and driver installs need --allow-write over MCP too - analyst effort: a per-model effort or thinking option wins over the default - adapter: bound the answer_commit history kept across turns - tool CLI: keep spilled results under the state directory, not /tmp - skill mirror: skip a symlinked .claude or .claude/skills; swap copies without deleting the old one first - docs: callers in one directory share the daemon Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cking files Files that reset the native dispatcher or mock the MCP SDK client modules now restore them, so tests that run later in the same process see the real ones. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…space skill mirror Copies carry a marker naming the skill, so a renamed copy is the user's; nested CLAUDE.md files are not copied; staging uses an .altimate-staging- infix, gets its .gitignore first and is swept when abandoned; an unreadable snapshot, or one missing while it is swapped, leaves the copies alone; and the mirror and the plugin's sync take turns under one mkdir lock in .claude/skills. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # packages/opencode/src/altimate/api/client.ts # packages/opencode/src/memory/store.ts
…uard read-only warehouse_add - daemon: reload connections when a connections file changes, only when no other call is in flight, with calls arriving during a reload waiting for it; key the socket on the caller's ALTIMATE_CODE_CONN_* environment; resolve --connections once - registry: validate a connections file before closing anything; never release in-memory DuckDB/SQLite connections - mcp serve: release file database locks when no call is in flight - warehouse_add: a read-only session cannot replace a connection named in the active registry, the global or project file, or the credential store - adapter: bound the answer_commit history on every call, not only on commit - skill mirror: keep copies when the snapshot or a skill cannot be read; let withdrawal run with ALTIMATE_CLAUDE_SKILL_SYNC=0 - tool CLI: decide the workspace tools default from the saved link, without a network call - workspace-client: telemetry name and disabled-workspace stub - docs and help text for the daemon, workspace tools and --all-tools Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nder concurrent and partial reads The mkdir lock shared with the plugin is renewed by its holder, taken over from a dead one by rename, released only by the holder (an empty token directory), and never waited on past its deadline; a holder that lost it stops before the next swap. The manifest is read without following symlinks and re-read before the removal pass; a listed skill directory that is gone, or credentials that exist but cannot be read, change nothing. One skill's failed swap no longer stops the others; tree comparison streams one file pair at a time; frontmatter whitespace matches Python's; readManifest rejects a skills array. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the function - Oracle, Trino and Fabric are lexed as the supported dialects they are, so backslashes and # in their reads no longer make the SQL uncertain. - TRUNCATE followed by a closed block comment and ( counts as the function. - The dbt adapter-type tests cover the modification-time part of the cache key and the saved altimate-dbt config. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
sahrizvi
left a comment
There was a problem hiding this comment.
Consensus re-review (round 3) at c9a9cf9, five independent reviewers; the findings were reproduced on the PR's lockfile deps.
Earlier findings
- MAJOR: an omitted
warehousewas classified with generic lexing. The bypass half is fixed. The SQL is now read under the generic lexing, the registry default's dialect and the dbt manifest type, and the most restrictive result wins. Probed: the BigQuery defaultSELECT 'x\'' ; DROP SCHEMA ds CASCADE; -- 'is blocked, the MySQL default# note\nDROP SCHEMA prodis blocked with--allow-write, and the Databricks comment-with-;case is blocked. The false-positive half is still open (a Postgres dollar-quoted read is still hard-blocked whenwarehouseis omitted). It is folded into the minor onsql-execute.ts:673. - Windows pipe owner check, dbt env in daemon identity, daemon client timeout/cancel,
warehouse_listcredential hint: unchanged and listed under Follow-ups. Reasonably deferred.
This round
- MAJOR (inline,
sql-tokenize.ts:93): on SQL Server/Fabric,E'\'is read as a Postgres escape string. A followingDELETE, orDROP DATABASE, then runs in the default read-only mode, named or omitted warehouse. Blocking. - MINOR (inline,
sql-execute.ts:673): the generic reading is always added for an omitted warehouse, even when dbt cannot take the call, so ordinary default-connection reads are refused. Not blocking on its own (it fails closed, and naming the warehouse avoids it), but it is a one-condition change worth making with the MAJOR fix. - MINOR (inline,
dbt-adapter-type.ts:33): the dbt dialect comes from the manifest, not the live target. Not blocking (edge input; safe in a follow-up).
Nits and checked items are in a separate PR comment. 179 tests pass across sql-classify, dbt-adapter-type, tool-cli/* and sql-execute-batch.
| for (let j = i - 1; j > start && sql[j] === "\\"; j--) run++ | ||
| if (run % 2 === 1) complete = false | ||
| } | ||
| if ((escaped || backslashEscapes) && sql[i] === "\\") { |
There was a problem hiding this comment.
MAJOR: on SQL Server/Fabric, E'\' is lexed as a Postgres escape string, so a following statement is read as part of the literal.
The escaped check (line 61: /[eE]/.test(sql[i]) && sql[i + 1] === "'") is not gated by dialect, and this line then lets a backslash skip the closing quote. T-SQL has no E-strings. In
SELECT E'\' FROM (SELECT 1 E) x; DELETE FROM dbo.t; -- 'E is a column, '\' is a one-character string used as a column alias (SELECT expr 'alias' is valid T-SQL), and the DELETE runs in the same batch. The tokenizer, under sqlserver, fabric (canonical sqlserver), tsql and the generic reading, sees one literal running to the final ', so classifyAndCheck returns read, one statement, not blocked.
Reproduced through createRunner().run with spyOn(Registry, "get") returning a fake connector that records its SQL:
{ ms: { type: "sqlserver" } }, no--allow-write, withwarehouse: "ms"and withwarehouseomitted: status ok, and the driver receives the exact text. The SQL Server driver insertsTOP 1001afterSELECTand runs the batch withpool.request().query, so theDELETEexecutes.{ fb: { type: "fabric" } }, warehouse omitted,SELECT E'\' FROM (SELECT 1 E) x; DROP DATABASE prod; -- ': ok with and without--allow-write, and the driver receives theDROP DATABASE. Because the SQL is classified as a read, it never reaches the write ask or the hard block.- No semicolons are needed in T-SQL:
SELECT * FROM dbo.t WHERE x = E'\' DROP DATABASE prod --'also classifies asreadundertsql/fabric/sqlserver.
The escaped line predates this PR. It matters here for the same reason as last round's finding: this PR makes the classifier the read-only boundary for external agents and advertises DROP DATABASE as non-overridable, and the SQL Server driver does not enforce read-only itself.
Suggested fix: recognize E'...' only for dialects that have it (postgres, redshift, duckdb). In the generic reading, treat E' followed by a backslash as uncertain. Everywhere else, treat E as ordinary code. Add runner tests for sqlserver and fabric, named and default, with and without --allow-write.
Blocker: yes. It makes "read-only by default; DROP DATABASE cannot be overridden" false for a supported warehouse on the path this PR exposes.
There was a problem hiding this comment.
Fixed in dac4caa. E'...' is now read as an escape string only in PostgreSQL, Redshift and DuckDB. Everywhere else E is ordinary code.
For T-SQL (sqlserver, mssql, tsql, fabric) and any dialect the lexer doesn't model, statements need no ;. So a read must contain no statement keyword in its code, with literals, identifiers and comments masked. Words that are also common column names, such as open, close, fetch, use, enable and throw, count only in their statement forms. Fabric now also gets SQL Server's nested comments and bracket identifiers.
Your three probes now give:
SELECT E'\' FROM (SELECT 1 E) x; DELETE FROM dbo.t; -- 'is a write.- The
DROP DATABASEvariant is blocked with and without--allow-write. - The separator-free
... = E'\' DROP DATABASE prod --'is blocked.
Runner tests cover sqlserver and fabric, named and default, with and without --allow-write.
| const { dbtProjectAdapterType } = await import("../native/connections/dbt-adapter-type") | ||
| const dbt = await dbtProjectAdapterType() | ||
| const dialects = [ | ||
| ...new Set([undefined, ...[fallback?.type, dbt.type].map((type) => (type ? (Registry.canonicalType(type) ?? undefined) : undefined))]), |
There was a problem hiding this comment.
MINOR: the generic reading is always in the set, even when dbt cannot take the call, so ordinary reads on the default connection are refused.
sql.execute skips dbt for every read in read-only MCP / tool run (!context?.forceReadOnly in register.ts) and whenever there is no dbt project. In both cases execution reaches only the registry default, but undefined is still added here. Probed with warehouse omitted:
- Postgres default,
SELECT $$a; DROP SCHEMA s; b$$ AS v: hard-blocked (the earlier false positive). - BigQuery default,
SELECT 'it\'s' AS v: refused as a write. - SQL Server default,
SELECT 'C:\reports\' AS pandSELECT * FROM #temp/##stage: refused as a write. These were reads at 20fb04e; 88f03f4's unmodeled-dialect guards now refuse them. - Trino/Oracle default,
LIKE '%\_%' ESCAPE '\': refused as a write. So c9a9cf9's "Oracle, Trino and Fabric keep their reads" holds only for a named warehouse.
The refusal says "Re-run with --allow-write", which steers an agent toward enabling writes for a read.
Suggested fix: add undefined only when dbt can actually be reached and its type is unknown (a dbt project and not forceReadOnly, or no resolvable type). Add omitted-warehouse read tests for Postgres, BigQuery and SQL Server defaults.
Blocker: no. It fails closed, naming the warehouse avoids it, and the PR body documents the dollar-quote limit. It is a small change worth making with the fix above.
There was a problem hiding this comment.
Fixed in dac4caa. With warehouse omitted, the SQL is now classified under each dialect the call can actually reach:
- Registry default: always included.
- dbt: included only when dbt can take the call (a dbt project, and not forced read-only). It adds its manifest type and every target type in the project's profile.
- Every supported dialect plus generic lexing: used only when dbt can take the call and its types can't be read.
Your probes on a default connection are reads again:
- Postgres
SELECT $$a; DROP SCHEMA s; b$$ AS v - BigQuery
SELECT 'it\'s' AS v - SQL Server
SELECT 'C:\reports\' AS pandSELECT * FROM #temp - Trino
LIKE '%\_%' ESCAPE '\'
These are covered by runner tests with a fake connector.
| const key = `${manifest}|${stat.size}|${stat.mtimeMs}` | ||
| if (cached?.key !== key) { | ||
| const raw = JSON.parse(readFileSync(manifest, "utf8")) | ||
| const type = String(raw?.metadata?.adapter_type ?? "").toLowerCase() || undefined |
There was a problem hiding this comment.
MINOR: the dbt dialect comes from the manifest's metadata.adapter_type, not the target dbt will connect with.
resolveDefaultTarget asks the live adapter first (getAdapterType()), but defaultTarget() in sql-execute.ts uses only this value. With a profiles.yml whose targets use different adapters (e.g. dev on DuckDB, prod on Snowflake) and a manifest last written for the other target, the dialect dbt actually uses is missing from the set.
This reaches only the hard block under --allow-write (dbt is skipped in read-only mode), and the generic reading covers most cases. One it misses: Snowflake through dbt with SELECT 1 // '\n; DROP SCHEMA s; -- ' is not blocked under the generic reading but is blocked under snowflake. Also, when a dbt project exists, the write ask carries no warehouse even though the call can fall back to the registry default.
Project selection itself is fine: the saved config root, else findProjectRoot(), is the same precedence the dispatcher's readDbtConfig uses, so this cannot read a different project from the one execution uses.
Suggested fix: reuse the type the adapter reports once it is initialised, or include every adapter type in the project's profiles.yml targets. This extends the follow-up you already list for an uncompiled project.
Blocker: no. It needs a multi-adapter profile, a stale manifest, --allow-write and adversarial SQL, so it is safe in a follow-up.
There was a problem hiding this comment.
Fixed in dac4caa. The dbt dialects now include every target type in the profile dbt_project.yml names, alongside the manifest type. A templated profile name counts every profile's targets.
The project is the one dbt's routing uses:
- Once the adapter has been created, it is the adapter's cached project.
- Before that, it is the saved config's root, or discovery when there is no config.
- A root without
dbt_project.ymlmeans no dbt project.
Your Snowflake SELECT 1 // '\n; DROP SCHEMA s; -- ' case through a dev-DuckDB / prod-Snowflake profile is blocked. Tests cover profile targets, templated names, a stale root and the cached adapter.
|
Consensus re-review at c9a9cf9: nits and notes not tied to a diff line (companion to the review above; none of these block) Nits (all fail closed):
Checked and fine:
|
… on every reachable path - SQL Server/Fabric: no E'' strings; statements without a separator count only in their statement forms - BigQuery triple-quoted and backslash-escaped strings, Snowflake // comments and $$ quotes, Spark comment line continuation, MySQL -- before ASCII whitespace, non-ASCII dollar tags - uncertain comments and backslash runs make SQL uncertain; uncertain SQL is checked word by word - dynamic SQL literals naming DROP DATABASE/SCHEMA or TRUNCATE are blocked; sequence advances are writes - an omitted warehouse is classified under each dialect execution can reach, with dbt following its cached adapter - data_diff and the write ask check the whole SQL, not a preview Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
| "i", | ||
| ) | ||
|
|
||
| const CODE_HARD_DENY = /\bDROP\s+(DATABASE|SCHEMA)\b|\bTRUNCATE\b(?!\s*\()/i |
There was a problem hiding this comment.
WARNING: TRUNCATE inside an unquoted identifier is hard-blocked
maskSql blanks quoted text but leaves unquoted words in code, so this arm matches any unquoted identifier named truncate (SELECT truncate FROM t, SELECT 1 AS truncate, WITH truncate AS (...)). MySQL, BigQuery and SQLite do not reserve truncate, so those are valid reads, yet classifyAndCheck returns blocked: true and both sql_execute and askFor refuse with "This cannot be overridden" (quoting the identifier is the only workaround). BATCH_STATEMENT above already restricts the ambiguity-prone words (USE/FETCH/OPEN/CLOSE/...) to their statement forms; give TRUNCATE the same treatment (statement start, after ;, or TRUNCATE TABLE) instead of matching every occurrence in code.
| // When quoting or comments cannot be read with certainty, no reading of the literals or comments | ||
| // can be trusted to hide a hard deny (a misread literal can make a real statement look like a | ||
| // comment), so the words alone decide; the TRUNCATE() function does not count. | ||
| const lexical = tokenizeSql(sql, dialect) |
There was a problem hiding this comment.
SUGGESTION: This is the third tokenization of the same SQL
classifyFallback already calls statementsOf (line 181) and tokenizeSql (line 184), and classifySql has already tokenized before calling this function, so a single classification now walks the input up to three times. Hoist one tokenizeSql result and reuse its tokens and complete here and at line 184.
| ] | ||
| // The hard deny is checked on the whole SQL here; the ask's patterns are 120-character previews, | ||
| // which a cut inside a literal would make read differently. | ||
| const dialectOf = (warehouse?: string) => Registry.canonicalType(warehouse ? Registry.getConfig(warehouse)?.type : undefined) ?? undefined |
There was a problem hiding this comment.
SUGGESTION: Third copy of the same dialect helper
This duplicates tool-cli/permissions.ts:21 and sql-execute.ts's defaultTarget dialectOf, with a slightly different guard (no typeof === "string" check). Extract one shared helper so the dialect a permission check reads cannot drift from the one the classification used.
| } | ||
| // A root without dbt_project.yml is rejected by the routing, which then uses the registry default. | ||
| if (!projectRoot || !existsSync(path.join(projectRoot, "dbt_project.yml"))) return { project: false, targetTypes: [] } | ||
| const targetTypes = await profileTargetTypes(projectRoot) |
There was a problem hiding this comment.
SUGGESTION: profiles.yml is re-read and re-parsed on every call
The manifest read above is cached by path/size/mtime, but profileTargetTypes re-reads dbt_project.yml and profiles.yml, re-parses the YAML and re-resolves every env_var on each call. dbtProjectAdapterType runs for every sql_execute that omits the warehouse outside the forced read-only path, so the profile read deserves the same mtime cache as the manifest.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Replying to the 20fb04e3 nits:
|
|
Replying to the c9a9cf91 nits:
A note on scope: later rounds kept finding ways to hide a destructive statement where the warehouse builds or decodes it at run time: dynamic SQL in literals, escape encodings, routine bodies, dbt Jinja. Many of those are now caught. Still, the DROP DATABASE / DROP SCHEMA / TRUNCATE block reads the SQL text, so the PR now documents it as best effort, the same as on |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
sahrizvi
left a comment
There was a problem hiding this comment.
Approved at fa31614 (consensus re-review, round 4: five reviewers, one convergence round).
Earlier findings:
- MAJOR, SQL Server/Fabric
E'\': fixed. ThroughcreateRunner().runwith a recording connector, the DELETE payload is refused without--allow-write, named and omitted. Both DROP DATABASE payloads (with and without;) are blocked in all four combinations. - Minor, omitted-warehouse false refusals: fixed. Postgres
$$...$$, BigQuery'it\'s', SQL Server'C:\reports\'and#temp, and TrinoESCAPE '\'run on a default connection. - Minor, dbt manifest vs live target: fixed. Profile target types are added, the cached adapter's project is followed, and an unreadable profile adds every dialect.
- Earlier deferred minors (Windows pipe owner check, dbt env in the daemon identity, daemon client deadline,
warehouse_addcredential hint): still listed under Follow-ups. Reasonably deferred.
Minors posted inline, none blocking:
- ClickHouse
"/`backslash escapes are not modeled (sql-tokenize.ts:128). Not blocking: hardening, contained by single-statement execution. - A bare
sp_executesql N'DROP ...'skips the hard block under--allow-write(sql-classify.ts:232). Not blocking: needs--allow-writeand wrapped dynamic SQL, the documented best-effort class; a one-line fix is safe in a follow-up. - Postgres literals ending in a backslash are refused (
sql-tokenize.ts:47). Not blocking: fails closed, documented known limit. docs/docs/usage/tool.md:54still promises an unconditional block. Not blocking: the gap predates this PR, and the best-effort wording is already in the description.
Nits and probe notes are in a separate comment.
| @@ -68,7 +127,15 @@ export function tokenizeSql(sql: string, dialect?: string): { tokens: SqlToken[] | |||
| ((quote === "'" && /^(mysql|mariadb|clickhouse|snowflake)$/i.test(dialect ?? "")) || | |||
| (quote === '"' && /^(mysql|mariadb)$/i.test(dialect ?? ""))) | |||
There was a problem hiding this comment.
MINOR (hardening): ClickHouse "..." and `...` take backslash escapes, but only ' is treated as uncertain here.
ClickHouse's lexer (quotedString in Lexer.cpp) skips the character after \ in all three quote kinds. This tokenizer reads \` as a backslash, a closing backtick and then a doubled one, so the identifier runs on to the last backtick and the code between is masked.
Confirmed through createRunner().run with a recording fake connector ({ w: { type: "clickhouse" } }), named and omitted, with and without --allow-write. In every case the call returns ok and the driver receives the text:
SELECT 1 AS `a\; DELETE FROM t; -- ` `` classifies as a read.SELECT 1 AS "a\"" ; DROP DATABASE x; -- "classifies as a read, and the DROP DATABASE is not blocked.
Related and older: the ClickHouse parser accepts SELECT 1 DROP TABLE t and SELECT 1 // c\nDROP TABLE t as one query.
Nothing runs today. The driver sends a single query over HTTP, the server rejects multi-statements, and a statement that starts with SELECT cannot write. So here the boundary rests on the driver, not the classifier.
Suggested fix: add clickhouse to the " backslash check on this line, add the same check for backticks, and add a test.
Blocker: no. Single-statement execution contains it; this is hardening.
| ) | ||
|
|
||
| const CODE_HARD_DENY = /\bDROP\s+(DATABASE|SCHEMA)\b|\bTRUNCATE\b(?!\s*\()/i | ||
| const DYNAMIC_SQL = /\bEXEC(UTE)?\b|(^|;)\s*(PREPARE|DO)\b/i |
There was a problem hiding this comment.
MINOR: a bare sp_executesql N'DROP DATABASE prod' (no EXEC) skips the hard block under --allow-write.
T-SQL lets a procedure call omit EXEC when it is the first statement of a batch. DYNAMIC_SQL matches only EXEC/EXECUTE/PREPARE/DO, so the literal's words are never read.
Confirmed through the runner with a recording connector, named and omitted:
- Without
--allow-write, the call is refused. - With
--allow-write, it runs (the driver receives it).sp_executesql N'TRUNCATE TABLE dbo.t'behaves the same. EXEC sp_executesql N'DROP DATABASE prod'is blocked.
Concatenated dynamic SQL also passes with --allow-write: EXEC('DR' + 'OP DATABASE prod'), Snowflake EXECUTE IMMEDIATE 'DR' || 'OP DATABASE prod', and Postgres DO $$BEGIN EXECUTE 'DR' || 'OP SCHEMA s CASCADE'; END$$. Your description already lists that class as best effort, the same as on main, with enforcement in the database as a follow-up.
This was the same at c9a9cf9. The PR description's "This covers ... sp_executesql" describes code that is not at fa31614.
Suggested fix (one line): add sp_executesql|sp_sqlexec|xp_execresultset to this pattern, with a test.
Blocker: no. It needs --allow-write plus SQL deliberately wrapped as dynamic SQL, which is the documented best-effort class. It is worth folding into your next push.
| // may still escape a quote with a backslash or start a comment with #, so both make the SQL | ||
| // uncertain. Oracle, Trino and Fabric do neither. | ||
| const unmodeled = !isModeledDialect(dialect) | ||
| const ambiguousBackslash = unmodeled || /^(postgres|postgresql|spark|databricks|hive)$/i.test(dialect ?? "") |
There was a problem hiding this comment.
MINOR (false positive): on PostgreSQL, a literal ending in an odd run of backslashes is now refused, named or omitted.
Adding postgres here makes '...\' uncertain. These were reads at c9a9cf9 and are now refused with "Re-run with --allow-write". Confirmed through the runner on a named and a default Postgres warehouse:
SELECT replace(p, '\', '/') FROM tSELECT * FROM t WHERE note LIKE '%\_%' ESCAPE '\'SELECT 'C:\reports\' AS p
Your description lists this as a known limit, because standard_conforming_strings may be off. An option that keeps the safety and the reads: have the Postgres driver set standard_conforming_strings = on for its session. The lexing is then certain, and Postgres can leave ambiguousBackslash.
Blocker: no. It fails closed and is documented.
|
|
||
| Runs are read-only by default. Reads such as `sql_execute` on a SELECT and `schema_inspect` are allowed; anything else is refused with `Permission denied: <kind>. Re-run with --allow-write to enable writes.` Tools that change local configuration (`warehouse_add`, `warehouse_remove`, `warehouse_install_driver`) also need `--allow-write`. | ||
|
|
||
| `DROP DATABASE`, `DROP SCHEMA` and `TRUNCATE` stay blocked even with `--allow-write`. |
There was a problem hiding this comment.
MINOR (docs): this line still says the block holds unconditionally.
At fa31614 it reads "DROP DATABASE, DROP SCHEMA and TRUNCATE stay blocked even with --allow-write." The PR description says this page documents the block as best effort, but that wording is not pushed yet. Dynamic SQL gets past the block with --allow-write (see the sql-classify.ts comment).
Suggested fix: push the best-effort wording from the description, e.g. "are refused even with --allow-write, by reading the SQL text; SQL the warehouse assembles at run time (dynamic SQL, stored routines) can get past it — use a role without these privileges for a guarantee."
Blocker: no. The gap predates this PR on main, and the wording is already written in the description.
|
Consensus re-review at fa31614: nits and probe notes not tied to a diff line (companion to the approval above; none of these block) Nits:
Probed and fine (classifier at fa31614; key cases also run through
|
Another test file in the same bun process can create a dbt adapter and leave its project recorded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
🔁 @hkaltai — this change was promoted to the help-docs hub: https://github.com/AltimateAI/help-docs/pull/147 — please review/merge to keep the docs in sync. |
main's tree is #1609's head, which this branch already contains.
Issue for this PR
No linked issue (new feature).
Type of change
What does this PR do?
Lets external coding agents such as Claude Code call altimate-code's data tools directly, over MCP or from the shell, instead of delegating each task to a second altimate-code agent with its own model. It builds on #1543 (
mcp serve) and #1576 (workspace tools), which are included here.altimate-code mcp serveand a new model-freealtimate-code toolCLI (list,schema,run,context,daemon) execute tools through the same adapter: one allow-list, one permission layer, the same compact schemas, result spill files andanswer_commit. Organisations that disallow MCP servers keep the same tools throughtool run.altimate-code mcp serveexposes data tools to MCP clients without an LLM #1543 tools plusimpact_analysis,dbt_lineage,altimate_core_column_lineage,schema_detect_pii,data_diff,altimate_core_equivalenceandsql_translate;--all-toolsexposes the full allow-list.toolcalls go through a per-project daemon (auto-start, exits after 10 idle minutes, socket scoped to version and connections) that keeps connections and the schema cache warm and releases DuckDB/SQLite file locks between calls.workspace_context,workspace_skillandworkspace_executeare on by default fortoolcommands in linked projects (--no-workspace-toolsturns them off);mcp serveexposes them with--workspace-tools.tool contextprints the analyst method, connections, the linked workspace and its skills.<project>/.claude/skills/, so linking or refreshing a project makes them available when a Claude Code session starts. User-written skills of the same name are never touched, unlink removes the copies, projects without.claude/are left alone,ALTIMATE_CLAUDE_SKILL_SYNC=0disables it, and a mirror failure never changes the sync result.--allow-write;DROP DATABASE,DROP SCHEMAandTRUNCATEstay blocked even then.link --claude-md(opt-in) writes CLI routing guidance intoCLAUDE.mdfor clients that cannot use MCP.Behaviour changes in shared tools:
schema_inspectwithout a table lists schemas, or up to 500 tables with catalog row estimates (no scans); MongoDB catalog discovery is bounded.answer_commitacceptsaskedandreadings; its rejection checks moved to their own module with per-turn limits.The CI change from #1543 is intentionally not included. Companion Claude Code plugin: AltimateAI/altimate-claude-plugin#11.
How did you verify your code works?
bun run typecheckclean after merging currentmain.bun test test/altimate/mcp-server test/altimate/tool-cli test/altimate/workspace test/altimate/analyst-effort.test.ts test/agent/analyst-data-explorer.test.ts: 1994 tests, 0 failures (includes the new.claude/skillsmirror tests).workspace refreshagainst a linked workspace writing the Claude Code skill copies.tool run --allow-write, no test runner.tool list(70 tools, and--json),tool context,tool schema.sql_executereads:tablesvalue profiles;queriesusing{{label.column}};sql_executerefusals:--allow-write(also a DELETE after a comment) and run with it;SELECT 1; TRUNCATEare blocked even with--allow-write;schema_inspect: schemas, then one table with value samples;data_diff: found the 1 extra and 2 changed rows of 100;sql_materializeintoscratch, then a query inscratch;sql_explainplan,sql_translate,lineage_check,schema_detect_pii.bash), invalid arguments, andwarehouse_addwithout--allow-write.--no-daemon;tool daemon statusandstopwork.mcp serveover stdio:initializereturns the instructions, andtools/listthe 12 default tools;--allow-write, the DELETE runs and DROP SCHEMA stays blocked.tool run, with no false positives on the 25 tasks that should not use it and unchanged accuracy (94–95/100).Cost side effects
No new model calls: the
toolCLI and MCP server run tools without an LLM. The analyst agent now requests high reasoning effort on adaptive-thinking models when neither the agent nor the model config sets one, so analyst runs use more reasoning tokens;ALTIMATE_ANALYST_EFFORT(e.g.mediumoroff), the agent'soptionsor the model'soptionsoverride it. Warehouse queries run only when an agent calls a tool, with results capped by the existing output limits and spill files.schema_inspectlistings are capped at 500 tables and use catalog estimates rather than scans. The per-project daemon exits after 10 idle minutes.Screenshots / recordings
Not a UI change.
Checklist
Review round (853bfa0, 7109869)
The 88 open review threads (cubic, CodeRabbit, Kilo) each have a reply: 60 fixed in 853bfa0, 22 kept with a reason, 4 out of scope, 2 false positives.
bun test test/altimate/: 10,271 pass, 0 fail, 757 skip;ALTIMATE_MCP_E2E=1 bun test test/altimate/mcp-server/: 113 pass;bun test test/altimate/tool-cli: 75 pass; typecheck clean.Review round (human consensus review of 7109869)
All 27 threads (9 review, 18 bot) and the companion comment have replies. Fixed in 4652bed:
data_diffvalidatessource,targetandwhere_clauseas single read-only queries, checks every statement it runs, and opens every connector read-only.isBlockedStatementclassifies the whole pattern..claude/skillscopies are opt-in withALTIMATE_CLAUDE_SKILL_SYNC=1(the Claude Code plugin sets it).Rebutted: the connector release in
serverInstructionsruns only at server construction.Verified after merging
main: typecheck clean;bun test test/altimate/tool-cli test/altimate/mcp-server test/altimate/workspace test/altimate/toolsand the data-diff suites pass (0 fail), as does the DuckDB data-diff e2e (ALTIMATE_RUN_WAREHOUSE_E2E=1).Known limit, unchanged here: outside Postgres, SQLite and DuckDB, read-only is enforced by SQL classification. A read that calls a stored function which writes is accepted by
sql_execute,sql_checkanddata_diffalike. Sequence advances (nextval(),NEXT VALUE FOR,.NEXTVAL) are writes. Use a read-only database user, or one withoutDROP/TRUNCATE, for the strongest boundary.Review round (human consensus review of c9a9cf9)
All three threads have replies. Fixed in dac4caa (merged with
mainin fa31614):MAJOR:
E'...'is an escape string only in PostgreSQL, Redshift and DuckDB. For T-SQL (sqlserver,mssql,tsql,fabric) and any dialect the lexer does not model, a read may hold no statement keyword in its code, because statements there need no;.Omitted warehouse: the SQL is classified under each dialect the call can reach:
The dbt project is the one dbt's routing uses: the cached adapter's project once it is created, before that the saved config's root, else discovery.
Lexing per engine:
//comments, and only$$quotes.--comment continuing past a trailing backslash.--is a comment only before ASCII whitespace./*inside a comment, where an engine's nesting is not known.Hard block:
EXEC(...),EXECUTE IMMEDIATE,PREPARE ... FROM,DO $$...$$) count too.data_diffand the write ask check the whole SQL, not the preview.Tests cover each case, plus runner tests with a fake connector for SQL Server and Fabric, named and default, with and without
--allow-write, and for omitted-warehouse reads on Postgres, BigQuery, SQL Server and Trino defaults. Classification stays linear on large inputs.Further hardening from later review rounds is in the stacked PR #1644, kept out of this approved PR. It covers
sql_explaininput checks, more write-as-read cases, a best-effort note in the docs, and the false refusals listed below. Most of those gaps also exist onmain.Scope of the DROP DATABASE / DROP SCHEMA / TRUNCATE block. The block reads the SQL text, so it is best effort. A statement the warehouse builds or decodes at run time can get past it, for example:
The same bypasses exist on
main. #1644 documents the block as best effort indocs/docs/usage/tool.md; making it a guarantee is left to a follow-up PR (below). Reads, and writes needing--allow-write, are enforced as before.Known limits at this PR's head (#1644 fixes the first three and the last). All but the last refuse more than needed; the last can run a second statement:
truncate(SELECT truncate FROM t) is refused even with--allow-write.SELECT 1 AS throwon SQL Server, and a column namednextval, are refused as writes.target_warehouseindata_diffis checked against the source warehouse but run on the registry default.LIKE ... ESCAPE '\') are refused as uncertain, sincestandard_conforming_stringsmay be off.q'...'quotes and ClickHouse heredocs are refused.--allow-write.GRANT DROP DATABASE,GRANT TRUNCATE,BEFORE TRUNCATE) are refused.TRUNCATE /* c */ (is refused.sql_explainputs EXPLAIN in front of the text as given. On drivers that run every statement in it (Redshift, DuckDB), a second statement such asSELECT 1; DELETE FROM truns.sql_explainis not in the default MCP tool set; fix: harden SQL classification for the tool CLI and MCP server #1644 refuses such input.Follow-ups (not in this PR)
schema_detect_piiandschema_searchcan answer from another database indexed under the same name (pre-existing onmain).Pre-existing on
main:bindsargument ofexecute(packages/drivers/src/mysql.ts,redshift.ts); this PR inlines an escaped literal where it needs a schema filter.Deferred:
{{label.column}}value contains a single quote is refused on those dialects (7109869 escapes it with a backslash instead of sending'').listTables()(Oracle, SQL Server, Databricks, ClickHouse, Trino).ALTIMATE_MCP_E2E=1suite to CI (CI changes are excluded from this PR).answer_commitelicitation should also trigger on a single-entryreadingslist.answer_commithistory if concurrent clients on one daemon become a real use.env_var,DBT_PROFILES_DIR) should be part of the daemon identity.Tool.isErrorResultand reuse it inmcp-server/adapter.ts.warehouse_addaccepting a dbt profile/target or env-var reference, so the agent does not have to read raw credentials.warehouse_add's local configuration writes in read-only MCP.sql_checkpassingforceReadOnly(DuckDB/SQLite file open).claude-md.ts: write withO_NOFOLLOWor a temp file plus rename, and avoid a second block when the markers were removed.main). Options: run agent calls with a restricted role, or in a read-only transaction where the driver supports it (Postgres already does for reads); otherwise refuse the call. This covers statements the warehouse assembles at run time, which no text check can see.Registry.canonicalType(permissions,sql_execute) andwarehouseTypeToDialect(data_diff), which return different strings for SQL Server and Fabric.data_diffrejectswhere_clausein one dialect.tool daemon stopwaiting for, or reporting, another client's in-flight call instead of closing it with "closed the connection before replying".standard_conforming_strings=onon Postgres connections, so literals ending in a backslash can be read as reads.q'...'quotes and ClickHouse heredocs instead of refusing them.config/config.ts:698(on main): the agent loader takesdirnameof amobileconfig:managed-preferences source, so a relative{file:...}there does not resolve; the MCP output settings strip the prefix since fdeddbf.higheffort default, theanswer_commitnumber check and the opt-in.claude/skillscopies (documented indocs/docs/configure/).Summary by CodeRabbit