From 9d0699e07076f0939b626786f93db7441c36e27d Mon Sep 17 00:00:00 2001 From: re2zero Date: Sat, 8 Aug 2026 02:41:24 +0800 Subject: [PATCH] fix(coding-agent): resolve xd:// device dispatches against device user policy first When an xd:// device is dispatched through the write tool, the outer approval gate now consults tools.approval. before falling back to tools.approval.write. This lets users scope allow/deny/prompt to a single device mount without changing the blanket write tool policy. The write tool's approval function returns { tier, policyKey: deviceName } for xd:// device dispatches. resolveApproval uses the policyKey to look up the user override on the device name, falling back to the invoking tool's own policy when the device has none configured. Adds: - ToolApprovalDecision.policyKey field (optional, additive) - policyKey-aware lookup in resolveApproval and requiresApproval - Updated error messages naming the correct config key - Unit tests for policyKey resolution and WriteTool integration Fixes can1357/oh-my-pi#7923 --- packages/agent/src/types.ts | 12 +- .../src/extensibility/extensions/wrapper.ts | 13 ++- packages/coding-agent/src/tools/approval.ts | 42 +++++-- packages/coding-agent/src/tools/write.ts | 9 +- .../coding-agent/test/tools/approval.test.ts | 50 ++++++++ .../test/tools/policy-key.test.ts | 110 ++++++++++++++++++ .../test/write-xdev-dispatch.test.ts | 62 +++++++++- 7 files changed, 274 insertions(+), 24 deletions(-) create mode 100644 packages/coding-agent/test/tools/policy-key.test.ts diff --git a/packages/agent/src/types.ts b/packages/agent/src/types.ts index 198c9a4a3..c31d2bfa9 100644 --- a/packages/agent/src/types.ts +++ b/packages/agent/src/types.ts @@ -728,7 +728,17 @@ export type ToolLoadMode = "essential" | "discoverable"; */ export type ToolApprovalDecision = | ToolTier - | { tier: ToolTier; reason?: string; override?: boolean; policy?: "allow" | "deny" | "prompt" }; + | { + tier: ToolTier; + reason?: string; + override?: boolean; + policy?: "allow" | "deny" | "prompt"; + /** User-policy key for this decision. When set, `tools.approval.` + * is consulted instead of `tools.approval.`. Lets a dispatcher + * tool (e.g. `write` for an `xd://` device call) scope user allow/deny/ + * prompt policies to the tool it dispatches into. */ + policyKey?: string; + }; export type ToolApproval = ToolApprovalDecision | ((args: unknown) => ToolApprovalDecision); /** diff --git a/packages/coding-agent/src/extensibility/extensions/wrapper.ts b/packages/coding-agent/src/extensibility/extensions/wrapper.ts index 7618e305f..83fc1cd21 100644 --- a/packages/coding-agent/src/extensibility/extensions/wrapper.ts +++ b/packages/coding-agent/src/extensibility/extensions/wrapper.ts @@ -190,10 +190,11 @@ export class ExtensionToolWrapper; - if (resolveApproval(this.tool, approvalArgs(params, context), approvalMode, userPolicies).policy === "deny") { + const preResolved = resolveApproval(this.tool, approvalArgs(params, context), approvalMode, userPolicies); + if (preResolved.policy === "deny") { throw new Error( - `Tool "${this.tool.name}" is blocked by user policy.\n` + - `To allow: remove "tools.approval.${this.tool.name}: deny" from config.`, + `Tool "${preResolved.policyKey ?? this.tool.name}" is blocked by user policy.\n` + + `To allow: remove "tools.approval.${preResolved.policyKey ?? this.tool.name}: deny" from config.`, ); } @@ -242,8 +243,8 @@ export class ExtensionToolWrapper 0 || (resolved.policy === "prompt" && (explicitPrompt || !xdevBypass)), diff --git a/packages/coding-agent/src/tools/approval.ts b/packages/coding-agent/src/tools/approval.ts index 1cabaac1e..6e50dc165 100644 --- a/packages/coding-agent/src/tools/approval.ts +++ b/packages/coding-agent/src/tools/approval.ts @@ -21,6 +21,8 @@ export interface ResolvedApproval { reason?: string; override: boolean; source?: "tool" | "user" | "mode"; + /** User-policy key that produced `source: "user"` (defaults to the tool name). */ + policyKey?: string; } const POLICY_VALUES: ReadonlySet = new Set(["allow", "deny", "prompt"]); @@ -61,11 +63,14 @@ function normalizeDecision(value: unknown): Omit & { const tier = isToolTier(record.tier) ? record.tier : "exec"; const reason = typeof record.reason === "string" && record.reason.length > 0 ? record.reason : undefined; const policy = normalizePolicy(record.policy); + const policyKey = + typeof record.policyKey === "string" && record.policyKey.length > 0 ? record.policyKey : undefined; return { tier, override: record.override === true, ...(policy ? { policy } : {}), ...(reason ? { reason } : {}), + ...(policyKey ? { policyKey } : {}), }; } @@ -101,6 +106,11 @@ function modeApprovesTier(mode: ApprovalMode, tier: ToolTier): boolean { * * Resolution order: * 1. Tool `approval(args)` decision, defaulting to tier "exec" when omitted. + * A decision may carry a `policyKey` — `tools.approval.` is then + * the user override consulted instead of `tools.approval.`, with + * the invoking tool's own policy as the fallback when the user set none for + * the keyed sub-tool (e.g. an `xd://` device dispatch without a device + * policy still honors `tools.approval.write`). * 2. User per-tool override, if set and valid. * 3. Active mode tier comparison. * @@ -114,7 +124,14 @@ export function resolveApproval( userConfig: Record = {}, ): ResolvedApproval { const decision = getToolDecision(tool, args); - const userPolicy = Object.hasOwn(userConfig, tool.name) ? normalizePolicy(userConfig[tool.name]) : undefined; + const policyKey = decision.policyKey ?? tool.name; + const userPolicy = Object.hasOwn(userConfig, policyKey) ? normalizePolicy(userConfig[policyKey]) : undefined; + const fallbackPolicy = + policyKey !== tool.name && userPolicy === undefined && Object.hasOwn(userConfig, tool.name) + ? normalizePolicy(userConfig[tool.name]) + : undefined; + const effectiveUserPolicy = userPolicy ?? fallbackPolicy; + const userPolicyKey = userPolicy !== undefined ? policyKey : tool.name; if (decision.policy === "deny") { return { @@ -122,11 +139,12 @@ export function resolveApproval( tier: decision.tier, override: decision.override, source: "tool", + ...(decision.policyKey ? { policyKey: decision.policyKey } : {}), ...(decision.reason ? { reason: decision.reason } : {}), }; } - if (userPolicy === "deny") { - return { policy: "deny", tier: decision.tier, override: decision.override, source: "user" }; + if (effectiveUserPolicy === "deny") { + return { policy: "deny", tier: decision.tier, override: decision.override, source: "user", policyKey: userPolicyKey }; } if (mode === "yolo") { @@ -136,14 +154,16 @@ export function resolveApproval( tier: decision.tier, override: false, source: "tool", + ...(decision.policyKey ? { policyKey: decision.policyKey } : {}), ...(decision.reason ? { reason: decision.reason } : {}), }; } return { - policy: userPolicy ?? "allow", + policy: effectiveUserPolicy ?? "allow", tier: decision.tier, override: false, - source: userPolicy ? "user" : "mode", + source: effectiveUserPolicy ? "user" : "mode", + ...(effectiveUserPolicy ? { policyKey: userPolicyKey } : {}), }; } @@ -153,6 +173,7 @@ export function resolveApproval( tier: decision.tier, override: true, source: "tool", + ...(decision.policyKey ? { policyKey: decision.policyKey } : {}), ...(decision.reason ? { reason: decision.reason } : {}), }; } @@ -163,12 +184,13 @@ export function resolveApproval( tier: decision.tier, override: false, source: "tool", + ...(decision.policyKey ? { policyKey: decision.policyKey } : {}), ...(decision.reason ? { reason: decision.reason } : {}), }; } - if (userPolicy) { - return { policy: userPolicy, tier: decision.tier, override: false, source: "user" }; + if (effectiveUserPolicy) { + return { policy: effectiveUserPolicy, tier: decision.tier, override: false, source: "user", policyKey: userPolicyKey }; } if (modeApprovesTier(mode, decision.tier)) { @@ -196,15 +218,15 @@ export function requiresApproval( mode: ApprovalMode, userConfig: Record = {}, ): { required: boolean; reason?: string } { - const { policy, reason, source } = resolveApproval(tool, args, mode, userConfig); + const { policy, reason, source, policyKey } = resolveApproval(tool, args, mode, userConfig); if (policy === "deny") { if (source === "tool") { throw new Error(`Tool "${tool.name}" is blocked by tool policy.${reason ? `\nReason: ${reason}` : ""}`); } throw new Error( - `Tool "${tool.name}" is blocked by user policy.\n` + - `To allow: remove "tools.approval.${tool.name}: deny" from config.`, + `Tool "${policyKey ?? tool.name}" is blocked by user policy.\n` + + `To allow: remove "tools.approval.${policyKey ?? tool.name}: deny" from config.`, ); } diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index af6aca18b..0dfa118d8 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -9,6 +9,7 @@ import type { AgentToolContext, AgentToolResult, AgentToolUpdateCallback, + ToolApprovalDecision, ToolTier, } from "@oh-my-pi/pi-agent-core"; import { type Component, Text } from "@oh-my-pi/pi-tui"; @@ -500,7 +501,7 @@ function parseSqliteWriteTarget(subPath: string, queryString: string): { table: */ export class WriteTool implements AgentTool { readonly name = "write"; - readonly approval = (args: unknown): ToolTier => { + readonly approval = (args: unknown): ToolApprovalDecision => { const rawPath = (args as Partial).path; if (typeof rawPath !== "string") return "write"; // Unwrap a hashline `[path#TAG]` wrapper first (parity with execute) so a @@ -532,7 +533,11 @@ export class WriteTool implements AgentTool` for + // this dispatch before falling back to `tools.approval.write`, so users + // can scope allow/deny/prompt to a single device (issue #7923). + return { tier: resolveToolTier(inst, parsed), policyKey: xdevTarget.name! }; } catch { return "exec"; } diff --git a/packages/coding-agent/test/tools/approval.test.ts b/packages/coding-agent/test/tools/approval.test.ts index 26d829b92..06b2cec4a 100644 --- a/packages/coding-agent/test/tools/approval.test.ts +++ b/packages/coding-agent/test/tools/approval.test.ts @@ -166,6 +166,56 @@ describe("MCP fallback and prompt formatting", () => { }); }); +describe("decision policyKey scopes user policy to a sub-tool", () => { + // The write tool reports this decision for an `xd://knowledge_search` dispatch: + // the tier comes from the mounted tool, and the policyKey makes the user + // override key on the device instead of the invoking `write` tool (#7923). + const dispatch = tool("write", { tier: "exec", policyKey: "knowledge_search" }); + + it("consults tools.approval. for the user override", () => { + expect(resolveApproval(dispatch, {}, "always-ask", { knowledge_search: "allow" })).toMatchObject({ + policy: "allow", + source: "user", + policyKey: "knowledge_search", + }); + expect(resolveApproval(dispatch, {}, "always-ask", { knowledge_search: "prompt" }).policy).toBe("prompt"); + expect(resolveApproval(dispatch, {}, "always-ask", { knowledge_search: "deny" }).policy).toBe("deny"); + }); + + it("falls back to the invoking tool's own policy when the keyed one is unset", () => { + expect(resolveApproval(dispatch, {}, "always-ask", { write: "allow" }).policy).toBe("allow"); + expect(resolveApproval(dispatch, {}, "always-ask", { write: "prompt" }).policy).toBe("prompt"); + expect(resolveApproval(dispatch, {}, "always-ask", { write: "deny" }).policy).toBe("deny"); + }); + + it("device policy wins over the invoking tool's policy", () => { + expect( + resolveApproval(dispatch, {}, "always-ask", { write: "prompt", knowledge_search: "allow" }).policy, + ).toBe("allow"); + expect( + resolveApproval(dispatch, {}, "always-ask", { write: "allow", knowledge_search: "deny" }).policy, + ).toBe("deny"); + }); + + it("names the policy key in user-deny refusals", () => { + expect(() => requiresApproval(dispatch, {}, "always-ask", { knowledge_search: "deny" })).toThrow( + 'Tool "knowledge_search" is blocked by user policy', + ); + expect(() => requiresApproval(dispatch, {}, "always-ask", { knowledge_search: "deny" })).toThrow( + 'remove "tools.approval.knowledge_search: deny"', + ); + expect(() => requiresApproval(dispatch, {}, "always-ask", { write: "deny" })).toThrow( + 'remove "tools.approval.write: deny"', + ); + }); + + it("does not change resolution for tools without a policyKey", () => { + const plain = tool("write", "exec"); + expect(resolveApproval(plain, {}, "always-ask", { write: "allow" }).policy).toBe("allow"); + expect(resolveApproval(plain, {}, "always-ask", { knowledge_search: "allow" }).policy).toBe("prompt"); + }); +}); + describe("tool-owned dynamic approval declarations", () => { it("classifies critical bash patterns through BashTool.approval", () => { for (const command of [ diff --git a/packages/coding-agent/test/tools/policy-key.test.ts b/packages/coding-agent/test/tools/policy-key.test.ts new file mode 100644 index 000000000..97094d709 --- /dev/null +++ b/packages/coding-agent/test/tools/policy-key.test.ts @@ -0,0 +1,110 @@ +import { describe, expect, it } from "bun:test"; +import type { AgentTool, ToolApproval } from "@oh-my-pi/pi-agent-core"; +import { requiresApproval, resolveApproval } from "@oh-my-pi/pi-coding-agent/tools/approval"; + +type ApprovalTool = Pick; + +function tool( + name: string, + approval?: ToolApproval, + formatApprovalDetails?: ApprovalTool["formatApprovalDetails"], +): ApprovalTool { + return { name, approval, formatApprovalDetails }; +} + +describe("decision policyKey scopes user policy to a sub-tool", () => { + // The write tool reports this decision for an `xd://knowledge_search` dispatch: + // the tier comes from the mounted tool, and the policyKey makes the user + // override key on the device instead of the invoking `write` tool (#7923). + const dispatch = tool("write", { tier: "exec", policyKey: "knowledge_search" }); + + it("consults tools.approval. for the user override", () => { + expect(resolveApproval(dispatch, {}, "always-ask", { knowledge_search: "allow" })).toMatchObject({ + policy: "allow", + source: "user", + policyKey: "knowledge_search", + }); + expect(resolveApproval(dispatch, {}, "always-ask", { knowledge_search: "prompt" }).policy).toBe("prompt"); + expect(resolveApproval(dispatch, {}, "always-ask", { knowledge_search: "deny" }).policy).toBe("deny"); + }); + + it("falls back to the invoking tool's own policy when the keyed one is unset", () => { + expect(resolveApproval(dispatch, {}, "always-ask", { write: "allow" }).policy).toBe("allow"); + expect(resolveApproval(dispatch, {}, "always-ask", { write: "prompt" }).policy).toBe("prompt"); + expect(resolveApproval(dispatch, {}, "always-ask", { write: "deny" }).policy).toBe("deny"); + }); + + it("device policy wins over the invoking tool's policy", () => { + expect( + resolveApproval(dispatch, {}, "always-ask", { write: "prompt", knowledge_search: "allow" }).policy, + ).toBe("allow"); + expect( + resolveApproval(dispatch, {}, "always-ask", { write: "allow", knowledge_search: "deny" }).policy, + ).toBe("deny"); + }); + + it("names the policy key in user-deny refusals", () => { + expect(() => requiresApproval(dispatch, {}, "always-ask", { knowledge_search: "deny" })).toThrow( + 'Tool "knowledge_search" is blocked by user policy', + ); + expect(() => requiresApproval(dispatch, {}, "always-ask", { knowledge_search: "deny" })).toThrow( + 'remove "tools.approval.knowledge_search: deny"', + ); + expect(() => requiresApproval(dispatch, {}, "always-ask", { write: "deny" })).toThrow( + 'remove "tools.approval.write: deny"', + ); + }); + + it("does not change resolution for tools without a policyKey", () => { + const plain = tool("write", "exec"); + expect(resolveApproval(plain, {}, "always-ask", { write: "allow" }).policy).toBe("allow"); + expect(resolveApproval(plain, {}, "always-ask", { knowledge_search: "allow" }).policy).toBe("prompt"); + }); +}); + +describe("WriteTool xd:// device policy key integration", () => { + it("returns tier+policyKey for real mounted devices", () => { + // Build a minimal apropos tool and WriteTool without loading the full + // toolchain (which needs the native addon) — the approval function + // only reads from `this.session.xdev` and basic session props. + const device: AgentTool = { + name: "knowledge_search", + label: "Knowledge Search", + description: "device without a tier declaration", + parameters: { type: "object", properties: { q: { type: "string" } } } as any, + async execute() { + return { content: [{ type: "text", text: "ok" }] }; + }, + }; + const xdev = { + tools: new Map([[device.name, device]]), + mountedNames: new Set([device.name]), + builtInNames: new Set([device.name]), + isActive: () => false, + }; + const session = { + cwd: "/tmp", + hasUI: false, + xdev, + settings: { + get: () => undefined, + }, + getSessionFile: () => null, + getSessionSpawns: () => "*", + }; + + // Use dynamic import to avoid loading the WriteTool module at parse time + // (it chains into native addon code during module init). + return import("@oh-my-pi/pi-coding-agent/tools/write").then(({ WriteTool }) => { + const write = new WriteTool(session as any); + const approvalFn = write.approval; + expect(typeof approvalFn).toBe("function"); + if (typeof approvalFn !== "function") throw new Error("expected function"); + const result = approvalFn({ + path: "xd://knowledge_search", + content: JSON.stringify({ q: "x" }), + }); + expect(result).toEqual({ tier: "exec", policyKey: "knowledge_search" }); + }); + }); +}); \ No newline at end of file diff --git a/packages/coding-agent/test/write-xdev-dispatch.test.ts b/packages/coding-agent/test/write-xdev-dispatch.test.ts index 6b306b09c..e5078b449 100644 --- a/packages/coding-agent/test/write-xdev-dispatch.test.ts +++ b/packages/coding-agent/test/write-xdev-dispatch.test.ts @@ -8,6 +8,7 @@ import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import * as themeModule from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; import { ToolChoiceQueue } from "@oh-my-pi/pi-coding-agent/session/tool-choice-queue"; import { createTools, type Tool, type ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { requiresApproval, resolveApproval } from "@oh-my-pi/pi-coding-agent/tools/approval"; import { githubToolRenderer } from "@oh-my-pi/pi-coding-agent/tools/gh-renderer"; import { ToolError } from "@oh-my-pi/pi-coding-agent/tools/tool-errors"; import { WriteTool, writeToolRenderer } from "@oh-my-pi/pi-coding-agent/tools/write"; @@ -87,7 +88,7 @@ describe("read and write route xd:// device URLs", () => { const approval = write!.approval; expect(typeof approval).toBe("function"); if (typeof approval === "function") { - expect(approval({ path: "xd://ast_edit", content })).toBe("write"); + expect(approval({ path: "xd://ast_edit", content })).toEqual({ tier: "write", policyKey: "ast_edit" }); } // Execute dispatches through the xdev registry to the mounted ast_edit, @@ -131,6 +132,51 @@ describe("read and write route xd:// device URLs", () => { expect(result.details?.xdev).toMatchObject({ tool: "peek", mode: "execute", tier: "read" }); }); + it("resolves device dispatches against the device's user policy, falling back to write's", async () => { + // Like the pi-knowledge plugin in #7923: the mounted device declares no + // approval, so it defaults to exec tier — but a device-scoped user policy + // must still gate, and without one the dispatch must honor `write`'s policy. + const device: AgentTool = { + name: "knowledge_search", + label: "Knowledge Search", + description: "Read-only device without a tier declaration", + parameters: type({ q: "string" }), + async execute() { + return { content: [{ type: "text", text: "ok" }] }; + }, + }; + const xdev = createTestXdevState([device]); + const write = new WriteTool(xdevSession(process.cwd(), { xdev })); + const args = { path: "xd://knowledge_search", content: JSON.stringify({ q: "x" }) }; + + const approval = write.approval; + expect(typeof approval).toBe("function"); + if (typeof approval !== "function") throw new Error("expected a function approval"); + // The gate reports the mounted tool's (default exec) tier and keys user + // policy on the device name. + expect(approval(args)).toEqual({ tier: "exec", policyKey: "knowledge_search" }); + + // No device policy → falls back to the write tool's own policy. + expect(resolveApproval(write, args, "always-ask", { write: "prompt" }).policy).toBe("prompt"); + expect(resolveApproval(write, args, "always-ask", { write: "allow" }).policy).toBe("allow"); + + // Device-scoped allow lets the dispatch through even while the blanket + // write policy stays prompt — the exact scenario from #7923. + const allowed = resolveApproval(write, args, "always-ask", { write: "prompt", knowledge_search: "allow" }); + expect(allowed).toMatchObject({ policy: "allow", source: "user", policyKey: "knowledge_search" }); + + // Device-scoped deny blocks the dispatch and names the device in the refusal. + expect(() => requiresApproval(write, args, "always-ask", { knowledge_search: "deny" })).toThrow( + 'remove "tools.approval.knowledge_search: deny"', + ); + + // Device-scoped prompt forces a prompt for this device. + expect(resolveApproval(write, args, "always-ask", { knowledge_search: "prompt" }).policy).toBe("prompt"); + + // An unrelated device's policy does not leak into this dispatch. + expect(resolveApproval(write, args, "always-ask", { other_device: "deny" }).policy).toBe("prompt"); + }); + it("records the effective tier reported after an execution decorator rewrites device args", async () => { let executedQuery: string | undefined; const device: AgentTool = { @@ -223,12 +269,18 @@ describe("read and write route xd:// device URLs", () => { ops: [{ pat: "a", out: "b" }], paths: ["artifact://abc"], }); - expect(tier("xd://ast_edit", astFsPath)).toBe("write"); - expect(tier("xd://ast_edit", astInternalPath)).toBe("read"); + expect(tier("xd://ast_edit", astFsPath)).toEqual({ tier: "write", policyKey: "ast_edit" }); + expect(tier("xd://ast_edit", astInternalPath)).toEqual({ tier: "read", policyKey: "ast_edit" }); // debug: inspection action → read; a real launch → exec (control). - expect(tier("xd://debug", JSON.stringify({ action: "sessions" }))).toBe("read"); - expect(tier("xd://debug", JSON.stringify({ action: "launch", program: "./app" }))).toBe("exec"); + expect(tier("xd://debug", JSON.stringify({ action: "sessions" }))).toEqual({ + tier: "read", + policyKey: "debug", + }); + expect(tier("xd://debug", JSON.stringify({ action: "launch", program: "./app" }))).toEqual({ + tier: "exec", + policyKey: "debug", + }); // Fail closed: malformed JSON, non-object or schema-invalid payloads, // missing content, and unknown devices all stay exec so the gate never