Skip to content

feat: model-free tool CLI and MCP server for external coding agents - #1609

Merged
hkaltai merged 28 commits into
mainfrom
tool-cli-daemon
Nov 3, 2026
Merged

hkaltai merged 28 commits into
mainfrom
tool-cli-daemon

Conversation

@hkaltai

@hkaltai hkaltai commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Issue for this PR

No linked issue (new feature).

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

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.

  • One adapter, two entry points. altimate-code mcp serve and a new model-free altimate-code tool CLI (list, schema, run, context, daemon) execute tools through the same adapter: one allow-list, one permission layer, the same compact schemas, result spill files and answer_commit. Organisations that disallow MCP servers keep the same tools through tool run.
  • Default MCP tools. The five feat(mcp): altimate-code mcp serve exposes data tools to MCP clients without an LLM #1543 tools plus impact_analysis, dbt_lineage, altimate_core_column_lineage, schema_detect_pii, data_diff, altimate_core_equivalence and sql_translate; --all-tools exposes the full allow-list.
  • Warm per-project process. tool calls 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 tools. workspace_context, workspace_skill and workspace_execute are on by default for tool commands in linked projects (--no-workspace-tools turns them off); mcp serve exposes them with --workspace-tools. tool context prints the analyst method, connections, the linked workspace and its skills.
  • Workspace skills for Claude Code. Every workspace skill sync also writes sanitised copies (frontmatter reduced to name and description, no command blocks, no hook or plugin component files) into <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=0 disables it, and a mirror failure never changes the sync result.
  • Safety. MCP requests are read-only unless --allow-write; DROP DATABASE, DROP SCHEMA and TRUNCATE stay blocked even then.
  • link --claude-md (opt-in) writes CLI routing guidance into CLAUDE.md for clients that cannot use MCP.

Behaviour changes in shared tools:

  • schema_inspect without a table lists schemas, or up to 500 tables with catalog row estimates (no scans); MongoDB catalog discovery is bounded.
  • answer_commit accepts asked and readings; its rejection checks moved to their own module with per-turn limits.
  • The analyst requests high reasoning effort on adaptive-thinking models unless the user or agent config sets one.

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 typecheck clean after merging current main.
  • 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/skills mirror tests).
  • End to end, earlier: workspace refresh against a linked workspace writing the Claude Code skill copies.
  • Manual end to end at ee6add5: the CLI built from source, a local DuckDB warehouse seeded through tool run --allow-write, no test runner.
    • Discovery: tool list (70 tools, and --json), tool context, tool schema.
    • sql_execute reads:
      • a plain read;
      • tables value profiles;
      • chained queries using {{label.column}};
      • an omitted warehouse uses the default connection.
    • sql_execute refusals:
      • writes are refused without --allow-write (also a DELETE after a comment) and run with it;
      • DROP SCHEMA, DROP DATABASE and SELECT 1; TRUNCATE are blocked even with --allow-write;
      • a multi-statement read and an unknown warehouse get clear errors.
    • Other tools:
      • schema_inspect: schemas, then one table with value samples;
      • data_diff: found the 1 extra and 2 changed rows of 100;
      • sql_materialize into scratch, then a query in scratch;
      • sql_explain plan, sql_translate, lineage_check, schema_detect_pii.
    • Rejected calls: an unknown tool, an excluded coding tool (bash), invalid arguments, and warehouse_add without --allow-write.
    • Daemon:
      • starts on the first call;
      • warm calls take about 1 s, against about 3 s with --no-daemon;
      • reloads after the connections file is edited;
      • tool daemon status and stop work.
    • mcp serve over stdio:
      • initialize returns the instructions, and tools/list the 12 default tools;
      • reads run, a DELETE is refused ("Restart with --allow-write"), and DROP SCHEMA is blocked;
      • with --allow-write, the DELETE runs and DROP SCHEMA stays blocked.
    • With the Claude Code plugin (chore(deps-dev): Bump @parcel/watcher-darwin-arm64 from 2.5.1 to 2.5.6 #11): live sessions in MCP mode and in CLI mode answered warehouse questions correctly and reported the refused DELETE.
  • Benchmarks with Claude Code (Sonnet 4.5):
    • 100 dbt and warehouse tasks: routing to altimate-code rose from 55/75 to 74/75 over MCP and 75/75 through tool run, with no false positives on the 25 tasks that should not use it and unchanged accuracy (94–95/100).
    • Questions that need workspace skills: setups using this PR had the skills available at the start of every session (100/100 vs 0/100 without), and accuracy matched the existing MCP flow (58% vs 55% on 100 questions) at 12% lower cost per run.

Cost side effects

No new model calls: the tool CLI 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. medium or off), the agent's options or the model's options override it. Warehouse queries run only when an agent calls a tool, with results capped by the existing output limits and spill files. schema_inspect listings 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

  • I have tested my changes locally
  • I have not included unrelated changes in this PR
  • I checked for cost side effects (model calls, queries or cloud resources that grow with user data) and described them, or wrote "none"

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:

  • MAJOR: data_diff validates source, target and where_clause as single read-only queries, checks every statement it runs, and opens every connector read-only.
  • isBlockedStatement classifies the whole pattern.
  • Redshift backslash escaping.
  • Daemon idle/release handling (only a request that ran a tool releases; a stop is answered without waiting) and an owner-only socket.
  • Workspace refresh and snapshot-read fixes; analyst-effort guards; managed output-preference paths.
  • .claude/skills copies are opt-in with ALTIMATE_CLAUDE_SKILL_SYNC=1 (the Claude Code plugin sets it).
  • Docs, and tests for each.

Rebutted: the connector release in serverInstructions runs 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/tools and 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_check and data_diff alike. Sequence advances (nextval(), NEXT VALUE FOR, .NEXTVAL) are writes. Use a read-only database user, or one without DROP/TRUNCATE, for the strongest boundary.

Review round (human consensus review of c9a9cf9)

All three threads have replies. Fixed in dac4caa (merged with main in 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 registry default;
    • dbt, only when it can take the call (a dbt project, not forced read-only), adding its manifest type and every target type of the project's profile;
    • every supported dialect plus generic lexing, only when dbt can take the call and its types cannot be read.

    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:

    • BigQuery: triple-quoted, raw and backslash-escaped strings.
    • Snowflake: // comments, and only $$ quotes.
    • Spark: a -- comment continuing past a trailing backslash.
    • MySQL: -- is a comment only before ASCII whitespace.
    • Non-ASCII characters in dollar tags and identifiers.
    • A /* inside a comment, where an engine's nesting is not known.
    • A quote after an odd run of backslashes, where it depends on server settings.
  • Hard block:

    • Uncertain SQL is checked word by word. Fully read SQL is checked on its masked statement words.
    • Literals in dynamic SQL (EXEC(...), EXECUTE IMMEDIATE, PREPARE ... FROM, DO $$...$$) count too.
    • data_diff and 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_explain input 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 on main.

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:

  • dynamic SQL assembled by concatenation or functions;
  • a stored routine created earlier;
  • a literal encoding the check does not model.

The same bypasses exist on main. #1644 documents the block as best effort in docs/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:

  • A column or alias named truncate (SELECT truncate FROM t) is refused even with --allow-write.
  • SELECT 1 AS throw on SQL Server, and a column named nextval, are refused as writes.
  • An empty target_warehouse in data_diff is checked against the source warehouse but run on the registry default.
  • PostgreSQL literals ending in a backslash (LIKE ... ESCAPE '\') are refused as uncertain, since standard_conforming_strings may be off.
  • Oracle q'...' quotes and ClickHouse heredocs are refused.
  • BigQuery escaped-backtick identifiers can be refused by the parser.
  • Dynamic SQL whose literals mention DROP DATABASE/SCHEMA or TRUNCATE is refused even with --allow-write.
  • Privilege and trigger names (GRANT DROP DATABASE, GRANT TRUNCATE, BEFORE TRUNCATE) are refused.
  • In SQL that cannot be read with certainty, TRUNCATE /* c */ ( is refused.
  • The write ask names the registry warehouse even when dbt may run the write.
  • sql_explain puts EXPLAIN in front of the text as given. On drivers that run every statement in it (Redshift, DuckDB), a second statement such as SELECT 1; DELETE FROM t runs. sql_explain is 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)

  • The schema cache is keyed by warehouse name only, so schema_detect_pii and schema_search can answer from another database indexed under the same name (pre-existing on main).

Pre-existing on main:

  • MySQL and Redshift drivers ignore the binds argument of execute (packages/drivers/src/mysql.ts, redshift.ts); this PR inlines an escaped literal where it needs a schema filter.

Deferred:

  • The read-only SQL tokenizer does not lex backslash escapes for BigQuery and Databricks, so a batch step whose {{label.column}} value contains a single quote is refused on those dialects (7109869 escapes it with a backslash instead of sending '').
  • Bound the fallback schema/table listing for drivers that only expose listTables() (Oracle, SQL Server, Databricks, ClickHouse, Trino).
  • Add the ALTIMATE_MCP_E2E=1 suite to CI (CI changes are excluded from this PR).
  • Decide whether answer_commit elicitation should also trigger on a single-entry readings list.
  • Per-request answer_commit history if concurrent clients on one daemon become a real use.
  • Whether per-shell dbt credentials (env_var, DBT_PROFILES_DIR) should be part of the daemon identity.
  • Export Tool.isErrorResult and reuse it in mcp-server/adapter.ts.
  • Windows named-pipe owner/ACL check for the tool daemon.
  • Daemon client deadline with cancellation of the running request.
  • warehouse_add accepting a dbt profile/target or env-var reference, so the agent does not have to read raw credentials.
  • A separate flag for warehouse_add's local configuration writes in read-only MCP.
  • sql_check passing forceReadOnly (DuckDB/SQLite file open).
  • claude-md.ts: write with O_NOFOLLOW or a temp file plus rename, and avoid a second block when the markers were removed.
  • A debug log for engine operations dropped from the workspace catalog.
  • Carry the classified default connection through dispatch for an omitted warehouse (keeping dbt-first routing), so a connection change during one call cannot reach an unclassified dialect.
  • Enforce the DROP DATABASE / DROP SCHEMA / TRUNCATE block in the database, not only by reading the SQL (pre-existing on 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.
  • One dialect mapper for Registry.canonicalType (permissions, sql_execute) and warehouseTypeToDialect (data_diff), which return different strings for SQL Server and Fabric.
  • Name the side when a cross-dialect data_diff rejects where_clause in one dialect.
  • tool daemon stop waiting for, or reporting, another client's in-flight call instead of closing it with "closed the connection before replying".
  • Pin standard_conforming_strings=on on Postgres connections, so literals ending in a backslash can be read as reads.
  • Lex Oracle q'...' quotes and ClickHouse heredocs instead of refusing them.
  • Teach the tokenizer Snowflake/ClickHouse backslash escapes, so batch values with a backslash run there instead of being refused; also for an omitted warehouse whose default is one of them.
  • config/config.ts:698 (on main): the agent loader takes dirname of a mobileconfig: managed-preferences source, so a relative {file:...} there does not resolve; the MCP output settings strip the prefix since fdeddbf.
  • A CHANGELOG entry for the analyst high effort default, the answer_commit number check and the opt-in .claude/skills copies (documented in docs/docs/configure/).

Summary by CodeRabbit

  • New Features
    • Added a headless tool CLI for listing, inspecting, and running tools, with optional daemon reuse and workspace tools.
    • Added an MCP server over stdio with configurable tools, workspace integration, output limits, and write permissions.
    • Added workspace client commands and optional Claude Code routing guidance.
    • Schema inspection can list schemas and tables; SQL execution supports batches and references to earlier results.
  • Improvements
    • Added bounded catalog discovery across supported databases, including MongoDB collection counts.
    • Strengthened read-only checks for data comparisons and tool execution.
    • Improved answer validation, analyst reasoning defaults, and compact MCP result output.
    • Workspace skills can be mirrored to Claude Code when enabled; sign-out withdraws managed copies.
  • Documentation
    • Added guidance for the tool CLI, MCP server, workspace tools, analyst settings, and Claude Code skill copies.

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>
@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • Resume reviews
  • Trigger review
Walkthrough

The PR adds MCP and model-free CLI interfaces for data tools and workspace operations. It adds bounded schema discovery and SQL execution, workspace skill mirroring, and changes to analyst answer validation and reasoning options.

Changes: External Tool Interfaces

Layer / File(s) Summary
MCP server and request handling
packages/opencode/src/altimate/mcp-server/*, packages/opencode/src/cli/cmd/mcp.ts, packages/opencode/src/index.ts, docs/docs/configure/mcp-servers.md, packages/opencode/test/altimate/mcp-server/*
Adds an MCP stdio server, configurable tool exposure and write access, an analyze prompt, result links, workspace routing, and shutdown handling.
SQL execution and catalog operations
packages/opencode/src/altimate/tools/*, packages/opencode/src/altimate/native/connections/*, packages/drivers/src/mongodb.ts, related tests
Adds SQL batches and result references, MCP CSV spills, bounded schema listings, local catalog discovery, read-only propagation, and MongoDB cursor limits.
Model-free Tool CLI
packages/opencode/src/altimate/tool-cli/*, packages/opencode/src/cli/cmd/tool.ts, packages/opencode/src/index.ts, docs/docs/usage/*, docs/mkdocs.yml, related tests
Adds tool listing, context, schema, run, and daemon commands. The CLI supports stdin JSON, connection reloads, optional workspace tools, and write permissions.
Workspace client and skill integrations
packages/opencode/src/altimate/workspace/*, packages/opencode/src/altimate/mcp-server/workspace.ts, packages/opencode/src/altimate/api/*, related tests
Adds workspace identity, context, skill and memory operations; allow-listed workspace engine reads; and managed Claude Code skill mirroring.
Analyst answer and reasoning changes
packages/opencode/src/altimate/tools/answer-commit*.ts, packages/opencode/src/altimate/prompts/analyst.txt, packages/opencode/src/altimate/session/analyst-effort.ts, packages/opencode/src/session/llm.ts, related tests
Adds answer coverage, reading, and number-provenance checks, plus analyst reasoning-effort selection.

Estimated code review effort: 5 (Critical) | ~120 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Client as MCP client or CLI
  participant Entry as MCP server or Tool CLI
  participant Adapter as Tool adapter
  participant Data as Data tools and connections
  Client->>Entry: Request tool list or execution
  Entry->>Adapter: Select and invoke exposed tool
  Adapter->>Data: Run query or workspace operation
  Data-->>Adapter: Return result
  Adapter-->>Entry: Format result and metadata
  Entry-->>Client: Return output and resource links
Loading

Possibly related PRs

Suggested reviewers: sahrizvi

Merge Risk: Moderate · up to fdedd

The opt-in CLAUDE.md writer can modify a symlink target outside the project if the file changes during the write, and managed output limits can be ignored. Resolve these before merging; also make the daemon shutdown test wait for the run it intends to check.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 58.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the main changes: a model-free tool CLI and MCP server for external coding agents.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The pull request description includes all required template sections. It clearly explains the feature, verification steps, cost side effects, screenshots status, checklist completion, and known limita…
Full details: Description check

Explanation

The pull request description includes all required template sections. It clearly explains the feature, verification steps, cost side effects, screenshots status, checklist completion, and known limitations.

Full details: Docstring Coverage

Explanation

Docstring coverage is 58.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 175 functions across 77 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

A rabbit taps the tools awake
Through pipes and prompts, new paths they make
A bounded query finds its way
While careful checks inspect the day
Workspace skills hop into view
Then carrot crumbs say, “Done for you!”

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1d811c10-f911-4679-9471-f8ac59645cf1
Commits

Reviewing files that changed from the base of the PR and between fcc3c44 and fdeddbf.

Files selected for processing: 11
  • packages/opencode/src/altimate/mcp-server/output-settings.ts
  • packages/opencode/src/altimate/tool-cli/daemon.ts
  • packages/opencode/src/altimate/tool-cli/permissions.ts
  • packages/opencode/src/altimate/tools/data-diff.ts
  • packages/opencode/src/altimate/tools/read-only-sql.ts
  • packages/opencode/src/altimate/tools/sql-execute.ts
  • packages/opencode/test/altimate/analyst-effort.test.ts
  • packages/opencode/test/altimate/mcp-server/output-settings.test.ts
  • packages/opencode/test/altimate/tool-cli/daemon.test.ts
  • packages/opencode/test/altimate/tool-cli/runner.test.ts
  • packages/opencode/test/altimate/tools/sql-execute-batch.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

@github-actions

Copy link
Copy Markdown

Hey! Your PR title [DO NOT MERGE] Experiment: model-free tool CLI and MCP server on one adapter doesn't follow conventional commit format.

Please update it to start with one of:

  • feat: or feat(scope): new feature
  • fix: or fix(scope): bug fix
  • docs: or docs(scope): documentation changes
  • chore: or chore(scope): maintenance tasks
  • refactor: or refactor(scope): code refactoring
  • test: or test(scope): adding or updating tests

Where scope is the package name (e.g., app, desktop, opencode).

See CONTRIBUTING.md for details.

@github-actions

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

@github-actions

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

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>
@github-actions

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

1 similar comment
@github-actions

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

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>
@github-actions

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

1 similar comment
@github-actions

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

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
@github-actions

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

@hkaltai hkaltai changed the title [DO NOT MERGE] Experiment: model-free tool CLI and MCP server on one adapter feat: model-free tool CLI and MCP server for external coding agents Oct 29, 2026
@github-actions

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

@github-actions

Copy link
Copy Markdown

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>
hkaltai and others added 5 commits October 30, 2026 09:25
…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>
@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

@sahrizvi sahrizvi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 warehouse was 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 default SELECT 'x\'' ; DROP SCHEMA ds CASCADE; -- ' is blocked, the MySQL default # note\nDROP SCHEMA prod is 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 when warehouse is omitted). It is folded into the minor on sql-execute.ts:673.
  • Windows pipe owner check, dbt env in daemon identity, daemon client timeout/cancel, warehouse_list credential 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 following DELETE, or DROP 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] === "\\") {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, with warehouse: "ms" and with warehouse omitted: status ok, and the driver receives the exact text. The SQL Server driver inserts TOP 1001 after SELECT and runs the batch with pool.request().query, so the DELETE executes.
  • { 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 the DROP 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 as read under tsql/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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 DATABASE variant 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))]),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 p and SELECT * 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 p and SELECT * 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.yml means 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.

@sahrizvi

sahrizvi commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator

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):

  • Oracle q'[it's]' alternative quoting is not lexed, so such reads are refused (this predates the PR).
  • Once SQL is uncertain, the words-alone hard block also catches words inside literals or backtick identifiers, e.g. SELECT 'a\' AS x, 'DROP' AS y, 'schema' AS z on MySQL is blocked. TRUNCATE -- c\n(x, 2) (a line comment before the function's () is blocked too, because the lookahead skips only whitespace and block comments. The first is your stated trade-off.

Checked and fine:

  • Words-alone scan: case-folded, a single linear pass, ignores quotes and comments, so /*!SCHEMA*/, /*!50000DROP*/, non-breaking spaces and mixed case do not evade it. A split keyword (DR/**/OP) is not a keyword to any engine either.
  • TRUNCATE as a function: no supported engine accepts TRUNCATE (t) or TRUNCATE /*! t */ ( as DDL, and a complete TRUNCATE (t) statement is still blocked by the per-statement pattern.
  • dbt adapter type: no subprocess. It honors target-path and DBT_TARGET_PATH, the cache key is path, size and mtime, and a missing manifest falls back to { project: true } (fail closed). It uses the same project precedence as dispatch.
  • BigQuery raw r'\' , '; DELETE ...': the tokenizer closes the raw literal early, but the parser disagrees, so it is classified as a write. Not a bypass.

hkaltai and others added 2 commits November 3, 2026 16:51
… 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>
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

1 similar comment
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

@hkaltai

hkaltai commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor Author

Replying to the 20fb04e3 nits:

  • data_diff ask on a 120-character prefix: fixed in dac4caa. The tool checks the hard block on the whole source and target SQL, each in its own warehouse's dialect, before it asks. A destructive input now gets "blocked for safety", and a long input whose cut lands inside a literal is no longer refused. where_clause is still checked by the native requireReadOnlyQuery.
  • Empty target_warehouse: fixed in the stacked PR fix: harden SQL classification for the tool CLI and MCP server #1644, which keeps this approved PR unchanged. The tool passes "" to the native layer as undefined, so both layers run the target on the source warehouse, as the parameter description says.
  • Two dialect mappers (Registry.canonicalType and warehouseTypeToDialect): listed as a follow-up in the PR description. Both are handled or fail closed today.
  • Which side rejected where_clause in a cross-dialect diff: follow-up. The message is the only difference.
  • tool daemon stop while another client's call runs: follow-up. As you note, nothing is re-sent, so only the message is affected.
  • Missing tests: added through the runner. A BigQuery default with your backslash payload (and an ASSERT variant) and a MySQL default with # x\nDROP SCHEMA s are both blocked even with --allow-write. MCP calls go through the same adapter and askFor as tool run.

@hkaltai

hkaltai commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor Author

Replying to the c9a9cf91 nits:

  • Oracle q'[it's]': still refused. In the stacked PR fix: harden SQL classification for the tool CLI and MCP server #1644, a q'/nq' quote also makes the SQL uncertain, so the hard block's word scan covers it. Lexing q-quotes properly is a follow-up in the PR description.
  • Words-alone scan catching literals in uncertain SQL, and TRUNCATE -- c\n(x, 2): both kept as the stated trade-off. In uncertain SQL, TRUNCATE now counts unless ( follows it directly, so a comment of either kind before the ( is refused. That removed the comment-skipping lookahead, which later rounds showed could be misled where comment nesting is uncertain. Both cases are listed under known limits.
  • Checked items: thanks. One change in the stacked PR fix: harden SQL classification for the tool CLI and MCP server #1644: the dbt helper uses the project of the adapter dbt has already created (register.ts's cache), and before that the same saved-config-then-discovery precedence. A saved root without dbt_project.yml now means no dbt project, as dispatch treats it.

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 main. Making it a guarantee in the database is a separate follow-up. See "Scope of the DROP DATABASE / DROP SCHEMA / TRUNCATE block" in the description.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

1 similar comment
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

sahrizvi
sahrizvi previously approved these changes Nov 3, 2026

@sahrizvi sahrizvi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at fa31614 (consensus re-review, round 4: five reviewers, one convergence round).

Earlier findings:

  • MAJOR, SQL Server/Fabric E'\': fixed. Through createRunner().run with 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 Trino ESCAPE '\' 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_add credential hint): still listed under Follow-ups. Reasonably deferred.

Minors posted inline, none blocking:

  1. ClickHouse "/` backslash escapes are not modeled (sql-tokenize.ts:128). Not blocking: hardening, contained by single-statement execution.
  2. A bare sp_executesql N'DROP ...' skips the hard block under --allow-write (sql-classify.ts:232). Not blocking: needs --allow-write and wrapped dynamic SQL, the documented best-effort class; a one-line fix is safe in a follow-up.
  3. Postgres literals ending in a backslash are refused (sql-tokenize.ts:47). Not blocking: fails closed, documented known limit.
  4. docs/docs/usage/tool.md:54 still 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 ?? "")))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?? "")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 t
  • SELECT * 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.

Comment thread docs/docs/usage/tool.md

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`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sahrizvi

sahrizvi commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator

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:

  • The PR description is ahead of the head. It describes DROP CATALOG/DROP NAMESPACE, the dbt Jinja check, Oracle q' marked uncertain, sp_executesql coverage, the empty target_warehouse fix and the best-effort docs wording. None of these is at fa31614. This review covers the head as pushed.
  • Ordinary reads still refused (all fail closed): Oracle q'[...]', Postgres/Trino U&'...' (with or without UESCAPE), Snowflake 'C:\\x', MySQL 'it\'s', and nested /* /* */ */ on Redshift, Snowflake and BigQuery.
  • T-SQL statement rule: OPEN MASTER KEY ... and RECEIVE a, b FROM q do not match it. Both are still writes, because the parser rejects them.

Probed and fine (classifier at fa31614; key cases also run through createRunner().run with a recording connector):

  • SQL Server/Fabric: N'', [a]]b], nested comments (T-SQL nests them, per Microsoft's docs), GO, bare procedure calls after a SELECT, IF/ELSE DELETE, UPDATE STATISTICS, COPY INTO, WAITFOR and mixed-case MERGE/EXEC are all writes. Columns named update/open_date, OFFSET ... FETCH, OPTION (USE HINT ...), WITH (NOLOCK) and #temp stay reads.
  • Postgres/Redshift/DuckDB: E'\', U&'\', $tag$, non-ASCII tags, nested comments, 1E'\', x$$, Redshift '\'.
  • Snowflake: //, $$, $a$ (not a quote) and '\''.
  • BigQuery: '''/""", r'\', b'\'', `a\ `` and #.
  • MySQL: '\'', -- followed by NBSP or DEL, and /*!.
  • Databricks: '\', a -- comment continued by a trailing backslash, r'\' and "\"".
  • Oracle/Trino: '\' and U&'\'.
  • Unmodeled dialects: E'', $$, nested comments, // and -- ending in a backslash.
  • Each of these classifies as the engine reads it, or fails closed. Apart from ClickHouse quoted identifiers (inline), the design now fails closed for SQL it cannot read with certainty in the target dialect.
  • hardDenyChecked: the whole SQL is checked under every reachable dialect before the ask, so skipping the 200-character preview check loses nothing.
  • Tests: sql-classify, dbt-adapter-type, tool-cli/*, sql-comment-safety and sql-execute pass (342 pass, 0 fail).

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>
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

1 similar comment
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

2 similar comments
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

👋 This PR was automatically closed by our quality checks.

Common reasons:

  • New GitHub account with limited contribution history
  • PR description doesn't meet our guidelines
  • Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

@hkaltai
hkaltai merged commit 2d59604 into main Nov 3, 2026
33 of 45 checks passed
@hkaltai
hkaltai deleted the tool-cli-daemon branch November 3, 2026 18:42
@altimate-harness-bot

Copy link
Copy Markdown
Contributor

🔁 @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.

@sahrizvi sahrizvi mentioned this pull request Nov 3, 2026
7 of 8 tasks
hkaltai added a commit that referenced this pull request Nov 4, 2026
main's tree is #1609's head, which this branch already contains.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants