From 841165a716fb17a871ecb5d57effd9cc3019c4ce Mon Sep 17 00:00:00 2001 From: H-TTTTT Date: Sat, 11 Jul 2026 17:50:38 +0800 Subject: [PATCH] fix(permission): clear resolved prompts --- src/app/managers/permission-manager.ts | 50 +++++++- .../managers/summary-aggregation-manager.ts | 45 ++++++- .../callbacks/permission-callback-handler.ts | 26 ++++ src/bot/menus/permission-menu.ts | 16 ++- .../services/event-subscription-service.ts | 38 +++++- .../summary-aggregation-manager.test.ts | 4 +- .../permission-callback-handler.test.ts | 88 +++++++++++++ .../event-subscription-service.test.ts | 121 ++++++++++++++++++ tests/helpers/reset-singleton-state.ts | 2 + 9 files changed, 378 insertions(+), 12 deletions(-) diff --git a/src/app/managers/permission-manager.ts b/src/app/managers/permission-manager.ts index bc634ab6..04ee5854 100644 --- a/src/app/managers/permission-manager.ts +++ b/src/app/managers/permission-manager.ts @@ -5,15 +5,26 @@ class PermissionManager { private state: PermissionState = { requestsByMessageId: new Map(), }; + private resolvedRequestIDs = new Set(); + private generation = 0; /** * Register a new permission request message */ - startPermission(request: PermissionRequest, messageId: number): void { + startPermission( + request: PermissionRequest, + messageId: number, + generation: number = this.generation, + ): boolean { logger.debug( `[PermissionManager] startPermission: id=${request.id}, permission=${request.permission}, messageId=${messageId}`, ); + if (generation !== this.generation || this.resolvedRequestIDs.has(request.id)) { + logger.debug(`[PermissionManager] Ignoring stale or already resolved request: id=${request.id}`); + return false; + } + if (this.state.requestsByMessageId.has(messageId)) { logger.warn(`[PermissionManager] Message ID already tracked, replacing: ${messageId}`); } @@ -23,6 +34,8 @@ class PermissionManager { logger.info( `[PermissionManager] New permission request: type=${request.permission}, patterns=${request.patterns.join(", ")}, pending=${this.state.requestsByMessageId.size}`, ); + + return true; } /** @@ -101,6 +114,39 @@ class PermissionManager { return request; } + /** + * Remove all Telegram messages tracking an OpenCode permission request ID + */ + resolveRequest(requestID: string): number[] { + this.resolvedRequestIDs.add(requestID); + const removedMessageIds: number[] = []; + + for (const [messageId, request] of this.state.requestsByMessageId) { + if (request.id !== requestID) { + continue; + } + + this.state.requestsByMessageId.delete(messageId); + removedMessageIds.push(messageId); + } + + if (removedMessageIds.length > 0) { + logger.debug( + `[PermissionManager] Removed resolved permission request: id=${requestID}, messages=${removedMessageIds.length}, pending=${this.state.requestsByMessageId.size}`, + ); + } + + return removedMessageIds; + } + + isResolved(requestID: string): boolean { + return this.resolvedRequestIDs.has(requestID); + } + + getGeneration(): number { + return this.generation; + } + /** * Get number of active permission requests */ @@ -126,6 +172,8 @@ class PermissionManager { this.state = { requestsByMessageId: new Map(), }; + this.resolvedRequestIDs.clear(); + this.generation++; } } diff --git a/src/app/managers/summary-aggregation-manager.ts b/src/app/managers/summary-aggregation-manager.ts index bc78e925..c90581be 100644 --- a/src/app/managers/summary-aggregation-manager.ts +++ b/src/app/managers/summary-aggregation-manager.ts @@ -151,7 +151,9 @@ type SessionRetryCallback = (retryInfo: SessionRetryInfo) => void; type SessionIdleCallback = (sessionId: string) => void; -type PermissionCallback = (request: PermissionRequest) => void; +type PermissionCallback = (request: PermissionRequest) => void | Promise; + +type PermissionRepliedCallback = (sessionId: string, requestID: string) => void | Promise; type SessionDiffCallback = (sessionId: string, diffs: FileChange[]) => void; @@ -251,6 +253,7 @@ class SummaryAggregator { private onSessionRetryCallback: SessionRetryCallback | null = null; private onSessionIdleCallback: SessionIdleCallback | null = null; private onPermissionCallback: PermissionCallback | null = null; + private onPermissionRepliedCallback: PermissionRepliedCallback | null = null; private onSessionDiffCallback: SessionDiffCallback | null = null; private onFileChangeCallback: FileChangeCallback | null = null; private onClearedCallback: ClearedCallback | null = null; @@ -350,6 +353,10 @@ class SummaryAggregator { this.onPermissionCallback = callback; } + setOnPermissionReplied(callback: PermissionRepliedCallback): void { + this.onPermissionRepliedCallback = callback; + } + setOnSessionDiff(callback: SessionDiffCallback): void { this.onSessionDiffCallback = callback; } @@ -466,7 +473,7 @@ class SummaryAggregator { this.handlePermissionAsked(event); break; case "permission.replied": - logger.info(`[Aggregator] Permission replied: requestID=${event.properties.requestID}`); + this.handlePermissionReplied(event); break; default: logger.debug(`[Aggregator] Unhandled event type: ${event.type}`); @@ -2110,11 +2117,41 @@ class SummaryAggregator { if (this.onPermissionCallback) { const callback = this.onPermissionCallback; + try { + void Promise.resolve(callback(request as PermissionRequest)).catch((err) => { + logger.error("[Aggregator] Error in permission callback:", err); + }); + } catch (err) { + logger.error("[Aggregator] Error in permission callback:", err); + } + } + } + + private handlePermissionReplied( + event: Event & { + type: "permission.replied"; + }, + ): void { + const { sessionID, requestID } = event.properties; + const isCurrent = sessionID === this.currentSessionId; + const isTrackedChild = this.isTrackedChildSession(sessionID); + + if (!isCurrent && !isTrackedChild) { + logger.debug( + `[Aggregator] Ignoring permission.replied for different session: ${sessionID} (current: ${this.currentSessionId})`, + ); + return; + } + + logger.info(`[Aggregator] Permission replied: requestID=${requestID}`); + + if (this.onPermissionRepliedCallback) { + const callback = this.onPermissionRepliedCallback; setImmediate(async () => { try { - await callback(request as PermissionRequest); + await callback(sessionID, requestID); } catch (err) { - logger.error("[Aggregator] Error in permission callback:", err); + logger.error("[Aggregator] Error in permission replied callback:", err); } }); } diff --git a/src/bot/callbacks/permission-callback-handler.ts b/src/bot/callbacks/permission-callback-handler.ts index 59251dfd..771a26a0 100644 --- a/src/bot/callbacks/permission-callback-handler.ts +++ b/src/bot/callbacks/permission-callback-handler.ts @@ -24,6 +24,27 @@ function isPermissionReply(value: string): value is PermissionReply { return value === "once" || value === "always" || value === "reject"; } +function isPermissionRequestNotFound(error: unknown): boolean { + if (!error || typeof error !== "object") { + return false; + } + + const candidate = error as { + name?: unknown; + message?: unknown; + data?: { message?: unknown }; + }; + + if (candidate.name === "NotFoundError") { + return true; + } + + return [candidate.message, candidate.data?.message].some( + (message) => + typeof message === "string" && message.toLowerCase().includes("permission request not found"), + ); +} + export async function handlePermissionCallback(ctx: Context): Promise { const data = ctx.callbackQuery?.data; if (!data) return false; @@ -121,6 +142,11 @@ async function handlePermissionReply( }), onSuccess: ({ error }) => { if (error) { + if (isPermissionRequestNotFound(error)) { + logger.debug(`[PermissionHandler] Permission request already resolved: ${requestID}`); + return; + } + logger.error("[PermissionHandler] Failed to send permission reply:", error); if (ctx.api && chatId) { void ctx.api.sendMessage(chatId, t("permission.send_reply_error")).catch(() => {}); diff --git a/src/bot/menus/permission-menu.ts b/src/bot/menus/permission-menu.ts index da7f86d5..2e824b20 100644 --- a/src/bot/menus/permission-menu.ts +++ b/src/bot/menus/permission-menu.ts @@ -82,9 +82,18 @@ export async function showPermissionRequest( bot: Context["api"], chatId: number, request: PermissionRequest, + generation: number = permissionManager.getGeneration(), ): Promise { logger.debug(`[PermissionHandler] Showing permission request: ${request.permission}`); + if ( + generation !== permissionManager.getGeneration() || + permissionManager.isResolved(request.id) + ) { + logger.debug(`[PermissionHandler] Skipping stale or already resolved request: ${request.id}`); + return; + } + const text = formatPermissionText(request); const keyboard = buildPermissionKeyboard(); @@ -94,7 +103,12 @@ export async function showPermissionRequest( }); logger.debug(`[PermissionHandler] Message sent, messageId=${message.message_id}`); - permissionManager.startPermission(request, message.message_id); + if (!permissionManager.startPermission(request, message.message_id, generation)) { + await bot.deleteMessage(chatId, message.message_id).catch((err) => { + logger.warn(`[PermissionHandler] Failed to delete stale permission message:`, err); + }); + return; + } syncPermissionInteractionState({ requestID: request.id, diff --git a/src/bot/services/event-subscription-service.ts b/src/bot/services/event-subscription-service.ts index a6fa8b02..b035f285 100644 --- a/src/bot/services/event-subscription-service.ts +++ b/src/bot/services/event-subscription-service.ts @@ -69,9 +69,13 @@ import { } from "../../app/managers/background-session-manager.js"; import { buildBackgroundSessionOpenKeyboard } from "../menus/session-selection-menu.js"; import { questionManager } from "../../app/managers/question-manager.js"; +import { permissionManager } from "../../app/managers/permission-manager.js"; import { showCurrentQuestion } from "../menus/question-menu.js"; -import { showPermissionRequest } from "../menus/permission-menu.js"; -import { clearAllInteractionState } from "../../app/managers/interaction-manager.js"; +import { showPermissionRequest, syncPermissionInteractionState } from "../menus/permission-menu.js"; +import { + clearAllInteractionState, + interactionManager, +} from "../../app/managers/interaction-manager.js"; import { stopEventListening, subscribeToEvents } from "../../opencode/events.js"; const TELEGRAM_DOCUMENT_CAPTION_MAX_LENGTH = 1024; @@ -679,6 +683,8 @@ class EventSubscriptionService implements BotEventSubscriptionService { }); summaryAggregator.setOnPermission(async (request) => { + const generation = permissionManager.getGeneration(); + if (!this.botInstance || !this.chatIdInstance) { logger.error("Bot or chat ID not available for showing permission request"); return; @@ -703,7 +709,33 @@ class EventSubscriptionService implements BotEventSubscriptionService { logger.info( `[Bot] Received permission request from agent: type=${request.permission}, requestID=${request.id}, subagent=${isSubagent}`, ); - await showPermissionRequest(this.botInstance.api, this.chatIdInstance, request); + await showPermissionRequest(this.botInstance.api, this.chatIdInstance, request, generation); + }); + + summaryAggregator.setOnPermissionReplied(async (_sessionId, requestID) => { + const messageIds = permissionManager.resolveRequest(requestID); + const interaction = interactionManager.getSnapshot(); + if (!permissionManager.isActive() || !interaction || interaction.kind === "permission") { + syncPermissionInteractionState({ resolvedRequestID: requestID }); + } + + if (this.botInstance && this.chatIdInstance) { + const api = this.botInstance.api; + const chatId = this.chatIdInstance; + await Promise.all( + messageIds.map((messageId) => + api.deleteMessage(chatId, messageId).catch((err) => { + logger.warn(`[Bot] Failed to delete resolved permission message ${messageId}:`, err); + }), + ), + ); + } + + if (messageIds.length > 0) { + logger.info( + `[Bot] Cleared resolved permission prompt: requestID=${requestID}, messages=${messageIds.length}`, + ); + } }); summaryAggregator.setOnThinking(async (update) => { diff --git a/tests/app/managers/summary-aggregation-manager.test.ts b/tests/app/managers/summary-aggregation-manager.test.ts index e723e04d..87feadbd 100644 --- a/tests/app/managers/summary-aggregation-manager.test.ts +++ b/tests/app/managers/summary-aggregation-manager.test.ts @@ -1826,7 +1826,7 @@ describe("summary/aggregator", () => { expect(onTokens).not.toHaveBeenCalled(); }); - it("forwards permission.asked events from tracked subagent (child) sessions", async () => { + it("forwards permission.asked events from tracked subagent (child) sessions synchronously", () => { const onPermission = vi.fn(); summaryAggregator.setOnPermission(onPermission); summaryAggregator.setSession("root-session"); @@ -1876,8 +1876,6 @@ describe("summary/aggregator", () => { }, } as unknown as Event); - await new Promise((resolve) => setImmediate(resolve)); - expect(onPermission).toHaveBeenCalledTimes(1); expect(onPermission.mock.calls[0][0]).toEqual( expect.objectContaining({ diff --git a/tests/bot/callbacks/permission-callback-handler.test.ts b/tests/bot/callbacks/permission-callback-handler.test.ts index c6050ec2..426ede58 100644 --- a/tests/bot/callbacks/permission-callback-handler.test.ts +++ b/tests/bot/callbacks/permission-callback-handler.test.ts @@ -181,6 +181,61 @@ describe("bot permission menu/callbacks", () => { expect(state?.metadata.pendingCount).toBe(2); }); + it("does not show a permission request that was already resolved", async () => { + const botApi = createBotApi(502); + permissionManager.resolveRequest("perm-resolved"); + + await showPermissionRequest(botApi, 777, createPermissionRequest("perm-resolved")); + + expect(botApi.sendMessage).not.toHaveBeenCalled(); + expect(permissionManager.isActive()).toBe(false); + expect(interactionManager.getSnapshot()).toBeNull(); + }); + + it("does not send a permission message from a cleared lifecycle", async () => { + const botApi = createBotApi(503); + const generation = permissionManager.getGeneration(); + permissionManager.clear(); + + await showPermissionRequest( + botApi, + 777, + createPermissionRequest("perm-old-lifecycle"), + generation, + ); + + expect(botApi.sendMessage).not.toHaveBeenCalled(); + expect(permissionManager.isActive()).toBe(false); + }); + + it("discards an in-flight permission message after state is cleared", async () => { + let resolveSend: (message: { message_id: number }) => void = () => {}; + const pendingSend = new Promise<{ message_id: number }>((resolve) => { + resolveSend = resolve; + }); + const botApi = { + sendMessage: vi.fn().mockReturnValue(pendingSend), + deleteMessage: vi.fn().mockResolvedValue(true), + } as unknown as Context["api"]; + + const showTask = showPermissionRequest( + botApi, + 777, + createPermissionRequest("perm-old-session"), + ); + await vi.waitFor(() => { + expect(botApi.sendMessage).toHaveBeenCalledTimes(1); + }); + permissionManager.clear(); + resolveSend({ message_id: 503 }); + + await showTask; + + expect(botApi.deleteMessage).toHaveBeenCalledWith(777, 503); + expect(permissionManager.isActive()).toBe(false); + expect(interactionManager.getSnapshot()).toBeNull(); + }); + it("rejects callback from unknown permission message", async () => { const botApi = createBotApi(500); @@ -280,6 +335,39 @@ describe("bot permission menu/callbacks", () => { expect(interactionManager.getSnapshot()).toBeNull(); }); + it("does not report an error when the permission request was already resolved", async () => { + const botApi = createBotApi(750); + await showPermissionRequest(botApi, 777, createPermissionRequest("perm-stale")); + mocked.permissionReplyMock.mockResolvedValueOnce({ + error: { + name: "NotFoundError", + data: { message: "Permission request not found" }, + }, + }); + + const ctx = createPermissionCallbackContext("permission:always", 750); + await handlePermissionCallback(ctx); + await flushMicrotasks(); + + expect(ctx.api.sendMessage).not.toHaveBeenCalled(); + expect(permissionManager.isActive()).toBe(false); + expect(interactionManager.getSnapshot()).toBeNull(); + }); + + it("keeps reporting non-stale permission reply errors", async () => { + const botApi = createBotApi(751); + await showPermissionRequest(botApi, 777, createPermissionRequest("perm-error")); + mocked.permissionReplyMock.mockResolvedValueOnce({ + error: { name: "ServerError", data: { message: "Permission service unavailable" } }, + }); + + const ctx = createPermissionCallbackContext("permission:once", 751); + await handlePermissionCallback(ctx); + await flushMicrotasks(); + + expect(ctx.api.sendMessage).toHaveBeenCalledWith(777, t("permission.send_reply_error")); + }); + it("clears states when permission message cannot be sent", async () => { const botApi = { sendMessage: vi.fn().mockRejectedValue(new Error("send failed")), diff --git a/tests/bot/services/event-subscription-service.test.ts b/tests/bot/services/event-subscription-service.test.ts index 7e87ac26..cfa910ed 100644 --- a/tests/bot/services/event-subscription-service.test.ts +++ b/tests/bot/services/event-subscription-service.test.ts @@ -135,6 +135,37 @@ function emitSessionIdle(summaryAggregator: { processEvent(event: Event): void } } as unknown as Event); } +function emitPermissionAsked( + summaryAggregator: { processEvent(event: Event): void }, + requestID: string, +): void { + summaryAggregator.processEvent({ + type: "permission.asked", + properties: { + id: requestID, + sessionID: "session-1", + permission: "external_directory", + patterns: ["D:/shared/*"], + metadata: {}, + always: ["D:/shared/*"], + }, + } as unknown as Event); +} + +function emitPermissionReplied( + summaryAggregator: { processEvent(event: Event): void }, + requestID: string, +): void { + summaryAggregator.processEvent({ + type: "permission.replied", + properties: { + sessionID: "session-1", + requestID, + reply: "always", + }, + } as unknown as Event); +} + describe("bot/services/event-subscription-service", () => { let tempHome: string; let activeService: { cleanup(reason: string): void } | null = null; @@ -357,4 +388,94 @@ describe("bot/services/event-subscription-service", () => { expect(api.sendMessage.mock.calls[1][1]).toContain("test-provider/test-model"); expect(api.sendMessageDraft).not.toHaveBeenCalled(); }); + + it("clears permission prompts when OpenCode resolves pending requests", async () => { + const { api, summaryAggregator } = await setupService(true); + const [{ permissionManager }, { interactionManager }] = await Promise.all([ + import("../../../src/app/managers/permission-manager.js"), + import("../../../src/app/managers/interaction-manager.js"), + ]); + api.sendMessage + .mockResolvedValueOnce({ message_id: 500 }) + .mockResolvedValueOnce({ message_id: 501 }); + + emitPermissionAsked(summaryAggregator, "permission-1"); + emitPermissionAsked(summaryAggregator, "permission-2"); + + await vi.waitFor(() => { + expect(permissionManager.getPendingCount()).toBe(2); + }); + + emitPermissionReplied(summaryAggregator, "permission-2"); + + await vi.waitFor(() => { + expect(permissionManager.getPendingCount()).toBe(1); + }); + expect(api.deleteMessage).toHaveBeenCalledWith(42, 501); + expect(permissionManager.getRequestID(500)).toBe("permission-1"); + expect(interactionManager.getSnapshot()?.metadata.pendingCount).toBe(1); + + emitPermissionReplied(summaryAggregator, "permission-1"); + + await vi.waitFor(() => { + expect(permissionManager.isActive()).toBe(false); + expect(interactionManager.getSnapshot()).toBeNull(); + }); + expect(api.deleteMessage).toHaveBeenCalledWith(42, 500); + }); + + it("discards a permission prompt resolved while its Telegram message is being sent", async () => { + const { api, summaryAggregator } = await setupService(true); + const [{ permissionManager }, { interactionManager }] = await Promise.all([ + import("../../../src/app/managers/permission-manager.js"), + import("../../../src/app/managers/interaction-manager.js"), + ]); + let resolveSend: (message: { message_id: number }) => void = () => {}; + const pendingSend = new Promise<{ message_id: number }>((resolve) => { + resolveSend = resolve; + }); + api.sendMessage.mockReturnValueOnce(pendingSend); + + emitPermissionAsked(summaryAggregator, "permission-race"); + await vi.waitFor(() => { + expect(api.sendMessage).toHaveBeenCalledTimes(1); + }); + + emitPermissionReplied(summaryAggregator, "permission-race"); + await vi.waitFor(() => { + expect(permissionManager.isResolved("permission-race")).toBe(true); + }); + + resolveSend({ message_id: 502 }); + + await vi.waitFor(() => { + expect(api.deleteMessage).toHaveBeenCalledWith(42, 502); + }); + expect(permissionManager.isActive()).toBe(false); + expect(interactionManager.getSnapshot()).toBeNull(); + }); + + it("does not replace a newer interaction when a permission is resolved", async () => { + const { api, summaryAggregator } = await setupService(true); + const [{ permissionManager }, { interactionManager }] = await Promise.all([ + import("../../../src/app/managers/permission-manager.js"), + import("../../../src/app/managers/interaction-manager.js"), + ]); + api.sendMessage + .mockResolvedValueOnce({ message_id: 510 }) + .mockResolvedValueOnce({ message_id: 511 }); + emitPermissionAsked(summaryAggregator, "permission-1"); + emitPermissionAsked(summaryAggregator, "permission-2"); + await vi.waitFor(() => { + expect(permissionManager.getPendingCount()).toBe(2); + }); + interactionManager.start({ kind: "rename", expectedInput: "text" }); + + emitPermissionReplied(summaryAggregator, "permission-2"); + + await vi.waitFor(() => { + expect(permissionManager.getPendingCount()).toBe(1); + }); + expect(interactionManager.getSnapshot()?.kind).toBe("rename"); + }); }); diff --git a/tests/helpers/reset-singleton-state.ts b/tests/helpers/reset-singleton-state.ts index c4f573b6..e0f1b1ce 100644 --- a/tests/helpers/reset-singleton-state.ts +++ b/tests/helpers/reset-singleton-state.ts @@ -10,6 +10,7 @@ interface SummaryAggregatorPrivateState { onSessionCompactedCallback: null; onSessionErrorCallback: null; onPermissionCallback: null; + onPermissionRepliedCallback: null; onSessionDiffCallback: null; onFileChangeCallback: null; bot: null; @@ -88,6 +89,7 @@ export async function resetSingletonState(): Promise { aggregator.onSessionCompactedCallback = null; aggregator.onSessionErrorCallback = null; aggregator.onPermissionCallback = null; + aggregator.onPermissionRepliedCallback = null; aggregator.onSessionDiffCallback = null; aggregator.onFileChangeCallback = null; aggregator.bot = null;