smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent): resolve xd:// device dispatches against device user policy first

Browse Source 
When an xd:// device is dispatched through the write tool, the outer
approval gate now consults tools.approval.<deviceName> 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
This commit is contained in:
re2zero
2026-08-08 02:41:24 +08:00
parent 39477ba39b
commit 9d0699e070
7 changed files with 274 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/agent/src/types.ts
+11 -1
View File
@@ -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.<policyKey>`
* is consulted instead of `tools.approval.<tool.name>`. 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);
/**
packages/coding-agent/src/extensibility/extensions/wrapper.ts
+7 -6
View File
@@ -190,10 +190,11 @@ export class ExtensionToolWrapper<TParameters extends TSchema = TSchema, TDetail
const configuredMode = (settings?.get("tools.approvalMode") ?? "yolo") as ApprovalMode;
const approvalMode: ApprovalMode = cliAutoApprove ? "yolo" : configuredMode;
const userPolicies = (settings?.get("tools.approval") ?? {}) as Record<string, unknown>;
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<TParameters extends TSchema = TSchema, TDetail
context?.xdevTierResolved?.(resolved.tier);
if (resolved.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 "${resolved.policyKey ?? this.tool.name}" is blocked by user policy.\n` +
`To allow: remove "tools.approval.${resolved.policyKey ?? this.tool.name}: deny" from config.`,
);
}
const pendingSafetyChecks = computerSafetyChecks(context);
@@ -255,7 +256,7 @@ export class ExtensionToolWrapper<TParameters extends TSchema = TSchema, TDetail
// and tool-demanded overrides still prompt. Provider safety checks are
// stronger: yolo, per-tool allow, and xdev approval never acknowledge
// them on the user's behalf.
const explicitPrompt = resolved.override || Object.hasOwn(userPolicies, this.tool.name);
const explicitPrompt = resolved.override || Object.hasOwn(userPolicies, resolved.policyKey ?? this.tool.name);
const xdevBypass = context?.xdevApproved === true && effectiveParams === params;
const approvalCheck = {
required: pendingSafetyChecks.length > 0 || (resolved.policy === "prompt" && (explicitPrompt || !xdevBypass)),
packages/coding-agent/src/tools/approval.ts
+32 -10
View File
@@ -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<ApprovalPolicy> = new Set(["allow", "deny", "prompt"]);
@@ -61,11 +63,14 @@ function normalizeDecision(value: unknown): Omit<ResolvedApproval, "policy"> & {
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.<policyKey>` is then
* the user override consulted instead of `tools.approval.<tool.name>`, 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<string, unknown> = {},
): 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<string, unknown> = {},
): { 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.`,
);
}
packages/coding-agent/src/tools/write.ts
+7 -2
View File
@@ -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<typeof writeSchema, WriteToolDetails> {
readonly name = "write";
readonly approval = (args: unknown): ToolTier => {
readonly approval = (args: unknown): ToolApprovalDecision => {
const rawPath = (args as Partial<WriteParams>).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<typeof writeSchema, WriteToolDetails
}
if (!isRecord(parsed)) return "exec";
try {
return resolveToolTier(inst, parsed);
// The tier is the mounted tool's own (argument-dependent) approval; the
// policyKey makes the outer gate consult `tools.approval.<device>` 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";
}
packages/coding-agent/test/tools/approval.test.ts
+50
View File
@@ -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.<policyKey> 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 [
packages/coding-agent/test/tools/policy-key.test.ts
+110
View File
@@ -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<AgentTool, "name" | "approval" | "formatApprovalDetails">;
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.<policyKey> 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" });
});
});
});
packages/coding-agent/test/write-xdev-dispatch.test.ts
+57 -5
View File
@@ -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