diff --git a/docs/docs/configure/agents.md b/docs/docs/configure/agents.md index a45858470c..ee3c8b22de 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, 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 @@ -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 (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. @@ -66,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" } } } @@ -121,7 +122,7 @@ Each agent can have its own permission overrides: ```json { "agent": { - "analyst": { + "builder": { "permission": { "write": "deny", "edit": "deny", @@ -137,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 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/configure/permissions.md b/docs/docs/configure/permissions.md index f381029b8d..65bbc9393e 100644 --- a/docs/docs/configure/permissions.md +++ b/docs/docs/configure/permissions.md @@ -65,14 +65,14 @@ Override permissions for specific agents: ```json { "agent": { - "analyst": { + "builder": { "permission": { "write": "deny", "edit": "deny", "bash": { + "*": "deny", "SELECT *": "allow", - "dbt docs *": "allow", - "*": "deny" + "dbt docs *": "allow" } } } @@ -80,6 +80,10 @@ 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, 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. + ## All Permissioned Tools | Tool | Supports Patterns | Description | @@ -209,15 +213,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..9d37c34977 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" @@ -58,13 +58,13 @@ Disable a tool for a specific agent by setting its permission to `"deny"`: ```json { "agent": { - "analyst": { + "builder": { "permission": { "write": "deny", "edit": "deny", "bash": { - "dbt run *": "deny", - "*": "ask" + "*": "ask", + "dbt run *": "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 "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'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 @@ -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/data-engineering/agent-modes.md b/docs/docs/data-engineering/agent-modes.md index 2c8b0c3fc8..b254bc1b7c 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 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. +- **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..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: @@ -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 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 Define project-wide conventions in an `AGENTS.md` file at your project root. These rules are automatically loaded into every agent's system prompt: @@ -271,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 (safe commands auto-allowed) | 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 | --- diff --git a/docs/docs/reference/security-faq.md b/docs/docs/reference/security-faq.md index 720e460b22..9cc1780d84 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'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 { "agent": { - "analyst": { + "builder": { "permission": { "write": "deny", "edit": "deny", "bash": { - "SELECT *": "allow", - "*": "deny" + "*": "deny", + "SELECT *": "allow" } } } @@ -300,8 +300,8 @@ 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. -- **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. +- **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. - **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/core/src/util/wildcard.ts b/packages/core/src/util/wildcard.ts index 0a67817bcb..694f7a0ef2 100644 --- a/packages/core/src/util/wildcard.ts +++ b/packages/core/src/util/wildcard.ts @@ -1,14 +1,23 @@ export * as Wildcard from "./wildcard" +// altimate_change start — preserve POSIX backslashes when matching filesystem paths export function match(input: string, pattern: string) { - const normalized = input.replaceAll("\\", "/") + return matchNormalized(input.replaceAll("\\", "/"), pattern.replaceAll("\\", "/")) +} + +export function matchPath(input: string, pattern: string) { + if (process.platform === "win32") return match(input, pattern) + return matchNormalized(input, pattern) +} + +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) } +// altimate_change end diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index 91e413e8ac..0884f50bbc 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 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 // archived), but cannot create or edit memory. Saving and deleting stay denied, so a @@ -633,12 +636,48 @@ 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. + // 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") { - 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 dataExplorerAction = Permission.evaluate("task", "data-explorer", agent.permission).action + agent.permission = Permission.merge( + agent.permission, + Permission.fromConfig({ + ...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]), + ), + // A trailing "*" deny is what removes task from the tool list when nothing is allowed. + task: dataExplorerAction === "deny" ? "deny" : { "*": "deny", "data-explorer": dataExplorerAction }, + }), + ) } // altimate_change end const explicit = agent.permission.some((r) => { diff --git a/packages/opencode/src/permission/evaluate.ts b/packages/opencode/src/permission/evaluate.ts index ce8280a936..da967754b0 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 @@ -8,14 +14,67 @@ 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" || permission === "sensitive_write" + ? Wildcard.matchPath(input, pattern) + : 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 + // 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) + } 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) => { + // Every rule here already applies to external_directory, including wildcard-permission rules. + const twin = twinOf(rule.pattern) + 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 authored = rules.findLast((rule) => Wildcard.match(pattern, rule.pattern)) ?? { + 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, pattern: "*", @@ -23,14 +82,23 @@ 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 } 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( @@ -45,10 +113,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/next.ts b/packages/opencode/src/permission/next.ts index d28e5edb91..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()) @@ -260,7 +266,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" || permission === "sensitive_write" + ? Wildcard.matchPath(pattern, rule.pattern) + : Wildcard.match(pattern, rule.pattern)), ) return match ?? { action: "ask", permission, pattern: "*" } } diff --git a/packages/opencode/src/session/llm.ts b/packages/opencode/src/session/llm.ts index 1bf82c7e32..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, @@ -477,7 +480,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/native-runtime.ts b/packages/opencode/src/session/llm/native-runtime.ts index f67c73cb6f..a103112f39 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" @@ -89,6 +90,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,26 +114,26 @@ 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( - 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), @@ -149,6 +151,19 @@ export function stream(input: StreamInput): StreamResult { } } +function repairToolCall(tools: ReturnType, call: ToolCallPart): ToolCallPart { + const hasTool = (name: string) => Object.prototype.hasOwnProperty.call(tools, name) + if (hasTool(call.name)) return call + const lower = call.name.toLowerCase() + if (hasTool(lower)) return { ...call, name: lower } + if (!hasTool("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/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/src/tool/external-directory.ts b/packages/opencode/src/tool/external-directory.ts index 844d8961d5..57ac38a164 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 { Filesystem } from "@/util/filesystem" +// altimate_change end import { Effect } from "effect" import { InstanceState } from "@/effect/instance-state" import type * as Tool from "./tool" @@ -19,6 +22,50 @@ type Options = { kind?: Kind } +// altimate_change start — symlink-resolved directory of an external_directory target +function directoryGlob(dir: string) { + 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 +// 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] + 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(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. 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 + 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* ( ctx: Tool.Context, target?: string, @@ -29,24 +76,34 @@ 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 - if (containsPath(full, ins)) return false + // 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(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" 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 — also ask for the symlink-resolved directory + const patterns = externalDirectoryPatterns(full, real, kind) yield* ctx.ask({ permission: "external_directory", - patterns: [glob], - always: [glob], + patterns, + // 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 }) @@ -69,17 +126,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.`, }, }) }) @@ -88,18 +146,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.`, }, }), ) @@ -118,23 +177,23 @@ export async function assertExternalDirectoryLegacy( if (!target) return false if (options?.bypass) return false - const full = process.platform === "win32" ? FSUtil.normalizePath(target) : target - if (Instance.containsPath(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) - const glob = - process.platform === "win32" - ? FSUtil.normalizePathPattern(path.join(dir, "*")) - : path.join(dir, "*").replaceAll("\\", "/") + const patterns = externalDirectoryPatterns(full, real, kind) await ctx.ask({ permission: "external_directory", - patterns: [glob], - always: [glob], + patterns, + always: patterns.slice(0, 1), metadata: { filepath: full, parentDir: dir, + resolvedPatterns: patterns.slice(1), }, }) return true diff --git a/packages/opencode/src/util/filesystem.ts b/packages/opencode/src/util/filesystem.ts index a5d520b1df..7831bf18f7 100644 --- a/packages/opencode/src/util/filesystem.ts +++ b/packages/opencode/src/util/filesystem.ts @@ -4,6 +4,10 @@ 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 — path and link helpers for Filesystem.realLocation +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" // altimate_change end @@ -203,6 +207,57 @@ 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. 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. 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" ? /[\\/]/ : "/") + 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 = danglingLinkTarget(next, real, budget) ?? next + } + } + return real + } + + function danglingLinkTarget(link: string, dir: string, budget: { links: number }) { + let target: string + try { + if (!lstatSync(link).isSymbolicLink()) return undefined + target = readlinkSync(link) + } catch { + return undefined + } + 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) + } + // 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/src/util/wildcard.ts b/packages/opencode/src/util/wildcard.ts index b2affbf849..21b7bf1c93 100644 --- a/packages/opencode/src/util/wildcard.ts +++ b/packages/opencode/src/util/wildcard.ts @@ -18,6 +18,10 @@ export function match(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/agent/analyst-data-explorer.test.ts b/packages/opencode/test/agent/analyst-data-explorer.test.ts index 72babad45d..bacd21a74f 100644 --- a/packages/opencode/test/agent/analyst-data-explorer.test.ts +++ b/packages/opencode/test/agent/analyst-data-explorer.test.ts @@ -1,8 +1,20 @@ -import { expect } from "bun:test" +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 { PermissionEvaluate } from "../../src/permission/evaluate" +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" import { testEffect } from "../lib/effect" const it = testEffect(Agent.defaultLayer) @@ -273,3 +285,374 @@ 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", + "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])) + +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" } } } }, + }, + ) +} + +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 } + 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 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", + "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", + "lsp", + "plan_exit", + "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 ids = yield* (yield* ToolRegistry.Service).ids() + const analyst = (yield* Agent.Service.use((svc) => svc.get("analyst")))! + expect(ids).toContain("batch") + // 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) + }), + { config: { permission: { "*": "allow" }, experimental: { batch_tool: true } } }, +) + +// 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 allows external_directory access to 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] } } }, +) + +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" } } } }, + }), + }, +) + +// 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] } } }, + ) +} + +// 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" }, + }, + }, + }, + }, + }, +) + +// 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"), + 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 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))! + 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 1a90ae9383..24314d3866 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,139 @@ test("fromConfig - expands tilde to home directory", () => { expect(result).toEqual([{ permission: "external_directory", pattern: `${os.homedir()}/projects/*`, action: "allow" }]) }) +test.skipIf(process.platform === "win32")( + "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 { + 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")) + 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" }, + ]) + // An allowed folder reached through a symlink stays allowed. + expect(decide(rules({ [glob("link")]: "allow" }), glob("link", "x"), glob("real", "x"))).toBe("allow") + expect( + 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(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 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( + 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 }) + } + }, +) + +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.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" }]) @@ -596,8 +731,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/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-native.test.ts b/packages/opencode/test/session/llm-native.test.ts index 17ee289a42..b841cbb871 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,75 @@ 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 invalidInputs: unknown[] = [] + let visibleCalls = 0 + const invalid = tool({ + description: "Internal fallback", + inputSchema: jsonSchema<{ tool: string; error: string }>({ + type: "object", + properties: { tool: { type: "string" }, error: { type: "string" } }, + required: ["tool", "error"], + }), + execute: async (input) => { + invalidInputs.push(input) + return "fallback" + }, + }) + const visible = tool({ + description: "Visible tool", + inputSchema: jsonSchema({ type: "object" }), + 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-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" }), + ]) + }, + 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(visibleCalls).toBe(1) + 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" }), + expect.objectContaining({ type: "tool-result", name: "visible" }), + 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/session/llm.test.ts b/packages/opencode/test/session/llm.test.ts index 8af231fc3c..779e95346f 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,199 @@ 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("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) { diff --git a/packages/opencode/test/tool/external-directory.test.ts b/packages/opencode/test/tool/external-directory.test.ts index 69a48bad7a..f6a5610441 100644 --- a/packages/opencode/test/tool/external-directory.test.ts +++ b/packages/opencode/test/tool/external-directory.test.ts @@ -1,13 +1,20 @@ 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" import type { Tool } from "@/tool/tool" -import { assertExternalDirectoryEffect } 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" import { TestInstance, tmpdirScoped } from "../fixture/fixture" -import type { Permission } from "../../src/permission" import { SessionID, MessageID } from "../../src/session/schema" import { testEffect } from "../lib/effect" @@ -23,8 +30,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> = [] @@ -60,6 +66,118 @@ 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") + // 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(outside)), "*") + : glob(path.join(outside, "*")) + + 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) + }), + ) + + 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* () { + 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 + 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) + + // 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) + }), + ) + } + it.instance("asks with a single canonical glob", () => Effect.gen(function* () { const test = yield* TestInstance @@ -104,6 +222,82 @@ 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")) + 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() + yield* assertExternalDirectoryEffect(ctx, target) + const expected = [glob(path.join(path.dirname(target), "*")), glob(path.join(elsewhere, "*"))] + expect(requests[0]!.patterns, 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 + // 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], + // 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() + 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 legacyAsks: Parameters[0][] = [] + const instance = yield* InstanceState.context + yield* Effect.promise(() => + Instance.restore(instance, () => + assertExternalDirectoryLegacy( + { + ...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") + 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", @@ -153,3 +347,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"))) + }), + ) +}) 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) }), diff --git a/packages/opencode/test/util/filesystem.test.ts b/packages/opencode/test/util/filesystem.test.ts index 5b5b97632c..f64b8c2077 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(10_000) + }) + }) }) 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)