From d50cb5becc928a1444a94b595e81c4fd934ec16a Mon Sep 17 00:00:00 2001 From: Tom Rochette Date: Tue, 28 Jul 2026 10:36:00 -0400 Subject: [PATCH] fix(ui): deny open permission prompts on send (#2445) Sending a message while a permission prompt is open now denies every pending permission in the session subtree (optimistically, then formally via permission.reply reject) and queues the message for next-turn delivery, mirroring the question-dismiss path from #1740. - Add dismissOpenPermissionsForSession plus isPermissionRequestNotFoundError and removePermissionRequestFromChildStores helpers to session-actions - Extend dismissPermission with not-found cleanup, parallel to rejectQuestion - Wire handleSubmit to deny permissions and dismiss questions together, queueing once if either prompt type was open - Add unit tests mirroring the dismissOpenQuestionsForSession suite Closes #1958 --- packages/ui/src/components/chat/ChatInput.tsx | 16 +- packages/ui/src/sync/session-actions.test.ts | 159 ++++++++++++++++++ packages/ui/src/sync/session-actions.ts | 137 ++++++++++++++- 3 files changed, 303 insertions(+), 9 deletions(-) diff --git a/packages/ui/src/components/chat/ChatInput.tsx b/packages/ui/src/components/chat/ChatInput.tsx index f8473f81..51a0d889 100644 --- a/packages/ui/src/components/chat/ChatInput.tsx +++ b/packages/ui/src/components/chat/ChatInput.tsx @@ -978,8 +978,20 @@ const ChatInputComponent: React.FC = ({ onOpenSettings, scrollTo } if (currentSessionId && !queuedOnly) { - const dismissedQuestions = await sessionActions.dismissOpenQuestionsForSession(currentSessionId); - if (dismissedQuestions) { + // Sending is authoritative for blocking prompts: deny pending + // permissions and dismiss open questions for the session subtree, + // then queue the message once if either was open. The deny/clear + // vanishes the card instantly (optimistic); rejecting unblocks the + // agent's tool but does NOT end its turn, so a direct send would + // race with the still-active run and be silently discarded by the + // OpenCode runner. Instead we queue; the queued-message auto-send + // hook delivers it as the next turn once the rejected turn winds + // down and the session returns to idle (parity with #1740). + const [deniedPermissions, dismissedQuestions] = await Promise.all([ + sessionActions.dismissOpenPermissionsForSession(currentSessionId), + sessionActions.dismissOpenQuestionsForSession(currentSessionId), + ]); + if (deniedPermissions || dismissedQuestions) { handleQueueMessage(); return; } diff --git a/packages/ui/src/sync/session-actions.test.ts b/packages/ui/src/sync/session-actions.test.ts index 5f1d5b41..ccb6d5c6 100644 --- a/packages/ui/src/sync/session-actions.test.ts +++ b/packages/ui/src/sync/session-actions.test.ts @@ -9,6 +9,7 @@ const registeredSessionDirectories: Array<{ sessionID: string; directory: string let sessionRevertResult: { data?: unknown; error?: unknown; response?: { status?: number } } = {} let questionReplyError: unknown | null = null let questionRejectError: unknown | null = null +let permissionReplyError: unknown | null = null let sessionShareResult: { data?: unknown; error?: unknown; response?: { status?: number } } = {} let sessionUpdateResult: { data?: unknown; error?: unknown; response?: { status?: number } } = {} let sessionMessagesResult: { data?: unknown; error?: unknown; response?: { status?: number } } = { data: [] } @@ -22,6 +23,10 @@ const mockScopedClient = { permission: { reply: mock((params: Record) => { replyCalls.push({ method: "permission.reply", params }) + if (permissionReplyError) { + const status = (permissionReplyError as { status?: number })?.status ?? 404 + return Promise.resolve({ error: permissionReplyError, response: { status } }) + } return Promise.resolve({ data: true }) }), }, @@ -85,6 +90,10 @@ const mockSdk = { permission: { reply: mock((params: Record) => { replyCalls.push({ method: "permission.reply", params }) + if (permissionReplyError) { + const status = (permissionReplyError as { status?: number })?.status ?? 404 + return Promise.resolve({ error: permissionReplyError, response: { status } }) + } return Promise.resolve({ data: true }) }), }, @@ -970,6 +979,7 @@ describe("dismissPermission passes directory", () => { replyCalls.length = 0 scopedClientDirectories.length = 0 questionReplyError = null + permissionReplyError = null }) test("passes directory and reply=reject", async () => { @@ -1084,6 +1094,17 @@ function buildQuestion(id: string, sessionId: string): QuestionRequest { } } +function buildPermission(id: string, sessionId: string): PermissionRequest { + return { + id, + sessionID: sessionId, + permission: "edit", + patterns: [], + metadata: {}, + always: [], + } +} + describe("dismissOpenQuestionsForSession", () => { beforeEach(() => { replyCalls.length = 0 @@ -1157,3 +1178,141 @@ describe("dismissOpenQuestionsForSession", () => { expect(store.getState().question["session-a"]).toBe(undefined) }) }) + +describe("dismissPermission not-found handling", () => { + beforeEach(() => { + replyCalls.length = 0 + scopedClientDirectories.length = 0 + permissionReplyError = null + }) + + test("clears the stale permission and rethrows on PermissionNotFoundError", async () => { + const permission = buildPermission("perm-stale", "session-a") + const store = createStore({ "session-a": [permission] }) + const childStores = createChildStores([["/test/project", store]]) + permissionReplyError = Object.assign(new Error("permission.reply failed (404): PermissionNotFoundError"), { status: 404 }) + + const { setActionRefs, dismissPermission } = await import("./session-actions") + setActionRefs(mockSdk as unknown as OpencodeClient, childStores, () => "/test/project") + + await expect(dismissPermission("session-a", "perm-stale")).rejects.toThrow() + expect(replyCalls.filter((call) => call.method === "permission.reply")).toHaveLength(1) + // The stale entry is cleared from the store even though the server reported not-found. + expect(store.getState().permission["session-a"]).toBe(undefined) + }) + + test("does not clear the store on a non-not-found failure (rethrow only)", async () => { + const permission = buildPermission("perm-500", "session-a") + const store = createStore({ "session-a": [permission] }) + const childStores = createChildStores([["/test/project", store]]) + permissionReplyError = Object.assign(new Error("permission.reply failed (500)"), { status: 500 }) + + const { setActionRefs, dismissPermission } = await import("./session-actions") + setActionRefs(mockSdk as unknown as OpencodeClient, childStores, () => "/test/project") + + await expect(dismissPermission("session-a", "perm-500")).rejects.toThrow() + // A non-not-found failure leaves store reconciliation to the next server event. + expect(store.getState().permission["session-a"]).toHaveLength(1) + }) +}) + +describe("dismissOpenPermissionsForSession", () => { + beforeEach(() => { + replyCalls.length = 0 + scopedClientDirectories.length = 0 + permissionReplyError = null + }) + + test("returns false and rejects nothing when no permissions are pending", async () => { + const store = createStore({}, { session: [{ id: "session-a", time: { created: 1 } } as Session] }) + const childStores = createChildStores([["/test/project", store]]) + + const { setActionRefs, dismissOpenPermissionsForSession } = await import("./session-actions") + setActionRefs(mockSdk as unknown as OpencodeClient, childStores, () => "/test/project") + + const dismissed = await dismissOpenPermissionsForSession("session-a") + + expect(dismissed).toBe(false) + expect(replyCalls.filter((call) => call.method === "permission.reply")).toHaveLength(0) + }) + + test("rejects every pending permission in the session subtree (root + subagent child)", async () => { + const rootPermission = buildPermission("perm-root", "session-a") + const childPermission = buildPermission("perm-child", "session-child") + const store = createStore({ + "session-a": [rootPermission], + "session-child": [childPermission], + }, { + session: [ + { id: "session-a", time: { created: 1 } } as Session, + { id: "session-child", parentID: "session-a", time: { created: 2 } } as Session, + ], + }) + const childStores = createChildStores([["/test/project", store]]) + + const { setActionRefs, dismissOpenPermissionsForSession } = await import("./session-actions") + setActionRefs(mockSdk as unknown as OpencodeClient, childStores, () => "/test/project") + + const dismissed = await dismissOpenPermissionsForSession("session-a") + + expect(dismissed).toBe(true) + const replyCallsForPermissions = replyCalls.filter((call) => call.method === "permission.reply") + expect(replyCallsForPermissions).toHaveLength(2) + const rejectedIds = replyCallsForPermissions.map((call) => call.params.requestID).sort() + expect(rejectedIds).toEqual(["perm-child", "perm-root"]) + expect(replyCallsForPermissions.every((call) => call.params.reply === "reject")).toBe(true) + // Optimistic clear: the permissions are removed from the local store so the + // prompt disappears instantly, without waiting for the reject round-trip. + expect(store.getState().permission["session-a"]).toBe(undefined) + expect(store.getState().permission["session-child"]).toBe(undefined) + }) + + test("swallows PermissionNotFoundError so a stranded permission never blocks the send", async () => { + const stalePermission = buildPermission("perm-stale", "session-a") + const store = createStore({ "session-a": [stalePermission] }, { + session: [{ id: "session-a", time: { created: 1 } } as Session], + }) + const childStores = createChildStores([["/test/project", store]]) + permissionReplyError = Object.assign(new Error("permission.reply failed (404): PermissionNotFoundError"), { status: 404 }) + + const { setActionRefs, dismissOpenPermissionsForSession } = await import("./session-actions") + setActionRefs(mockSdk as unknown as OpencodeClient, childStores, () => "/test/project") + + const dismissed = await dismissOpenPermissionsForSession("session-a") + + expect(dismissed).toBe(true) + const replyCallsForPermissions = replyCalls.filter((call) => call.method === "permission.reply") + expect(replyCallsForPermissions).toHaveLength(1) + expect(replyCallsForPermissions[0].params.requestID).toBe("perm-stale") + // The stale entry is cleared from the store even though the server reported not-found. + expect(store.getState().permission["session-a"]).toBe(undefined) + }) + + test("swallows and logs a non-not-found reject failure so the send is never blocked", async () => { + const permission = buildPermission("perm-500", "session-a") + const store = createStore({ "session-a": [permission] }, { + session: [{ id: "session-a", time: { created: 1 } } as Session], + }) + const childStores = createChildStores([["/test/project", store]]) + permissionReplyError = Object.assign(new Error("permission.reply failed (500)"), { status: 500 }) + + const { setActionRefs, dismissOpenPermissionsForSession } = await import("./session-actions") + setActionRefs(mockSdk as unknown as OpencodeClient, childStores, () => "/test/project") + + const errors: unknown[][] = [] + const originalError = console.error + console.error = (...args: unknown[]) => { errors.push(args) } + try { + const dismissed = await dismissOpenPermissionsForSession("session-a") + + expect(dismissed).toBe(true) + const replyCallsForPermissions = replyCalls.filter((call) => call.method === "permission.reply") + expect(replyCallsForPermissions).toHaveLength(1) + expect(replyCallsForPermissions[0].params.requestID).toBe("perm-500") + expect(errors).toHaveLength(1) + expect(String(errors[0]?.[0])).toContain("[session-actions]") + } finally { + console.error = originalError + } + }) +}) diff --git a/packages/ui/src/sync/session-actions.ts b/packages/ui/src/sync/session-actions.ts index b3357368..94324037 100644 --- a/packages/ui/src/sync/session-actions.ts +++ b/packages/ui/src/sync/session-actions.ts @@ -553,6 +553,56 @@ function removeQuestionRequestFromChildStores(sessionId: string, requestId: stri return removed } +function isPermissionRequestNotFoundError(error: unknown): boolean { + if (error && typeof error === "object") { + const status = (error as { status?: unknown }).status + if (status === 404) return true + } + + let message = "" + if (error instanceof Error) { + message = error.message + } else if (typeof error === "string") { + message = error + } + + return /Permission(?:\.)?NotFoundError|Permission request not found/i.test(message) +} + +function removePermissionRequestFromChildStores(sessionId: string, requestId: string): boolean { + const stores = _childStores + if (!stores || !requestId) return false + + let removed = false + for (const [, store] of stores.children) { + const current = store.getState().permission ?? {} + let nextPermission: typeof current | null = null + const sessionIds = new Set([sessionId, ...Object.keys(current)].filter(Boolean)) + + for (const candidateSessionId of sessionIds) { + const requests = current[candidateSessionId] + if (!requests?.length) continue + + const nextRequests = requests.filter((request) => request.id !== requestId) + if (nextRequests.length === requests.length) continue + + nextPermission ??= { ...current } + if (nextRequests.length > 0) { + nextPermission[candidateSessionId] = nextRequests + } else { + delete nextPermission[candidateSessionId] + } + removed = true + } + + if (nextPermission) { + store.setState({ permission: nextPermission }) + } + } + + return removed +} + function getRequestReplyClient( type: "permission" | "question", sessionId: string, @@ -1093,16 +1143,89 @@ export async function dismissPermission( const directory = resolveDirectoryForBlockingRequest("permission", sessionId, requestId) || getSessionDirectory(sessionId) || dir() - const result = await getRequestReplyClient("permission", sessionId, requestId).permission.reply({ - requestID: requestId, - reply: "reject", - ...(directory ? { directory } : {}), - }) - if (assertSdkData(result, "permission.reply") !== true) { - throw new Error("Permission dismissal failed") + try { + const result = await getRequestReplyClient("permission", sessionId, requestId).permission.reply({ + requestID: requestId, + reply: "reject", + ...(directory ? { directory } : {}), + }) + if (assertSdkData(result, "permission.reply") !== true) { + throw new Error("Permission dismissal failed") + } + } catch (error) { + if (isPermissionRequestNotFoundError(error)) { + removePermissionRequestFromChildStores(sessionId, requestId) + } + throw error } } +/** + * Dismiss every pending permission for the session subtree rooted at `sessionId` + * (the session itself plus any subagent children). Used by the chat send path: + * sending a message while a permission prompt is open must cancel/supersede the + * open permission so it cannot linger or block the new turn. + * + * The permissions are removed from the local store OPTIMISTICALLY (before any + * network call) so the prompt disappears instantly instead of waiting on the + * `permission.reply` round-trip. Each permission is then formally rejected on + * the backend via `permission.reply` with `reply: "reject"`, which fires + * `permission.replied` for reconciliation. + * + * Returns true when at least one permission was dismissed. Rejection failures are + * swallowed (a stranded permission must never block the send); + * PermissionNotFoundError also clears the stale entry from the child store via + * {@link dismissPermission}. + * + * NOTE: rejecting unblocks the agent's tool but does NOT end its turn. Callers + * that need to send the next message right away (the chat send path) must also + * queue the message so the OpenCode runner reaches `idle` — otherwise the new + * prompt arrives while the run is still active and is discarded by the runner's + * `ensureRunning`. + */ +export async function dismissOpenPermissionsForSession(sessionId: string): Promise { + if (!sessionId) return false + const stores = _childStores + if (!stores) return false + + const toDismiss: Array<{ sessionId: string; requestId: string }> = [] + for (const [, store] of stores.children) { + const state = store.getState() + const scopedIds = computeSubtreeIds(state.session, sessionId) + if (scopedIds.size === 0) continue + const permissionsBySession = state.permission ?? {} + for (const scopedId of scopedIds) { + const requests = permissionsBySession[scopedId] + if (!requests) continue + for (const request of requests) { + toDismiss.push({ sessionId: scopedId, requestId: request.id }) + } + } + } + + if (toDismiss.length === 0) return false + + // Optimistically clear the permissions from the local store so the prompt + // disappears immediately, before the reject round-trip. + for (const { sessionId: scopedSessionId, requestId } of toDismiss) { + removePermissionRequestFromChildStores(scopedSessionId, requestId) + } + + await Promise.all( + toDismiss.map(async ({ sessionId: scopedSessionId, requestId }) => { + try { + await dismissPermission(scopedSessionId, requestId) + } catch (error) { + if (isPermissionRequestNotFoundError(error)) return + // Swallow: a failed dismissal must not block the send. The next + // permission.asked / permission.replied event reconciles the store. + console.error("[session-actions] Failed to dismiss open permission on send:", error) + } + }), + ) + return true +} + // --------------------------------------------------------------------------- // Questions // ---------------------------------------------------------------------------