From 49d67d96db7e966a5b7cbbac9e112d9346a84e44 Mon Sep 17 00:00:00 2001 From: Helge Tesdal Date: Tue, 28 Apr 2026 15:30:00 +0200 Subject: [PATCH] refactor(run): collapse dual permission handling paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit run.ts had two permission responders — the SSE-loop branch (lines 559-574) and the RunEvents bus subscriber — gated only by 'if (runEventsHandle)'. The gate is correct today but the structure invites a future race where both fire. Investigation showed runEventsHandle is null exactly when args.attach is set (run.ts:648), and the remote opencode server does not currently spin up its own RunEvents handler. So the SSE-loop branch has real semantics in attach mode: it is the only auto-responder for permission asks visible to the local user. Choice: option 2 from the F10 plan — keep the attach-mode reply, but extract it into a clearly-named exported helper (replyPermissionAttachMode) with a coupling-note doc block that documents the invariant (and how a future server-side RunEvents would make this helper a redundant double-responder). The SSE-loop block is now structurally: if (runEventsHandle) { /* log only, then continue */ } await replyPermissionAttachMode({...}) eliminating the dual-path ambiguity while preserving today's behavior. Tests: 4 new unit tests in test/cli/run-attach-permission.test.ts covering each cell of the (skipPermissions × jsonMode) matrix and a no-double-reply pin. 18/18 pass across run-events + new file. Addresses audit finding F10 (Opus diamond review, 2026-04-22). --- packages/opencode/src/cli/cmd/run.ts | 161 ++++++++++--- .../test/cli/run-attach-permission.test.ts | 222 ++++++++++++++++++ 2 files changed, 353 insertions(+), 30 deletions(-) create mode 100644 packages/opencode/test/cli/run-attach-permission.test.ts diff --git a/packages/opencode/src/cli/cmd/run.ts b/packages/opencode/src/cli/cmd/run.ts index 8f6c97328f33..8c2e8214ab5c 100644 --- a/packages/opencode/src/cli/cmd/run.ts +++ b/packages/opencode/src/cli/cmd/run.ts @@ -212,6 +212,125 @@ function normalizePath(input?: string) { return input } +/** + * Reply to a `permission.asked` SSE event in attach mode. + * + * Coupling note: in non-attach mode `RunEvents.make` runs in-process alongside + * `prompt.loop` and owns the auto-reply contract for the root session and its + * descendants (it is *local* to this CLI process, not server-side). In attach + * mode, the local CLI is just an SSE viewer of a remote opencode server, and + * the remote server does not currently spin up its own RunEvents handler — + * so this function is the only auto-responder for permission asks visible to + * the local user. If a future change makes the remote server attach-aware + * (i.e., it runs its own RunEvents per attached client), this helper becomes + * a redundant double-responder and must be removed (along with the dispatch + * in `dispatchPermissionAsked` and its call site in run.ts's SSE loop). + * + * Behavior matrix for attach mode: + * - skipPermissions=true → reply "once" (silent; symmetric with auto-approve flow) + * - skipPermissions=false, jsonMode=false → log + reply "reject" + * - skipPermissions=false, jsonMode=true → reply "reject" without UI or JSON + * emission (no parity with non-attach `auto-reject` JSON event today; attach + * mode has no equivalent emitter — see followup note below) + * + * Followup (non-blocking): attach + jsonMode silently auto-rejects without + * emitting an `auto-reject` JSON event (non-attach mode emits one via + * RunEvents). Reaching parity would require either an attach-side JSON + * emitter here or moving JSON emission into a sink that both modes share. + * Out of scope for F10 (which only collapses the dual permission paths). + * + * Each invocation produces exactly one `sdk.permission.reply` call. Caller + * `dispatchPermissionAsked` invokes this exactly once per `permission.asked` + * SSE event matching the active sessionID. + */ +type PermissionReplyClient = { + readonly permission: { + readonly reply: (input: { + requestID: string + reply: "once" | "always" | "reject" + }) => Promise + } +} + +export async function replyPermissionAttachMode(input: { + sdk: PermissionReplyClient + permission: { id: string; permission: string; patterns: readonly string[] } + skipPermissions: boolean + jsonMode: boolean + println: (message: string) => void +}): Promise { + if (input.skipPermissions) { + await input.sdk.permission.reply({ requestID: input.permission.id, reply: "once" }) + return + } + if (!input.jsonMode) { + input.println( + `permission requested: ${input.permission.permission} (${input.permission.patterns.join(", ")}); auto-rejecting`, + ) + } + await input.sdk.permission.reply({ requestID: input.permission.id, reply: "reject" }) +} + +/** + * Dispatch a `permission.asked` SSE event to either the no-op-with-log path + * (non-attach: `runEventsHandle` is set, in-process RunEvents owns the reply) + * or the attach-mode reply path (`runEventsHandle` is null, this client must + * reply via SDK). + * + * Exported for unit-test access only — the call site is `run.ts`'s SSE loop. + * Keeping it exported gives the F10 dual-path invariant a testable seam without + * having to drive the whole CLI. + * + * Invariant: `hasRunEventsHandle === !args.attach` (enforced at the + * `runEventsHandle` ternary in run.ts; see comment near construction). If that + * invariant ever drifts — e.g. attach mode also gets a server-side RunEvents + * — the dual-responder race F10 was raised against returns. The unit tests + * for this dispatch pin the contract: at most one `sdk.permission.reply` per + * `permission.asked` event. + * + * Returns true if the event was for this session (and was therefore handled), + * false if filtered out by sessionID mismatch. + */ +export async function dispatchPermissionAsked(input: { + permission: { id: string; sessionID: string; permission: string; patterns: readonly string[] } + // sessionID is the active session for this run. Typed as `string | undefined` + // because the call site closure (run.ts's `loop()`) is declared before the + // null guard on `await session(sdk)`. At runtime sessionID is always defined + // (process.exit(1) on the null branch); a stray undefined value here would + // simply filter out all events, which is safe-by-default. + sessionID: string | undefined + hasRunEventsHandle: boolean + sdk: PermissionReplyClient + skipPermissions: boolean + jsonMode: boolean + println: (message: string) => void +}): Promise { + if (input.permission.sessionID !== input.sessionID) return false + + if (input.hasRunEventsHandle) { + // Non-attach mode: in-process RunEvents owns the auto-reply contract; + // here we only surface a UI line (skipped under dangerously-skip-permissions + // and under jsonMode, where the matching auto-reject JSON event is emitted + // by RunEvents instead). + if (!input.skipPermissions && !input.jsonMode) { + input.println( + `permission requested: ${input.permission.permission} (${input.permission.patterns.join(", ")}); auto-rejecting`, + ) + } + return true + } + + // Attach mode: see replyPermissionAttachMode coupling note. + await replyPermissionAttachMode({ + sdk: input.sdk, + permission: input.permission, + skipPermissions: input.skipPermissions, + jsonMode: input.jsonMode, + println: input.println, + }) + return true +} + export const RunCommand = cmd({ command: "run [message..]", describe: "run opencode with a message", @@ -542,36 +661,18 @@ export const RunCommand = cmd({ } if (event.type === "permission.asked") { - const permission = event.properties - if (permission.sessionID !== sessionID) continue - - if (runEventsHandle) { - if (!args["dangerously-skip-permissions"] && !jsonMode) { - UI.println( - UI.Style.TEXT_WARNING_BOLD + "!", - UI.Style.TEXT_NORMAL + - `permission requested: ${permission.permission} (${permission.patterns.join(", ")}); auto-rejecting`, - ) - } - continue - } - - if (args["dangerously-skip-permissions"]) { - await sdk.permission.reply({ - requestID: permission.id, - reply: "once", - }) - } else { - UI.println( - UI.Style.TEXT_WARNING_BOLD + "!", - UI.Style.TEXT_NORMAL + - `permission requested: ${permission.permission} (${permission.patterns.join(", ")}); auto-rejecting`, - ) - await sdk.permission.reply({ - requestID: permission.id, - reply: "reject", - }) - } + await dispatchPermissionAsked({ + permission: event.properties, + sessionID, + // Invariant: hasRunEventsHandle === !args.attach (see runEventsHandle + // construction below). If that invariant drifts, dispatchPermissionAsked's + // contract breaks — see its doc block. + hasRunEventsHandle: runEventsHandle !== null, + sdk, + skipPermissions: !!args["dangerously-skip-permissions"], + jsonMode, + println: (msg) => UI.println(UI.Style.TEXT_WARNING_BOLD + "!", UI.Style.TEXT_NORMAL + msg), + }) } } } diff --git a/packages/opencode/test/cli/run-attach-permission.test.ts b/packages/opencode/test/cli/run-attach-permission.test.ts new file mode 100644 index 000000000000..11854d89208e --- /dev/null +++ b/packages/opencode/test/cli/run-attach-permission.test.ts @@ -0,0 +1,222 @@ +import { describe, expect, test } from "bun:test" +import { dispatchPermissionAsked, replyPermissionAttachMode } from "../../src/cli/cmd/run" + +// In attach mode, `runEventsHandle` is null because the local `opencode run --attach` +// process is just an SSE viewer of a remote opencode server — and that server does +// not currently spin up its own RunEvents handler. So the local SSE-loop branch is +// the only auto-responder for `permission.asked` events. F10 collapses the dispatch +// into `dispatchPermissionAsked`; these tests pin the contract that: +// - non-attach (hasRunEventsHandle=true) NEVER calls sdk.permission.reply +// - attach (hasRunEventsHandle=false) calls sdk.permission.reply EXACTLY ONCE +// per permission.asked event matching the active sessionID. + +type ReplyCall = { requestID: string; reply: "once" | "always" | "reject" } + +function makeStubSdk() { + const calls: ReplyCall[] = [] + return { + sdk: { + permission: { + reply: async (input: { requestID: string; reply: "once" | "always" | "reject" }) => { + calls.push({ requestID: input.requestID, reply: input.reply }) + return { data: undefined } + }, + }, + }, + calls, + } +} + +const ROOT_SESSION = "ses_root_0000000000000000000000" + +const askedEvent = { + id: "perm_abc", + sessionID: ROOT_SESSION, + permission: "bash", + patterns: ["rm -rf /"], +} + +describe("cli/run replyPermissionAttachMode (helper unit tests)", () => { + test("dangerously-skip-permissions=true: replies once, no UI", async () => { + const { sdk, calls } = makeStubSdk() + const printed: string[] = [] + + await replyPermissionAttachMode({ + sdk, + permission: askedEvent, + skipPermissions: true, + jsonMode: false, + println: (msg) => printed.push(msg), + }) + + expect(calls).toHaveLength(1) + expect(calls[0]).toEqual({ requestID: "perm_abc", reply: "once" }) + expect(printed).toEqual([]) + }) + + test("dangerously-skip-permissions=false, jsonMode=false: rejects and prints UI", async () => { + const { sdk, calls } = makeStubSdk() + const printed: string[] = [] + + await replyPermissionAttachMode({ + sdk, + permission: askedEvent, + skipPermissions: false, + jsonMode: false, + println: (msg) => printed.push(msg), + }) + + expect(calls).toHaveLength(1) + expect(calls[0]).toEqual({ requestID: "perm_abc", reply: "reject" }) + expect(printed).toHaveLength(1) + expect(printed[0]).toContain("permission requested: bash") + expect(printed[0]).toContain("rm -rf /") + expect(printed[0]).toContain("auto-rejecting") + }) + + test("dangerously-skip-permissions=false, jsonMode=true: rejects without UI", async () => { + const { sdk, calls } = makeStubSdk() + const printed: string[] = [] + + await replyPermissionAttachMode({ + sdk, + permission: askedEvent, + skipPermissions: false, + jsonMode: true, + println: (msg) => printed.push(msg), + }) + + expect(calls).toHaveLength(1) + expect(calls[0]).toEqual({ requestID: "perm_abc", reply: "reject" }) + expect(printed).toEqual([]) + }) +}) + +describe("cli/run dispatchPermissionAsked (dual-path contract)", () => { + test("F10 invariant: non-attach (hasRunEventsHandle=true) NEVER calls sdk.permission.reply", async () => { + const { sdk, calls } = makeStubSdk() + const printed: string[] = [] + + const handled = await dispatchPermissionAsked({ + permission: askedEvent, + sessionID: ROOT_SESSION, + hasRunEventsHandle: true, + sdk, + skipPermissions: false, + jsonMode: false, + println: (msg) => printed.push(msg), + }) + + expect(handled).toBe(true) + // The defining invariant: when RunEvents is active in-process, the SSE loop + // must NOT also reply via SDK or both responders fire on the same event. + expect(calls).toHaveLength(0) + expect(printed).toHaveLength(1) + expect(printed[0]).toContain("auto-rejecting") + }) + + test("non-attach + skipPermissions: still no SDK reply, and no UI line (RunEvents owns both)", async () => { + const { sdk, calls } = makeStubSdk() + const printed: string[] = [] + + await dispatchPermissionAsked({ + permission: askedEvent, + sessionID: ROOT_SESSION, + hasRunEventsHandle: true, + sdk, + skipPermissions: true, + jsonMode: false, + println: (msg) => printed.push(msg), + }) + + expect(calls).toHaveLength(0) + expect(printed).toEqual([]) + }) + + test("non-attach + jsonMode: still no SDK reply, no UI line (RunEvents emits JSON)", async () => { + const { sdk, calls } = makeStubSdk() + const printed: string[] = [] + + await dispatchPermissionAsked({ + permission: askedEvent, + sessionID: ROOT_SESSION, + hasRunEventsHandle: true, + sdk, + skipPermissions: false, + jsonMode: true, + println: (msg) => printed.push(msg), + }) + + expect(calls).toHaveLength(0) + expect(printed).toEqual([]) + }) + + test("F10 invariant: attach (hasRunEventsHandle=false) replies EXACTLY ONCE", async () => { + const { sdk, calls } = makeStubSdk() + const printed: string[] = [] + + const handled = await dispatchPermissionAsked({ + permission: askedEvent, + sessionID: ROOT_SESSION, + hasRunEventsHandle: false, + sdk, + skipPermissions: false, + jsonMode: false, + println: (msg) => printed.push(msg), + }) + + expect(handled).toBe(true) + expect(calls).toHaveLength(1) + expect(calls[0]).toEqual({ requestID: "perm_abc", reply: "reject" }) + }) + + test("filters out events for non-active sessions and does not reply or log", async () => { + const { sdk, calls } = makeStubSdk() + const printed: string[] = [] + + const handled = await dispatchPermissionAsked({ + permission: { ...askedEvent, sessionID: "ses_other_0000000000000000000000" }, + sessionID: ROOT_SESSION, + hasRunEventsHandle: false, + sdk, + skipPermissions: false, + jsonMode: false, + println: (msg) => printed.push(msg), + }) + + expect(handled).toBe(false) + expect(calls).toEqual([]) + expect(printed).toEqual([]) + }) + + test("non-attach branch is inert under all flag combinations: even invoked alongside the attach branch on one event, total replies = 1", async () => { + // The F10 invariant is enforced by the call site (one dispatch per event). + // This test pins the *dispatcher* contract that makes that enforcement + // possible: regardless of skipPermissions/jsonMode, the non-attach branch + // must never reply via SDK. If a future change accidentally fans the + // non-attach branch out to also call sdk.permission.reply, this test fails. + const { sdk, calls } = makeStubSdk() + + await dispatchPermissionAsked({ + permission: askedEvent, + sessionID: ROOT_SESSION, + hasRunEventsHandle: true, + sdk, + skipPermissions: false, + jsonMode: true, + println: () => {}, + }) + await dispatchPermissionAsked({ + permission: askedEvent, + sessionID: ROOT_SESSION, + hasRunEventsHandle: false, + sdk, + skipPermissions: false, + jsonMode: true, + println: () => {}, + }) + + expect(calls).toHaveLength(1) + expect(calls[0].requestID).toBe("perm_abc") + }) +})