From af28cf34d60c665875374a452aaf3e2e03aeb70e Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 07:56:34 +0530 Subject: [PATCH 01/25] fix(analyst): keep write denies after config grants and allow reading global skill files A permission config such as `"*": "allow"` came after analyst's own denies and won under last-match-wins, turning memory saves, file edits, SQL writes and delegation to any subagent back on. Only bash was denied again. Analyst now re-applies all of these after config, keeping whatever config resolved for data-explorer delegation. Analyst's `"*": "deny"` also dropped the default external_directory allow for skill directories, so files bundled with a global skill stored outside the project could not be read. Those directories are allowed again. Docs: the analyst pages said safe bash commands were auto-allowed; bash has been fully denied for analyst, so they now say so. Co-Authored-By: Claude Opus 5.5 --- docs/docs/configure/agents.md | 7 +- docs/docs/data-engineering/agent-modes.md | 5 +- packages/opencode/src/agent/agent.ts | 23 ++++- .../test/agent/analyst-data-explorer.test.ts | 83 ++++++++++++++++++- 4 files changed, 112 insertions(+), 6 deletions(-) diff --git a/docs/docs/configure/agents.md b/docs/docs/configure/agents.md index a45858470c..33dfaf9c49 100644 --- a/docs/docs/configure/agents.md +++ b/docs/docs/configure/agents.md @@ -12,7 +12,7 @@ Agents define different AI personas with specific models, prompts, permissions, | Agent | Description | Access Level | |-------|------------|-------------| | `builder` | Create and modify dbt models, SQL pipelines, and data transformations | Full read/write. SQL mutations prompt for approval. | -| `analyst` | Answer questions about your data — explore it, run SELECT queries, inspect schemas, generate insights | Read-only (enforced). SQL writes denied. Safe bash commands auto-allowed. | +| `analyst` | Answer questions about your data — explore it, run SELECT queries, inspect schemas, generate insights | Read-only (enforced). SQL writes, file edits, shell and memory saves denied, even if config allows them. | | `plan` | Plan before acting — restricted to planning files only | Minimal — no edits, no bash, no SQL | ### Builder @@ -25,10 +25,11 @@ The agent for asking questions about your data. Truly read-only mode for safe da - **File access**: Read, grep, glob — no prompts - **SQL**: SELECT queries execute freely. Write queries are denied (not prompted — blocked entirely) -- **Bash**: Safe commands auto-allowed (`ls`, `grep`, `cat`, `head`, `tail`, `find`, `wc`). dbt read commands allowed (`dbt list`, `dbt ls`, `dbt debug`). Everything else denied. +- **Bash**: Denied. Use read, grep and glob to read files - **Web**: Fetch and search allowed without prompts - **Schema/warehouse/finops**: All inspection tools available -- **Workspace knowledge**: Loads workspace and project skills; reads workspace memory and views this checkout's memory audit log. Reloading memory syncs the local copy with the workspace, which can remove a local copy of a block the workspace archived. Saving memory is done with `builder`. +- **Workspace knowledge**: Loads workspace and project skills; reads workspace memory and views this checkout's memory audit log. Can read files bundled with skills, including global skills stored outside the project. Reloading memory syncs the local copy with the workspace, which can remove a local copy of a block the workspace archived. Saving memory is done with `builder`. +- **Config overrides**: A permission config such as `"*": "allow"` cannot turn file edits, SQL writes, shell, memory saves or delegation beyond `data-explorer` back on !!! tip Use `analyst` when exploring data to ensure no accidental writes. Switch to `builder` when you're ready to create or modify models. diff --git a/docs/docs/data-engineering/agent-modes.md b/docs/docs/data-engineering/agent-modes.md index 2c8b0c3fc8..fd82b122e6 100644 --- a/docs/docs/data-engineering/agent-modes.md +++ b/docs/docs/data-engineering/agent-modes.md @@ -88,7 +88,10 @@ Analyst mode enforces strict guardrails: - **SELECT only.** INSERT, UPDATE, DELETE, DROP are denied (not prompted, blocked entirely) - **No file writes.** Cannot modify your codebase -- **Safe bash only.** `ls`, `grep`, `cat`, `head`, `tail`, `find`, `wc` auto-allowed. dbt read commands allowed (`dbt list`, `dbt ls`, `dbt debug`, `dbt deps`). Everything else denied. +- **No shell.** Bash is denied. Files are read with the read, grep and glob tools +- **No memory saves.** Workspace memory can be read, but not saved, extracted or deleted (use builder) +- **Config cannot lift these.** A permission config such as `"*": "allow"` does not turn file edits, SQL writes, shell, memory saves or delegation beyond `data-explorer` back on for analyst +- **Skill files readable.** Files bundled with a skill, including global skills stored outside the project, can be read - **Full web access.** Fetch and search allowed without prompts - **All inspection tools.** Schema, warehouse, finops, lineage, and core analysis tools available diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index 91e413e8ac..1c84bc80e0 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/packages/opencode/src/agent/agent.ts @@ -334,6 +334,9 @@ export const layer = Layer.effect( question: "allow", // Skills return instruction text only (no side effects), so they are safe for a read-only agent. skill: "allow", + // "*": "deny" above also drops the default allow for skill directories, so files + // bundled with a global skill (stored outside the project) could not be read. + external_directory: Object.fromEntries(skillDirs.map((dir) => [path.join(dir, "*"), "allow"])), // Workspace memory: read, reload and view the audit log. Reloading syncs like the // load every session runs (it can drop a local copy of a block the workspace has // archived), but cannot create or edit memory. Saving and deleting stay denied, so a @@ -637,8 +640,26 @@ export const layer = Layer.effect( // Permission.disabled requires the final bash rule to deny "*". Without // it, narrower safety rules leave the tool exposed to commands that emit // no permission patterns (e.g. `>out.txt`). data-explorer is capped above. + // The other write tools are denied again too: last-match-wins would otherwise let a + // config grant such as "*": "allow" turn file edits, SQL writes, memory saves or + // broader delegation back on for the read-only agent. if (name === "analyst") { - agent.permission = Permission.merge(agent.permission, Permission.fromConfig({ bash: "deny" })) + // Keep what config resolved for data-explorer (it may deny or ask), but nothing broader. + const explorer = Permission.evaluate("task", "data-explorer", agent.permission).action + agent.permission = Permission.merge( + agent.permission, + Permission.fromConfig({ + bash: "deny", + sql_execute_write: "deny", + edit: "deny", + write: "deny", + apply_patch: "deny", + altimate_memory_write: "deny", + altimate_memory_delete: "deny", + altimate_memory_extract: "deny", + task: { "*": "deny", "data-explorer": explorer }, + }), + ) } // altimate_change end const explicit = agent.permission.some((r) => { diff --git a/packages/opencode/test/agent/analyst-data-explorer.test.ts b/packages/opencode/test/agent/analyst-data-explorer.test.ts index 72babad45d..0ac0beacb0 100644 --- a/packages/opencode/test/agent/analyst-data-explorer.test.ts +++ b/packages/opencode/test/agent/analyst-data-explorer.test.ts @@ -1,5 +1,8 @@ -import { expect } from "bun:test" +import { afterAll, expect } from "bun:test" import { Effect } from "effect" +import fs from "fs" +import os from "os" +import path from "path" import { Agent } from "../../src/agent/agent" import { deriveSubagentSessionPermission } from "../../src/agent/subagent-permissions" import { Permission } from "../../src/permission" @@ -273,3 +276,81 @@ for (const override of ["global", "agent"] as const) { }, ) } + +// Write tools a config grant must not re-enable for analyst (last-match-wins would otherwise apply). +const analystWrites = [ + "altimate_memory_write", + "altimate_memory_delete", + "altimate_memory_extract", + "edit", + "write", + "apply_patch", + "sql_execute_write", + "bash", +] +const writeGrants = Object.fromEntries(analystWrites.map((tool) => [tool, "allow" as const])) + +for (const override of ["global", "agent"] as const) { + it.instance( + `analyst keeps write and memory-save denies after ${override} wildcard and explicit grants`, + () => + Effect.gen(function* () { + const analyst = (yield* Agent.use.get("analyst"))! + for (const tool of analystWrites) expect(action(analyst, tool), tool).toBe("deny") + expect(Permission.disabled(analystWrites, analyst.permission)).toEqual(new Set(analystWrites)) + expect(action(analyst, "task", "data-explorer")).toBe("allow") + for (const subagent of ["general", "builder", "explore"]) { + expect(action(analyst, "task", subagent), subagent).toBe("deny") + } + for (const tool of ["altimate_memory_read", "altimate_memory_refresh", "altimate_memory_audit", "skill"]) { + expect(action(analyst, tool), tool).toBe("allow") + } + expect(action(analyst, "read", "models/a.sql")).toBe("allow") + expect(action(analyst, "sql_execute")).toBe("allow") + }), + { + config: + override === "global" + ? { permission: { "*": "allow", ...writeGrants, task: "allow" } } + : { agent: { analyst: { permission: { "*": "allow", ...writeGrants, task: "allow" } } } }, + }, + ) +} + +it.instance( + "analyst keeps a configured deny for delegating to data-explorer", + () => + Effect.gen(function* () { + const analyst = (yield* Agent.use.get("analyst"))! + expect(action(analyst, "task", "data-explorer")).toBe("deny") + }), + { config: { agent: { analyst: { permission: { task: { "data-explorer": "deny" } } } } } }, +) + +// A skill stored outside the project, like a global skill under the home directory. +const externalSkills = fs.mkdtempSync(path.join(os.tmpdir(), "analyst-global-skills-")) +fs.mkdirSync(path.join(externalSkills, "naming", "references"), { recursive: true }) +fs.writeFileSync( + path.join(externalSkills, "naming", "SKILL.md"), + "---\nname: naming\ndescription: Naming conventions.\n---\n\nSee references/rules.md.\n", +) +fs.writeFileSync(path.join(externalSkills, "naming", "references", "rules.md"), "Use snake_case.\n") +afterAll(() => fs.rmSync(externalSkills, { recursive: true, force: true })) + +it.instance( + "analyst can read files bundled with a skill stored outside the project", + () => + Effect.gen(function* () { + const analyst = (yield* Agent.use.get("analyst"))! + const skillDir = path.join(externalSkills, "naming") + // The read tool asks for the bundled file's directory with a trailing "/*". + for (const dir of [skillDir, path.join(skillDir, "references")]) { + expect(action(analyst, "external_directory", path.join(dir, "*")), dir).toBe("allow") + } + expect(action(analyst, "external_directory", "/some/other/path/*")).toBe("deny") + // data-explorer is unchanged: no external directory access beyond the shared truncation glob. + const de = (yield* Agent.use.get("data-explorer"))! + expect(action(de, "external_directory", path.join(skillDir, "*"))).toBe("deny") + }), + { config: { skills: { paths: [externalSkills] } } }, +) From 7ae5a8832b2bbe55acce5879df95f84dbccc18c9 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 08:01:48 +0530 Subject: [PATCH 02/25] refactor(analyst): describe the post-config write denies in the marker comment Co-Authored-By: Claude Opus 5.5 --- packages/opencode/src/agent/agent.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index 1c84bc80e0..a003964bb9 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/packages/opencode/src/agent/agent.ts @@ -334,8 +334,8 @@ export const layer = Layer.effect( question: "allow", // Skills return instruction text only (no side effects), so they are safe for a read-only agent. skill: "allow", - // "*": "deny" above also drops the default allow for skill directories, so files - // bundled with a global skill (stored outside the project) could not be read. + // "*": "deny" above also overrides the default allow for skill directories; add it back + // so files bundled with a global skill (stored outside the project) stay readable. external_directory: Object.fromEntries(skillDirs.map((dir) => [path.join(dir, "*"), "allow"])), // Workspace memory: read, reload and view the audit log. Reloading syncs like the // load every session runs (it can drop a local copy of a block the workspace has @@ -636,7 +636,7 @@ export const layer = Layer.effect( // Ensure Truncate.GLOB is allowed unless explicitly configured for (const name in agents) { const agent = agents[name] - // altimate_change start — keep the analyst shell disabled after config and safety rules + // altimate_change start — keep analyst's shell and write tools disabled after config and safety rules // Permission.disabled requires the final bash rule to deny "*". Without // it, narrower safety rules leave the tool exposed to commands that emit // no permission patterns (e.g. `>out.txt`). data-explorer is capped above. From 3a6f72dc0c836ae375fb070e86b7aeae842c9fb2 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 08:25:20 +0530 Subject: [PATCH 03/25] fix(analyst): deny every built-in write tool after config, not just file and memory writes A wildcard grant still re-enabled built-in tools that change state without asking: training imports, warehouse connection add/remove and driver installs, MCP config changes (mcp_discover, datamate_manager), sample project setup, feedback submission, and data_diff, which runs caller SQL without the write classification sql_execute applies. batch is denied too, since it runs other tools without the tool-list filter. A test now checks every registered built-in tool: any tool a wildcard grant leaves enabled for analyst must be on a reviewed read-only list, so a new write tool fails until it is classified. Delegation tests cover a config deny and ask for data-explorer from both global and agent config. Docs: training entries can still be saved by analyst; the quickstart table no longer claims analyst auto-allows safe shell commands. Co-Authored-By: Claude Opus 5.5 --- docs/docs/configure/agents.md | 4 +- docs/docs/data-engineering/agent-modes.md | 4 +- docs/docs/getting-started/quickstart.md | 2 +- packages/opencode/src/agent/agent.ts | 39 +++-- .../test/agent/analyst-data-explorer.test.ts | 143 +++++++++++++++++- 5 files changed, 171 insertions(+), 21 deletions(-) diff --git a/docs/docs/configure/agents.md b/docs/docs/configure/agents.md index 33dfaf9c49..9111b99195 100644 --- a/docs/docs/configure/agents.md +++ b/docs/docs/configure/agents.md @@ -12,7 +12,7 @@ Agents define different AI personas with specific models, prompts, permissions, | Agent | Description | Access Level | |-------|------------|-------------| | `builder` | Create and modify dbt models, SQL pipelines, and data transformations | Full read/write. SQL mutations prompt for approval. | -| `analyst` | Answer questions about your data — explore it, run SELECT queries, inspect schemas, generate insights | Read-only (enforced). SQL writes, file edits, shell and memory saves denied, even if config allows them. | +| `analyst` | Answer questions about your data — explore it, run SELECT queries, inspect schemas, generate insights | Read-only (enforced). SQL writes, file edits, shell, memory saves and other write tools denied, even if config allows them. | | `plan` | Plan before acting — restricted to planning files only | Minimal — no edits, no bash, no SQL | ### Builder @@ -29,7 +29,7 @@ The agent for asking questions about your data. Truly read-only mode for safe da - **Web**: Fetch and search allowed without prompts - **Schema/warehouse/finops**: All inspection tools available - **Workspace knowledge**: Loads workspace and project skills; reads workspace memory and views this checkout's memory audit log. Can read files bundled with skills, including global skills stored outside the project. Reloading memory syncs the local copy with the workspace, which can remove a local copy of a block the workspace archived. Saving memory is done with `builder`. -- **Config overrides**: A permission config such as `"*": "allow"` cannot turn file edits, SQL writes, shell, memory saves or delegation beyond `data-explorer` back on +- **Config overrides**: A permission config such as `"*": "allow"` cannot turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on !!! tip Use `analyst` when exploring data to ensure no accidental writes. Switch to `builder` when you're ready to create or modify models. diff --git a/docs/docs/data-engineering/agent-modes.md b/docs/docs/data-engineering/agent-modes.md index fd82b122e6..6b2402c8c5 100644 --- a/docs/docs/data-engineering/agent-modes.md +++ b/docs/docs/data-engineering/agent-modes.md @@ -89,8 +89,8 @@ Analyst mode enforces strict guardrails: - **SELECT only.** INSERT, UPDATE, DELETE, DROP are denied (not prompted, blocked entirely) - **No file writes.** Cannot modify your codebase - **No shell.** Bash is denied. Files are read with the read, grep and glob tools -- **No memory saves.** Workspace memory can be read, but not saved, extracted or deleted (use builder) -- **Config cannot lift these.** A permission config such as `"*": "allow"` does not turn file edits, SQL writes, shell, memory saves or delegation beyond `data-explorer` back on for analyst +- **No memory saves.** Workspace memory blocks can be read, but not saved, extracted or deleted (use builder). Training entries can still be saved +- **Config cannot lift these.** A permission config such as `"*": "allow"` does not turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on for analyst - **Skill files readable.** Files bundled with a skill, including global skills stored outside the project, can be read - **Full web access.** Fetch and search allowed without prompts - **All inspection tools.** Schema, warehouse, finops, lineage, and core analysis tools available diff --git a/docs/docs/getting-started/quickstart.md b/docs/docs/getting-started/quickstart.md index 2d2ec7ad1a..4df10880f9 100644 --- a/docs/docs/getting-started/quickstart.md +++ b/docs/docs/getting-started/quickstart.md @@ -274,7 +274,7 @@ Define project-wide conventions in an `AGENTS.md` file at your project root. The | Agent | File writes | SQL writes | Bash | Training | | ------- | ----------- | -------------------------- | --------------------------------- | -------- | | Builder | allow | ask (prompts for approval) | ask | allow | -| Analyst | deny | deny (blocked entirely) | deny (safe commands auto-allowed) | allow | +| Analyst | deny | deny (blocked entirely) | deny | allow | | Plan | deny | deny | deny | deny | --- diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index a003964bb9..a81ef75344 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/packages/opencode/src/agent/agent.ts @@ -640,23 +640,40 @@ export const layer = Layer.effect( // Permission.disabled requires the final bash rule to deny "*". Without // it, narrower safety rules leave the tool exposed to commands that emit // no permission patterns (e.g. `>out.txt`). data-explorer is capped above. - // The other write tools are denied again too: last-match-wins would otherwise let a - // config grant such as "*": "allow" turn file edits, SQL writes, memory saves or - // broader delegation back on for the read-only agent. + // The built-in tools that change files, memory, config, connections or external + // systems are denied again too: last-match-wins would otherwise let a config grant + // such as "*": "allow" turn them back on for the read-only agent. Most of them never + // call ctx.ask, so removing them from the tool list is their only gate. batch is + // denied because it runs other tools without that tool-list filter. if (name === "analyst") { // Keep what config resolved for data-explorer (it may deny or ask), but nothing broader. const explorer = Permission.evaluate("task", "data-explorer", agent.permission).action agent.permission = Permission.merge( agent.permission, Permission.fromConfig({ - bash: "deny", - sql_execute_write: "deny", - edit: "deny", - write: "deny", - apply_patch: "deny", - altimate_memory_write: "deny", - altimate_memory_delete: "deny", - altimate_memory_extract: "deny", + ...Object.fromEntries( + [ + "bash", + "sql_execute_write", + "edit", + "write", + "apply_patch", + "altimate_memory_write", + "altimate_memory_delete", + "altimate_memory_extract", + "training_import", + "warehouse_add", + "warehouse_remove", + "warehouse_install_driver", + "mcp_discover", + "datamate_manager", + "sample_setup", + "feedback_submit", + // Runs caller-supplied SQL without the write classification sql_execute applies. + "data_diff", + "batch", + ].map((tool) => [tool, "deny" as const]), + ), task: { "*": "deny", "data-explorer": explorer }, }), ) diff --git a/packages/opencode/test/agent/analyst-data-explorer.test.ts b/packages/opencode/test/agent/analyst-data-explorer.test.ts index 0ac0beacb0..3b3b02f596 100644 --- a/packages/opencode/test/agent/analyst-data-explorer.test.ts +++ b/packages/opencode/test/agent/analyst-data-explorer.test.ts @@ -1,11 +1,14 @@ import { afterAll, expect } from "bun:test" import { Effect } from "effect" +import { LayerNode } from "@opencode-ai/core/effect/layer-node" import fs from "fs" import os from "os" import path from "path" import { Agent } from "../../src/agent/agent" import { deriveSubagentSessionPermission } from "../../src/agent/subagent-permissions" import { Permission } from "../../src/permission" +import { RuntimeFlags } from "../../src/effect/runtime-flags" +import { ToolRegistry } from "../../src/tool/registry" import { testEffect } from "../lib/effect" const it = testEffect(Agent.defaultLayer) @@ -287,6 +290,16 @@ const analystWrites = [ "apply_patch", "sql_execute_write", "bash", + "training_import", + "warehouse_add", + "warehouse_remove", + "warehouse_install_driver", + "mcp_discover", + "datamate_manager", + "sample_setup", + "feedback_submit", + "data_diff", + "batch", ] const writeGrants = Object.fromEntries(analystWrites.map((tool) => [tool, "allow" as const])) @@ -317,14 +330,134 @@ for (const override of ["global", "agent"] as const) { ) } -it.instance( - "analyst keeps a configured deny for delegating to data-explorer", +for (const configured of ["deny", "ask"] as const) { + for (const override of ["global", "agent"] as const) { + const task = { "*": "allow" as const, "data-explorer": configured } + it.instance( + `analyst keeps ${override === "agent" ? "an agent" : "a global"} config ${configured} for delegating to data-explorer`, + () => + Effect.gen(function* () { + const analyst = (yield* Agent.use.get("analyst"))! + expect(action(analyst, "task", "data-explorer")).toBe(configured) + expect(action(analyst, "task", "general")).toBe("deny") + }), + { + config: override === "global" ? { permission: { task } } : { agent: { analyst: { permission: { task } } } }, + }, + ) + } +} + +// Every registered built-in tool that a wildcard grant leaves enabled for analyst must be a +// reviewed read-only tool. A new tool that changes files, memory, config, connections or +// external systems fails here until it is added to analyst's post-config denies. +const analystConfigEnabled = new Set([ + ...[ + "check", + "classify_pii", + "column_lineage", + "compare", + "complete", + "correct", + "equivalence", + "export_ddl", + "extract_metadata", + "fingerprint", + "fix", + "grade", + "import_ddl", + "introspection_sql", + "migration", + "optimize_context", + "parse_dbt", + "policy", + "prune_schema", + "query_pii", + "resolve_term", + "rewrite", + "schema_diff", + "semantics", + "testgen", + "track_lineage", + "validate", + ].map((name) => `altimate_core_${name}`), + "altimate_memory_audit", + "altimate_memory_read", + "altimate_memory_refresh", + "answer_commit", + "codesearch", + "dbt_lineage", + "dbt_manifest", + "dbt_pr_review", + "dbt_profiles", + "dbt_unit_test_gen", + "finops_analyze_credits", + "finops_expensive_queries", + "finops_query_history", + "finops_role_grants", + "finops_role_hierarchy", + "finops_unused_resources", + "finops_user_roles", + "finops_warehouse_advice", + "glob", + "grep", + "impact_analysis", + "invalid", + "lineage_check", + "project_scan", + "question", + "read", + "schema_cache_status", + "schema_detect_pii", + "schema_diff", + "schema_index", + "schema_inspect", + "schema_search", + "schema_tags", + "schema_tags_list", + "skill", + "sql_analyze", + "sql_autocomplete", + "sql_check", + "sql_diff", + "sql_execute", + "sql_explain", + "sql_fix", + "sql_format", + "sql_materialize", + "sql_optimize", + "sql_rewrite", + "sql_translate", + "task", + "todowrite", + "tool_lookup", + "training_list", + "training_remove", + "training_save", + "warehouse_discover", + "warehouse_list", + "warehouse_test", + "webfetch", + "websearch", +]) +const withRegistry = testEffect( + LayerNode.buildLayer(LayerNode.group([ToolRegistry.node, Agent.node]), { + replacements: [LayerNode.replace(RuntimeFlags.node, RuntimeFlags.layer())], + }), +) + +withRegistry.instance( + "a wildcard grant enables only reviewed read-only built-in tools for analyst", () => Effect.gen(function* () { - const analyst = (yield* Agent.use.get("analyst"))! - expect(action(analyst, "task", "data-explorer")).toBe("deny") + const ids = yield* (yield* ToolRegistry.Service).ids() + const analyst = (yield* Agent.Service.use((svc) => svc.get("analyst")))! + expect(ids).toContain("batch") + const disabled = Permission.disabled(ids, analyst.permission) + expect(ids.filter((id) => !disabled.has(id) && !analystConfigEnabled.has(id))).toEqual([]) + for (const tool of analystWrites.filter((tool) => ids.includes(tool))) expect(disabled.has(tool), tool).toBe(true) }), - { config: { agent: { analyst: { permission: { task: { "data-explorer": "deny" } } } } } }, + { config: { permission: { "*": "allow" }, experimental: { batch_tool: true } } }, ) // A skill stored outside the project, like a global skill under the home directory. From 5857f4926a61ed56d9e66ef4f50fc035d5337847 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 08:40:16 +0530 Subject: [PATCH 04/25] fix(analyst): drop the task tool when config denies all delegation When config denied task entirely, the re-applied rule ended with a data-explorer pattern, so the task tool stayed in analyst's tool list with no subagent it could start. A full deny is now re-applied as a "*" deny. Tests: config can still deny a skill folder outside the project, task is dropped when global or agent config denies it, and the registry check classifies flag-gated tools so it does not depend on the environment. Docs: permission examples no longer grant analyst shell commands, which its post-config deny has always overridden. Co-Authored-By: Claude Opus 5.5 --- docs/docs/configure/permissions.md | 15 +++---- docs/docs/configure/tools.md | 6 +-- packages/opencode/src/agent/agent.ts | 3 +- .../test/agent/analyst-data-explorer.test.ts | 44 +++++++++++++++++-- 4 files changed, 51 insertions(+), 17 deletions(-) diff --git a/docs/docs/configure/permissions.md b/docs/docs/configure/permissions.md index f381029b8d..0c476bf300 100644 --- a/docs/docs/configure/permissions.md +++ b/docs/docs/configure/permissions.md @@ -65,7 +65,7 @@ Override permissions for specific agents: ```json { "agent": { - "analyst": { + "builder": { "permission": { "write": "deny", "edit": "deny", @@ -80,6 +80,8 @@ Override permissions for specific agents: } ``` +The `analyst` agent is read-only regardless of config: its shell and write tools (file edits, SQL writes, memory saves and other built-in write tools) stay denied even if its permission config allows them. Config can still restrict analyst further. + ## All Permissioned Tools | Tool | Supports Patterns | Description | @@ -209,15 +211,8 @@ Give each agent only the permissions it needs: "agent": { "analyst": { "permission": { - "write": "deny", - "edit": "deny", - "sql_execute_write": "deny", - "bash": { - "*": "deny", - "ls *": "allow", - "cat *": "allow", - "dbt list *": "allow" - } + "webfetch": "deny", + "websearch": "deny" } }, "builder": { diff --git a/docs/docs/configure/tools.md b/docs/docs/configure/tools.md index 2690c1f99c..ec68051889 100644 --- a/docs/docs/configure/tools.md +++ b/docs/docs/configure/tools.md @@ -58,7 +58,7 @@ Disable a tool for a specific agent by setting its permission to `"deny"`: ```json { "agent": { - "analyst": { + "builder": { "permission": { "write": "deny", "edit": "deny", @@ -72,8 +72,8 @@ Disable a tool for a specific agent by setting its permission to `"deny"`: } ``` -!!! example "Read-only analyst" - The configuration above creates an analyst agent that cannot modify files. It can only read and explore the codebase, and must ask before running shell commands (except `dbt run`, which is blocked entirely). +!!! example "Read-only builder" + The configuration above makes the builder agent unable to modify files. It can only read and explore the codebase, and must ask before running shell commands (except `dbt run`, which is blocked entirely). The `analyst` agent is already read-only: its shell and write tools stay denied whatever its permission config says. ## Tool Behavior diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index a81ef75344..13ebdee670 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/packages/opencode/src/agent/agent.ts @@ -674,7 +674,8 @@ export const layer = Layer.effect( "batch", ].map((tool) => [tool, "deny" as const]), ), - task: { "*": "deny", "data-explorer": explorer }, + // A trailing "*" deny is what removes task from the tool list when nothing is allowed. + task: explorer === "deny" ? "deny" : { "*": "deny", "data-explorer": explorer }, }), ) } diff --git a/packages/opencode/test/agent/analyst-data-explorer.test.ts b/packages/opencode/test/agent/analyst-data-explorer.test.ts index 3b3b02f596..984576efe2 100644 --- a/packages/opencode/test/agent/analyst-data-explorer.test.ts +++ b/packages/opencode/test/agent/analyst-data-explorer.test.ts @@ -330,6 +330,24 @@ for (const override of ["global", "agent"] as const) { ) } +for (const override of ["global", "agent"] as const) { + it.instance( + `analyst drops the task tool when ${override} config denies all delegation`, + () => + Effect.gen(function* () { + const analyst = (yield* Agent.use.get("analyst"))! + expect(action(analyst, "task", "data-explorer")).toBe("deny") + expect(Permission.disabled(["task"], analyst.permission)).toEqual(new Set(["task"])) + }), + { + config: + override === "global" + ? { permission: { task: "deny" } } + : { agent: { analyst: { permission: { task: "deny" } } } }, + }, + ) +} + for (const configured of ["deny", "ask"] as const) { for (const override of ["global", "agent"] as const) { const task = { "*": "allow" as const, "data-explorer": configured } @@ -348,9 +366,12 @@ for (const configured of ["deny", "ask"] as const) { } } -// Every registered built-in tool that a wildcard grant leaves enabled for analyst must be a -// reviewed read-only tool. A new tool that changes files, memory, config, connections or -// external systems fails here until it is added to analyst's post-config denies. +// Every registered built-in tool that a wildcard grant leaves enabled for analyst must be on this +// reviewed list: read-only tools, plus the local writes analyst is designed to make (training +// entries, scratch tables, the schema index, session todos). A new tool that changes files, +// workspace memory, config, connections or external systems fails here until it is added to +// analyst's post-config denies. Flag-gated tools are listed too, so the check does not depend +// on the environment. const analystConfigEnabled = new Set([ ...[ "check", @@ -404,6 +425,8 @@ const analystConfigEnabled = new Set([ "impact_analysis", "invalid", "lineage_check", + "lsp", + "plan_exit", "project_scan", "question", "read", @@ -487,3 +510,18 @@ it.instance( }), { config: { skills: { paths: [externalSkills] } } }, ) + +it.instance( + "config can still deny analyst access to a skill folder outside the project", + () => + Effect.gen(function* () { + const analyst = (yield* Agent.use.get("analyst"))! + expect(action(analyst, "external_directory", path.join(externalSkills, "naming", "*"))).toBe("deny") + }), + { + config: () => ({ + skills: { paths: [externalSkills] }, + agent: { analyst: { permission: { external_directory: { [path.join(externalSkills, "*")]: "deny" } } } }, + }), + }, +) From 45f6da7d2280d655ffc96ad213add322221ac979 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 09:02:49 +0530 Subject: [PATCH 05/25] docs(analyst): state exactly what the permission examples deny The builder example in the tools page denies only the write and edit tools, so it is no longer called read-only. The quickstart example no longer grants analyst a shell command its post-config deny always overrides. The agent table names the writes analyst keeps: training entries and session scratch tables. The skill-folder test title now says it checks the permission. Co-Authored-By: Claude Opus 5.5 --- docs/docs/configure/agents.md | 2 +- docs/docs/configure/tools.md | 4 ++-- docs/docs/getting-started/quickstart.md | 9 +++------ .../opencode/test/agent/analyst-data-explorer.test.ts | 2 +- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/docs/docs/configure/agents.md b/docs/docs/configure/agents.md index 9111b99195..f1237866bc 100644 --- a/docs/docs/configure/agents.md +++ b/docs/docs/configure/agents.md @@ -12,7 +12,7 @@ Agents define different AI personas with specific models, prompts, permissions, | Agent | Description | Access Level | |-------|------------|-------------| | `builder` | Create and modify dbt models, SQL pipelines, and data transformations | Full read/write. SQL mutations prompt for approval. | -| `analyst` | Answer questions about your data — explore it, run SELECT queries, inspect schemas, generate insights | Read-only (enforced). SQL writes, file edits, shell, memory saves and other write tools denied, even if config allows them. | +| `analyst` | Answer questions about your data — explore it, run SELECT queries, inspect schemas, generate insights | Read-only (enforced). SQL writes, file edits, shell, memory saves and other write tools denied, even if config allows them. Training entries and session scratch tables are still allowed. | | `plan` | Plan before acting — restricted to planning files only | Minimal — no edits, no bash, no SQL | ### Builder diff --git a/docs/docs/configure/tools.md b/docs/docs/configure/tools.md index ec68051889..2b4e173972 100644 --- a/docs/docs/configure/tools.md +++ b/docs/docs/configure/tools.md @@ -72,8 +72,8 @@ Disable a tool for a specific agent by setting its permission to `"deny"`: } ``` -!!! example "Read-only builder" - The configuration above makes the builder agent unable to modify files. It can only read and explore the codebase, and must ask before running shell commands (except `dbt run`, which is blocked entirely). The `analyst` agent is already read-only: its shell and write tools stay denied whatever its permission config says. +!!! example "Builder with file edit tools denied" + The configuration above denies the builder agent's `write` and `edit` tools. The builder can read and explore the codebase. It must ask before running other shell commands, and `dbt run` is blocked entirely. An approved shell command can still modify files. The `analyst` agent is already read-only: its shell and write tools stay denied whatever its permission config says. It can still save training entries and create session scratch tables for analysis. ## Tool Behavior diff --git a/docs/docs/getting-started/quickstart.md b/docs/docs/getting-started/quickstart.md index 4df10880f9..8aaf352218 100644 --- a/docs/docs/getting-started/quickstart.md +++ b/docs/docs/getting-started/quickstart.md @@ -234,12 +234,7 @@ Set tool permissions for each agent mode in `altimate-code.json`: "agent": { "analyst": { "permission": { - "write": "deny", - "edit": "deny", - "bash": { - "dbt docs generate": "allow", - "*": "deny" - } + "webfetch": "deny" } }, "builder": { @@ -256,6 +251,8 @@ Set tool permissions for each agent mode in `altimate-code.json`: } ``` +Analyst's shell and write tools are always denied, so config can only restrict analyst further. + ### Project rules with AGENTS.md Define project-wide conventions in an `AGENTS.md` file at your project root. These rules are automatically loaded into every agent's system prompt: diff --git a/packages/opencode/test/agent/analyst-data-explorer.test.ts b/packages/opencode/test/agent/analyst-data-explorer.test.ts index 984576efe2..b692eaef40 100644 --- a/packages/opencode/test/agent/analyst-data-explorer.test.ts +++ b/packages/opencode/test/agent/analyst-data-explorer.test.ts @@ -494,7 +494,7 @@ fs.writeFileSync(path.join(externalSkills, "naming", "references", "rules.md"), afterAll(() => fs.rmSync(externalSkills, { recursive: true, force: true })) it.instance( - "analyst can read files bundled with a skill stored outside the project", + "analyst allows external_directory access to files bundled with a skill stored outside the project", () => Effect.gen(function* () { const analyst = (yield* Agent.use.get("analyst"))! From a86cb9050e5deefd74b4b006da0515073046e3b4 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 09:19:08 +0530 Subject: [PATCH 06/25] fix(permission): check the symlink-resolved directory for external_directory access An allowed external directory could hold a symlink to a directory that is otherwise denied, letting reads follow it. The external_directory check now also asks for the directory the path resolves to, and external_directory rules from config cover their resolved spelling so symlinked allowed directories keep working. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/docs/data-engineering/agent-modes.md | 2 +- packages/opencode/src/agent/agent.ts | 4 +- packages/opencode/src/permission/index.ts | 34 +++++++++++++-- .../opencode/src/tool/external-directory.ts | 41 ++++++++++++++++--- .../test/agent/analyst-data-explorer.test.ts | 36 ++++++++++++++++ .../opencode/test/permission/next.test.ts | 22 ++++++++++ .../test/tool/external-directory.test.ts | 30 ++++++++++++++ 7 files changed, 157 insertions(+), 12 deletions(-) diff --git a/docs/docs/data-engineering/agent-modes.md b/docs/docs/data-engineering/agent-modes.md index 6b2402c8c5..50ee53e2ac 100644 --- a/docs/docs/data-engineering/agent-modes.md +++ b/docs/docs/data-engineering/agent-modes.md @@ -89,7 +89,7 @@ Analyst mode enforces strict guardrails: - **SELECT only.** INSERT, UPDATE, DELETE, DROP are denied (not prompted, blocked entirely) - **No file writes.** Cannot modify your codebase - **No shell.** Bash is denied. Files are read with the read, grep and glob tools -- **No memory saves.** Workspace memory blocks can be read, but not saved, extracted or deleted (use builder). Training entries can still be saved +- **No memory saves.** Workspace memory blocks can be read, but not saved, extracted or deleted (use builder). Training entries can still be saved and removed - **Config cannot lift these.** A permission config such as `"*": "allow"` does not turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on for analyst - **Skill files readable.** Files bundled with a skill, including global skills stored outside the project, can be read - **Full web access.** Fetch and search allowed without prompts diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index 13ebdee670..0884f50bbc 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/packages/opencode/src/agent/agent.ts @@ -647,7 +647,7 @@ export const layer = Layer.effect( // denied because it runs other tools without that tool-list filter. if (name === "analyst") { // Keep what config resolved for data-explorer (it may deny or ask), but nothing broader. - const explorer = Permission.evaluate("task", "data-explorer", agent.permission).action + const dataExplorerAction = Permission.evaluate("task", "data-explorer", agent.permission).action agent.permission = Permission.merge( agent.permission, Permission.fromConfig({ @@ -675,7 +675,7 @@ export const layer = Layer.effect( ].map((tool) => [tool, "deny" as const]), ), // A trailing "*" deny is what removes task from the tool list when nothing is allowed. - task: explorer === "deny" ? "deny" : { "*": "deny", "data-explorer": explorer }, + task: dataExplorerAction === "deny" ? "deny" : { "*": "deny", "data-explorer": dataExplorerAction }, }), ) } diff --git a/packages/opencode/src/permission/index.ts b/packages/opencode/src/permission/index.ts index 786d4f013a..76dbce8bb6 100644 --- a/packages/opencode/src/permission/index.ts +++ b/packages/opencode/src/permission/index.ts @@ -4,6 +4,11 @@ import { InstanceState } from "@/effect/instance-state" import { Wildcard } from "@opencode-ai/core/util/wildcard" import { Deferred, Effect, Layer, Context } from "effect" import os from "os" +// altimate_change start — resolve symlinked external_directory rule paths +import path from "path" +import { realpathSync } from "fs" +import { FSUtil } from "@opencode-ai/core/fs-util" +// altimate_change end import { PermissionV1 } from "@opencode-ai/core/v1/permission" import { EventV2Bridge } from "@/event-v2-bridge" import { EventV2 } from "@opencode-ai/core/event" @@ -210,13 +215,36 @@ export function fromConfig(permission: ConfigPermissionV1.Info) { ruleset.push({ permission: key, action: value, pattern: "*" }) continue } - ruleset.push( - ...Object.entries(value).map(([pattern, action]) => ({ permission: key, pattern: expand(pattern), action })), - ) + // altimate_change start — external_directory also asks for a path's symlink-resolved directory, + // so a directory rule covers its resolved spelling too (e.g. macOS /tmp -> /private/tmp) + for (const [pattern, action] of Object.entries(value)) { + ruleset.push({ permission: key, pattern: expand(pattern), action }) + const resolved = key === "external_directory" ? resolvedDirectoryPattern(expand(pattern)) : undefined + if (resolved) ruleset.push({ permission: key, pattern: resolved, action }) + } + // altimate_change end } return ruleset } +// altimate_change start — resolved twin of a literal "/*" external_directory pattern +function resolvedDirectoryPattern(pattern: string) { + const suffix = [`${path.sep}*`, "/*"].find((end) => pattern.endsWith(end)) + if (!suffix) return + const dir = pattern.slice(0, -suffix.length) + if (/[*?]/.test(dir)) return + try { + const resolved = realpathSync(dir) + if (resolved === dir) return + return process.platform === "win32" + ? FSUtil.normalizePathPattern(path.join(resolved, "*")) + : path.join(resolved, "*") + } catch { + return + } +} +// altimate_change end + export function merge(...rulesets: PermissionV1.Ruleset[]): PermissionV1.Rule[] { return rulesets.flat() } diff --git a/packages/opencode/src/tool/external-directory.ts b/packages/opencode/src/tool/external-directory.ts index 844d8961d5..093972848b 100644 --- a/packages/opencode/src/tool/external-directory.ts +++ b/packages/opencode/src/tool/external-directory.ts @@ -1,4 +1,7 @@ import path from "path" +// altimate_change start — resolve symlinked external_directory targets +import { realpathSync } from "fs" +// altimate_change end import { Effect } from "effect" import { InstanceState } from "@/effect/instance-state" import type * as Tool from "./tool" @@ -19,6 +22,29 @@ type Options = { kind?: Kind } +// altimate_change start — symlink-resolved directory of an external_directory target +function directoryGlob(dir: string) { + return process.platform === "win32" + ? FSUtil.normalizePathPattern(path.join(dir, "*")) + : path.join(dir, "*").replaceAll("\\", "/") +} + +function resolvedDirectory(full: string, kind: Kind) { + try { + const real = realpathSync(full) + return kind === "directory" ? real : path.dirname(real) + } catch { + if (kind === "directory") return + // A file that does not exist yet (a write target): resolve the directory it would land in. + try { + return realpathSync(path.dirname(full)) + } catch { + return + } + } +} +// altimate_change end + export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirectory")(function* ( ctx: Tool.Context, target?: string, @@ -34,15 +60,18 @@ export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirec const kind = options?.kind ?? "file" const dir = kind === "directory" ? full : path.dirname(full) - const glob = - process.platform === "win32" - ? FSUtil.normalizePathPattern(path.join(dir, "*")) - : path.join(dir, "*").replaceAll("\\", "/") + // altimate_change start — an allowed directory can hold a symlink to somewhere else, so also ask + // for the directory the path really resolves to (unless that is inside the project) + const glob = directoryGlob(dir) + const real = resolvedDirectory(full, kind) + const realGlob = real && directoryGlob(real) + const patterns = realGlob && realGlob !== glob && !containsPath(real, ins) ? [glob, realGlob] : [glob] yield* ctx.ask({ permission: "external_directory", - patterns: [glob], - always: [glob], + patterns, + always: patterns, + // altimate_change end metadata: { filepath: full, parentDir: dir, diff --git a/packages/opencode/test/agent/analyst-data-explorer.test.ts b/packages/opencode/test/agent/analyst-data-explorer.test.ts index b692eaef40..47ae7b7bf0 100644 --- a/packages/opencode/test/agent/analyst-data-explorer.test.ts +++ b/packages/opencode/test/agent/analyst-data-explorer.test.ts @@ -9,6 +9,9 @@ import { deriveSubagentSessionPermission } from "../../src/agent/subagent-permis import { Permission } from "../../src/permission" import { RuntimeFlags } from "../../src/effect/runtime-flags" import { ToolRegistry } from "../../src/tool/registry" +import { assertExternalDirectoryEffect } from "../../src/tool/external-directory" +import { MessageID, SessionID } from "../../src/session/schema" +import type { Tool } from "../../src/tool/tool" import { testEffect } from "../lib/effect" const it = testEffect(Agent.defaultLayer) @@ -525,3 +528,36 @@ it.instance( }), }, ) + +// A symlink inside a skill folder must not reach a directory analyst is otherwise denied. +const elsewhere = fs.mkdtempSync(path.join(os.tmpdir(), "analyst-elsewhere-")) +fs.writeFileSync(path.join(elsewhere, "secret.txt"), "x") +if (process.platform !== "win32") fs.symlinkSync(elsewhere, path.join(externalSkills, "naming", "linked")) +afterAll(() => fs.rmSync(elsewhere, { recursive: true, force: true })) + +if (process.platform !== "win32") { + it.instance( + "analyst cannot read through a symlink in a skill folder that points outside it", + () => + Effect.gen(function* () { + const analyst = (yield* Agent.use.get("analyst"))! + const asked: (readonly string[])[] = [] + const ctx = { + sessionID: SessionID.make("ses_test"), + messageID: MessageID.make("msg_test"), + callID: "", + agent: "analyst", + abort: AbortSignal.any([]), + messages: [], + metadata: () => Effect.void, + ask: (req) => Effect.sync(() => void asked.push(req.patterns)), + } satisfies Tool.Context + yield* assertExternalDirectoryEffect(ctx, path.join(externalSkills, "naming", "linked", "secret.txt")) + const [authored, resolved] = asked[0]! + expect(action(analyst, "external_directory", authored!)).toBe("allow") + expect(resolved).toBe(path.join(fs.realpathSync(elsewhere), "*")) + expect(action(analyst, "external_directory", resolved!)).toBe("deny") + }), + { config: { skills: { paths: [externalSkills] } } }, + ) +} diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index 1a90ae9383..884c5ce5ca 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/packages/opencode/test/permission/next.test.ts @@ -1,6 +1,8 @@ import { PermissionV1 } from "@opencode-ai/core/v1/permission" import { test, expect } from "bun:test" +import fs from "fs" import os from "os" +import path from "path" import { Cause, Deferred, Effect, Exit, Fiber, Layer } from "effect" import { EventV2Bridge } from "../../src/event-v2-bridge" import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" @@ -119,6 +121,26 @@ test("fromConfig - expands tilde to home directory", () => { expect(result).toEqual([{ permission: "external_directory", pattern: `${os.homedir()}/projects/*`, action: "allow" }]) }) +test.skipIf(process.platform === "win32")( + "fromConfig - external_directory rule also covers the symlink-resolved directory", + () => { + const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "permission-resolved-"))) + try { + fs.mkdirSync(path.join(root, "real")) + fs.symlinkSync(path.join(root, "real"), path.join(root, "link")) + expect(Permission.fromConfig({ external_directory: { [path.join(root, "link", "*")]: "allow" } })).toEqual([ + { permission: "external_directory", pattern: path.join(root, "link", "*"), action: "allow" }, + { permission: "external_directory", pattern: path.join(root, "real", "*"), action: "allow" }, + ]) + // Other permissions and non-symlinked directories get no extra rule. + expect(Permission.fromConfig({ read: { [path.join(root, "link", "*")]: "allow" } })).toHaveLength(1) + expect(Permission.fromConfig({ external_directory: { [path.join(root, "real", "*")]: "allow" } })).toHaveLength(1) + } finally { + fs.rmSync(root, { recursive: true, force: true }) + } + }, +) + test("fromConfig - expands $HOME to home directory", () => { const result = Permission.fromConfig({ external_directory: { "$HOME/projects/*": "allow" } }) expect(result).toEqual([{ permission: "external_directory", pattern: `${os.homedir()}/projects/*`, action: "allow" }]) diff --git a/packages/opencode/test/tool/external-directory.test.ts b/packages/opencode/test/tool/external-directory.test.ts index 69a48bad7a..5c1da6f85b 100644 --- a/packages/opencode/test/tool/external-directory.test.ts +++ b/packages/opencode/test/tool/external-directory.test.ts @@ -1,5 +1,6 @@ import { PermissionV1 } from "@opencode-ai/core/v1/permission" import { describe, expect } from "bun:test" +import fs from "fs" import path from "path" import { Effect } from "effect" import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" @@ -104,6 +105,35 @@ describe("tool.assertExternalDirectory", () => { }), ) + if (process.platform !== "win32") { + it.instance("also asks for the resolved directory of a symlink that points elsewhere", () => + Effect.gen(function* () { + const outerTmp = fs.realpathSync(yield* tmpdirScoped()) + const allowed = path.join(outerTmp, "allowed") + const elsewhere = path.join(outerTmp, "elsewhere") + fs.mkdirSync(allowed) + fs.mkdirSync(elsewhere) + fs.writeFileSync(path.join(elsewhere, "secret.txt"), "x") + fs.symlinkSync(elsewhere, path.join(allowed, "linked-dir")) + fs.symlinkSync(path.join(elsewhere, "secret.txt"), path.join(allowed, "linked-file.txt")) + + for (const target of [path.join(allowed, "linked-dir", "secret.txt"), path.join(allowed, "linked-file.txt")]) { + const { requests, ctx } = makeCtx() + yield* assertExternalDirectoryEffect(ctx, target) + const expected = [glob(path.join(path.dirname(target), "*")), glob(path.join(elsewhere, "*"))] + expect(requests[0]!.patterns, target).toEqual(expected) + expect(requests[0]!.always, target).toEqual(expected) + } + + // A plain file in the allowed directory asks only for that directory. + fs.writeFileSync(path.join(allowed, "plain.txt"), "x") + const { requests, ctx } = makeCtx() + yield* assertExternalDirectoryEffect(ctx, path.join(allowed, "plain.txt")) + expect(requests[0]!.patterns).toEqual([glob(path.join(allowed, "*"))]) + }), + ) + } + if (process.platform === "win32") { it.instance( "normalizes Windows path variants to one glob", From 4555a8b9bed69f3b5e63369d66821d0ac18e853d Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 09:45:12 +0530 Subject: [PATCH 07/25] docs(security): note that external_directory checks follow symlinks Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/docs/reference/security-faq.md | 2 +- packages/opencode/test/permission/next.test.ts | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/docs/docs/reference/security-faq.md b/docs/docs/reference/security-faq.md index 720e460b22..1af82ca67f 100644 --- a/docs/docs/reference/security-faq.md +++ b/docs/docs/reference/security-faq.md @@ -300,7 +300,7 @@ For additional safety: Altimate Code includes several layers of protection to keep the agent within your project: -- **Project boundary enforcement.** File operations check that paths stay within your project directory (or git worktree for monorepos). Attempts to read or write outside the project trigger an `external_directory` permission prompt. +- **Project boundary enforcement.** File operations check that paths stay within your project directory (or git worktree for monorepos). Attempts to read or write outside the project trigger an `external_directory` permission prompt. A path that goes through a symlink is also checked against the directory it resolves to, so an allowed directory cannot be used to reach a denied one. - **Symlink-aware path resolution.** Symlinks inside the project that point outside are detected and blocked. This prevents an agent from reading or writing outside your project through symlinks. - **Path traversal blocking.** Paths containing `../` sequences that would escape the project are rejected with an "Access denied" error. - **Sensitive file protection.** Writing to credential files (`.env`, `.ssh/`, `.aws/`, private keys) triggers a confirmation prompt, even inside the project. See [below](#why-am-i-being-prompted-to-edit-env-files) for details. diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index 884c5ce5ca..88d3d45d1d 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/packages/opencode/test/permission/next.test.ts @@ -618,8 +618,14 @@ it.instance( metadata: { permissionAliases: { "ls /project": [["ls .."]] } }, always: [], ruleset: [{ permission: "bash", pattern: "ls /project", action: "deny" }], - }).then(() => "allowed", (err: unknown) => err) - const result = await Promise.race([request, new Promise((resolve) => setTimeout(() => resolve("pending"), 100))]) + }).then( + () => "allowed", + (err: unknown) => err, + ) + const result = await Promise.race([ + request, + new Promise((resolve) => setTimeout(() => resolve("pending"), 100)), + ]) for (const item of await LegacyPermission.list()) { await LegacyPermission.reply({ requestID: item.id, reply: "reject" }) } From 6d5378f9be7364ec030bd6bb646168ad66ad189b Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 10:08:42 +0530 Subject: [PATCH 08/25] fix(permission): resolve symlinks for glob and ls, new write targets and wildcard rules glob and ls used a separate external_directory check that still asked only for the path as written. Both checks now share one helper that resolves the path segment by segment, so ".." after a link and files in directories that do not exist yet resolve where the kernel would put them. Wildcard and not-yet-created external_directory rules get a resolved twin too, so they keep covering their own directory. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/docs/configure/agents.md | 2 +- packages/opencode/src/permission/index.ts | 26 +++++------- .../opencode/src/tool/external-directory.ts | 40 +++++++------------ packages/opencode/src/util/filesystem.ts | 34 ++++++++++++++++ .../opencode/test/permission/next.test.ts | 10 +++++ .../test/tool/external-directory.test.ts | 36 ++++++++++++++++- 6 files changed, 105 insertions(+), 43 deletions(-) diff --git a/docs/docs/configure/agents.md b/docs/docs/configure/agents.md index f1237866bc..547634022b 100644 --- a/docs/docs/configure/agents.md +++ b/docs/docs/configure/agents.md @@ -12,7 +12,7 @@ Agents define different AI personas with specific models, prompts, permissions, | Agent | Description | Access Level | |-------|------------|-------------| | `builder` | Create and modify dbt models, SQL pipelines, and data transformations | Full read/write. SQL mutations prompt for approval. | -| `analyst` | Answer questions about your data — explore it, run SELECT queries, inspect schemas, generate insights | Read-only (enforced). SQL writes, file edits, shell, memory saves and other write tools denied, even if config allows them. Training entries and session scratch tables are still allowed. | +| `analyst` | Answer questions about your data — explore it, run SELECT queries, inspect schemas, generate insights | Read-only (enforced). SQL writes, file edits, shell, memory saves and other built-in write tools denied, even if config allows them. MCP and custom tools follow your config. Training entries and session scratch tables are still allowed. | | `plan` | Plan before acting — restricted to planning files only | Minimal — no edits, no bash, no SQL | ### Builder diff --git a/packages/opencode/src/permission/index.ts b/packages/opencode/src/permission/index.ts index 76dbce8bb6..021bf82d72 100644 --- a/packages/opencode/src/permission/index.ts +++ b/packages/opencode/src/permission/index.ts @@ -6,8 +6,8 @@ import { Deferred, Effect, Layer, Context } from "effect" import os from "os" // altimate_change start — resolve symlinked external_directory rule paths import path from "path" -import { realpathSync } from "fs" import { FSUtil } from "@opencode-ai/core/fs-util" +import { Filesystem } from "@/util/filesystem" // altimate_change end import { PermissionV1 } from "@opencode-ai/core/v1/permission" import { EventV2Bridge } from "@/event-v2-bridge" @@ -227,21 +227,17 @@ export function fromConfig(permission: ConfigPermissionV1.Info) { return ruleset } -// altimate_change start — resolved twin of a literal "/*" external_directory pattern +// altimate_change start — resolved twin of an external_directory pattern: its literal leading +// directory (up to the first segment with a wildcard) resolved through symlinks, rest unchanged function resolvedDirectoryPattern(pattern: string) { - const suffix = [`${path.sep}*`, "/*"].find((end) => pattern.endsWith(end)) - if (!suffix) return - const dir = pattern.slice(0, -suffix.length) - if (/[*?]/.test(dir)) return - try { - const resolved = realpathSync(dir) - if (resolved === dir) return - return process.platform === "win32" - ? FSUtil.normalizePathPattern(path.join(resolved, "*")) - : path.join(resolved, "*") - } catch { - return - } + const segments = pattern.split(process.platform === "win32" ? /[\\/]/ : "/") + const wildcard = segments.findIndex((segment) => /[*?]/.test(segment)) + if (wildcard <= 0) return + const dir = segments.slice(0, wildcard).join(path.sep) || path.sep + const resolved = Filesystem.realLocation(dir) + if (!resolved || resolved === dir) return + const twin = path.join(resolved, ...segments.slice(wildcard)) + return process.platform === "win32" ? FSUtil.normalizePathPattern(twin) : twin } // altimate_change end diff --git a/packages/opencode/src/tool/external-directory.ts b/packages/opencode/src/tool/external-directory.ts index 093972848b..58dbfa6731 100644 --- a/packages/opencode/src/tool/external-directory.ts +++ b/packages/opencode/src/tool/external-directory.ts @@ -1,6 +1,6 @@ import path from "path" // altimate_change start — resolve symlinked external_directory targets -import { realpathSync } from "fs" +import { Filesystem } from "@/util/filesystem" // altimate_change end import { Effect } from "effect" import { InstanceState } from "@/effect/instance-state" @@ -29,19 +29,14 @@ function directoryGlob(dir: string) { : path.join(dir, "*").replaceAll("\\", "/") } -function resolvedDirectory(full: string, kind: Kind) { - try { - const real = realpathSync(full) - return kind === "directory" ? real : path.dirname(real) - } catch { - if (kind === "directory") return - // A file that does not exist yet (a write target): resolve the directory it would land in. - try { - return realpathSync(path.dirname(full)) - } catch { - return - } - } +// Globs to ask for: the directory as written, plus the directory it really resolves to when a +// symlink takes it elsewhere, so an allowed directory cannot be used to reach a denied one. +function externalDirectoryPatterns(full: string, kind: Kind) { + const glob = directoryGlob(kind === "directory" ? full : path.dirname(full)) + const real = Filesystem.realLocation(full) + if (!real) return [glob] + const realGlob = directoryGlob(kind === "directory" ? real : path.dirname(real)) + return realGlob === glob ? [glob] : [glob, realGlob] } // altimate_change end @@ -60,12 +55,8 @@ export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirec const kind = options?.kind ?? "file" const dir = kind === "directory" ? full : path.dirname(full) - // altimate_change start — an allowed directory can hold a symlink to somewhere else, so also ask - // for the directory the path really resolves to (unless that is inside the project) - const glob = directoryGlob(dir) - const real = resolvedDirectory(full, kind) - const realGlob = real && directoryGlob(real) - const patterns = realGlob && realGlob !== glob && !containsPath(real, ins) ? [glob, realGlob] : [glob] + // altimate_change start — also ask for the symlink-resolved directory + const patterns = externalDirectoryPatterns(full, kind) yield* ctx.ask({ permission: "external_directory", @@ -152,15 +143,12 @@ export async function assertExternalDirectoryLegacy( const kind = options?.kind ?? "file" const dir = kind === "directory" ? full : path.dirname(full) - const glob = - process.platform === "win32" - ? FSUtil.normalizePathPattern(path.join(dir, "*")) - : path.join(dir, "*").replaceAll("\\", "/") + const patterns = externalDirectoryPatterns(full, kind) await ctx.ask({ permission: "external_directory", - patterns: [glob], - always: [glob], + patterns, + always: patterns, metadata: { filepath: full, parentDir: dir, diff --git a/packages/opencode/src/util/filesystem.ts b/packages/opencode/src/util/filesystem.ts index a5d520b1df..cc604fab5f 100644 --- a/packages/opencode/src/util/filesystem.ts +++ b/packages/opencode/src/util/filesystem.ts @@ -4,6 +4,9 @@ import { randomBytes } from "crypto" import { lookup } from "mime-types" import { realpathSync } from "fs" import { basename, dirname, isAbsolute, join, relative, resolve as pathResolve } from "path" +// altimate_change start — parse() finds the root for Filesystem.realLocation +import { parse } from "path" +// altimate_change end // altimate_change start — upstream_fix: convert Git Bash/Cygwin/WSL paths before Windows realpath import { win32 } from "path" // altimate_change end @@ -203,6 +206,37 @@ export namespace Filesystem { return !rel.startsWith("..") } + // altimate_change start — real location of a path that may not exist yet + // Resolves symlinks one segment at a time, so ".." steps out of where a link really points (as + // the kernel does; realpath normalizes ".." lexically first) and the segments that do not exist + // yet are kept as written. Undefined for a relative path. + export function realLocation(p: string): string | undefined { + const { root } = parse(p) + if (!root) return undefined + const segments = p.slice(root.length).split(process.platform === "win32" ? /[\\/]/ : "/") + if (!segments.includes("..")) { + try { + return realpathSync(p) + } catch {} + } + let real = root + for (const segment of segments) { + if (!segment || segment === ".") continue + if (segment === "..") { + real = dirname(real) + continue + } + const next = join(real, segment) + try { + real = realpathSync(next) + } catch { + real = next + } + } + return real + } + // altimate_change end + /** * Symlink-aware containment check. Resolves both paths to their real * filesystem location before comparing, preventing symlink escape attacks. diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index 88d3d45d1d..fc8beebf4a 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/packages/opencode/test/permission/next.test.ts @@ -132,6 +132,16 @@ test.skipIf(process.platform === "win32")( { permission: "external_directory", pattern: path.join(root, "link", "*"), action: "allow" }, { permission: "external_directory", pattern: path.join(root, "real", "*"), action: "allow" }, ]) + // Wildcard rules keep their wildcard part, and a directory that does not exist yet resolves + // through its nearest existing ancestor. + expect(Permission.fromConfig({ external_directory: { [path.join(root, "link", "**")]: "allow" } })[1]).toEqual({ + permission: "external_directory", + pattern: path.join(root, "real", "**"), + action: "allow", + }) + expect( + Permission.fromConfig({ external_directory: { [path.join(root, "link", "later", "*")]: "deny" } })[1], + ).toEqual({ permission: "external_directory", pattern: path.join(root, "real", "later", "*"), action: "deny" }) // Other permissions and non-symlinked directories get no extra rule. expect(Permission.fromConfig({ read: { [path.join(root, "link", "*")]: "allow" } })).toHaveLength(1) expect(Permission.fromConfig({ external_directory: { [path.join(root, "real", "*")]: "allow" } })).toHaveLength(1) diff --git a/packages/opencode/test/tool/external-directory.test.ts b/packages/opencode/test/tool/external-directory.test.ts index 5c1da6f85b..ec06dcfb6c 100644 --- a/packages/opencode/test/tool/external-directory.test.ts +++ b/packages/opencode/test/tool/external-directory.test.ts @@ -5,8 +5,10 @@ import path from "path" import { Effect } from "effect" import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" import type { Tool } from "@/tool/tool" -import { assertExternalDirectoryEffect } from "../../src/tool/external-directory" +import { assertExternalDirectoryEffect, assertExternalDirectoryLegacy } from "../../src/tool/external-directory" import { Filesystem } from "@/util/filesystem" +import { InstanceState } from "@/effect/instance-state" +import { Instance } from "../../src/project/instance" import { TestInstance, tmpdirScoped } from "../fixture/fixture" import type { Permission } from "../../src/permission" import { SessionID, MessageID } from "../../src/session/schema" @@ -125,6 +127,38 @@ describe("tool.assertExternalDirectory", () => { expect(requests[0]!.always, target).toEqual(expected) } + // A directory reached through the link (grep), a file in a subdirectory that does not exist + // yet (write), and ".." after the link all resolve to where the kernel would go. + const cases: Array<[string, "file" | "directory", string]> = [ + [path.join(allowed, "linked-dir"), "directory", elsewhere], + [ + path.join(allowed, "linked-dir", "new", "deeper", "file.txt"), + "file", + path.join(elsewhere, "new", "deeper"), + ], + // Written by hand: path.join would collapse the ".." before the link is followed. + [`${allowed}/linked-dir/../file.txt`, "file", outerTmp], + ] + for (const [target, kind, real] of cases) { + const { requests, ctx } = makeCtx() + yield* assertExternalDirectoryEffect(ctx, target, { kind }) + expect(requests[0]!.patterns.at(-1), target).toBe(glob(path.join(real, "*"))) + } + + // glob and ls use the Promise-based variant; it asks for the resolved directory too. + const legacy: string[][] = [] + const instance = yield* InstanceState.context + yield* Effect.promise(() => + Instance.restore(instance, () => + assertExternalDirectoryLegacy( + { ...baseCtx, metadata: () => {}, ask: async (req) => void legacy.push([...req.patterns]) }, + path.join(allowed, "linked-dir"), + { kind: "directory" }, + ), + ), + ) + expect(legacy).toEqual([[glob(path.join(allowed, "linked-dir", "*")), glob(path.join(elsewhere, "*"))]]) + // A plain file in the allowed directory asks only for that directory. fs.writeFileSync(path.join(allowed, "plain.txt"), "x") const { requests, ctx } = makeCtx() From a16d5db02c65bb1a67a9403f3897be20633a141e Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 10:25:17 +0530 Subject: [PATCH 09/25] fix(permission): follow dangling links and decide containment on the real location A link whose target does not exist yet is now followed, so a write through it is checked where it lands. The in-project early return now uses the resolved location too: whole-path realpath collapses ".." lexically, so "/link/../x" passed as inside the project whenever that lexical path existed. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../opencode/src/tool/external-directory.ts | 7 ++-- packages/opencode/src/util/filesystem.ts | 21 ++++++++--- .../opencode/test/permission/next.test.ts | 10 ++---- .../test/tool/external-directory.test.ts | 36 +++++++++++++++++++ 4 files changed, 60 insertions(+), 14 deletions(-) diff --git a/packages/opencode/src/tool/external-directory.ts b/packages/opencode/src/tool/external-directory.ts index 58dbfa6731..ab197ed0e8 100644 --- a/packages/opencode/src/tool/external-directory.ts +++ b/packages/opencode/src/tool/external-directory.ts @@ -51,7 +51,10 @@ export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirec const ins = yield* InstanceState.context const full = process.platform === "win32" ? FSUtil.normalizePath(target) : target - if (containsPath(full, ins)) return false + // altimate_change start — decide containment on where the path really lands: realpath collapses + // ".." lexically, so "/link/../x" would otherwise pass as inside the project + if (containsPath(Filesystem.realLocation(full) ?? full, ins)) return false + // altimate_change end const kind = options?.kind ?? "file" const dir = kind === "directory" ? full : path.dirname(full) @@ -139,7 +142,7 @@ export async function assertExternalDirectoryLegacy( if (options?.bypass) return false const full = process.platform === "win32" ? FSUtil.normalizePath(target) : target - if (Instance.containsPath(full)) return false + if (Instance.containsPath(Filesystem.realLocation(full) ?? full)) return false const kind = options?.kind ?? "file" const dir = kind === "directory" ? full : path.dirname(full) diff --git a/packages/opencode/src/util/filesystem.ts b/packages/opencode/src/util/filesystem.ts index cc604fab5f..cc70ffcbde 100644 --- a/packages/opencode/src/util/filesystem.ts +++ b/packages/opencode/src/util/filesystem.ts @@ -5,7 +5,8 @@ import { lookup } from "mime-types" import { realpathSync } from "fs" import { basename, dirname, isAbsolute, join, relative, resolve as pathResolve } from "path" // altimate_change start — parse() finds the root for Filesystem.realLocation -import { parse } from "path" +import { parse, sep } from "path" +import { lstatSync, readlinkSync } from "fs" // altimate_change end // altimate_change start — upstream_fix: convert Git Bash/Cygwin/WSL paths before Windows realpath import { win32 } from "path" @@ -209,8 +210,9 @@ export namespace Filesystem { // altimate_change start — real location of a path that may not exist yet // Resolves symlinks one segment at a time, so ".." steps out of where a link really points (as // the kernel does; realpath normalizes ".." lexically first) and the segments that do not exist - // yet are kept as written. Undefined for a relative path. - export function realLocation(p: string): string | undefined { + // yet are kept as written. A link whose target does not exist yet is followed too, so a write + // through it is checked where it lands. Undefined for a relative path. + export function realLocation(p: string, hops = 0): string | undefined { const { root } = parse(p) if (!root) return undefined const segments = p.slice(root.length).split(process.platform === "win32" ? /[\\/]/ : "/") @@ -230,11 +232,22 @@ export namespace Filesystem { try { real = realpathSync(next) } catch { - real = next + real = (hops < 40 && danglingLinkTarget(next, real, hops)) || next } } return real } + + function danglingLinkTarget(link: string, dir: string, hops: number) { + try { + if (!lstatSync(link).isSymbolicLink()) return undefined + const target = readlinkSync(link) + // Concatenated, not joined: join would collapse ".." in the target before the walk follows it. + return realLocation(isAbsolute(target) ? target : `${dir}${sep}${target}`, hops + 1) + } catch { + return undefined + } + } // altimate_change end /** diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index fc8beebf4a..9e6bf078d6 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/packages/opencode/test/permission/next.test.ts @@ -628,14 +628,8 @@ it.instance( metadata: { permissionAliases: { "ls /project": [["ls .."]] } }, always: [], ruleset: [{ permission: "bash", pattern: "ls /project", action: "deny" }], - }).then( - () => "allowed", - (err: unknown) => err, - ) - const result = await Promise.race([ - request, - new Promise((resolve) => setTimeout(() => resolve("pending"), 100)), - ]) + }).then(() => "allowed", (err: unknown) => err) + const result = await Promise.race([request, new Promise((resolve) => setTimeout(() => resolve("pending"), 100))]) for (const item of await LegacyPermission.list()) { await LegacyPermission.reply({ requestID: item.id, reply: "reject" }) } diff --git a/packages/opencode/test/tool/external-directory.test.ts b/packages/opencode/test/tool/external-directory.test.ts index ec06dcfb6c..e8b0d27953 100644 --- a/packages/opencode/test/tool/external-directory.test.ts +++ b/packages/opencode/test/tool/external-directory.test.ts @@ -63,6 +63,39 @@ describe("tool.assertExternalDirectory", () => { }), ) + if (process.platform !== "win32") { + it.instance("asks when '..' after a link inside the project leads outside it", () => + Effect.gen(function* () { + const test = yield* TestInstance + const outside = fs.realpathSync(yield* tmpdirScoped()) + fs.mkdirSync(path.join(outside, "deep")) + fs.symlinkSync(path.join(outside, "deep"), path.join(test.directory, "link")) + // Written by hand so ".." follows the link, as the kernel does. The lexical spelling + // (/file.txt) exists, which is what made whole-path realpath call it in-project. + fs.writeFileSync(path.join(test.directory, "file.txt"), "decoy") + fs.writeFileSync(path.join(outside, "file.txt"), "outside") + const target = `${test.directory}/link/../file.txt` + const expected = glob(path.join(outside, "*")) + + const { requests, ctx } = makeCtx() + yield* assertExternalDirectoryEffect(ctx, target) + expect(requests[0]?.patterns.at(-1)).toBe(expected) + + const legacy: string[][] = [] + const instance = yield* InstanceState.context + yield* Effect.promise(() => + Instance.restore(instance, () => + assertExternalDirectoryLegacy( + { ...baseCtx, metadata: () => {}, ask: async (req) => void legacy.push([...req.patterns]) }, + target, + ), + ), + ) + expect(legacy[0]?.at(-1)).toBe(expected) + }), + ) + } + it.instance("asks with a single canonical glob", () => Effect.gen(function* () { const test = yield* TestInstance @@ -118,6 +151,7 @@ describe("tool.assertExternalDirectory", () => { fs.writeFileSync(path.join(elsewhere, "secret.txt"), "x") fs.symlinkSync(elsewhere, path.join(allowed, "linked-dir")) fs.symlinkSync(path.join(elsewhere, "secret.txt"), path.join(allowed, "linked-file.txt")) + fs.symlinkSync(path.join(elsewhere, "not-yet.txt"), path.join(allowed, "dangling.txt")) for (const target of [path.join(allowed, "linked-dir", "secret.txt"), path.join(allowed, "linked-file.txt")]) { const { requests, ctx } = makeCtx() @@ -138,6 +172,8 @@ describe("tool.assertExternalDirectory", () => { ], // Written by hand: path.join would collapse the ".." before the link is followed. [`${allowed}/linked-dir/../file.txt`, "file", outerTmp], + // A link whose target does not exist yet: a write through it lands in the target's directory. + [path.join(allowed, "dangling.txt"), "file", elsewhere], ] for (const [target, kind, real] of cases) { const { requests, ctx } = makeCtx() From 698012c59b6ac55f591744e6c27eabb28912bb78 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 10:33:45 +0530 Subject: [PATCH 10/25] fix(permission): bound dangling-link resolution by one shared link budget Following dangling links was capped per recursion depth, so links that name the next link more than once branched exponentially. One budget now covers the whole resolution and running out throws, like the kernel's ELOOP, so the tool call fails instead of treating the path as resolved. The external check computes the real location once per call. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/opencode/src/permission/index.ts | 9 +++++- .../opencode/src/tool/external-directory.ts | 13 +++++---- packages/opencode/src/util/filesystem.ts | 23 ++++++++++----- .../test/tool/external-directory.test.ts | 6 ++++ .../opencode/test/util/filesystem.test.ts | 29 +++++++++++++++++++ 5 files changed, 65 insertions(+), 15 deletions(-) diff --git a/packages/opencode/src/permission/index.ts b/packages/opencode/src/permission/index.ts index 021bf82d72..f325cb477b 100644 --- a/packages/opencode/src/permission/index.ts +++ b/packages/opencode/src/permission/index.ts @@ -234,7 +234,14 @@ function resolvedDirectoryPattern(pattern: string) { const wildcard = segments.findIndex((segment) => /[*?]/.test(segment)) if (wildcard <= 0) return const dir = segments.slice(0, wildcard).join(path.sep) || path.sep - const resolved = Filesystem.realLocation(dir) + // A link loop in a configured directory just leaves the rule without a twin. + const resolved = (() => { + try { + return Filesystem.realLocation(dir) + } catch { + return undefined + } + })() if (!resolved || resolved === dir) return const twin = path.join(resolved, ...segments.slice(wildcard)) return process.platform === "win32" ? FSUtil.normalizePathPattern(twin) : twin diff --git a/packages/opencode/src/tool/external-directory.ts b/packages/opencode/src/tool/external-directory.ts index ab197ed0e8..ea515e06c3 100644 --- a/packages/opencode/src/tool/external-directory.ts +++ b/packages/opencode/src/tool/external-directory.ts @@ -31,9 +31,8 @@ function directoryGlob(dir: string) { // Globs to ask for: the directory as written, plus the directory it really resolves to when a // symlink takes it elsewhere, so an allowed directory cannot be used to reach a denied one. -function externalDirectoryPatterns(full: string, kind: Kind) { +function externalDirectoryPatterns(full: string, real: string | undefined, kind: Kind) { const glob = directoryGlob(kind === "directory" ? full : path.dirname(full)) - const real = Filesystem.realLocation(full) if (!real) return [glob] const realGlob = directoryGlob(kind === "directory" ? real : path.dirname(real)) return realGlob === glob ? [glob] : [glob, realGlob] @@ -53,13 +52,14 @@ export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirec const full = process.platform === "win32" ? FSUtil.normalizePath(target) : target // altimate_change start — decide containment on where the path really lands: realpath collapses // ".." lexically, so "/link/../x" would otherwise pass as inside the project - if (containsPath(Filesystem.realLocation(full) ?? full, ins)) return false + const real = Filesystem.realLocation(full) + if (containsPath(real ?? full, ins)) return false // altimate_change end const kind = options?.kind ?? "file" const dir = kind === "directory" ? full : path.dirname(full) // altimate_change start — also ask for the symlink-resolved directory - const patterns = externalDirectoryPatterns(full, kind) + const patterns = externalDirectoryPatterns(full, real, kind) yield* ctx.ask({ permission: "external_directory", @@ -142,11 +142,12 @@ export async function assertExternalDirectoryLegacy( if (options?.bypass) return false const full = process.platform === "win32" ? FSUtil.normalizePath(target) : target - if (Instance.containsPath(Filesystem.realLocation(full) ?? full)) return false + const real = Filesystem.realLocation(full) + if (Instance.containsPath(real ?? full)) return false const kind = options?.kind ?? "file" const dir = kind === "directory" ? full : path.dirname(full) - const patterns = externalDirectoryPatterns(full, kind) + const patterns = externalDirectoryPatterns(full, real, kind) await ctx.ask({ permission: "external_directory", diff --git a/packages/opencode/src/util/filesystem.ts b/packages/opencode/src/util/filesystem.ts index cc70ffcbde..0f81fdc63c 100644 --- a/packages/opencode/src/util/filesystem.ts +++ b/packages/opencode/src/util/filesystem.ts @@ -4,7 +4,7 @@ import { randomBytes } from "crypto" import { lookup } from "mime-types" import { realpathSync } from "fs" import { basename, dirname, isAbsolute, join, relative, resolve as pathResolve } from "path" -// altimate_change start — parse() finds the root for Filesystem.realLocation +// altimate_change start — path and link helpers for Filesystem.realLocation import { parse, sep } from "path" import { lstatSync, readlinkSync } from "fs" // altimate_change end @@ -211,8 +211,13 @@ export namespace Filesystem { // Resolves symlinks one segment at a time, so ".." steps out of where a link really points (as // the kernel does; realpath normalizes ".." lexically first) and the segments that do not exist // yet are kept as written. A link whose target does not exist yet is followed too, so a write - // through it is checked where it lands. Undefined for a relative path. - export function realLocation(p: string, hops = 0): string | undefined { + // through it is checked where it lands. Undefined for a relative path. Throws, like the kernel's + // ELOOP, when more than 40 such links are followed in one resolution. + export function realLocation(p: string): string | undefined { + return resolveLocation(p, { links: 40 }) + } + + function resolveLocation(p: string, budget: { links: number }): string | undefined { const { root } = parse(p) if (!root) return undefined const segments = p.slice(root.length).split(process.platform === "win32" ? /[\\/]/ : "/") @@ -232,21 +237,23 @@ export namespace Filesystem { try { real = realpathSync(next) } catch { - real = (hops < 40 && danglingLinkTarget(next, real, hops)) || next + real = danglingLinkTarget(next, real, budget) ?? next } } return real } - function danglingLinkTarget(link: string, dir: string, hops: number) { + function danglingLinkTarget(link: string, dir: string, budget: { links: number }) { + let target: string try { if (!lstatSync(link).isSymbolicLink()) return undefined - const target = readlinkSync(link) - // Concatenated, not joined: join would collapse ".." in the target before the walk follows it. - return realLocation(isAbsolute(target) ? target : `${dir}${sep}${target}`, hops + 1) + target = readlinkSync(link) } catch { return undefined } + if (--budget.links < 0) throw new Error(`Too many symbolic links while resolving ${link}`) + // Concatenated, not joined: join would collapse ".." in the target before the walk follows it. + return resolveLocation(isAbsolute(target) ? target : `${dir}${sep}${target}`, budget) } // altimate_change end diff --git a/packages/opencode/test/tool/external-directory.test.ts b/packages/opencode/test/tool/external-directory.test.ts index e8b0d27953..7fc7617fae 100644 --- a/packages/opencode/test/tool/external-directory.test.ts +++ b/packages/opencode/test/tool/external-directory.test.ts @@ -92,6 +92,12 @@ describe("tool.assertExternalDirectory", () => { ), ) expect(legacy[0]?.at(-1)).toBe(expected) + + // A link in the project whose target does not exist yet: a write through it lands outside. + fs.symlinkSync(path.join(outside, "new.txt"), path.join(test.directory, "dangling")) + const dangling = makeCtx() + yield* assertExternalDirectoryEffect(dangling.ctx, path.join(test.directory, "dangling")) + expect(dangling.requests[0]?.patterns.at(-1)).toBe(expected) }), ) } diff --git a/packages/opencode/test/util/filesystem.test.ts b/packages/opencode/test/util/filesystem.test.ts index 5b5b97632c..04a2e6e13a 100644 --- a/packages/opencode/test/util/filesystem.test.ts +++ b/packages/opencode/test/util/filesystem.test.ts @@ -651,4 +651,33 @@ describe("filesystem", () => { expect(Filesystem.normalizePathPattern(path.join(root, "*"))).toBe(path.join(root, "*")) }) }) + describe.skipIf(process.platform === "win32")("realLocation()", () => { + test("follows links, '..' after a link, missing segments and dangling links", async () => { + await using tmp = await tmpdir() + const root = realpathSync(tmp.path) + await fs.mkdir(path.join(root, "a", "b"), { recursive: true }) + await fs.symlink(path.join(root, "a", "b"), path.join(root, "link")) + await fs.symlink(path.join(root, "a", "later.txt"), path.join(root, "dangling")) + + expect(Filesystem.realLocation(path.join(root, "link", "x.txt"))).toBe(path.join(root, "a", "b", "x.txt")) + expect(Filesystem.realLocation(`${root}/link/../x.txt`)).toBe(path.join(root, "a", "x.txt")) + expect(Filesystem.realLocation(path.join(root, "link", "new", "x.txt"))).toBe( + path.join(root, "a", "b", "new", "x.txt"), + ) + expect(Filesystem.realLocation(path.join(root, "dangling"))).toBe(path.join(root, "a", "later.txt")) + expect(Filesystem.realLocation("relative/path")).toBeUndefined() + }) + + test("stops a chain of dangling links that would branch exponentially", async () => { + await using tmp = await tmpdir() + const root = realpathSync(tmp.path) + // Each link names the next one twice, so following them per level would double the work. + for (let i = 0; i < 30; i++) await fs.symlink(`l${i + 1}/../l${i + 1}`, path.join(root, `l${i}`)) + await fs.symlink("end", path.join(root, "l30")) + + const started = Date.now() + expect(() => Filesystem.realLocation(path.join(root, "l0"))).toThrow("Too many symbolic links") + expect(Date.now() - started).toBeLessThan(2000) + }) + }) }) From 6cbc8c429c6620678bb8370a47f9317983601c7f Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 10:37:35 +0530 Subject: [PATCH 11/25] test(filesystem): leave headroom in the link-chain time bound Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/opencode/test/util/filesystem.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opencode/test/util/filesystem.test.ts b/packages/opencode/test/util/filesystem.test.ts index 04a2e6e13a..f64b8c2077 100644 --- a/packages/opencode/test/util/filesystem.test.ts +++ b/packages/opencode/test/util/filesystem.test.ts @@ -677,7 +677,7 @@ describe("filesystem", () => { const started = Date.now() expect(() => Filesystem.realLocation(path.join(root, "l0"))).toThrow("Too many symbolic links") - expect(Date.now() - started).toBeLessThan(2000) + expect(Date.now() - started).toBeLessThan(10_000) }) }) }) From e5beb920129108f016e84b4b2086260888ae844a Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 12:39:49 +0530 Subject: [PATCH 12/25] docs(permission): put wildcard rules first in examples and describe outside symlinks accurately Permission rules are last-match-wins and config keeps key order, so a "*" rule listed after a specific pattern overrode it: "dbt run *": "deny" followed by "*": "ask" still let dbt run through after approval. Every example now lists "*" first, and the permissions page says so. The security FAQ example restricting analyst's shell now uses builder, since analyst's shell is always denied, and the symlink bullet says outside symlinks go through the external_directory check rather than being blocked. The permissions page names the training tools analyst keeps. The analyst registry test now checks tools through LLM.removedTools, the helper that removes tools from each model request. Co-Authored-By: Claude Opus 5.5 --- docs/docs/configure/agents.md | 4 ++-- docs/docs/configure/permissions.md | 8 +++++--- docs/docs/configure/tools.md | 12 ++++++------ docs/docs/reference/security-faq.md | 10 +++++----- .../test/agent/analyst-data-explorer.test.ts | 8 +++++++- 5 files changed, 25 insertions(+), 17 deletions(-) diff --git a/docs/docs/configure/agents.md b/docs/docs/configure/agents.md index 547634022b..797c59351e 100644 --- a/docs/docs/configure/agents.md +++ b/docs/docs/configure/agents.md @@ -67,8 +67,8 @@ Define custom agents in `altimate-code.json`: "write": "deny", "edit": "deny", "bash": { - "dbt docs generate": "allow", - "*": "deny" + "*": "deny", + "dbt docs generate": "allow" } } } diff --git a/docs/docs/configure/permissions.md b/docs/docs/configure/permissions.md index 0c476bf300..8caa837d23 100644 --- a/docs/docs/configure/permissions.md +++ b/docs/docs/configure/permissions.md @@ -70,9 +70,9 @@ Override permissions for specific agents: "write": "deny", "edit": "deny", "bash": { + "*": "deny", "SELECT *": "allow", - "dbt docs *": "allow", - "*": "deny" + "dbt docs *": "allow" } } } @@ -80,7 +80,9 @@ Override permissions for specific agents: } ``` -The `analyst` agent is read-only regardless of config: its shell and write tools (file edits, SQL writes, memory saves and other built-in write tools) stay denied even if its permission config allows them. Config can still restrict analyst further. +The `analyst` agent is read-only regardless of config: its shell and write tools (file edits, SQL writes, memory saves and other built-in write tools) stay denied even if its permission config allows them. Training entries remain available through `training_save` and `training_remove`. Config can still restrict analyst further. + +Rules are evaluated in order and the last matching rule wins, so put a `"*"` rule before the more specific patterns it should not override. ## All Permissioned Tools diff --git a/docs/docs/configure/tools.md b/docs/docs/configure/tools.md index 2b4e173972..ab7a87b2d4 100644 --- a/docs/docs/configure/tools.md +++ b/docs/docs/configure/tools.md @@ -38,9 +38,9 @@ Control which tools agents can use via the [permission system](permissions.md): { "permission": { "bash": { + "*": "ask", "dbt *": "allow", - "rm *": "deny", - "*": "ask" + "rm *": "deny" }, "write": "ask", "read": "allow" @@ -63,8 +63,8 @@ Disable a tool for a specific agent by setting its permission to `"deny"`: "write": "deny", "edit": "deny", "bash": { - "dbt run *": "deny", - "*": "ask" + "*": "ask", + "dbt run *": "deny" } } } @@ -85,11 +85,11 @@ The `bash` tool executes shell commands in the project directory. Commands run i { "permission": { "bash": { + "*": "ask", "dbt *": "allow", "git *": "allow", "python *": "allow", - "rm -rf *": "deny", - "*": "ask" + "rm -rf *": "deny" } } } diff --git a/docs/docs/reference/security-faq.md b/docs/docs/reference/security-faq.md index 1af82ca67f..c07f74732c 100644 --- a/docs/docs/reference/security-faq.md +++ b/docs/docs/reference/security-faq.md @@ -96,18 +96,18 @@ Yes. Use pattern-based permissions to deny destructive SQL: } ``` -You can also configure per-agent permissions. For example, restrict the `analyst` agent to read-only: +You can also configure per-agent permissions. For example, keep the `builder` agent from editing files and allow only `SELECT` shell commands. The `analyst` agent is already read-only: its shell and write tools stay denied whatever its config says. ```json { "agent": { - "analyst": { + "builder": { "permission": { "write": "deny", "edit": "deny", "bash": { - "SELECT *": "allow", - "*": "deny" + "*": "deny", + "SELECT *": "allow" } } } @@ -301,7 +301,7 @@ For additional safety: Altimate Code includes several layers of protection to keep the agent within your project: - **Project boundary enforcement.** File operations check that paths stay within your project directory (or git worktree for monorepos). Attempts to read or write outside the project trigger an `external_directory` permission prompt. A path that goes through a symlink is also checked against the directory it resolves to, so an allowed directory cannot be used to reach a denied one. -- **Symlink-aware path resolution.** Symlinks inside the project that point outside are detected and blocked. This prevents an agent from reading or writing outside your project through symlinks. +- **Symlink-aware path resolution.** Symlinks inside the project that point outside are resolved, so following one triggers the same `external_directory` permission check as any other path outside the project. Whether it is allowed depends on that permission. - **Path traversal blocking.** Paths containing `../` sequences that would escape the project are rejected with an "Access denied" error. - **Sensitive file protection.** Writing to credential files (`.env`, `.ssh/`, `.aws/`, private keys) triggers a confirmation prompt, even inside the project. See [below](#why-am-i-being-prompted-to-edit-env-files) for details. - **Bash command analysis.** The bash tool parses commands with tree-sitter to detect file operations (`rm`, `cp`, `mv`, etc.) targeting paths outside your project, and prompts for permission. diff --git a/packages/opencode/test/agent/analyst-data-explorer.test.ts b/packages/opencode/test/agent/analyst-data-explorer.test.ts index 47ae7b7bf0..77f55aba31 100644 --- a/packages/opencode/test/agent/analyst-data-explorer.test.ts +++ b/packages/opencode/test/agent/analyst-data-explorer.test.ts @@ -9,6 +9,8 @@ import { deriveSubagentSessionPermission } from "../../src/agent/subagent-permis import { Permission } from "../../src/permission" import { RuntimeFlags } from "../../src/effect/runtime-flags" import { ToolRegistry } from "../../src/tool/registry" +import { LLM } from "../../src/session/llm" +import type { MessageV2 } from "../../src/session/message-v2" import { assertExternalDirectoryEffect } from "../../src/tool/external-directory" import { MessageID, SessionID } from "../../src/session/schema" import type { Tool } from "../../src/tool/tool" @@ -479,7 +481,11 @@ withRegistry.instance( const ids = yield* (yield* ToolRegistry.Service).ids() const analyst = (yield* Agent.Service.use((svc) => svc.get("analyst")))! expect(ids).toContain("batch") - const disabled = Permission.disabled(ids, analyst.permission) + // The same helper that removes tools from each model request. + const disabled = LLM.removedTools(ids, { + agent: analyst, + user: { tools: undefined } as unknown as MessageV2.User, + }) expect(ids.filter((id) => !disabled.has(id) && !analystConfigEnabled.has(id))).toEqual([]) for (const tool of analystWrites.filter((tool) => ids.includes(tool))) expect(disabled.has(tool), tool).toBe(true) }), From 93685b38ed3de37c89f61d9eea6f2bd365a05448 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 13:09:49 +0530 Subject: [PATCH 13/25] docs(analyst): name what analyst keeps instead of calling it read-only The FAQ, quickstart, tools and permissions pages said analyst's write tools stay denied without exceptions. Only its shell and built-in write tools are re-denied after config: it still saves training entries and creates session scratch tables, and MCP or custom tools follow the user's config. Each page now says so, matching the agents table. Co-Authored-By: Claude Opus 5.5 --- docs/docs/configure/permissions.md | 2 +- docs/docs/configure/tools.md | 2 +- docs/docs/getting-started/quickstart.md | 2 +- docs/docs/reference/security-faq.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/docs/configure/permissions.md b/docs/docs/configure/permissions.md index 8caa837d23..b644fed08d 100644 --- a/docs/docs/configure/permissions.md +++ b/docs/docs/configure/permissions.md @@ -80,7 +80,7 @@ Override permissions for specific agents: } ``` -The `analyst` agent is read-only regardless of config: its shell and write tools (file edits, SQL writes, memory saves and other built-in write tools) stay denied even if its permission config allows them. Training entries remain available through `training_save` and `training_remove`. Config can still restrict analyst further. +The `analyst` agent's shell and built-in write tools (file edits, SQL writes, memory saves and the other built-in tools that change state) stay denied even if its permission config allows them. Training entries remain available through `training_save` and `training_remove`, and analyst can create session scratch tables. MCP and custom tools follow your config, and config can still restrict analyst further. Rules are evaluated in order and the last matching rule wins, so put a `"*"` rule before the more specific patterns it should not override. diff --git a/docs/docs/configure/tools.md b/docs/docs/configure/tools.md index ab7a87b2d4..9d37c34977 100644 --- a/docs/docs/configure/tools.md +++ b/docs/docs/configure/tools.md @@ -73,7 +73,7 @@ Disable a tool for a specific agent by setting its permission to `"deny"`: ``` !!! example "Builder with file edit tools denied" - The configuration above denies the builder agent's `write` and `edit` tools. The builder can read and explore the codebase. It must ask before running other shell commands, and `dbt run` is blocked entirely. An approved shell command can still modify files. The `analyst` agent is already read-only: its shell and write tools stay denied whatever its permission config says. It can still save training entries and create session scratch tables for analysis. + The configuration above denies the builder agent's `write` and `edit` tools. The builder can read and explore the codebase. It must ask before running other shell commands, and `dbt run` is blocked entirely. An approved shell command can still modify files. The `analyst` agent's shell and built-in write tools are already denied whatever its permission config says. It can still save training entries and create session scratch tables for analysis, and MCP or custom tools follow your config. ## Tool Behavior diff --git a/docs/docs/getting-started/quickstart.md b/docs/docs/getting-started/quickstart.md index 8aaf352218..36a7c64563 100644 --- a/docs/docs/getting-started/quickstart.md +++ b/docs/docs/getting-started/quickstart.md @@ -251,7 +251,7 @@ Set tool permissions for each agent mode in `altimate-code.json`: } ``` -Analyst's shell and write tools are always denied, so config can only restrict analyst further. +Analyst's shell and built-in write tools are always denied, so config cannot turn them on. It can still save training entries, and MCP or custom tools follow your config. ### Project rules with AGENTS.md diff --git a/docs/docs/reference/security-faq.md b/docs/docs/reference/security-faq.md index c07f74732c..7db7608bad 100644 --- a/docs/docs/reference/security-faq.md +++ b/docs/docs/reference/security-faq.md @@ -96,7 +96,7 @@ Yes. Use pattern-based permissions to deny destructive SQL: } ``` -You can also configure per-agent permissions. For example, keep the `builder` agent from editing files and allow only `SELECT` shell commands. The `analyst` agent is already read-only: its shell and write tools stay denied whatever its config says. +You can also configure per-agent permissions. For example, keep the `builder` agent from editing files and allow only `SELECT` shell commands. The `analyst` agent's shell and built-in write tools are already denied whatever its config says. It can still save training entries and create session scratch tables, and MCP or custom tools follow your config. ```json { From 5c883ad0a3aa1c66cf8a506ebdc05022ac84c060 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 15:02:37 +0530 Subject: [PATCH 14/25] fix(llm): preserve internal fallback for denied tool calls --- packages/opencode/src/session/llm.ts | 5 +- packages/opencode/src/session/llm/request.ts | 3 +- .../opencode/test/provider/transform.test.ts | 20 +++- packages/opencode/test/session/llm.test.ts | 105 ++++++++++++++++++ 4 files changed, 130 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/session/llm.ts b/packages/opencode/src/session/llm.ts index 1bf82c7e32..103e20fbde 100644 --- a/packages/opencode/src/session/llm.ts +++ b/packages/opencode/src/session/llm.ts @@ -477,7 +477,10 @@ export namespace LLM { // Tools removed before the model call: denied by the agent, or switched off on the request. export function removedTools(names: string[], input: Pick): Set { const disabled = PermissionNext.disabled(names, input.agent.permission) - return new Set(names.filter((tool) => input.user.tools?.[tool] === false || disabled.has(tool))) + // The hidden SDK fallback reports rejected calls; it grants no tool capability. + return new Set( + names.filter((tool) => tool !== "invalid" && (input.user.tools?.[tool] === false || disabled.has(tool))), + ) } async function resolveTools(input: Pick) { diff --git a/packages/opencode/src/session/llm/request.ts b/packages/opencode/src/session/llm/request.ts index 992a4ef1ac..e724bd9367 100644 --- a/packages/opencode/src/session/llm/request.ts +++ b/packages/opencode/src/session/llm/request.ts @@ -245,7 +245,8 @@ function resolveTools(input: Pick input.user.tools?.[k] !== false && !disabled.has(k)) + // Keep the hidden error handler even when the agent denies every callable tool. + return Record.filter(input.tools, (_, k) => k === "invalid" || (input.user.tools?.[k] !== false && !disabled.has(k))) } export function hasToolCalls(messages: ModelMessage[]): boolean { diff --git a/packages/opencode/test/provider/transform.test.ts b/packages/opencode/test/provider/transform.test.ts index f5ac6420c2..90e6a8dc0a 100644 --- a/packages/opencode/test/provider/transform.test.ts +++ b/packages/opencode/test/provider/transform.test.ts @@ -12,6 +12,7 @@ import { OutputTokenBudgetError, } from "@/provider/output-token-budget" import { LLMRequestPrep } from "@/session/llm/request" +import type { Agent } from "@/agent/agent" // ProviderTransform.message expects a Provider.Model with the fork's ModelID/ProviderID brands. import { ModelID, ProviderID } from "@/provider/schema" @@ -5500,6 +5501,8 @@ describe("LLMRequestPrep.prepare - output token reservation", () => { readonly modelHeaders?: Record readonly chatHeaders?: Record readonly isWorkflow?: boolean + readonly permission?: Agent.Info["permission"] + readonly userTools?: Record } = {}, ) => Effect.runPromise( @@ -5511,6 +5514,7 @@ describe("LLMRequestPrep.prepare - output token reservation", () => { time: { created: Date.now() }, agent: "test", model: { providerID: "openai-compatible", modelID: "large-window-model" }, + tools: overrides.userTools, } as any, sessionID, model: { ...model, headers: overrides.modelHeaders ?? model.headers }, @@ -5518,7 +5522,7 @@ describe("LLMRequestPrep.prepare - output token reservation", () => { name: "test", mode: "primary", options: overrides.agentOptions ?? {}, - permission: [], + permission: overrides.permission ?? [], prompt: systemPrompt, } as any, system: [], @@ -5547,6 +5551,20 @@ describe("LLMRequestPrep.prepare - output token reservation", () => { }), ) + test("wildcard-deny request preparation retains the internal fallback only", async () => { + const fallback = tool({ inputSchema: jsonSchema({ type: "object", properties: {} }) }) + const result = await run("Read only", { + permission: [ + { permission: "*", pattern: "*", action: "deny" }, + { permission: "read", pattern: "*", action: "allow" }, + ], + userTools: { invalid: false, read: false }, + tools: { invalid: fallback, write: fallback, read: fallback }, + }) + expect(Object.keys(result.tools)).toEqual(["invalid"]) + expect(result.tools.invalid).toBe(fallback) + }) + // Prose with no code or JSON characters, so Token.estimate uses its default 3.7 chars/token. const PROSE = "the quick brown fox jumps over the lazy dog " const largePrompt = PROSE.repeat(Math.ceil((52_180 * 3.7) / PROSE.length)) diff --git a/packages/opencode/test/session/llm.test.ts b/packages/opencode/test/session/llm.test.ts index 8af231fc3c..6f0e21090c 100644 --- a/packages/opencode/test/session/llm.test.ts +++ b/packages/opencode/test/session/llm.test.ts @@ -142,6 +142,12 @@ describe("session.llm.addHistoricalToolStubs", () => { }) describe("session.llm.removedTools", () => { + test("keeps the internal fallback under wildcard deny and request switches", () => { + const agent = { permission: PermissionNext.fromConfig({ "*": "deny", read: "allow" }) } as Agent.Info + const user = { tools: { invalid: false, read: false } } as unknown as MessageV2.User + expect(LLM.removedTools(["invalid", "write", "read"], { agent, user })).toEqual(new Set(["write", "read"])) + }) + test("removes a tool the agent denies or the request switches off, and keeps the rest", () => { const agent = { permission: PermissionNext.fromConfig({ "*": "allow", altimate_memory_write: "deny" }), @@ -293,6 +299,105 @@ function createEventResponse(chunks: unknown[], includeDone = false) { } describe("session.llm.stream", () => { + test("wildcard-deny agents report the unavailable tool through the hidden fallback", async () => { + const providerID = "alibaba" + const { model } = await loadFixture(providerID, "qwen-plus") + const request = waitRequest( + "/chat/completions", + createEventResponse( + [ + { + id: "chatcmpl-denied", + object: "chat.completion.chunk", + choices: [ + { + delta: { + role: "assistant", + tool_calls: [ + { index: 0, id: "call-denied", type: "function", function: { name: "write", arguments: "{}" } }, + ], + }, + }, + ], + }, + { + id: "chatcmpl-denied", + object: "chat.completion.chunk", + choices: [{ delta: {}, finish_reason: "tool_calls" }], + }, + ], + true, + ), + ) + await using tmp = await tmpdir({ + config: { + enabled_providers: [providerID], + provider: { [providerID]: { options: { apiKey: "test-key", baseURL: `${state.server!.url.origin}/v1` } } }, + }, + }) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const resolved = await Provider.getModel(ProviderID.make(providerID), ModelID.make(model.id)) + const sessionID = SessionID.make("session-denied-tool") + const agent = { + name: "custom-readonly", + mode: "primary", + options: {}, + permission: PermissionNext.fromConfig({ "*": "deny", read: "allow" }), + } satisfies Agent.Info + let writes = 0 + const stream = await LLM.stream({ + user: { + id: MessageID.make("msg_denied-tool"), + sessionID, + role: "user", + time: { created: Date.now() }, + agent: agent.name, + model: { providerID: ProviderID.make(providerID), modelID: resolved.id }, + }, + sessionID, + model: resolved, + agent, + system: [], + abort: new AbortController().signal, + messages: [{ role: "user", content: "Write a file" }], + tools: { + read: tool({ inputSchema: jsonSchema({ type: "object", properties: {} }), execute: async () => "read" }), + write: tool({ + inputSchema: jsonSchema({ type: "object", properties: {} }), + execute: async () => { + writes++ + return "written" + }, + }), + invalid: tool({ + inputSchema: jsonSchema<{ tool: string; error: string }>({ + type: "object", + properties: { tool: { type: "string" }, error: { type: "string" } }, + required: ["tool", "error"], + }), + execute: async (input) => input, + }), + }, + }) + const events = [] + for await (const event of stream.fullStream) events.push(event) + const result = events.find((event) => event.type === "tool-result") + expect(result).toMatchObject({ + toolName: "invalid", + output: { tool: "write", error: expect.stringContaining("write") }, + }) + expect(events.some((event) => event.type === "tool-error" || event.type === "error")).toBe(false) + expect(writes).toBe(0) + const capture = await request + expect((capture.body.tools as { function: { name: string } }[]).map((item) => item.function.name)).toEqual([ + "read", + ]) + }, + }) + }) + test.skip("sends temperature, tokens, and reasoning options for openai-compatible models", async () => { const server = state.server if (!server) { From 05de6a330d581744d712fcce134e4d1569a50ea3 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 15:40:35 +0530 Subject: [PATCH 15/25] fix(permission): protect authored external paths --- packages/core/src/util/wildcard.ts | 15 ++++-- packages/opencode/src/permission/evaluate.ts | 10 ++-- packages/opencode/src/permission/next.ts | 6 ++- .../opencode/src/tool/external-directory.ts | 31 ++++++++---- packages/opencode/src/util/wildcard.ts | 13 ++++- .../opencode/test/permission/next.test.ts | 14 ++++++ .../test/tool/external-directory.test.ts | 49 +++++++++++++++++-- packages/opencode/test/util/wildcard.test.ts | 9 ++++ 8 files changed, 126 insertions(+), 21 deletions(-) diff --git a/packages/core/src/util/wildcard.ts b/packages/core/src/util/wildcard.ts index 0a67817bcb..65dc6ed752 100644 --- a/packages/core/src/util/wildcard.ts +++ b/packages/core/src/util/wildcard.ts @@ -1,14 +1,23 @@ export * as Wildcard from "./wildcard" export function match(input: string, pattern: string) { - const normalized = input.replaceAll("\\", "/") + return matchNormalized(input.replaceAll("\\", "/"), pattern.replaceAll("\\", "/")) +} + +// altimate_change start — preserve POSIX backslashes when matching filesystem paths +export function matchPath(input: string, pattern: string) { + if (process.platform === "win32") return match(input, pattern) + return matchNormalized(input, pattern) +} +// altimate_change end + +function matchNormalized(input: string, pattern: string) { let escaped = pattern - .replaceAll("\\", "/") .replace(/[.+^${}()|[\]\\]/g, "\\$&") .replace(/\*/g, ".*") .replace(/\?/g, ".") if (escaped.endsWith(" .*")) escaped = escaped.slice(0, -3) + "( .*)?" - return new RegExp("^" + escaped + "$", process.platform === "win32" ? "si" : "s").test(normalized) + return new RegExp("^" + escaped + "$", process.platform === "win32" ? "si" : "s").test(input) } diff --git a/packages/opencode/src/permission/evaluate.ts b/packages/opencode/src/permission/evaluate.ts index ce8280a936..d164a323a6 100644 --- a/packages/opencode/src/permission/evaluate.ts +++ b/packages/opencode/src/permission/evaluate.ts @@ -8,6 +8,10 @@ import { Wildcard } from "@opencode-ai/core/util/wildcard" // An alias match can never relax a stricter authored match. const STRICTNESS = { allow: 0, ask: 1, deny: 2 } as const +function matchPattern(permission: string, input: string, pattern: string) { + return permission === "external_directory" ? Wildcard.matchPath(input, pattern) : Wildcard.match(input, pattern) +} + function evaluatePatterns( permission: string, pattern: string, @@ -15,7 +19,7 @@ function evaluatePatterns( ...rulesets: PermissionV1.Ruleset[] ): PermissionV1.Rule { const rules = rulesets.flat().filter((rule) => Wildcard.match(permission, rule.permission)) - const authored = rules.findLast((rule) => Wildcard.match(pattern, rule.pattern)) ?? { + const authored = rules.findLast((rule) => matchPattern(permission, pattern, rule.pattern)) ?? { action: "ask" as const, permission, pattern: "*", @@ -23,8 +27,8 @@ function evaluatePatterns( // Rule order still decides: an allow rule only applies to the authored spelling. const aliased = rules.findLast( (rule) => - Wildcard.match(pattern, rule.pattern) || - (rule.action !== "allow" && aliases.some((alias) => Wildcard.match(alias, rule.pattern))), + matchPattern(permission, pattern, rule.pattern) || + (rule.action !== "allow" && aliases.some((alias) => matchPattern(permission, alias, rule.pattern))), ) return aliased && STRICTNESS[aliased.action] > STRICTNESS[authored.action] ? aliased : authored } diff --git a/packages/opencode/src/permission/next.ts b/packages/opencode/src/permission/next.ts index d28e5edb91..4382480a2e 100644 --- a/packages/opencode/src/permission/next.ts +++ b/packages/opencode/src/permission/next.ts @@ -260,7 +260,11 @@ export namespace PermissionNext { const merged = merge(...rulesets) log.info("evaluate", { permission, pattern, ruleset: merged }) const match = merged.findLast( - (rule) => Wildcard.match(permission, rule.permission) && Wildcard.match(pattern, rule.pattern), + (rule) => + Wildcard.match(permission, rule.permission) && + (permission === "external_directory" + ? Wildcard.matchPath(pattern, rule.pattern) + : Wildcard.match(pattern, rule.pattern)), ) return match ?? { action: "ask", permission, pattern: "*" } } diff --git a/packages/opencode/src/tool/external-directory.ts b/packages/opencode/src/tool/external-directory.ts index ea515e06c3..24a8cb64e0 100644 --- a/packages/opencode/src/tool/external-directory.ts +++ b/packages/opencode/src/tool/external-directory.ts @@ -24,9 +24,7 @@ type Options = { // altimate_change start — symlink-resolved directory of an external_directory target function directoryGlob(dir: string) { - return process.platform === "win32" - ? FSUtil.normalizePathPattern(path.join(dir, "*")) - : path.join(dir, "*").replaceAll("\\", "/") + return process.platform === "win32" ? path.join(path.resolve(FSUtil.windowsPath(dir)), "*") : path.join(dir, "*") } // Globs to ask for: the directory as written, plus the directory it really resolves to when a @@ -37,6 +35,17 @@ function externalDirectoryPatterns(full: string, real: string | undefined, kind: const realGlob = directoryGlob(kind === "directory" ? real : path.dirname(real)) return realGlob === glob ? [glob] : [glob, realGlob] } + +function authoredPath(target: string) { + if (process.platform !== "win32") return target + return path.resolve(FSUtil.windowsPath(target)) +} + +function containsAuthoredPath(filepath: string, directory: string, worktree: string) { + if (FSUtil.contains(directory, filepath)) return true + if (worktree === "/") return false + return FSUtil.contains(worktree, filepath) +} // altimate_change end export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirectory")(function* ( @@ -49,11 +58,14 @@ export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirec if (options?.bypass) return false const ins = yield* InstanceState.context - const full = process.platform === "win32" ? FSUtil.normalizePath(target) : target + // Keep the authored location separate from its real destination. In particular, resolving + // an external symlink here would hide the external directory entry that a delete or move + // operates on. Windows still needs its alternate drive spellings converted before comparison. + const full = authoredPath(target) // altimate_change start — decide containment on where the path really lands: realpath collapses // ".." lexically, so "/link/../x" would otherwise pass as inside the project - const real = Filesystem.realLocation(full) - if (containsPath(real ?? full, ins)) return false + const real = Filesystem.realLocation(process.platform === "win32" ? FSUtil.windowsPath(target) : target) + if (containsAuthoredPath(full, ins.directory, ins.worktree) && containsPath(real ?? full, ins)) return false // altimate_change end const kind = options?.kind ?? "file" @@ -141,9 +153,10 @@ export async function assertExternalDirectoryLegacy( if (!target) return false if (options?.bypass) return false - const full = process.platform === "win32" ? FSUtil.normalizePath(target) : target - const real = Filesystem.realLocation(full) - if (Instance.containsPath(real ?? full)) return false + const full = authoredPath(target) + const real = Filesystem.realLocation(process.platform === "win32" ? FSUtil.windowsPath(target) : target) + if (containsAuthoredPath(full, Instance.directory, Instance.worktree) && Instance.containsPath(real ?? full)) + return false const kind = options?.kind ?? "file" const dir = kind === "directory" ? full : path.dirname(full) diff --git a/packages/opencode/src/util/wildcard.ts b/packages/opencode/src/util/wildcard.ts index b2affbf849..768eb6981c 100644 --- a/packages/opencode/src/util/wildcard.ts +++ b/packages/opencode/src/util/wildcard.ts @@ -1,8 +1,17 @@ import { sortBy, pipe } from "remeda" export function match(str: string, pattern: string) { - if (str) str = str.replaceAll("\\", "/") - if (pattern) pattern = pattern.replaceAll("\\", "/") + return matchNormalized(str?.replaceAll("\\", "/"), pattern?.replaceAll("\\", "/")) +} + +// altimate_change start — preserve POSIX backslashes when matching filesystem paths +export function matchPath(str: string, pattern: string) { + if (process.platform === "win32") return match(str, pattern) + return matchNormalized(str, pattern) +} +// altimate_change end + +function matchNormalized(str: string, pattern: string) { let escaped = pattern .replace(/[.+^${}()|[\]\\]/g, "\\$&") // escape special regex chars .replace(/\*/g, ".*") // * becomes .* diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index 9e6bf078d6..31859649f0 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/packages/opencode/test/permission/next.test.ts @@ -151,6 +151,20 @@ test.skipIf(process.platform === "win32")( }, ) +test.skipIf(process.platform === "win32")( + "external_directory does not treat a POSIX backslash as a path separator", + () => { + const allowed = "/tmp/skill" + const sibling = `${allowed}\\outside/file.txt` + const config = { "*": "deny" as const, external_directory: { [`${allowed}/*`]: "allow" as const } } + + expect(Permission.evaluate("external_directory", sibling, Permission.fromConfig(config)).action).toBe("deny") + expect(LegacyPermission.evaluate("external_directory", sibling, LegacyPermission.fromConfig(config)).action).toBe( + "deny", + ) + }, +) + test("fromConfig - expands $HOME to home directory", () => { const result = Permission.fromConfig({ external_directory: { "$HOME/projects/*": "allow" } }) expect(result).toEqual([{ permission: "external_directory", pattern: `${os.homedir()}/projects/*`, action: "allow" }]) diff --git a/packages/opencode/test/tool/external-directory.test.ts b/packages/opencode/test/tool/external-directory.test.ts index 7fc7617fae..80c9a70779 100644 --- a/packages/opencode/test/tool/external-directory.test.ts +++ b/packages/opencode/test/tool/external-directory.test.ts @@ -10,7 +10,6 @@ import { Filesystem } from "@/util/filesystem" import { InstanceState } from "@/effect/instance-state" import { Instance } from "../../src/project/instance" import { TestInstance, tmpdirScoped } from "../fixture/fixture" -import type { Permission } from "../../src/permission" import { SessionID, MessageID } from "../../src/session/schema" import { testEffect } from "../lib/effect" @@ -26,8 +25,7 @@ const baseCtx: Omit = { metadata: () => Effect.void, } -const glob = (p: string) => - process.platform === "win32" ? Filesystem.normalizePathPattern(p) : p.replaceAll("\\", "/") +const glob = (p: string) => (process.platform === "win32" ? Filesystem.normalizePathPattern(p) : p) function makeCtx() { const requests: Array> = [] @@ -63,7 +61,52 @@ describe("tool.assertExternalDirectory", () => { }), ) + it.instance("asks for an external directory entry that resolves inside the project", () => + Effect.gen(function* () { + const test = yield* TestInstance + const outside = yield* tmpdirScoped() + const link = path.join(outside, "project-link") + fs.symlinkSync(test.directory, link, process.platform === "win32" ? "junction" : "dir") + const target = path.join(link, "file.txt") + const expected = + process.platform === "win32" + ? path.join(path.resolve(Filesystem.windowsPath(link)), "*") + : glob(path.join(link, "*")) + + const { requests, ctx } = makeCtx() + yield* assertExternalDirectoryEffect(ctx, target) + expect(requests[0]?.patterns).toContain(expected) + + const legacy: string[][] = [] + const instance = yield* InstanceState.context + yield* Effect.promise(() => + Instance.restore(instance, () => + assertExternalDirectoryLegacy( + { ...baseCtx, metadata: () => {}, ask: async (req) => void legacy.push([...req.patterns]) }, + target, + ), + ), + ) + expect(legacy[0]).toContain(expected) + }), + ) + if (process.platform !== "win32") { + it.instance("preserves a literal backslash in an external POSIX directory", () => + Effect.gen(function* () { + const outside = yield* tmpdirScoped() + const directory = path.join(outside, "allowed\\sibling") + fs.mkdirSync(directory) + const target = path.join(directory, "file.txt") + const { requests, ctx } = makeCtx() + + yield* assertExternalDirectoryEffect(ctx, target) + + expect(requests[0]?.patterns).toContain(path.join(directory, "*")) + expect(requests[0]?.patterns).not.toContain(path.join(outside, "allowed/sibling", "*")) + }), + ) + it.instance("asks when '..' after a link inside the project leads outside it", () => Effect.gen(function* () { const test = yield* TestInstance diff --git a/packages/opencode/test/util/wildcard.test.ts b/packages/opencode/test/util/wildcard.test.ts index 2f7b263287..b41733d60c 100644 --- a/packages/opencode/test/util/wildcard.test.ts +++ b/packages/opencode/test/util/wildcard.test.ts @@ -79,6 +79,15 @@ test("match normalizes slashes for cross-platform globbing", () => { expect(Wildcard.match("C:/Windows/System32/drivers", "C:\\Windows\\System32\\*")).toBe(true) }) +test("matchPath preserves literal backslashes on POSIX", () => { + if (process.platform === "win32") { + expect(Wildcard.matchPath("C:\\Windows\\System32\\drivers", "C:/Windows/System32/*")).toBe(true) + return + } + expect(Wildcard.matchPath("/tmp/skill\\outside/file", "/tmp/skill/*")).toBe(false) + expect(Wildcard.matchPath("/tmp/skill/file", "/tmp/skill/*")).toBe(true) +}) + test("match handles case-insensitivity on Windows", () => { if (process.platform === "win32") { expect(Wildcard.match("C:\\windows\\system32\\hosts", "C:/Windows/System32/*")).toBe(true) From c048b7d4f9766499b9ea4dfd089dd8d48cda1094 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 15:51:09 +0530 Subject: [PATCH 16/25] fix(permission): close resolved-path bypasses --- .../src/session/llm/native-runtime.ts | 3 +- .../opencode/src/tool/external-directory.ts | 45 +++++++++++------ .../opencode/test/session/llm-native.test.ts | 50 ++++++++++++++++++- .../test/tool/external-directory.test.ts | 33 +++++++++++- 4 files changed, 112 insertions(+), 19 deletions(-) diff --git a/packages/opencode/src/session/llm/native-runtime.ts b/packages/opencode/src/session/llm/native-runtime.ts index f67c73cb6f..ad795585a6 100644 --- a/packages/opencode/src/session/llm/native-runtime.ts +++ b/packages/opencode/src/session/llm/native-runtime.ts @@ -89,6 +89,7 @@ export function stream(input: StreamInput): StreamResult { // — if a field ever needs to differ between the two surfaces, the // translation belongs here, not split across both packages. const tools = nativeTools(input.tools, input) + const advertisedTools = Object.fromEntries(Object.entries(tools).filter(([name]) => name !== "invalid")) const request = LLMNative.request({ model: input.model, apiKey: current.apiKey, @@ -112,7 +113,7 @@ export function stream(input: StreamInput): StreamResult { const provider = input.llmClient .stream( LLMRequest.update(request, { - tools: [...request.tools, ...toDefinitions(tools)], + tools: [...request.tools, ...toDefinitions(advertisedTools)], }), ) .pipe( diff --git a/packages/opencode/src/tool/external-directory.ts b/packages/opencode/src/tool/external-directory.ts index 24a8cb64e0..93996c26d8 100644 --- a/packages/opencode/src/tool/external-directory.ts +++ b/packages/opencode/src/tool/external-directory.ts @@ -46,6 +46,17 @@ function containsAuthoredPath(filepath: string, directory: string, worktree: str if (worktree === "/") return false return FSUtil.contains(worktree, filepath) } + +function sensitiveWriteMatches(target: string, directory: string) { + const authored = authoredPath(target) + const resolved = Filesystem.realLocation(process.platform === "win32" ? FSUtil.windowsPath(target) : target) + return [...new Set([authored, resolved].filter((item): item is string => !!item))] + .map((filepath) => { + const relative = path.relative(directory, filepath) + return { filepath, relative, matched: Protected.isSensitiveWrite(relative) } + }) + .filter((item): item is typeof item & { matched: string } => !!item.matched) +} // altimate_change end export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirectory")(function* ( @@ -104,17 +115,18 @@ export const assertSensitiveWriteEffect = Effect.fn("Tool.assertSensitiveWrite") ) { if (!target) return const ins = yield* InstanceState.context - const relativePath = path.relative(ins.directory, target) - const matched = Protected.isSensitiveWrite(relativePath) - if (!matched) return + const matches = sensitiveWriteMatches(target, ins.directory) + if (!matches.length) return + const patterns = matches.map((item) => item.relative) + const matched = matches[0] yield* ctx.ask({ permission: "sensitive_write", - patterns: [relativePath], - always: [relativePath], + patterns, + always: patterns, metadata: { - filepath: target, - sensitive: matched, - reason: `This file is in a sensitive location (${matched}). Modifications could affect credentials, version control, or security configuration.`, + filepath: authoredPath(target), + sensitive: matched.matched, + reason: `This file is in a sensitive location (${matched.matched}). Modifications could affect credentials, version control, or security configuration.`, }, }) }) @@ -123,18 +135,19 @@ export const assertSensitiveWriteEffect = Effect.fn("Tool.assertSensitiveWrite") // (like the Legacy external-directory variant) rather than InstanceState.context. export async function assertSensitiveWrite(ctx: Tool.Context, target?: string): Promise { if (!target) return - const relativePath = path.relative(Instance.directory, target) - const matched = Protected.isSensitiveWrite(relativePath) - if (!matched) return + const matches = sensitiveWriteMatches(target, Instance.directory) + if (!matches.length) return + const patterns = matches.map((item) => item.relative) + const matched = matches[0] await Effect.runPromise( ctx.ask({ permission: "sensitive_write", - patterns: [relativePath], - always: [relativePath], + patterns, + always: patterns, metadata: { - filepath: target, - sensitive: matched, - reason: `This file is in a sensitive location (${matched}). Modifications could affect credentials, version control, or security configuration.`, + filepath: authoredPath(target), + sensitive: matched.matched, + reason: `This file is in a sensitive location (${matched.matched}). Modifications could affect credentials, version control, or security configuration.`, }, }), ) diff --git a/packages/opencode/test/session/llm-native.test.ts b/packages/opencode/test/session/llm-native.test.ts index 17ee289a42..c1157377b6 100644 --- a/packages/opencode/test/session/llm-native.test.ts +++ b/packages/opencode/test/session/llm-native.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test" -import { LLMEvent, ToolFailure } from "@opencode-ai/llm" +import { LLMEvent, ToolFailure, type LLMRequest } from "@opencode-ai/llm" import { LLMClient, RequestExecutor, WebSocketExecutor, type LLMClientShape } from "@opencode-ai/llm/route" import { jsonSchema, tool, type ModelMessage, type Tool } from "ai" import { Effect, Fiber, Layer, Stream } from "effect" @@ -555,6 +555,54 @@ describe("session.llm-native.request", () => { }), ) + it.effect("keeps the invalid fallback dispatchable without advertising it to the provider", () => + Effect.gen(function* () { + let advertised: string[] = [] + const invalid = tool({ + description: "Internal fallback", + inputSchema: jsonSchema({ type: "object" }), + execute: async () => "fallback", + }) + const visible = tool({ + description: "Visible tool", + inputSchema: jsonSchema({ type: "object" }), + execute: async () => "visible", + }) + const llmClient = { + prepare: () => Effect.die("unused"), + stream: (request: LLMRequest) => { + advertised = request.tools.map((item) => item.name) + return Stream.fromIterable([ + LLMEvent.toolCall({ id: "call-invalid", name: "invalid", input: {} }), + LLMEvent.finish({ reason: "tool-calls" }), + ]) + }, + generate: () => Effect.die("unused"), + } as LLMClientShape + const native = LLMNativeRuntime.stream({ + model: baseModel, + provider: providerInfo, + auth: undefined, + llmClient, + messages: [], + tools: { visible, invalid }, + headers: {}, + abort: new AbortController().signal, + }) + expect(native.type).toBe("supported") + if (native.type === "unsupported") throw new Error(native.reason) + + const events = Array.from(yield* native.stream.pipe(Stream.runCollect)) + expect(advertised).toEqual(["visible"]) + expect(events).toEqual( + expect.arrayContaining([ + expect.objectContaining({ type: "tool-call", name: "invalid" }), + expect.objectContaining({ type: "tool-result", name: "invalid" }), + ]), + ) + }), + ) + it.effect("emits native tool calls before overlapping local settlements complete", () => Effect.gen(function* () { const observed: string[] = [] diff --git a/packages/opencode/test/tool/external-directory.test.ts b/packages/opencode/test/tool/external-directory.test.ts index 80c9a70779..0524b4592d 100644 --- a/packages/opencode/test/tool/external-directory.test.ts +++ b/packages/opencode/test/tool/external-directory.test.ts @@ -5,7 +5,12 @@ import path from "path" import { Effect } from "effect" import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" import type { Tool } from "@/tool/tool" -import { assertExternalDirectoryEffect, assertExternalDirectoryLegacy } from "../../src/tool/external-directory" +import { + assertExternalDirectoryEffect, + assertExternalDirectoryLegacy, + assertSensitiveWrite, + assertSensitiveWriteEffect, +} from "../../src/tool/external-directory" import { Filesystem } from "@/util/filesystem" import { InstanceState } from "@/effect/instance-state" import { Instance } from "../../src/project/instance" @@ -302,3 +307,29 @@ describe("tool.assertExternalDirectory", () => { ) } }) + +describe("tool.assertSensitiveWrite", () => { + it.instance("checks the resolved destination of an innocently named project link", () => + Effect.gen(function* () { + const test = yield* TestInstance + const outside = yield* tmpdirScoped() + const sensitive = path.join(outside, ".ssh") + fs.mkdirSync(sensitive) + const link = path.join(test.directory, "config") + fs.symlinkSync(sensitive, link, process.platform === "win32" ? "junction" : "dir") + const target = path.join(link, "credentials") + + const { requests, ctx } = makeCtx() + yield* assertSensitiveWriteEffect(ctx, target) + expect(requests).toHaveLength(1) + expect(requests[0]?.permission).toBe("sensitive_write") + expect(requests[0]?.metadata.sensitive).toBe(".ssh") + + const legacy = makeCtx() + const instance = yield* InstanceState.context + yield* Effect.promise(() => Instance.restore(instance, () => assertSensitiveWrite(legacy.ctx, target))) + expect(legacy.requests).toHaveLength(1) + expect(legacy.requests[0]?.patterns).toContain(path.relative(test.directory, path.join(sensitive, "credentials"))) + }), + ) +}) From 35d03fe1618a2b3d3d5713008df96df0645b3969 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 16:04:42 +0530 Subject: [PATCH 17/25] fix(permission): preserve path and fallback identity --- packages/opencode/src/permission/evaluate.ts | 4 +- packages/opencode/src/permission/next.ts | 12 ++++-- .../src/session/llm/native-runtime.ts | 41 ++++++++++++------- .../opencode/test/permission/next.test.ts | 24 ++++++++++- .../opencode/test/session/llm-native.test.ts | 25 +++++++++-- 5 files changed, 82 insertions(+), 24 deletions(-) diff --git a/packages/opencode/src/permission/evaluate.ts b/packages/opencode/src/permission/evaluate.ts index d164a323a6..7ad7be22ce 100644 --- a/packages/opencode/src/permission/evaluate.ts +++ b/packages/opencode/src/permission/evaluate.ts @@ -9,7 +9,9 @@ import { Wildcard } from "@opencode-ai/core/util/wildcard" const STRICTNESS = { allow: 0, ask: 1, deny: 2 } as const function matchPattern(permission: string, input: string, pattern: string) { - return permission === "external_directory" ? Wildcard.matchPath(input, pattern) : Wildcard.match(input, pattern) + return permission === "external_directory" || permission === "sensitive_write" + ? Wildcard.matchPath(input, pattern) + : Wildcard.match(input, pattern) } function evaluatePatterns( diff --git a/packages/opencode/src/permission/next.ts b/packages/opencode/src/permission/next.ts index 4382480a2e..33c1bc8588 100644 --- a/packages/opencode/src/permission/next.ts +++ b/packages/opencode/src/permission/next.ts @@ -148,7 +148,10 @@ export namespace PermissionNext { timestamp: Date.now(), session_id: request.sessionID, tool_name: request.permission, - tool_category: Telemetry.categorizeToolName(request.permission, request.permission.startsWith("mcp__") ? "mcp" : "standard"), + tool_category: Telemetry.categorizeToolName( + request.permission, + request.permission.startsWith("mcp__") ? "mcp" : "standard", + ), source: "config_rule", }) throw new DeniedError(ruleset.filter((r) => Wildcard.match(request.permission, r.permission))) @@ -195,7 +198,10 @@ export namespace PermissionNext { timestamp: Date.now(), session_id: existing.info.sessionID, tool_name: existing.info.permission, - tool_category: Telemetry.categorizeToolName(existing.info.permission, existing.info.permission.startsWith("mcp__") ? "mcp" : "standard"), + tool_category: Telemetry.categorizeToolName( + existing.info.permission, + existing.info.permission.startsWith("mcp__") ? "mcp" : "standard", + ), source: "user", }) existing.reject(input.message ? new CorrectedError(input.message) : new RejectedError()) @@ -262,7 +268,7 @@ export namespace PermissionNext { const match = merged.findLast( (rule) => Wildcard.match(permission, rule.permission) && - (permission === "external_directory" + (permission === "external_directory" || permission === "sensitive_write" ? Wildcard.matchPath(pattern, rule.pattern) : Wildcard.match(pattern, rule.pattern)), ) diff --git a/packages/opencode/src/session/llm/native-runtime.ts b/packages/opencode/src/session/llm/native-runtime.ts index ad795585a6..1013f23bfd 100644 --- a/packages/opencode/src/session/llm/native-runtime.ts +++ b/packages/opencode/src/session/llm/native-runtime.ts @@ -17,6 +17,7 @@ import { toDefinitions, type JsonSchema, type LLMEvent, + type ToolCallPart, } from "@opencode-ai/llm" import type { LLMClientShape } from "@opencode-ai/llm/route" import { LLMNative } from "./native-request" @@ -117,22 +118,22 @@ export function stream(input: StreamInput): StreamResult { }), ) .pipe( - Stream.flatMap((event) => - event.type !== "tool-call" || event.providerExecuted - ? Stream.make(event) - : Stream.make(event).pipe( - Stream.concat( - Stream.fromEffectDrain( - ToolRuntime.dispatch(tools, event).pipe( - Effect.flatMap((dispatched) => Queue.offerAll(results, dispatched.events)), - Effect.catchCause((cause) => Queue.failCause(results, cause)), - Effect.asVoid, - FiberSet.run(settlements, { startImmediately: true }), - ), - ), + Stream.flatMap((event) => { + if (event.type !== "tool-call" || event.providerExecuted) return Stream.make(event) + const call = repairToolCall(tools, event) + return Stream.make(call).pipe( + Stream.concat( + Stream.fromEffectDrain( + ToolRuntime.dispatch(tools, call).pipe( + Effect.flatMap((dispatched) => Queue.offerAll(results, dispatched.events)), + Effect.catchCause((cause) => Queue.failCause(results, cause)), + Effect.asVoid, + FiberSet.run(settlements, { startImmediately: true }), ), ), - ), + ), + ) + }), Stream.concat( Stream.fromEffectDrain( FiberSet.awaitEmpty(settlements).pipe(Effect.andThen(Queue.end(results)), Effect.asVoid), @@ -150,6 +151,18 @@ export function stream(input: StreamInput): StreamResult { } } +function repairToolCall(tools: ReturnType, call: ToolCallPart): ToolCallPart { + if (tools[call.name]) return call + const lower = call.name.toLowerCase() + if (tools[lower]) return { ...call, name: lower } + if (!tools.invalid) return call + return { + ...call, + name: "invalid", + input: { tool: call.name, error: `Unknown tool: ${call.name}` }, + } +} + function providerFetch(input: Pick): typeof globalThis.fetch | undefined { if (input.provider.id !== "openai" || input.auth?.type !== "oauth") return undefined const value: unknown = input.provider.options.fetch diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index 31859649f0..5a60c1c53f 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/packages/opencode/test/permission/next.test.ts @@ -165,6 +165,20 @@ test.skipIf(process.platform === "win32")( }, ) +test.skipIf(process.platform === "win32")( + "sensitive_write does not treat a POSIX backslash as a path separator", + () => { + const literal = "/tmp/secrets\\credentials" + const descendant = "/tmp/secrets/credentials" + const config = { "*": "deny" as const, sensitive_write: { [literal]: "allow" as const } } + + expect(Permission.evaluate("sensitive_write", descendant, Permission.fromConfig(config)).action).toBe("deny") + expect(LegacyPermission.evaluate("sensitive_write", descendant, LegacyPermission.fromConfig(config)).action).toBe( + "deny", + ) + }, +) + test("fromConfig - expands $HOME to home directory", () => { const result = Permission.fromConfig({ external_directory: { "$HOME/projects/*": "allow" } }) expect(result).toEqual([{ permission: "external_directory", pattern: `${os.homedir()}/projects/*`, action: "allow" }]) @@ -642,8 +656,14 @@ it.instance( metadata: { permissionAliases: { "ls /project": [["ls .."]] } }, always: [], ruleset: [{ permission: "bash", pattern: "ls /project", action: "deny" }], - }).then(() => "allowed", (err: unknown) => err) - const result = await Promise.race([request, new Promise((resolve) => setTimeout(() => resolve("pending"), 100))]) + }).then( + () => "allowed", + (err: unknown) => err, + ) + const result = await Promise.race([ + request, + new Promise((resolve) => setTimeout(() => resolve("pending"), 100)), + ]) for (const item of await LegacyPermission.list()) { await LegacyPermission.reply({ requestID: item.id, reply: "reject" }) } diff --git a/packages/opencode/test/session/llm-native.test.ts b/packages/opencode/test/session/llm-native.test.ts index c1157377b6..4e3c69bcb6 100644 --- a/packages/opencode/test/session/llm-native.test.ts +++ b/packages/opencode/test/session/llm-native.test.ts @@ -558,22 +558,35 @@ describe("session.llm-native.request", () => { it.effect("keeps the invalid fallback dispatchable without advertising it to the provider", () => Effect.gen(function* () { let advertised: string[] = [] + let invalidInput: unknown + let visibleCalls = 0 const invalid = tool({ description: "Internal fallback", - inputSchema: jsonSchema({ type: "object" }), - execute: async () => "fallback", + inputSchema: jsonSchema<{ tool: string; error: string }>({ + type: "object", + properties: { tool: { type: "string" }, error: { type: "string" } }, + required: ["tool", "error"], + }), + execute: async (input) => { + invalidInput = input + return "fallback" + }, }) const visible = tool({ description: "Visible tool", inputSchema: jsonSchema({ type: "object" }), - execute: async () => "visible", + execute: async () => { + visibleCalls++ + return "visible" + }, }) const llmClient = { prepare: () => Effect.die("unused"), stream: (request: LLMRequest) => { advertised = request.tools.map((item) => item.name) return Stream.fromIterable([ - LLMEvent.toolCall({ id: "call-invalid", name: "invalid", input: {} }), + LLMEvent.toolCall({ id: "call-visible", name: "VISIBLE", input: {} }), + LLMEvent.toolCall({ id: "call-invalid", name: "write", input: {} }), LLMEvent.finish({ reason: "tool-calls" }), ]) }, @@ -594,8 +607,12 @@ describe("session.llm-native.request", () => { const events = Array.from(yield* native.stream.pipe(Stream.runCollect)) expect(advertised).toEqual(["visible"]) + expect(visibleCalls).toBe(1) + expect(invalidInput).toEqual({ tool: "write", error: "Unknown tool: write" }) expect(events).toEqual( expect.arrayContaining([ + expect.objectContaining({ type: "tool-call", name: "visible" }), + expect.objectContaining({ type: "tool-result", name: "visible" }), expect.objectContaining({ type: "tool-call", name: "invalid" }), expect.objectContaining({ type: "tool-result", name: "invalid" }), ]), From f48cac90eeaf73c22463f2ad2bccbf349b5ebeb9 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 16:05:39 +0530 Subject: [PATCH 18/25] docs(analyst): document scratch table exception --- docs/docs/getting-started/quickstart.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/docs/getting-started/quickstart.md b/docs/docs/getting-started/quickstart.md index 36a7c64563..3c08553923 100644 --- a/docs/docs/getting-started/quickstart.md +++ b/docs/docs/getting-started/quickstart.md @@ -147,11 +147,11 @@ Verify your connection: altimate ships with specialized agent modes, each with its own tool permissions: -| Mode | Access | Use when you want to... | -| ----------- | ---------- | ------------------------------------------------------------------------------ | -| **Builder** | Read/Write | Create and modify SQL, dbt models, pipelines. SQL writes prompt for approval. | -| **Analyst** | Read-only | Ask questions about your data, explore production data safely, run cost analysis. SQL writes denied entirely. | -| **Plan** | Minimal | Plan an approach before switching to builder to execute it | +| Mode | Access | Use when you want to... | +| ----------- | ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Builder** | Read/Write | Create and modify SQL, dbt models, pipelines. SQL writes prompt for approval. | +| **Analyst** | Read-only | Ask questions about your data, explore production data safely, run cost analysis. Production SQL writes are denied; session scratch tables are allowed. | +| **Plan** | Minimal | Plan an approach before switching to builder to execute it | Switch modes in the TUI: @@ -251,7 +251,7 @@ Set tool permissions for each agent mode in `altimate-code.json`: } ``` -Analyst's shell and built-in write tools are always denied, so config cannot turn them on. It can still save training entries, and MCP or custom tools follow your config. +Analyst's shell and built-in write tools are always denied, so config cannot turn them on. It can still save training entries and create session scratch tables, and MCP or custom tools follow your config. ### Project rules with AGENTS.md @@ -268,11 +268,11 @@ Define project-wide conventions in an `AGENTS.md` file at your project root. The ### Default permissions by agent mode -| Agent | File writes | SQL writes | Bash | Training | -| ------- | ----------- | -------------------------- | --------------------------------- | -------- | -| Builder | allow | ask (prompts for approval) | ask | allow | -| Analyst | deny | deny (blocked entirely) | deny | allow | -| Plan | deny | deny | deny | deny | +| Agent | File writes | SQL writes | Bash | Training | +| ------- | ----------- | --------------------------- | ---- | -------- | +| Builder | allow | ask (prompts for approval) | ask | allow | +| Analyst | deny | deny, except scratch tables | deny | allow | +| Plan | deny | deny | deny | deny | --- From 803511c7e468b270431f7dc65ad5f786e57a73d8 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 16:09:17 +0530 Subject: [PATCH 19/25] chore(upstream): guard wildcard path changes --- packages/core/src/util/wildcard.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/util/wildcard.ts b/packages/core/src/util/wildcard.ts index 65dc6ed752..694f7a0ef2 100644 --- a/packages/core/src/util/wildcard.ts +++ b/packages/core/src/util/wildcard.ts @@ -1,15 +1,14 @@ export * as Wildcard from "./wildcard" +// altimate_change start — preserve POSIX backslashes when matching filesystem paths export function match(input: string, pattern: string) { return matchNormalized(input.replaceAll("\\", "/"), pattern.replaceAll("\\", "/")) } -// altimate_change start — preserve POSIX backslashes when matching filesystem paths export function matchPath(input: string, pattern: string) { if (process.platform === "win32") return match(input, pattern) return matchNormalized(input, pattern) } -// altimate_change end function matchNormalized(input: string, pattern: string) { let escaped = pattern @@ -21,3 +20,4 @@ function matchNormalized(input: string, pattern: string) { return new RegExp("^" + escaped + "$", process.platform === "win32" ? "si" : "s").test(input) } +// altimate_change end From 5a4baca7ad0e6ce1e00ae5ace70ed3e7da319ebf Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 16:22:18 +0530 Subject: [PATCH 20/25] fix(permission): harden tool name repair --- .../opencode/src/session/llm/native-runtime.ts | 7 ++++--- packages/opencode/src/util/wildcard.ts | 17 ++++++----------- .../opencode/test/session/llm-native.test.ts | 10 +++++++--- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/opencode/src/session/llm/native-runtime.ts b/packages/opencode/src/session/llm/native-runtime.ts index 1013f23bfd..a103112f39 100644 --- a/packages/opencode/src/session/llm/native-runtime.ts +++ b/packages/opencode/src/session/llm/native-runtime.ts @@ -152,10 +152,11 @@ export function stream(input: StreamInput): StreamResult { } function repairToolCall(tools: ReturnType, call: ToolCallPart): ToolCallPart { - if (tools[call.name]) return call + const hasTool = (name: string) => Object.prototype.hasOwnProperty.call(tools, name) + if (hasTool(call.name)) return call const lower = call.name.toLowerCase() - if (tools[lower]) return { ...call, name: lower } - if (!tools.invalid) return call + if (hasTool(lower)) return { ...call, name: lower } + if (!hasTool("invalid")) return call return { ...call, name: "invalid", diff --git a/packages/opencode/src/util/wildcard.ts b/packages/opencode/src/util/wildcard.ts index 768eb6981c..21b7bf1c93 100644 --- a/packages/opencode/src/util/wildcard.ts +++ b/packages/opencode/src/util/wildcard.ts @@ -1,17 +1,8 @@ import { sortBy, pipe } from "remeda" export function match(str: string, pattern: string) { - return matchNormalized(str?.replaceAll("\\", "/"), pattern?.replaceAll("\\", "/")) -} - -// altimate_change start — preserve POSIX backslashes when matching filesystem paths -export function matchPath(str: string, pattern: string) { - if (process.platform === "win32") return match(str, pattern) - return matchNormalized(str, pattern) -} -// altimate_change end - -function matchNormalized(str: string, pattern: string) { + if (str) str = str.replaceAll("\\", "/") + if (pattern) pattern = pattern.replaceAll("\\", "/") let escaped = pattern .replace(/[.+^${}()|[\]\\]/g, "\\$&") // escape special regex chars .replace(/\*/g, ".*") // * becomes .* @@ -27,6 +18,10 @@ function matchNormalized(str: string, pattern: string) { return new RegExp("^" + escaped + "$", flags).test(str) } +// altimate_change start — preserve POSIX backslashes when matching filesystem paths +export { matchPath } from "@opencode-ai/core/util/wildcard" +// altimate_change end + export function all(input: string, patterns: Record) { const sorted = pipe(patterns, Object.entries, sortBy([([key]) => key.length, "asc"], [([key]) => key, "asc"])) let result = undefined diff --git a/packages/opencode/test/session/llm-native.test.ts b/packages/opencode/test/session/llm-native.test.ts index 4e3c69bcb6..b841cbb871 100644 --- a/packages/opencode/test/session/llm-native.test.ts +++ b/packages/opencode/test/session/llm-native.test.ts @@ -558,7 +558,7 @@ describe("session.llm-native.request", () => { it.effect("keeps the invalid fallback dispatchable without advertising it to the provider", () => Effect.gen(function* () { let advertised: string[] = [] - let invalidInput: unknown + const invalidInputs: unknown[] = [] let visibleCalls = 0 const invalid = tool({ description: "Internal fallback", @@ -568,7 +568,7 @@ describe("session.llm-native.request", () => { required: ["tool", "error"], }), execute: async (input) => { - invalidInput = input + invalidInputs.push(input) return "fallback" }, }) @@ -587,6 +587,7 @@ describe("session.llm-native.request", () => { return Stream.fromIterable([ LLMEvent.toolCall({ id: "call-visible", name: "VISIBLE", input: {} }), LLMEvent.toolCall({ id: "call-invalid", name: "write", input: {} }), + LLMEvent.toolCall({ id: "call-inherited", name: "constructor", input: {} }), LLMEvent.finish({ reason: "tool-calls" }), ]) }, @@ -608,7 +609,10 @@ describe("session.llm-native.request", () => { const events = Array.from(yield* native.stream.pipe(Stream.runCollect)) expect(advertised).toEqual(["visible"]) expect(visibleCalls).toBe(1) - expect(invalidInput).toEqual({ tool: "write", error: "Unknown tool: write" }) + expect(invalidInputs).toEqual([ + { tool: "write", error: "Unknown tool: write" }, + { tool: "constructor", error: "Unknown tool: constructor" }, + ]) expect(events).toEqual( expect.arrayContaining([ expect.objectContaining({ type: "tool-call", name: "visible" }), From 6b142167885b572dd2664dbf966bf166291b3d6e Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Mon, 26 Oct 2026 16:40:59 +0530 Subject: [PATCH 21/25] fix(llm): reject inherited tool names --- packages/opencode/src/session/llm.ts | 7 +- packages/opencode/test/session/llm.test.ts | 94 ++++++++++++++++++++++ 2 files changed, 99 insertions(+), 2 deletions(-) diff --git a/packages/opencode/src/session/llm.ts b/packages/opencode/src/session/llm.ts index 103e20fbde..1a1f97a60b 100644 --- a/packages/opencode/src/session/llm.ts +++ b/packages/opencode/src/session/llm.ts @@ -266,7 +266,10 @@ export namespace LLM { // altimate_change start — finalization must have no executable catalog, even for unsolicited tool calls const runBudget = await sessionRunBudget(input.sessionID) - const tools: Record = runBudget?.toolsDisabled ? {} : await resolveTools(input) + const resolvedTools: Record = runBudget?.toolsDisabled ? {} : await resolveTools(input) + // The AI SDK checks tool names by property lookup. A null-prototype catalog prevents + // inherited Object properties (for example, "constructor") from masquerading as tools. + const tools: Record = Object.assign(Object.create(null), resolvedTools) const toolChoice = runBudget?.toolsDisabled ? "none" : input.toolChoice // altimate_change end @@ -386,7 +389,7 @@ export namespace LLM { // altimate_change end async experimental_repairToolCall(failed) { const lower = failed.toolCall.toolName.toLowerCase() - if (lower !== failed.toolCall.toolName && tools[lower]) { + if (lower !== failed.toolCall.toolName && Object.prototype.hasOwnProperty.call(tools, lower)) { l.info("repairing tool call", { tool: failed.toolCall.toolName, repaired: lower, diff --git a/packages/opencode/test/session/llm.test.ts b/packages/opencode/test/session/llm.test.ts index 6f0e21090c..779e95346f 100644 --- a/packages/opencode/test/session/llm.test.ts +++ b/packages/opencode/test/session/llm.test.ts @@ -398,6 +398,100 @@ describe("session.llm.stream", () => { }) }) + test("inherited object property names report through the hidden fallback", async () => { + const providerID = "alibaba" + const { model } = await loadFixture(providerID, "qwen-plus") + const request = waitRequest( + "/chat/completions", + createEventResponse( + [ + { + id: "chatcmpl-inherited", + object: "chat.completion.chunk", + choices: [ + { + delta: { + role: "assistant", + tool_calls: [ + { + index: 0, + id: "call-inherited", + type: "function", + function: { name: "constructor", arguments: "{}" }, + }, + ], + }, + }, + ], + }, + { + id: "chatcmpl-inherited", + object: "chat.completion.chunk", + choices: [{ delta: {}, finish_reason: "tool_calls" }], + }, + ], + true, + ), + ) + await using tmp = await tmpdir({ + config: { + enabled_providers: [providerID], + provider: { [providerID]: { options: { apiKey: "test-key", baseURL: `${state.server!.url.origin}/v1` } } }, + }, + }) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const resolved = await Provider.getModel(ProviderID.make(providerID), ModelID.make(model.id)) + const sessionID = SessionID.make("session-inherited-tool") + const agent = { + name: "custom-builder", + mode: "primary", + options: {}, + permission: PermissionNext.fromConfig({ "*": "allow" }), + } satisfies Agent.Info + const stream = await LLM.stream({ + user: { + id: MessageID.make("msg_inherited-tool"), + sessionID, + role: "user", + time: { created: Date.now() }, + agent: agent.name, + model: { providerID: ProviderID.make(providerID), modelID: resolved.id }, + }, + sessionID, + model: resolved, + agent, + system: [], + abort: new AbortController().signal, + messages: [{ role: "user", content: "Call the requested tool" }], + tools: { + read: tool({ inputSchema: jsonSchema({ type: "object", properties: {} }), execute: async () => "read" }), + invalid: tool({ + inputSchema: jsonSchema<{ tool: string; error: string }>({ + type: "object", + properties: { tool: { type: "string" }, error: { type: "string" } }, + required: ["tool", "error"], + }), + execute: async (input) => input, + }), + }, + }) + const events = [] + for await (const event of stream.fullStream) events.push(event) + expect(events.find((event) => event.type === "tool-result")).toMatchObject({ + toolName: "invalid", + output: { tool: "constructor", error: expect.stringContaining("constructor") }, + }) + expect(events.some((event) => event.type === "tool-error" || event.type === "error")).toBe(false) + const capture = await request + expect((capture.body.tools as { function: { name: string } }[]).map((item) => item.function.name)).toEqual([ + "read", + ]) + }, + }) + }) + test.skip("sends temperature, tokens, and reasoning options for openai-compatible models", async () => { const server = state.server if (!server) { From a0b1eea1a2bd8e1c975ab8077272df694376aabe Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Tue, 27 Oct 2026 16:30:36 +0530 Subject: [PATCH 22/25] fix(permission): let resolved twins only tighten, and keep project files spelled through a link inside A generated allow twin was appended right after its rule, so it could land after a deny the user wrote for the real path and win under last-match-wins: {"/private/tmp/*": "deny", "/tmp/*": "allow"} allowed /private/tmp/x. Allow twins now go before the authored rules of the same config, so an authored deny or ask for the real path keeps winning while an unmatched real path still gets the allow. Deny and ask twins still follow their rule. The in-project check compared the authored path lexically against the realpath'd project directory, so a project file spelled through a link to the project (/var vs /private/var, a symlinked ~/code) was treated as external: builder prompted and analyst refused its own files. The authored side is now decided on the real location of the parent, so an outside link entry still asks while files under it stay inside. The link-budget error carries code ELOOP, and a rule whose resolved twin cannot be built is logged at debug level. Docs: the agent permissions example uses builder, since analyst's shell is always denied, and the analyst pages say a global "*": "allow" also turns on MCP and custom tools for analyst, including ones that write. Tests: twin vs an authored deny in both orders, a project file spelled through a link in both check variants, patterned analyst grants, and two skill folders with a sibling folder sharing a prefix. Co-Authored-By: Claude Opus 5.5 --- docs/docs/configure/agents.md | 6 ++- docs/docs/configure/permissions.md | 2 +- docs/docs/data-engineering/agent-modes.md | 2 +- packages/opencode/src/permission/index.ts | 23 ++++++-- .../opencode/src/tool/external-directory.ts | 7 ++- packages/opencode/src/util/filesystem.ts | 3 +- .../test/agent/analyst-data-explorer.test.ts | 53 +++++++++++++++++++ .../opencode/test/permission/next.test.ts | 27 +++++++++- .../test/tool/external-directory.test.ts | 34 ++++++++++-- 9 files changed, 142 insertions(+), 15 deletions(-) diff --git a/docs/docs/configure/agents.md b/docs/docs/configure/agents.md index 797c59351e..1e307fb06b 100644 --- a/docs/docs/configure/agents.md +++ b/docs/docs/configure/agents.md @@ -29,7 +29,7 @@ The agent for asking questions about your data. Truly read-only mode for safe da - **Web**: Fetch and search allowed without prompts - **Schema/warehouse/finops**: All inspection tools available - **Workspace knowledge**: Loads workspace and project skills; reads workspace memory and views this checkout's memory audit log. Can read files bundled with skills, including global skills stored outside the project. Reloading memory syncs the local copy with the workspace, which can remove a local copy of a block the workspace archived. Saving memory is done with `builder`. -- **Config overrides**: A permission config such as `"*": "allow"` cannot turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on +- **Config overrides**: A permission config such as `"*": "allow"` cannot turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on A global `"*": "allow"` does turn on MCP and custom tools for analyst, including ones that write (for example a SQL MCP server); to keep them from analyst, put the `"*": "allow"` under `agent.builder` instead of the global config, or deny those tools under `agent.analyst.permission`. !!! tip Use `analyst` when exploring data to ensure no accidental writes. Switch to `builder` when you're ready to create or modify models. @@ -122,7 +122,7 @@ Each agent can have its own permission overrides: ```json { "agent": { - "analyst": { + "builder": { "permission": { "write": "deny", "edit": "deny", @@ -138,6 +138,8 @@ Each agent can have its own permission overrides: } ``` +`analyst` already denies its shell and built-in write tools whatever its config says, so its config is only useful to restrict it further (for example `"webfetch": "deny"`) or to allow MCP and custom tools. + !!! warning Agent-specific permissions override global permissions. A `"deny"` at the agent level cannot be overridden by a global `"allow"`. diff --git a/docs/docs/configure/permissions.md b/docs/docs/configure/permissions.md index b644fed08d..65bbc9393e 100644 --- a/docs/docs/configure/permissions.md +++ b/docs/docs/configure/permissions.md @@ -80,7 +80,7 @@ Override permissions for specific agents: } ``` -The `analyst` agent's shell and built-in write tools (file edits, SQL writes, memory saves and the other built-in tools that change state) stay denied even if its permission config allows them. Training entries remain available through `training_save` and `training_remove`, and analyst can create session scratch tables. MCP and custom tools follow your config, and config can still restrict analyst further. +The `analyst` agent's shell and built-in write tools (file edits, SQL writes, memory saves and the other built-in tools that change state) stay denied even if its permission config allows them. Training entries remain available through `training_save` and `training_remove`, and analyst can create session scratch tables. MCP and custom tools follow your config, so a global `"*": "allow"` turns them on for analyst too, including ones that write. Config can still restrict analyst further. Rules are evaluated in order and the last matching rule wins, so put a `"*"` rule before the more specific patterns it should not override. diff --git a/docs/docs/data-engineering/agent-modes.md b/docs/docs/data-engineering/agent-modes.md index 50ee53e2ac..86a763e97f 100644 --- a/docs/docs/data-engineering/agent-modes.md +++ b/docs/docs/data-engineering/agent-modes.md @@ -90,7 +90,7 @@ Analyst mode enforces strict guardrails: - **No file writes.** Cannot modify your codebase - **No shell.** Bash is denied. Files are read with the read, grep and glob tools - **No memory saves.** Workspace memory blocks can be read, but not saved, extracted or deleted (use builder). Training entries can still be saved and removed -- **Config cannot lift these.** A permission config such as `"*": "allow"` does not turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on for analyst +- **Config cannot lift these.** A permission config such as `"*": "allow"` does not turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on for analyst MCP and custom tools are not covered: a global `"*": "allow"` turns them on for analyst too, including ones that write. - **Skill files readable.** Files bundled with a skill, including global skills stored outside the project, can be read - **Full web access.** Fetch and search allowed without prompts - **All inspection tools.** Schema, warehouse, finops, lineage, and core analysis tools available diff --git a/packages/opencode/src/permission/index.ts b/packages/opencode/src/permission/index.ts index f325cb477b..c6e41f8bdc 100644 --- a/packages/opencode/src/permission/index.ts +++ b/packages/opencode/src/permission/index.ts @@ -8,6 +8,11 @@ import os from "os" import path from "path" import { FSUtil } from "@opencode-ai/core/fs-util" import { Filesystem } from "@/util/filesystem" +// altimate_change start — log rules whose resolved twin cannot be built +import { Log } from "@/util/log" + +const log = Log.create({ service: "permission" }) +// altimate_change end // altimate_change end import { PermissionV1 } from "@opencode-ai/core/v1/permission" import { EventV2Bridge } from "@/event-v2-bridge" @@ -216,12 +221,21 @@ export function fromConfig(permission: ConfigPermissionV1.Info) { continue } // altimate_change start — external_directory also asks for a path's symlink-resolved directory, - // so a directory rule covers its resolved spelling too (e.g. macOS /tmp -> /private/tmp) + // so a directory rule covers its resolved spelling too (e.g. macOS /tmp -> /private/tmp). + // A generated twin may only tighten: deny/ask twins follow their rule, but allow twins go + // before this map's authored rules, so a deny or ask the user wrote for the real path still + // wins (last match wins) while an unmatched real path still gets the allow. + const allowTwins: PermissionV1.Rule[] = [] + const rules: PermissionV1.Rule[] = [] for (const [pattern, action] of Object.entries(value)) { - ruleset.push({ permission: key, pattern: expand(pattern), action }) + rules.push({ permission: key, pattern: expand(pattern), action }) const resolved = key === "external_directory" ? resolvedDirectoryPattern(expand(pattern)) : undefined - if (resolved) ruleset.push({ permission: key, pattern: resolved, action }) + if (!resolved) continue + const twin = { permission: key, pattern: resolved, action } + if (action === "allow") allowTwins.push(twin) + else rules.push(twin) } + ruleset.push(...allowTwins, ...rules) // altimate_change end } return ruleset @@ -238,7 +252,8 @@ function resolvedDirectoryPattern(pattern: string) { const resolved = (() => { try { return Filesystem.realLocation(dir) - } catch { + } catch (error) { + log.debug("no resolved twin for external_directory rule", { pattern, error: String(error) }) return undefined } })() diff --git a/packages/opencode/src/tool/external-directory.ts b/packages/opencode/src/tool/external-directory.ts index 93996c26d8..9ea9225d45 100644 --- a/packages/opencode/src/tool/external-directory.ts +++ b/packages/opencode/src/tool/external-directory.ts @@ -41,7 +41,12 @@ function authoredPath(target: string) { return path.resolve(FSUtil.windowsPath(target)) } -function containsAuthoredPath(filepath: string, directory: string, worktree: string) { +function containsAuthoredPath(target: string, directory: string, worktree: string) { + // Resolve the parent, not the entry: an outside link into the project stays outside (its + // delete or move acts on the outside entry), while a project file spelled through a link to + // the project (e.g. /var -> /private/var) stays inside. + const parent = Filesystem.realLocation(path.dirname(target)) + const filepath = parent ? path.join(parent, path.basename(target)) : target if (FSUtil.contains(directory, filepath)) return true if (worktree === "/") return false return FSUtil.contains(worktree, filepath) diff --git a/packages/opencode/src/util/filesystem.ts b/packages/opencode/src/util/filesystem.ts index 0f81fdc63c..7831bf18f7 100644 --- a/packages/opencode/src/util/filesystem.ts +++ b/packages/opencode/src/util/filesystem.ts @@ -251,7 +251,8 @@ export namespace Filesystem { } catch { return undefined } - if (--budget.links < 0) throw new Error(`Too many symbolic links while resolving ${link}`) + if (--budget.links < 0) + throw Object.assign(new Error(`Too many symbolic links while resolving ${link}`), { code: "ELOOP" }) // Concatenated, not joined: join would collapse ".." in the target before the walk follows it. return resolveLocation(isAbsolute(target) ? target : `${dir}${sep}${target}`, budget) } diff --git a/packages/opencode/test/agent/analyst-data-explorer.test.ts b/packages/opencode/test/agent/analyst-data-explorer.test.ts index 77f55aba31..055fc82852 100644 --- a/packages/opencode/test/agent/analyst-data-explorer.test.ts +++ b/packages/opencode/test/agent/analyst-data-explorer.test.ts @@ -567,3 +567,56 @@ if (process.platform !== "win32") { { config: { skills: { paths: [externalSkills] } } }, ) } + +// A second skill root, plus a sibling folder whose name only extends a skill folder's name. +const otherSkills = fs.mkdtempSync(path.join(os.tmpdir(), "analyst-team-skills-")) +fs.mkdirSync(path.join(otherSkills, "style"), { recursive: true }) +fs.writeFileSync( + path.join(otherSkills, "style", "SKILL.md"), + "---\nname: style\ndescription: SQL style.\n---\n\nSee style.md.\n", +) +fs.mkdirSync(path.join(externalSkills, "naming-evil"), { recursive: true }) +afterAll(() => fs.rmSync(otherSkills, { recursive: true, force: true })) + +it.instance( + "analyst can read every discovered skill folder but not a sibling folder sharing a prefix", + () => + Effect.gen(function* () { + const analyst = (yield* Agent.use.get("analyst"))! + expect(action(analyst, "external_directory", path.join(externalSkills, "naming", "*"))).toBe("allow") + expect(action(analyst, "external_directory", path.join(otherSkills, "style", "*"))).toBe("allow") + expect(action(analyst, "external_directory", path.join(externalSkills, "naming-evil", "*"))).toBe("deny") + expect(action(analyst, "external_directory", path.join(externalSkills, "*"))).toBe("deny") + }), + { config: { skills: { paths: [externalSkills, otherSkills] } } }, +) + +// Patterned grants match only some inputs; the re-applied "*" deny still wins for every input. +it.instance( + "analyst keeps write tools denied under patterned config grants", + () => + Effect.gen(function* () { + const analyst = (yield* Agent.use.get("analyst"))! + expect(action(analyst, "sql_execute_write", "INSERT INTO t VALUES (1)")).toBe("deny") + expect(action(analyst, "edit", "models/orders.sql")).toBe("deny") + expect(action(analyst, "bash", "ls -la")).toBe("deny") + expect(action(analyst, "altimate_memory_write", "naming-rule")).toBe("deny") + expect(Permission.disabled(["sql_execute_write", "edit", "bash"], analyst.permission)).toEqual( + new Set(["sql_execute_write", "edit", "bash"]), + ) + }), + { + config: { + agent: { + analyst: { + permission: { + sql_execute_write: { "INSERT *": "allow" }, + edit: { "models/*": "allow" }, + bash: { "ls *": "allow" }, + altimate_memory_write: { "naming-*": "allow" }, + }, + }, + }, + }, + }, +) diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index 5a60c1c53f..545666301e 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/packages/opencode/test/permission/next.test.ts @@ -128,13 +128,14 @@ test.skipIf(process.platform === "win32")( try { fs.mkdirSync(path.join(root, "real")) fs.symlinkSync(path.join(root, "real"), path.join(root, "link")) + // An allow twin goes before the authored rules, so it never outranks one of them. expect(Permission.fromConfig({ external_directory: { [path.join(root, "link", "*")]: "allow" } })).toEqual([ - { permission: "external_directory", pattern: path.join(root, "link", "*"), action: "allow" }, { permission: "external_directory", pattern: path.join(root, "real", "*"), action: "allow" }, + { permission: "external_directory", pattern: path.join(root, "link", "*"), action: "allow" }, ]) // Wildcard rules keep their wildcard part, and a directory that does not exist yet resolves // through its nearest existing ancestor. - expect(Permission.fromConfig({ external_directory: { [path.join(root, "link", "**")]: "allow" } })[1]).toEqual({ + expect(Permission.fromConfig({ external_directory: { [path.join(root, "link", "**")]: "allow" } })[0]).toEqual({ permission: "external_directory", pattern: path.join(root, "real", "**"), action: "allow", @@ -142,6 +143,28 @@ test.skipIf(process.platform === "win32")( expect( Permission.fromConfig({ external_directory: { [path.join(root, "link", "later", "*")]: "deny" } })[1], ).toEqual({ permission: "external_directory", pattern: path.join(root, "real", "later", "*"), action: "deny" }) + // A generated twin only tightens: a deny written for the real path keeps winning over a later + // allow written for the link, in either order, while an unmatched real path still gets the allow. + const deny = (first: "deny" | "allow") => + Permission.fromConfig({ + external_directory: + first === "deny" + ? { [path.join(root, "real", "secret", "*")]: "deny", [path.join(root, "link", "*")]: "allow" } + : { [path.join(root, "link", "*")]: "allow", [path.join(root, "real", "secret", "*")]: "deny" }, + }) + for (const first of ["deny", "allow"] as const) { + const rules = deny(first) + const at = (...p: string[]) => + Permission.evaluate("external_directory", path.join(root, ...p, "*"), rules).action + expect(at("real", "secret"), first).toBe("deny") + expect(at("real", "other"), first).toBe("allow") + expect(at("link", "other"), first).toBe("allow") + } + // A deny twin still follows its rule, so it tightens the real spelling too. + const denyLink = Permission.fromConfig({ + external_directory: { [path.join(root, "*")]: "allow", [path.join(root, "link", "*")]: "deny" }, + }) + expect(Permission.evaluate("external_directory", path.join(root, "real", "x", "*"), denyLink).action).toBe("deny") // Other permissions and non-symlinked directories get no extra rule. expect(Permission.fromConfig({ read: { [path.join(root, "link", "*")]: "allow" } })).toHaveLength(1) expect(Permission.fromConfig({ external_directory: { [path.join(root, "real", "*")]: "allow" } })).toHaveLength(1) diff --git a/packages/opencode/test/tool/external-directory.test.ts b/packages/opencode/test/tool/external-directory.test.ts index 0524b4592d..3dbda45c5a 100644 --- a/packages/opencode/test/tool/external-directory.test.ts +++ b/packages/opencode/test/tool/external-directory.test.ts @@ -72,11 +72,12 @@ describe("tool.assertExternalDirectory", () => { const outside = yield* tmpdirScoped() const link = path.join(outside, "project-link") fs.symlinkSync(test.directory, link, process.platform === "win32" ? "junction" : "dir") - const target = path.join(link, "file.txt") + // The link entry itself: deleting or moving it acts on the outside directory. + const target = link const expected = process.platform === "win32" - ? path.join(path.resolve(Filesystem.windowsPath(link)), "*") - : glob(path.join(link, "*")) + ? path.join(path.resolve(Filesystem.windowsPath(outside)), "*") + : glob(path.join(outside, "*")) const { requests, ctx } = makeCtx() yield* assertExternalDirectoryEffect(ctx, target) @@ -96,6 +97,33 @@ describe("tool.assertExternalDirectory", () => { }), ) + it.instance("treats a project file spelled through a link to the project as inside it", () => + Effect.gen(function* () { + const test = yield* TestInstance + const outside = yield* tmpdirScoped() + const link = path.join(outside, "project-link") + fs.symlinkSync(test.directory, link, process.platform === "win32" ? "junction" : "dir") + // Same shape as /var/... for a project at /private/var/... on macOS. + const target = path.join(link, "file.txt") + + const { requests, ctx } = makeCtx() + yield* assertExternalDirectoryEffect(ctx, target) + expect(requests).toEqual([]) + + const legacy: string[][] = [] + const instance = yield* InstanceState.context + yield* Effect.promise(() => + Instance.restore(instance, () => + assertExternalDirectoryLegacy( + { ...baseCtx, metadata: () => {}, ask: async (req) => void legacy.push([...req.patterns]) }, + target, + ), + ), + ) + expect(legacy).toEqual([]) + }), + ) + if (process.platform !== "win32") { it.instance("preserves a literal backslash in an external POSIX directory", () => Effect.gen(function* () { From 8eedd7ecf352e4878a93765c167838dbfbfeb7c8 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Tue, 27 Oct 2026 16:45:06 +0530 Subject: [PATCH 23/25] chore(analyst): close one marker block around the permission log, and end run-on doc sentences Co-Authored-By: Claude Opus 5.5 --- docs/docs/configure/agents.md | 2 +- docs/docs/data-engineering/agent-modes.md | 2 +- packages/opencode/src/permission/index.ts | 2 -- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/docs/configure/agents.md b/docs/docs/configure/agents.md index 1e307fb06b..919b8a7ce4 100644 --- a/docs/docs/configure/agents.md +++ b/docs/docs/configure/agents.md @@ -29,7 +29,7 @@ The agent for asking questions about your data. Truly read-only mode for safe da - **Web**: Fetch and search allowed without prompts - **Schema/warehouse/finops**: All inspection tools available - **Workspace knowledge**: Loads workspace and project skills; reads workspace memory and views this checkout's memory audit log. Can read files bundled with skills, including global skills stored outside the project. Reloading memory syncs the local copy with the workspace, which can remove a local copy of a block the workspace archived. Saving memory is done with `builder`. -- **Config overrides**: A permission config such as `"*": "allow"` cannot turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on A global `"*": "allow"` does turn on MCP and custom tools for analyst, including ones that write (for example a SQL MCP server); to keep them from analyst, put the `"*": "allow"` under `agent.builder` instead of the global config, or deny those tools under `agent.analyst.permission`. +- **Config overrides**: A permission config such as `"*": "allow"` cannot turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on. A global `"*": "allow"` does turn on MCP and custom tools for analyst, including ones that write (for example a SQL MCP server); to keep them from analyst, put the `"*": "allow"` under `agent.builder` instead of the global config, or deny those tools under `agent.analyst.permission`. !!! tip Use `analyst` when exploring data to ensure no accidental writes. Switch to `builder` when you're ready to create or modify models. diff --git a/docs/docs/data-engineering/agent-modes.md b/docs/docs/data-engineering/agent-modes.md index 86a763e97f..b254bc1b7c 100644 --- a/docs/docs/data-engineering/agent-modes.md +++ b/docs/docs/data-engineering/agent-modes.md @@ -90,7 +90,7 @@ Analyst mode enforces strict guardrails: - **No file writes.** Cannot modify your codebase - **No shell.** Bash is denied. Files are read with the read, grep and glob tools - **No memory saves.** Workspace memory blocks can be read, but not saved, extracted or deleted (use builder). Training entries can still be saved and removed -- **Config cannot lift these.** A permission config such as `"*": "allow"` does not turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on for analyst MCP and custom tools are not covered: a global `"*": "allow"` turns them on for analyst too, including ones that write. +- **Config cannot lift these.** A permission config such as `"*": "allow"` does not turn file edits, SQL writes (including data diffs of arbitrary queries), shell, memory saves, warehouse connection or MCP config changes, training imports, sample project setup, feedback submission, batch tool calls or delegation beyond `data-explorer` back on for analyst. MCP and custom tools are not covered: a global `"*": "allow"` turns them on for analyst too, including ones that write. - **Skill files readable.** Files bundled with a skill, including global skills stored outside the project, can be read - **Full web access.** Fetch and search allowed without prompts - **All inspection tools.** Schema, warehouse, finops, lineage, and core analysis tools available diff --git a/packages/opencode/src/permission/index.ts b/packages/opencode/src/permission/index.ts index c6e41f8bdc..358ea2e9f7 100644 --- a/packages/opencode/src/permission/index.ts +++ b/packages/opencode/src/permission/index.ts @@ -8,12 +8,10 @@ import os from "os" import path from "path" import { FSUtil } from "@opencode-ai/core/fs-util" import { Filesystem } from "@/util/filesystem" -// altimate_change start — log rules whose resolved twin cannot be built import { Log } from "@/util/log" const log = Log.create({ service: "permission" }) // altimate_change end -// altimate_change end import { PermissionV1 } from "@opencode-ai/core/v1/permission" import { EventV2Bridge } from "@/event-v2-bridge" import { EventV2 } from "@opencode-ai/core/event" From e433449233674866e6d6df6bbebae8cdff745090 Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Tue, 27 Oct 2026 19:24:41 +0530 Subject: [PATCH 24/25] fix(permission): check a symlink-resolved path without rewriting configured rules Generated "twin" rules lived in the configured ruleset, so wherever they landed in rule order they could outrank a rule the user wrote: an agent's allow on a link beat a global deny on the real path, and moving the twins broke a deny-volume / allow-linked-project carve-out instead. Configured rules are no longer rewritten; fromConfig and merge are back to their original behaviour. An external_directory request marks its symlink-resolved directory in metadata.resolvedPatterns, and only that pattern is evaluated with each external_directory rule followed by its own resolved spelling. The path as written is decided by the configured rules alone, and the request needs every pattern allowed, so the extra check can only tighten a decision: a path is never allowed where it was not before, while a symlink from an allowed folder into a folder that is not allowed is still caught. "Always" now remembers only the path as written. Remembering the resolved directory would also allow it when read directly, past rules configured for that real path. Tests decide requests the way production does: the reviewer's global-deny / agent-allow case and the carve-out, an allow carved out of a deny, a link into a folder that is not allowed, a link deny tightening the real path, an approval followed by a direct real-path read, and the metadata and always values both check variants send. Co-Authored-By: Claude Opus 5.5 --- docs/docs/configure/agents.md | 2 +- docs/docs/reference/security-faq.md | 2 +- packages/opencode/src/permission/evaluate.ts | 68 +++++++++- packages/opencode/src/permission/index.ts | 50 +------ .../opencode/src/tool/external-directory.ts | 17 ++- .../test/agent/analyst-data-explorer.test.ts | 32 +++++ .../opencode/test/permission/next.test.ts | 127 ++++++++++++------ .../test/tool/external-directory.test.ts | 16 ++- packages/opencode/test/tool/grep.test.ts | 3 +- 9 files changed, 217 insertions(+), 100 deletions(-) diff --git a/docs/docs/configure/agents.md b/docs/docs/configure/agents.md index 919b8a7ce4..ee3c8b22de 100644 --- a/docs/docs/configure/agents.md +++ b/docs/docs/configure/agents.md @@ -138,7 +138,7 @@ Each agent can have its own permission overrides: } ``` -`analyst` already denies its shell and built-in write tools whatever its config says, so its config is only useful to restrict it further (for example `"webfetch": "deny"`) or to allow MCP and custom tools. +`analyst` already denies its shell and built-in write tools whatever its config says, so its config is mainly useful to restrict it further (for example `"webfetch": "deny"`) or to allow MCP and custom tools. !!! warning Agent-specific permissions override global permissions. A `"deny"` at the agent level cannot be overridden by a global `"allow"`. diff --git a/docs/docs/reference/security-faq.md b/docs/docs/reference/security-faq.md index 7db7608bad..9cc1780d84 100644 --- a/docs/docs/reference/security-faq.md +++ b/docs/docs/reference/security-faq.md @@ -300,7 +300,7 @@ For additional safety: Altimate Code includes several layers of protection to keep the agent within your project: -- **Project boundary enforcement.** File operations check that paths stay within your project directory (or git worktree for monorepos). Attempts to read or write outside the project trigger an `external_directory` permission prompt. A path that goes through a symlink is also checked against the directory it resolves to, so an allowed directory cannot be used to reach a denied one. +- **Project boundary enforcement.** File operations check that paths stay within your project directory (or git worktree for monorepos). Attempts to read or write outside the project trigger an `external_directory` permission prompt. A path that goes through a symlink is also checked against the directory it resolves to, so a symlink inside an allowed directory cannot be used to reach a directory outside it that is not allowed. - **Symlink-aware path resolution.** Symlinks inside the project that point outside are resolved, so following one triggers the same `external_directory` permission check as any other path outside the project. Whether it is allowed depends on that permission. - **Path traversal blocking.** Paths containing `../` sequences that would escape the project are rejected with an "Access denied" error. - **Sensitive file protection.** Writing to credential files (`.env`, `.ssh/`, `.aws/`, private keys) triggers a confirmation prompt, even inside the project. See [below](#why-am-i-being-prompted-to-edit-env-files) for details. diff --git a/packages/opencode/src/permission/evaluate.ts b/packages/opencode/src/permission/evaluate.ts index 7ad7be22ce..0ba15fea5c 100644 --- a/packages/opencode/src/permission/evaluate.ts +++ b/packages/opencode/src/permission/evaluate.ts @@ -1,6 +1,12 @@ // altimate_change start — upstream_fix: match equivalent path spellings with one rule precedence import type { PermissionV1 } from "@opencode-ai/core/v1/permission" import { Wildcard } from "@opencode-ai/core/util/wildcard" +import { FSUtil } from "@opencode-ai/core/fs-util" +import path from "path" +import { Filesystem } from "@/util/filesystem" +import { Log } from "@/util/log" + +const log = Log.create({ service: "permission" }) // `pattern` is the spelling the caller authored; `aliases` are equivalent normalized // spellings. An alias only ever tightens a decision: the result is the strictest of the @@ -14,13 +20,57 @@ function matchPattern(permission: string, input: string, pattern: string) { : Wildcard.match(input, pattern) } +// An external_directory request also asks for the symlink-resolved directory of its path. That +// resolved pattern is evaluated with every external_directory rule followed by its own resolved +// spelling (its "twin"), so an allowed folder reached through a symlink stays allowed while a +// link from an allowed folder into a denied one is caught. The path as written is always decided +// by the configured rules alone, exactly as without this check, and the request needs both, so +// the resolved check can only tighten a decision, never loosen it. +function resolvedTwin(pattern: string) { + const segments = pattern.split(process.platform === "win32" ? /[\\/]/ : "/") + const wildcard = segments.findIndex((segment) => /[*?]/.test(segment)) + if (wildcard <= 0) return + const dir = segments.slice(0, wildcard).join(path.sep) || path.sep + const resolved = (() => { + try { + return Filesystem.realLocation(dir) + } catch (error) { + // A link loop in a configured directory just leaves the rule without a twin. + log.debug("no resolved twin for external_directory rule", { pattern, error: String(error) }) + return undefined + } + })() + if (!resolved || resolved === dir) return + const twin = path.join(resolved, ...segments.slice(wildcard)) + return process.platform === "win32" ? FSUtil.normalizePathPattern(twin) : twin +} + +function withResolvedTwins(rules: PermissionV1.Rule[]) { + const twins = new Map() + const twinOf = (pattern: string) => { + if (!twins.has(pattern)) twins.set(pattern, resolvedTwin(pattern)) + return twins.get(pattern) + } + return rules.flatMap((rule) => { + const twin = rule.permission === "external_directory" ? twinOf(rule.pattern) : undefined + return twin ? [rule, { ...rule, pattern: twin }] : [rule] + }) +} + +function resolvedPatterns(request: Pick) { + const value = request.metadata.resolvedPatterns + return Array.isArray(value) ? value.filter((item): item is string => typeof item === "string") : [] +} + function evaluatePatterns( permission: string, pattern: string, aliases: string[], + resolved: boolean, ...rulesets: PermissionV1.Ruleset[] ): PermissionV1.Rule { - const rules = rulesets.flat().filter((rule) => Wildcard.match(permission, rule.permission)) + const matching = rulesets.flat().filter((rule) => Wildcard.match(permission, rule.permission)) + const rules = resolved && permission === "external_directory" ? withResolvedTwins(matching) : matching const authored = rules.findLast((rule) => matchPattern(permission, pattern, rule.pattern)) ?? { action: "ask" as const, permission, @@ -36,7 +86,16 @@ function evaluatePatterns( } export function evaluate(permission: string, pattern: string, ...rulesets: PermissionV1.Ruleset[]): PermissionV1.Rule { - return evaluatePatterns(permission, pattern, [], ...rulesets) + return evaluatePatterns(permission, pattern, [], false, ...rulesets) +} + +// Evaluate a symlink-resolved external_directory pattern, as a request's resolved pattern is. +export function evaluateResolved( + permission: string, + pattern: string, + ...rulesets: PermissionV1.Ruleset[] +): PermissionV1.Rule { + return evaluatePatterns(permission, pattern, [], true, ...rulesets) } export function evaluateRequest( @@ -51,10 +110,11 @@ export function evaluateRequest( (group): group is string[] => Array.isArray(group) && group.every((value) => typeof value === "string"), ) : [] - if (!groups.length) return evaluate(request.permission, pattern, ...rulesets) + const resolved = resolvedPatterns(request).includes(pattern) + if (!groups.length) return evaluatePatterns(request.permission, pattern, [], resolved, ...rulesets) // Preserve rule order within each access. Distinct invocations must all be allowed, // even when they normalize to the same command. - const rules = groups.map((group) => evaluatePatterns(request.permission, pattern, group, ...rulesets)) + const rules = groups.map((group) => evaluatePatterns(request.permission, pattern, group, resolved, ...rulesets)) return rules.find((rule) => rule.action === "deny") ?? rules.find((rule) => rule.action === "ask") ?? rules[0]! } diff --git a/packages/opencode/src/permission/index.ts b/packages/opencode/src/permission/index.ts index 358ea2e9f7..786d4f013a 100644 --- a/packages/opencode/src/permission/index.ts +++ b/packages/opencode/src/permission/index.ts @@ -4,14 +4,6 @@ import { InstanceState } from "@/effect/instance-state" import { Wildcard } from "@opencode-ai/core/util/wildcard" import { Deferred, Effect, Layer, Context } from "effect" import os from "os" -// altimate_change start — resolve symlinked external_directory rule paths -import path from "path" -import { FSUtil } from "@opencode-ai/core/fs-util" -import { Filesystem } from "@/util/filesystem" -import { Log } from "@/util/log" - -const log = Log.create({ service: "permission" }) -// altimate_change end import { PermissionV1 } from "@opencode-ai/core/v1/permission" import { EventV2Bridge } from "@/event-v2-bridge" import { EventV2 } from "@opencode-ai/core/event" @@ -218,49 +210,13 @@ export function fromConfig(permission: ConfigPermissionV1.Info) { ruleset.push({ permission: key, action: value, pattern: "*" }) continue } - // altimate_change start — external_directory also asks for a path's symlink-resolved directory, - // so a directory rule covers its resolved spelling too (e.g. macOS /tmp -> /private/tmp). - // A generated twin may only tighten: deny/ask twins follow their rule, but allow twins go - // before this map's authored rules, so a deny or ask the user wrote for the real path still - // wins (last match wins) while an unmatched real path still gets the allow. - const allowTwins: PermissionV1.Rule[] = [] - const rules: PermissionV1.Rule[] = [] - for (const [pattern, action] of Object.entries(value)) { - rules.push({ permission: key, pattern: expand(pattern), action }) - const resolved = key === "external_directory" ? resolvedDirectoryPattern(expand(pattern)) : undefined - if (!resolved) continue - const twin = { permission: key, pattern: resolved, action } - if (action === "allow") allowTwins.push(twin) - else rules.push(twin) - } - ruleset.push(...allowTwins, ...rules) - // altimate_change end + ruleset.push( + ...Object.entries(value).map(([pattern, action]) => ({ permission: key, pattern: expand(pattern), action })), + ) } return ruleset } -// altimate_change start — resolved twin of an external_directory pattern: its literal leading -// directory (up to the first segment with a wildcard) resolved through symlinks, rest unchanged -function resolvedDirectoryPattern(pattern: string) { - const segments = pattern.split(process.platform === "win32" ? /[\\/]/ : "/") - const wildcard = segments.findIndex((segment) => /[*?]/.test(segment)) - if (wildcard <= 0) return - const dir = segments.slice(0, wildcard).join(path.sep) || path.sep - // A link loop in a configured directory just leaves the rule without a twin. - const resolved = (() => { - try { - return Filesystem.realLocation(dir) - } catch (error) { - log.debug("no resolved twin for external_directory rule", { pattern, error: String(error) }) - return undefined - } - })() - if (!resolved || resolved === dir) return - const twin = path.join(resolved, ...segments.slice(wildcard)) - return process.platform === "win32" ? FSUtil.normalizePathPattern(twin) : twin -} -// altimate_change end - export function merge(...rulesets: PermissionV1.Ruleset[]): PermissionV1.Rule[] { return rulesets.flat() } diff --git a/packages/opencode/src/tool/external-directory.ts b/packages/opencode/src/tool/external-directory.ts index 9ea9225d45..57ac38a164 100644 --- a/packages/opencode/src/tool/external-directory.ts +++ b/packages/opencode/src/tool/external-directory.ts @@ -28,7 +28,8 @@ function directoryGlob(dir: string) { } // Globs to ask for: the directory as written, plus the directory it really resolves to when a -// symlink takes it elsewhere, so an allowed directory cannot be used to reach a denied one. +// symlink takes it elsewhere, so a symlink inside an allowed directory cannot reach a directory +// outside it that is not allowed. function externalDirectoryPatterns(full: string, real: string | undefined, kind: Kind) { const glob = directoryGlob(kind === "directory" ? full : path.dirname(full)) if (!real) return [glob] @@ -44,7 +45,8 @@ function authoredPath(target: string) { function containsAuthoredPath(target: string, directory: string, worktree: string) { // Resolve the parent, not the entry: an outside link into the project stays outside (its // delete or move acts on the outside entry), while a project file spelled through a link to - // the project (e.g. /var -> /private/var) stays inside. + // the project (e.g. /var -> /private/var) stays inside. This check is lexical after the parent; + // callers pair it with containsPath(real) so an entry that itself links out stays external. const parent = Filesystem.realLocation(path.dirname(target)) const filepath = parent ? path.join(parent, path.basename(target)) : target if (FSUtil.contains(directory, filepath)) return true @@ -92,12 +94,16 @@ export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirec yield* ctx.ask({ permission: "external_directory", patterns, - always: patterns, - // altimate_change end + // Only the path as written is remembered: approving the resolved directory would also allow it + // when read directly, past rules configured for it. + always: patterns.slice(0, 1), metadata: { filepath: full, parentDir: dir, + // Evaluated with each rule's resolved spelling; the path as written is not. + resolvedPatterns: patterns.slice(1), }, + // altimate_change end }) return true }) @@ -183,10 +189,11 @@ export async function assertExternalDirectoryLegacy( await ctx.ask({ permission: "external_directory", patterns, - always: patterns, + always: patterns.slice(0, 1), metadata: { filepath: full, parentDir: dir, + resolvedPatterns: patterns.slice(1), }, }) return true diff --git a/packages/opencode/test/agent/analyst-data-explorer.test.ts b/packages/opencode/test/agent/analyst-data-explorer.test.ts index 055fc82852..a3ff7d219d 100644 --- a/packages/opencode/test/agent/analyst-data-explorer.test.ts +++ b/packages/opencode/test/agent/analyst-data-explorer.test.ts @@ -7,6 +7,7 @@ import path from "path" import { Agent } from "../../src/agent/agent" import { deriveSubagentSessionPermission } from "../../src/agent/subagent-permissions" import { Permission } from "../../src/permission" +import { PermissionEvaluate } from "../../src/permission/evaluate" import { RuntimeFlags } from "../../src/effect/runtime-flags" import { ToolRegistry } from "../../src/tool/registry" import { LLM } from "../../src/session/llm" @@ -620,3 +621,34 @@ it.instance( }, }, ) + +// A global deny on a real folder must survive a per-agent allow written through a link to its parent. +const layered = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "agent-twin-layers-"))) +fs.mkdirSync(path.join(layered, "real", "secret"), { recursive: true }) +fs.symlinkSync(path.join(layered, "real"), path.join(layered, "link")) +afterAll(() => fs.rmSync(layered, { recursive: true, force: true })) + +for (const name of ["builder", "analyst"] as const) { + it.instance( + `${name} keeps a global deny on a real folder when its agent config allows a link to it`, + () => + Effect.gen(function* () { + const agent = (yield* Agent.use.get(name))! + expect(action(agent, "external_directory", path.join(layered, "real", "secret", "*"))).toBe("deny") + // Reading through the link: its resolved directory is allowed by the link rule's resolved spelling. + expect( + PermissionEvaluate.evaluateResolved( + "external_directory", + path.join(layered, "real", "other", "*"), + agent.permission, + ).action, + ).toBe("allow") + }), + { + config: { + permission: { external_directory: { [path.join(layered, "real", "secret", "*")]: "deny" } }, + agent: { [name]: { permission: { external_directory: { [path.join(layered, "link", "*")]: "allow" } } } }, + }, + }, + ) +} diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index 545666301e..23080964c7 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/packages/opencode/test/permission/next.test.ts @@ -122,52 +122,101 @@ test("fromConfig - expands tilde to home directory", () => { }) test.skipIf(process.platform === "win32")( - "fromConfig - external_directory rule also covers the symlink-resolved directory", + "external_directory - the resolved pattern of a request only tightens the decision", () => { const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "permission-resolved-"))) try { - fs.mkdirSync(path.join(root, "real")) + for (const dir of ["real/secret/pub", "real/other", "vol/proj", "elsewhere", "skills/naming"]) { + fs.mkdirSync(path.join(root, ...dir.split("/")), { recursive: true }) + } fs.symlinkSync(path.join(root, "real"), path.join(root, "link")) - // An allow twin goes before the authored rules, so it never outranks one of them. - expect(Permission.fromConfig({ external_directory: { [path.join(root, "link", "*")]: "allow" } })).toEqual([ - { permission: "external_directory", pattern: path.join(root, "real", "*"), action: "allow" }, - { permission: "external_directory", pattern: path.join(root, "link", "*"), action: "allow" }, + fs.symlinkSync(path.join(root, "vol", "proj"), path.join(root, "proj")) + fs.symlinkSync(path.join(root, "elsewhere"), path.join(root, "skills", "naming", "linked")) + const glob = (...p: string[]) => path.join(root, ...p, "*") + const rules = (...layers: Record[]) => + Permission.merge(...layers.map((layer) => Permission.fromConfig({ external_directory: layer }))) + // A request as the external-directory check sends it: the path as written, plus its resolved + // directory when a symlink takes it elsewhere. Every pattern must be allowed. + const decide = (ruleset: PermissionV1.Ruleset, written: string, resolved?: string) => { + const patterns = resolved ? [written, resolved] : [written] + const actions = patterns.map( + (pattern) => + PermissionEvaluate.evaluateRequest( + { permission: "external_directory", metadata: { resolvedPatterns: patterns.slice(1) } }, + pattern, + ruleset, + ).action, + ) + return actions.includes("deny") ? "deny" : actions.includes("ask") ? "ask" : "allow" + } + + // Configured rules are not rewritten: no generated rule is added. + expect(rules({ [glob("link")]: "allow" })).toEqual([ + { permission: "external_directory", pattern: glob("link"), action: "allow" }, ]) - // Wildcard rules keep their wildcard part, and a directory that does not exist yet resolves - // through its nearest existing ancestor. - expect(Permission.fromConfig({ external_directory: { [path.join(root, "link", "**")]: "allow" } })[0]).toEqual({ - permission: "external_directory", - pattern: path.join(root, "real", "**"), - action: "allow", - }) + // An allowed folder reached through a symlink stays allowed. + expect(decide(rules({ [glob("link")]: "allow" }), glob("link", "x"), glob("real", "x"))).toBe("allow") expect( - Permission.fromConfig({ external_directory: { [path.join(root, "link", "later", "*")]: "deny" } })[1], - ).toEqual({ permission: "external_directory", pattern: path.join(root, "real", "later", "*"), action: "deny" }) - // A generated twin only tightens: a deny written for the real path keeps winning over a later - // allow written for the link, in either order, while an unmatched real path still gets the allow. - const deny = (first: "deny" | "allow") => - Permission.fromConfig({ - external_directory: - first === "deny" - ? { [path.join(root, "real", "secret", "*")]: "deny", [path.join(root, "link", "*")]: "allow" } - : { [path.join(root, "link", "*")]: "allow", [path.join(root, "real", "secret", "*")]: "deny" }, - }) - for (const first of ["deny", "allow"] as const) { - const rules = deny(first) - const at = (...p: string[]) => - Permission.evaluate("external_directory", path.join(root, ...p, "*"), rules).action - expect(at("real", "secret"), first).toBe("deny") - expect(at("real", "other"), first).toBe("allow") - expect(at("link", "other"), first).toBe("allow") - } - // A deny twin still follows its rule, so it tightens the real spelling too. - const denyLink = Permission.fromConfig({ - external_directory: { [path.join(root, "*")]: "allow", [path.join(root, "link", "*")]: "deny" }, + PermissionEvaluate.evaluateResolved("external_directory", glob("real", "x"), rules({ [glob("link")]: "allow" })) + .action, + ).toBe("allow") + // The path as written gets exactly the configured decision; the link allow does not reach it. + expect(decide(rules({ [glob("link")]: "allow" }), glob("real", "x"))).toBe("ask") + // A deny for the real path in an earlier layer keeps winning over a later allow for the link. + const layered = rules({ [glob("real", "secret")]: "deny" }, { [glob("link")]: "allow" }) + expect(decide(layered, glob("real", "secret"))).toBe("deny") + expect(decide(layered, glob("link", "other"), glob("real", "other"))).toBe("allow") + // A linked folder allowed inside a denied folder stays allowed (deny-volume / allow-project). + const carve = rules({ [glob("vol")]: "deny", [glob("proj")]: "allow" }) + expect(decide(carve, glob("proj", "x"), glob("vol", "proj", "x"))).toBe("allow") + expect(decide(carve, glob("vol", "other"))).toBe("deny") + // An allow carved out of a deny keeps working through the link. + const carved = rules({ + [glob("real", "secret")]: "deny", + [glob("real", "secret", "pub")]: "allow", + [glob("link")]: "allow", }) - expect(Permission.evaluate("external_directory", path.join(root, "real", "x", "*"), denyLink).action).toBe("deny") - // Other permissions and non-symlinked directories get no extra rule. - expect(Permission.fromConfig({ read: { [path.join(root, "link", "*")]: "allow" } })).toHaveLength(1) - expect(Permission.fromConfig({ external_directory: { [path.join(root, "real", "*")]: "allow" } })).toHaveLength(1) + expect(decide(carved, glob("real", "secret", "pub"))).toBe("allow") + expect(decide(carved, glob("real", "secret"))).toBe("deny") + // A symlink from an allowed folder into a folder that is not allowed is caught. + const skills = rules({ [glob("skills", "naming")]: "allow" }) + expect(decide(skills, glob("skills", "naming", "linked"), glob("elsewhere"))).toBe("ask") + expect( + decide( + rules({ [glob("skills", "naming")]: "allow", [glob("elsewhere")]: "deny" }), + glob("skills", "naming", "linked"), + glob("elsewhere"), + ), + ).toBe("deny") + // "Always" remembers only the path as written (allow link/*). A later direct read of the real + // path still gets the configured deny, and reads through the link stay allowed. + const approved = Permission.fromConfig({ external_directory: { [glob("link")]: "allow" } }) + const withApproval = Permission.merge(rules({ [glob("real", "secret")]: "deny" }), approved) + expect(decide(withApproval, glob("real", "secret"))).toBe("deny") + expect(decide(withApproval, glob("link", "x"), glob("real", "x"))).toBe("allow") + // A deny written for the link also tightens the real spelling. + expect( + decide( + rules({ [path.join(root, "*")]: "allow", [glob("link")]: "deny" }), + glob("link", "x"), + glob("real", "x"), + ), + ).toBe("deny") + // Wildcard rules and not-yet-created directories resolve too. + expect( + PermissionEvaluate.evaluateResolved( + "external_directory", + path.join(root, "real", "a", "b", "*"), + rules({ [path.join(root, "link", "**")]: "allow" }), + ).action, + ).toBe("allow") + expect( + PermissionEvaluate.evaluateResolved( + "external_directory", + glob("real", "later"), + rules({ [glob("link", "later")]: "deny" }), + ).action, + ).toBe("deny") } finally { fs.rmSync(root, { recursive: true, force: true }) } diff --git a/packages/opencode/test/tool/external-directory.test.ts b/packages/opencode/test/tool/external-directory.test.ts index 3dbda45c5a..f6a5610441 100644 --- a/packages/opencode/test/tool/external-directory.test.ts +++ b/packages/opencode/test/tool/external-directory.test.ts @@ -240,7 +240,9 @@ describe("tool.assertExternalDirectory", () => { yield* assertExternalDirectoryEffect(ctx, target) const expected = [glob(path.join(path.dirname(target), "*")), glob(path.join(elsewhere, "*"))] expect(requests[0]!.patterns, target).toEqual(expected) - expect(requests[0]!.always, target).toEqual(expected) + // Only the path as written is remembered; the resolved directory is marked for evaluation. + expect(requests[0]!.always, target).toEqual(expected.slice(0, 1)) + expect(requests[0]!.metadata.resolvedPatterns, target).toEqual(expected.slice(1)) } // A directory reached through the link (grep), a file in a subdirectory that does not exist @@ -265,17 +267,27 @@ describe("tool.assertExternalDirectory", () => { // glob and ls use the Promise-based variant; it asks for the resolved directory too. const legacy: string[][] = [] + const legacyAsks: Parameters[0][] = [] const instance = yield* InstanceState.context yield* Effect.promise(() => Instance.restore(instance, () => assertExternalDirectoryLegacy( - { ...baseCtx, metadata: () => {}, ask: async (req) => void legacy.push([...req.patterns]) }, + { + ...baseCtx, + metadata: () => {}, + ask: async (req) => { + legacy.push([...req.patterns]) + legacyAsks.push(req) + }, + }, path.join(allowed, "linked-dir"), { kind: "directory" }, ), ), ) expect(legacy).toEqual([[glob(path.join(allowed, "linked-dir", "*")), glob(path.join(elsewhere, "*"))]]) + expect(legacyAsks[0]!.always).toEqual([glob(path.join(allowed, "linked-dir", "*"))]) + expect(legacyAsks[0]!.metadata.resolvedPatterns).toEqual([glob(path.join(elsewhere, "*"))]) // A plain file in the allowed directory asks only for that directory. fs.writeFileSync(path.join(allowed, "plain.txt"), "x") diff --git a/packages/opencode/test/tool/grep.test.ts b/packages/opencode/test/tool/grep.test.ts index 0e83d0973c..352bfe6674 100644 --- a/packages/opencode/test/tool/grep.test.ts +++ b/packages/opencode/test/tool/grep.test.ts @@ -14,6 +14,7 @@ import { Agent } from "../../src/agent/agent" import { Ripgrep } from "@opencode-ai/core/ripgrep" import { FSUtil } from "@opencode-ai/core/fs-util" import { testEffect } from "../lib/effect" +import { PermissionEvaluate } from "@/permission/evaluate" import { Permission } from "../../src/permission" import type * as Tool from "../../src/tool/tool" import { Config } from "@/config/config" @@ -204,7 +205,7 @@ describe("tool.grep", () => { ask: (req) => Effect.sync(() => { const needsAsk = req.patterns.some( - (pattern) => Permission.evaluate(req.permission, pattern, ruleset).action !== "allow", + (pattern) => PermissionEvaluate.evaluateRequest(req, pattern, ruleset).action !== "allow", ) if (needsAsk) requests.push(req) }), From 489d22c2d2eb0d7a8cac33727b366a69b4acbacc Mon Sep 17 00:00:00 2001 From: Harish Kumar Date: Tue, 27 Oct 2026 19:40:25 +0530 Subject: [PATCH 25/25] fix(permission): resolve path rules under a wildcard permission too The resolved pattern of an external_directory request only gave a resolved spelling to rules whose permission is exactly external_directory, so a path grant such as {"*": {"/tmp/link/*": "allow"}} prompted again for the real directory although the path as written was allowed. Every rule that applies to the request now gets one, and only patterns whose directory part is an absolute path are resolved, so command patterns are left alone. The agent-level symlink test creates a junction on Windows, and its name now says it checks a direct read of the real path. Co-Authored-By: Claude Opus 5.5 --- packages/opencode/src/permission/evaluate.ts | 5 ++++- .../opencode/test/agent/analyst-data-explorer.test.ts | 8 ++++++-- packages/opencode/test/permission/next.test.ts | 3 +++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/permission/evaluate.ts b/packages/opencode/src/permission/evaluate.ts index 0ba15fea5c..da967754b0 100644 --- a/packages/opencode/src/permission/evaluate.ts +++ b/packages/opencode/src/permission/evaluate.ts @@ -31,6 +31,8 @@ function resolvedTwin(pattern: string) { const wildcard = segments.findIndex((segment) => /[*?]/.test(segment)) if (wildcard <= 0) return const dir = segments.slice(0, wildcard).join(path.sep) || path.sep + // Only path patterns have a directory to resolve; a pattern such as "rm -rf /tmp/*" does not. + if (!path.isAbsolute(dir)) return const resolved = (() => { try { return Filesystem.realLocation(dir) @@ -52,7 +54,8 @@ function withResolvedTwins(rules: PermissionV1.Rule[]) { return twins.get(pattern) } return rules.flatMap((rule) => { - const twin = rule.permission === "external_directory" ? twinOf(rule.pattern) : undefined + // Every rule here already applies to external_directory, including wildcard-permission rules. + const twin = twinOf(rule.pattern) return twin ? [rule, { ...rule, pattern: twin }] : [rule] }) } diff --git a/packages/opencode/test/agent/analyst-data-explorer.test.ts b/packages/opencode/test/agent/analyst-data-explorer.test.ts index a3ff7d219d..bacd21a74f 100644 --- a/packages/opencode/test/agent/analyst-data-explorer.test.ts +++ b/packages/opencode/test/agent/analyst-data-explorer.test.ts @@ -625,12 +625,16 @@ it.instance( // A global deny on a real folder must survive a per-agent allow written through a link to its parent. const layered = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "agent-twin-layers-"))) fs.mkdirSync(path.join(layered, "real", "secret"), { recursive: true }) -fs.symlinkSync(path.join(layered, "real"), path.join(layered, "link")) +fs.symlinkSync( + path.join(layered, "real"), + path.join(layered, "link"), + process.platform === "win32" ? "junction" : "dir", +) afterAll(() => fs.rmSync(layered, { recursive: true, force: true })) for (const name of ["builder", "analyst"] as const) { it.instance( - `${name} keeps a global deny on a real folder when its agent config allows a link to it`, + `${name} keeps a global deny for a direct read of the real path when its agent config allows a link to its parent`, () => Effect.gen(function* () { const agent = (yield* Agent.use.get(name))! diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index 23080964c7..24314d3866 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/packages/opencode/test/permission/next.test.ts @@ -194,6 +194,9 @@ test.skipIf(process.platform === "win32")( const withApproval = Permission.merge(rules({ [glob("real", "secret")]: "deny" }), approved) expect(decide(withApproval, glob("real", "secret"))).toBe("deny") expect(decide(withApproval, glob("link", "x"), glob("real", "x"))).toBe("allow") + // A path rule under a wildcard permission resolves too, so it keeps allowing reads through the link. + const wildcardPermission = Permission.fromConfig({ "*": { [glob("link")]: "allow" } }) + expect(decide(wildcardPermission, glob("link", "x"), glob("real", "x"))).toBe("allow") // A deny written for the link also tightens the real spelling. expect( decide(