diff --git a/extensions/copilot/src/permission-bridge.test.ts b/extensions/copilot/src/permission-bridge.test.ts index 16e19d3873c2..d21dfadb1ac3 100755 --- a/extensions/copilot/src/permission-bridge.test.ts +++ b/extensions/copilot/src/permission-bridge.test.ts @@ -1,15 +1,10 @@ // Copilot tests cover permission bridge plugin behavior. import type { PermissionRequest as SdkPermissionRequest, - PermissionRequestResult as SdkPermissionRequestResult, } from "@github/copilot-sdk"; import { describe, expect, it, vi } from "vitest"; import { - allowListPolicy, - allowOncePolicy, - composePolicies, createPermissionBridge, - delegatingPolicy, rejectAllPolicy, REJECT_ALL_FEEDBACK, type CopilotPermissionContext, @@ -52,168 +47,9 @@ describe("rejectAllPolicy", () => { }); }); -describe("allowOncePolicy", () => { - it("returns approve-once for every request kind", async () => { - for (const kind of [ - "shell", - "write", - "mcp", - "read", - "url", - "custom-tool", - "memory", - "hook", - ] as const) { - const result = await allowOncePolicy(makeCtx({ request: makeRequest({ kind }) })); - expect(result).toEqual({ kind: "approve-once" }); - } - }); -}); - -describe("allowListPolicy", () => { - it("approves listed kinds and rejects others with default feedback", async () => { - const policy = allowListPolicy({ kinds: ["read"] }); - const approved = await policy(makeCtx({ request: makeRequest({ kind: "read" }) })); - expect(approved).toEqual({ kind: "approve-once" }); - const rejected = await policy(makeCtx({ request: makeRequest({ kind: "shell" }) })); - expect(rejected).toEqual({ kind: "reject", feedback: REJECT_ALL_FEEDBACK }); - }); - - it("uses custom rejectFeedback when provided", async () => { - const policy = allowListPolicy({ - kinds: ["read"], - rejectFeedback: "only reads allowed", - }); - const result = await policy(makeCtx({ request: makeRequest({ kind: "write" }) })); - expect(result).toEqual({ kind: "reject", feedback: "only reads allowed" }); - }); - - it("supports multiple kinds in the allow-list", async () => { - const policy = allowListPolicy({ kinds: ["read", "write"] }); - expect(await policy(makeCtx({ request: makeRequest({ kind: "read" }) }))).toEqual({ - kind: "approve-once", - }); - expect(await policy(makeCtx({ request: makeRequest({ kind: "write" }) }))).toEqual({ - kind: "approve-once", - }); - expect((await policy(makeCtx({ request: makeRequest({ kind: "mcp" }) })))?.kind).toBe("reject"); - }); - - it("rejects all when given an empty allow-list", async () => { - const policy = allowListPolicy({ kinds: [] }); - for (const kind of ["shell", "read", "write"] as const) { - const result = await policy(makeCtx({ request: makeRequest({ kind }) })); - expect(result?.kind).toBe("reject"); - } - }); -}); - -describe("delegatingPolicy", () => { - it("forwards the request to the host callback and returns its decision", async () => { - const onRequest = vi.fn().mockResolvedValue({ - kind: "approve-for-session", - } satisfies SdkPermissionRequestResult); - const policy = delegatingPolicy({ onRequest }); - const ctx = makeCtx({ sessionId: "sess-xyz", request: makeRequest({ kind: "write" }) }); - const result = await policy(ctx); - expect(result).toEqual({ kind: "approve-for-session" }); - expect(onRequest).toHaveBeenCalledTimes(1); - expect(onRequest).toHaveBeenCalledWith(ctx); - }); - - it("returns the rejectAll default when host callback returns undefined", async () => { - const onRequest = vi.fn().mockResolvedValue(undefined); - const policy = delegatingPolicy({ onRequest }); - const result = await policy(makeCtx()); - expect(result).toEqual({ kind: "reject", feedback: REJECT_ALL_FEEDBACK }); - }); - - it("rejects with the error message when host callback throws", async () => { - const onRequest = vi - .fn() - .mockRejectedValue(new Error("host policy boom")); - const policy = delegatingPolicy({ onRequest }); - const result = await policy(makeCtx()); - expect(result?.kind).toBe("reject"); - expect((result as { feedback?: string }).feedback).toContain("host policy boom"); - }); - - it("falls back to onError policy when host callback throws", async () => { - const onError = vi.fn().mockResolvedValue({ kind: "approve-once" }); - const policy = delegatingPolicy({ - onRequest: () => { - throw new Error("host policy boom"); - }, - onError, - }); - const result = await policy(makeCtx()); - expect(result).toEqual({ kind: "approve-once" }); - expect(onError).toHaveBeenCalledTimes(1); - }); - - it("falls through to a hard-coded reject if onError also throws", async () => { - const policy = delegatingPolicy({ - onRequest: () => { - throw new Error("host boom"); - }, - onError: () => { - throw new Error("fallback boom"); - }, - }); - const result = await policy(makeCtx()); - expect(result?.kind).toBe("reject"); - expect((result as { feedback?: string }).feedback).toContain("host boom"); - }); - - it("formats non-Error throws via JSON.stringify", async () => { - const policy = delegatingPolicy({ - onRequest: () => { - throw { code: 42, msg: "weird" } as unknown as Error; - }, - }); - const result = await policy(makeCtx()); - expect((result as { feedback?: string }).feedback).toContain('"code":42'); - }); -}); - -describe("composePolicies", () => { - it("returns the first non-undefined result and skips subsequent policies", async () => { - const a: CopilotPermissionPolicy = () => undefined; - const b: CopilotPermissionPolicy = () => ({ kind: "approve-once" }); - const c = vi.fn(() => ({ - kind: "reject", - feedback: "should never run", - })); - const policy = composePolicies(a, b, c); - const result = await policy(makeCtx()); - expect(result).toEqual({ kind: "approve-once" }); - expect(c).not.toHaveBeenCalled(); - }); - - it("falls through to fail-closed reject when all policies return undefined", async () => { - const policy = composePolicies( - () => undefined, - () => undefined, - ); - const result = await policy(makeCtx()); - expect(result).toEqual({ kind: "reject", feedback: REJECT_ALL_FEEDBACK }); - }); - - it("short-circuits to reject if any policy throws (does not consult later policies)", async () => { - const later = vi.fn(() => ({ kind: "approve-once" })); - const policy = composePolicies(() => { - throw new Error("nope"); - }, later); - const result = await policy(makeCtx()); - expect(result?.kind).toBe("reject"); - expect((result as { feedback?: string }).feedback).toContain("nope"); - expect(later).not.toHaveBeenCalled(); - }); -}); - describe("createPermissionBridge", () => { it("adapts a policy to the SDK PermissionHandler shape", async () => { - const handler = createPermissionBridge(allowOncePolicy); + const handler = createPermissionBridge(() => ({ kind: "approve-once" })); const result = await handler(makeRequest(), { sessionId: "sess-1" }); expect(result).toEqual({ kind: "approve-once" }); }); @@ -251,7 +87,7 @@ describe("createPermissionBridge", () => { }); it("handles all SDK permission kinds without throwing", async () => { - const handler = createPermissionBridge(allowOncePolicy); + const handler = createPermissionBridge(() => ({ kind: "approve-once" })); for (const kind of [ "shell", "write", diff --git a/extensions/copilot/src/permission-bridge.ts b/extensions/copilot/src/permission-bridge.ts index 94b740e93d5b..b2191bfe9fa3 100755 --- a/extensions/copilot/src/permission-bridge.ts +++ b/extensions/copilot/src/permission-bridge.ts @@ -10,19 +10,13 @@ * 1. Defines a small `CopilotPermissionPolicy` contract that the * host can implement to mirror PI's policy decisions for the * copilot agent runtime. - * 2. Provides built-in policies for common defaults (fail-closed, - * approve-all-for-test, allow-list-by-kind). - * 3. Provides a `delegatingPolicy({ onRequest })` so the core layer - * can plug in a host-side callback that calls into - * `runBeforeToolCallHook` / `effective-tool-policy` and returns - * the SDK-shaped decision. - * 4. Adapts the resulting policy into the SDK's + * 2. Adapts the resulting policy into the SDK's * `PermissionHandler` shape via `createPermissionBridge(policy)`. * * Cross-package boundary note: the heavy `pi-tools.before-tool-call` * surface cannot be imported here (`tsconfig.package-boundary.base.json`). * The host bridges core PI logic into this module by injecting a - * `delegatingPolicy` from the core wiring layer that constructs + * `CopilotPermissionPolicy` from the core wiring layer that constructs * `AgentHarnessAttemptParams` for the copilot agent runtime. * * If PI's permission semantics change materially, the contract here @@ -67,117 +61,6 @@ export const rejectAllPolicy: CopilotPermissionPolicy = () => ({ feedback: REJECT_ALL_FEEDBACK, }); -/** - * Approve every request as "approve-once". Use only in tests / live - * smoke runs where the operator has accepted the risk. This is the - * SDK-bundled `approveAll` behavior re-exported as an explicit named - * policy so test sites can opt in without `@github/copilot-sdk` - * imports leaking into call sites. - */ -export const allowOncePolicy: CopilotPermissionPolicy = () => ({ - kind: "approve-once", -}); - -export interface AllowListPolicyOptions { - /** Permission kinds that should be approved once. */ - kinds: ReadonlyArray; - /** Optional feedback text attached to rejections. */ - rejectFeedback?: string; -} - -/** - * Approve requests whose `kind` is in the allow-list; reject everything - * else with `rejectFeedback` (defaulting to `REJECT_ALL_FEEDBACK`). - */ -export function allowListPolicy(options: AllowListPolicyOptions): CopilotPermissionPolicy { - const allowed = new Set(options.kinds); - const feedback = options.rejectFeedback ?? REJECT_ALL_FEEDBACK; - return ({ request }) => { - if (allowed.has(request.kind)) { - return { kind: "approve-once" }; - } - return { kind: "reject", feedback }; - }; -} - -export interface DelegatingPolicyOptions { - /** - * Host-supplied callback. Returning `undefined` falls through to the - * fail-closed default. Throwing falls back to the configured - * `onError` policy if provided; otherwise the throw is converted to a - * reject with the error message embedded in `feedback` (so the model - * sees the diagnostic instead of a generic RPC failure). - */ - onRequest: CopilotPermissionPolicy; - /** - * Optional fallback when `onRequest` throws. If omitted, throws are - * reflected back as `reject` with the error message in `feedback`. - * If supplied and `onError` also throws, fall through to the - * error-message reject. - */ - onError?: CopilotPermissionPolicy; -} - -/** - * Wrap a host callback into a policy, catching synchronous throws and - * async rejections so the SDK never sees an exception (which would - * surface as a generic RPC failure to the model). - */ -export function delegatingPolicy(options: DelegatingPolicyOptions): CopilotPermissionPolicy { - const { onRequest, onError } = options; - return async (ctx) => { - try { - const result = await onRequest(ctx); - if (result !== undefined) { - return result; - } - return { kind: "reject", feedback: REJECT_ALL_FEEDBACK }; - } catch (error) { - if (onError) { - try { - const fallback = await onError(ctx); - if (fallback !== undefined) { - return fallback; - } - } catch { - // fall through to error-message reject - } - } - return { - kind: "reject", - feedback: `copilot permission policy threw: ${formatError(error)}`, - }; - } - }; -} - -/** - * Compose policies in order. The first policy to return a non-undefined - * result wins. If all return undefined, a fail-closed `reject` is - * produced. Throws inside any policy short-circuit to `reject` with the - * error message; downstream policies are not consulted after a throw - * (so a misbehaving host policy cannot mask itself by being followed by - * an allow-policy). - */ -export function composePolicies(...policies: CopilotPermissionPolicy[]): CopilotPermissionPolicy { - return async (ctx) => { - for (const policy of policies) { - try { - const result = await policy(ctx); - if (result !== undefined) { - return result; - } - } catch (error) { - return { - kind: "reject", - feedback: `copilot permission policy threw: ${formatError(error)}`, - }; - } - } - return { kind: "reject", feedback: REJECT_ALL_FEEDBACK }; - }; -} - /** * Adapt a `CopilotPermissionPolicy` to the SDK's * `PermissionHandler` shape. The returned handler always resolves