fix(analyst): keep write tools denied under config grants and allow reading global skill files

HK
8h ago
+1462-126

Issue for this PR

Closes #1596

Type of change

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

What does this PR do?

analyst is meant to be read-only, but two gaps were left after #1570:

  1. Config could make analyst writable. Permissions are last-match-wins and user/per-agent config is merged after analyst's own denies, so "*": "allow" (or an explicit grant) turned its write tools back on. Only bash was denied again after config.
  2. Files bundled with global skills were unreadable. analyst's "*": "deny" also overrode the default external_directory allow for skill directories, so a skill stored outside the project loaded but the files next to its SKILL.md could not be read.

Changes in packages/opencode/src/agent/agent.ts:

  • After config and safety rules, analyst denies again every built-in tool that changes state:

    • bash, sql_execute_write, edit/write/apply_patch
    • altimate_memory_write/_delete/_extract
    • training_import
    • warehouse_add/_remove/_install_driver
    • mcp_discover, datamate_manager
    • sample_setup, feedback_submit
    • data_diff, which runs caller SQL without the write check sql_execute applies
    • batch, which runs other tools without the tool-list filter

    Most of these never call ctx.ask, so removing them from the tool list is their only gate.

  • task is limited to data-explorer. Whatever config resolved for data-explorer is kept (a config deny or ask still applies). A full deny is re-applied as "*", so the tool leaves the list.

  • analyst's own rules allow external_directory for each discovered skill directory, the same list the defaults use. They sit before config, so config can still restrict them.

The skill-folder allow exposed an older gap: external_directory was checked against the path as written, so a symlink inside an allowed folder reached a denied one. Changes in packages/opencode/src/tool/external-directory.ts, src/permission/index.ts and src/util/filesystem.ts:

  • The check also asks for the directory the path really resolves to. Both variants share this: the Effect one (read, grep, write, edit, apply_patch, lsp) and the Promise one (glob, ls).
  • Filesystem.realLocation resolves symlinks one segment at a time, so link/.. goes where the kernel goes (Bun's realpath collapses .. first) and files in directories that do not exist yet resolve through their nearest real ancestor. A link whose target does not exist yet is followed too, with one budget of 40 links per resolution; past that it throws, like ELOOP.
  • The in-project early return uses that location too. Before, <project>/link/../x passed as inside the project whenever the lexical <project>/x existed.
  • Configured rules are not rewritten. The check marks the resolved directory in metadata.resolvedPatterns, and only that pattern is evaluated with each external_directory rule followed by its own resolved spelling (/tmp/* also covers /private/tmp/*, including wildcard rules and directories not created yet), so allowed folders behind symlinks keep working.
  • The path as written is decided by the configured rules alone, exactly as before, and a request needs every pattern allowed. So the resolved check can only tighten a decision: nothing is allowed that was not allowed before, while a symlink from an allowed folder into a folder that is not allowed is caught.
  • "Always" remembers only the path as written, so an approval never allows the real directory on a direct read past rules configured for it.
  • The in-project check resolves the authored parent, so a project file spelled through a link to the project (/var vs /private/var, a symlinked ~/code) stays inside, while an outside link entry itself still asks.
  • Behaviour change for every agent: a symlink inside an allowed external folder that points somewhere else now asks for (or, under analyst, is denied) that other folder.

I re-deny known write tools rather than capping analyst to an allow-list (as data-explorer is), so MCP and custom tools users add to analyst keep working. To stop new built-in write tools from slipping through, a test lists every registered built-in tool and fails if one that a wildcard grant leaves enabled for analyst is not on a reviewed list.

Docs: five pages said analyst auto-allows "safe" shell commands or used analyst in examples that grant shell; bash has been fully denied for analyst. They now describe the actual behaviour, including that config cannot lift these denies and that training entries can still be saved. The security FAQ notes that the external-directory check follows symlinks. Permission examples list the "*" rule first: rules are last-match-wins and config keeps key order, so a trailing "*" used to override the specific patterns above it.

The session tool filters retain the internal invalid error handler for analyst and other agents using "*": "deny". When a model calls an unavailable tool, the error names the original tool, so it can recover. The handler stays hidden from the advertised tool list; denied tools remain unavailable.

Not changed here, worth follow-ups (pre-existing):

  • bash resolves its file operands with path.resolve (lexical ..) and its own copy of the ancestor walk. analyst denies bash.
  • Filesystem.containsReal has the same .. and dangling-link gap for its other callers.
  • Session permissions (SDK/HTTP sessions, session/llm/request.ts, session/tools.ts, session/prompt.ts) are merged after the agent ruleset, so a session rule {"permission": "*", "pattern": "*", "action": "allow"} puts analyst's write tools back. Same last-match-wins gap at the session layer.
  • A global "*": "allow" also turns on MCP and custom tools for analyst, including ones that write (documented). Honouring only named grants or agent.analyst grants for non-built-in tools would change config precedence for analyst.
  • plan and explore can still be widened by config the same way.
  • batch builds its tool map without the calling agent's tool-list filter, for every agent.
  • An "always" approval is per instance, not per agent, so one given in builder for sql_execute_write also applies to analyst in the same process.
  • sql_explain without analyze skips the single-statement check.
  • src/util/filesystem.ts is still an export namespace Filesystem module; moving it to flat exports is a separate refactor across its callers.

How did you verify your code works?

  • bun test --timeout 90000 test/agent/analyst-data-explorer.test.ts: 31 pass. The new tests fail on main (global and per-agent grant matrix, delegation deny/ask, registry classification, global skill read).

  • Agent, permission, skill, task, registry and identity suites: pass. bun run typecheck: clean. Marker check (--strict): ok.

  • Symlink cases: test/tool/external-directory.test.ts, test/util/filesystem.test.ts and test/permission/next.test.ts cover link to a directory or a file, link/.., a new subdirectory behind a link, a dangling link, the in-project decoy case, both check variants, a link chain that would branch exponentially, and the resolved twin for literal, wildcard and not-yet-created rules. Each new test fails on the previous commit.

  • Runtime with debug agent analyst: reading and globbing inside an external skill folder work; reading or globbing through a symlink from it to an outside folder is refused.

  • Full packages/opencode suite: the only failures (MCP headers/HttpApi, json-output time-budget) fail the same way on main.

  • Runtime with debug agent against a temp project and an external skill folder:

    • With {"*": "allow"}, analyst has no write tool enabled and builder still has all of them.
    • task: "deny" removes task for analyst.
    • analyst reads a file inside the external skill folder; a file in an unrelated outside folder is rejected.
    • A direct altimate_memory_write call is refused as disabled for analyst.
    • On main, the same config enabled analyst's memory and edit tools, and the skill file read was rejected.

  • End to end through the real run CLI with a scripted model (source build and a packaged darwin-arm64 binary, plus serve), 26 scenarios compared with main:

    • With "*": "allow", main let analyst write a memory block, write a file, write memory through batch, run a general subagent, run CREATE TABLE despite the analyst SQL policy, run data_diff and save a new warehouse connection. On this branch none of those tools are offered and calls to them are refused, with nothing written.
    • Reads (skill, read, memory read, SELECT), delegation to data-explorer, builder and data-explorer tool sets are the same as on main.
    • A global skill under a separate home directory: its bundled files (including a nested one) are readable on this branch and rejected on main. An unrelated outside file and a config-denied skill folder stay rejected.

  • Fallback diagnostics: both affected test suites pass, including a streamed unavailable write call through the AI SDK. The error names write, its handler is never executed, and invalid is absent from the advertised tools. Both request preparation paths are covered. Package typecheck, formatting, tracker-leak and fork marker/branding checks pass.

Cost side effects

None

Screenshots / recordings

N/A (no 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"

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Permissions
    • Analyst mode remains read-only regardless of permission grants: shell execution and restricted built-in tools remain unavailable. Training entries and session scratch tables remain available; MCP and custom tools follow configured permissions.
    • Analyst can read skill files outside the project, and delegation remains limited to data-explorer.
  • Security
    • External-directory permission checks account for symlinks and resolved locations, including paths with segments that do not yet exist.
  • Documentation
    • Updated permission, tools, agent modes, quickstart, and security guides with Analyst restrictions, Builder examples, and rule-order guidance.