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