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

Merge PR #7945: fix(coding-agent): resolve xd:// device dispatches against device user policy first (@re2zero)

Browse Source
This commit is contained in:
can1357
2026-08-11 15:06:11 +02:00
parent 261eb965ac 9d0699e070
commit 8504e4865f
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