From 334bf623709d91b9b9d5217fb17acc289ed16622 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Tue, 11 Aug 2026 00:14:27 +0200 Subject: [PATCH] Keep delegated-agent notifications alive through permission prompts (#3177) * fix(agents): preserve permission notification callbacks Permission requests consumed the one-shot completion watcher, so callers never heard about later approvals or the final result. Keep the watcher alive across permission checkpoints and include the actionable request payload in each notification. * fix(agents): notify concurrent permission requests --- docs/agent-lifecycle.md | 2 + .../src/server/agent/agent-prompt.test.ts | 172 ++++++++++++++++++ .../server/src/server/agent/agent-prompt.ts | 111 ++++++++--- 3 files changed, 261 insertions(+), 24 deletions(-) diff --git a/docs/agent-lifecycle.md b/docs/agent-lifecycle.md index eeed68322..ac3ceb053 100644 --- a/docs/agent-lifecycle.md +++ b/docs/agent-lifecycle.md @@ -49,6 +49,8 @@ Runtime ownership is resolved from explicit workspace ID and caller context, nev Users can also detach an existing subagent from the subagents track. Detach is deliberately a manual lifecycle gesture, not an agent-facing MCP tool. It removes the `paseo.parent-agent-id` label only: it does not stop, archive, move, or restart the agent. The agent keeps its current `cwd` and `workspaceId`, leaves the former parent's track, and behaves like a root agent for tab close, workspace activity, and future parent archive. `notifyOnFinish` defaults to `true` for agent-scoped creation and background prompt follow-ups because most delegated work needs to report back to the creating agent. Set it to `false` only for truly fire-and-forget agents or prompts. +Permission requests are notification checkpoints, not the end of that subscription. The caller is notified again after a permission response when the child finishes, errors, or requests another permission. +The permission notification includes the normalized request plus the child and request IDs, so the caller can inspect it and respond without fetching agent status. ## Provider-managed child agents diff --git a/packages/server/src/server/agent/agent-prompt.test.ts b/packages/server/src/server/agent/agent-prompt.test.ts index ba3c64f5e..742e19bfb 100644 --- a/packages/server/src/server/agent/agent-prompt.test.ts +++ b/packages/server/src/server/agent/agent-prompt.test.ts @@ -46,8 +46,13 @@ interface FinishNotificationScenarioOptions { interface FinishNotificationScenario { startWatchingChild(): void; + requestChildPermission(requestId?: string): void; + resolveChildPermission(requestId?: string): void; + resolveChildPermissionFromState(requestId?: string): void; + resolveChildPermissionWhileIdle(requestId?: string): void; finishChild(): void; finishChildAndReadParentPrompt(): Promise; + parentPrompts(): string[]; wasParentPrompted(): boolean; } @@ -57,11 +62,13 @@ function createFinishNotificationScenario( let subscriber: ((event: AgentManagerEvent) => void) | null = null; let resolveParentPrompt: ((prompt: string) => void) | null = null; let parentPrompted = false; + const parentPrompts: string[] = []; const childAgent: ManagedAgent = Object.create(null); Reflect.set(childAgent, "id", "child-agent"); Reflect.set(childAgent, "lifecycle", "idle"); Reflect.set(childAgent, "config", { title: "Child Agent" }); + Reflect.set(childAgent, "pendingPermissions", new Map()); const callerAgent: ManagedAgent = Object.create(null); Reflect.set(callerAgent, "id", "caller-agent"); @@ -91,6 +98,7 @@ function createFinishNotificationScenario( Reflect.set(agentManager, "hasInFlightRun", () => Boolean(options?.parentPromptError)); Reflect.set(agentManager, "streamAgent", (_agentId: string, prompt: string) => { parentPrompted = true; + parentPrompts.push(prompt); resolveParentPrompt?.(prompt); return (async function* noop() {})(); }); @@ -123,6 +131,65 @@ function createFinishNotificationScenario( logger: options?.logger ?? createTestLogger(), }); }, + requestChildPermission(requestId = "permission-1") { + childAgent.lifecycle = "running"; + childAgent.pendingPermissions.set(requestId, { + id: requestId, + provider: "claude", + kind: "tool", + name: "Run command", + description: "Write the QA sentinel", + input: { + file_path: "/tmp/permission-qa.txt", + content: "PASEO_PERMISSION_NOTIFY_QA_OK\n", + }, + }); + subscriber?.({ + type: "agent_state", + agent: childAgent, + }); + subscriber?.({ + type: "agent_stream", + agentId: "child-agent", + event: { + type: "permission_requested", + provider: "codex", + request: childAgent.pendingPermissions.get(requestId)!, + }, + }); + }, + resolveChildPermission(requestId = "permission-1") { + childAgent.pendingPermissions.delete(requestId); + subscriber?.({ + type: "agent_stream", + agentId: "child-agent", + event: { + type: "permission_resolved", + provider: "codex", + requestId, + resolution: { behavior: "allow" }, + }, + }); + }, + resolveChildPermissionFromState(requestId = "permission-1") { + childAgent.pendingPermissions.delete(requestId); + subscriber?.({ type: "agent_state", agent: childAgent }); + }, + resolveChildPermissionWhileIdle(requestId = "permission-1") { + childAgent.pendingPermissions.delete(requestId); + childAgent.lifecycle = "idle"; + subscriber?.({ type: "agent_state", agent: childAgent }); + subscriber?.({ + type: "agent_stream", + agentId: "child-agent", + event: { + type: "permission_resolved", + provider: "codex", + requestId, + resolution: { behavior: "allow" }, + }, + }); + }, finishChild() { childAgent.lifecycle = "running"; subscriber?.({ @@ -144,6 +211,9 @@ function createFinishNotificationScenario( return parentPrompt; }, + parentPrompts() { + return parentPrompts; + }, wasParentPrompted() { return parentPrompted; }, @@ -209,6 +279,107 @@ test("finish notifications tell the parent the child's last assistant message", ); }); +test("finish notifications survive permission responses", async () => { + const scenario = createFinishNotificationScenario(); + + scenario.startWatchingChild(); + scenario.requestChildPermission(); + + await vi.waitFor(() => { + expect(scenario.parentPrompts()).toHaveLength(1); + }); + expect(scenario.parentPrompts()[0]).toContain("needs permission."); + const permissionPayload = scenario + .parentPrompts()[0] + .match(/\n([\s\S]+?)\n<\/permission-request>/)?.[1]; + expect(permissionPayload).toBeDefined(); + expect(JSON.parse(permissionPayload!)).toEqual({ + agentId: "child-agent", + requestId: "permission-1", + request: { + id: "permission-1", + provider: "claude", + kind: "tool", + name: "Run command", + description: "Write the QA sentinel", + input: { + file_path: "/tmp/permission-qa.txt", + content: "PASEO_PERMISSION_NOTIFY_QA_OK\n", + }, + }, + }); + + scenario.resolveChildPermission(); + scenario.finishChild(); + + await vi.waitFor(() => { + expect(scenario.parentPrompts()).toHaveLength(2); + }); + expect(scenario.parentPrompts()[1]).toContain("finished."); +}); + +test("an idle permission resolution waits for the resumed run to finish", async () => { + const scenario = createFinishNotificationScenario(); + + scenario.startWatchingChild(); + scenario.requestChildPermission(); + await vi.waitFor(() => expect(scenario.parentPrompts()).toHaveLength(1)); + + scenario.resolveChildPermissionWhileIdle(); + scenario.requestChildPermission("permission-2"); + await vi.waitFor(() => expect(scenario.parentPrompts()).toHaveLength(2)); + expect(scenario.parentPrompts().every((prompt) => prompt.includes("needs permission."))).toBe( + true, + ); + + scenario.resolveChildPermission("permission-2"); + scenario.finishChild(); + await vi.waitFor(() => expect(scenario.parentPrompts()).toHaveLength(3)); + expect(scenario.parentPrompts()[2]).toContain("finished."); +}); + +test("finish notifications report every concurrently pending permission", async () => { + const scenario = createFinishNotificationScenario(); + + scenario.startWatchingChild(); + scenario.requestChildPermission("permission-1"); + scenario.requestChildPermission("permission-2"); + + await vi.waitFor(() => expect(scenario.parentPrompts()).toHaveLength(2)); + expect( + scenario.parentPrompts().map((prompt) => { + const payload = prompt.match(/\n([\s\S]+?)\n<\/permission-request>/)?.[1]; + return JSON.parse(payload!).requestId; + }), + ).toEqual(["permission-1", "permission-2"]); + + scenario.resolveChildPermission("permission-1"); + scenario.resolveChildPermission("permission-2"); + scenario.finishChild(); + + await vi.waitFor(() => expect(scenario.parentPrompts()).toHaveLength(3)); + expect(scenario.parentPrompts()[2]).toContain("finished."); +}); + +test("finish notifications survive repeated permission cycles", async () => { + const scenario = createFinishNotificationScenario(); + + scenario.startWatchingChild(); + scenario.requestChildPermission(); + await vi.waitFor(() => expect(scenario.parentPrompts()).toHaveLength(1)); + scenario.resolveChildPermissionFromState(); + + scenario.requestChildPermission(); + await vi.waitFor(() => expect(scenario.parentPrompts()).toHaveLength(2)); + scenario.resolveChildPermission(); + scenario.finishChild(); + + await vi.waitFor(() => expect(scenario.parentPrompts()).toHaveLength(3)); + expect( + scenario.parentPrompts().map((prompt) => prompt.match(/(needs permission|finished)\./)?.[1]), + ).toEqual(["needs permission", "needs permission", "finished"]); +}); + test("detaching a child ends its parent-owned finish notification", async () => { const scenario = createFinishNotificationScenario({ childParentAgentId: null, @@ -258,6 +429,7 @@ it("does not notify archived callers", async () => { Reflect.set(childAgent, "id", "child-agent"); Reflect.set(childAgent, "lifecycle", "idle"); Reflect.set(childAgent, "config", { title: "Child Agent" }); + Reflect.set(childAgent, "pendingPermissions", new Map()); const callerAgent: ManagedAgent = Object.create(null); Reflect.set(callerAgent, "id", "caller-agent"); diff --git a/packages/server/src/server/agent/agent-prompt.ts b/packages/server/src/server/agent/agent-prompt.ts index 53d367416..375f643b0 100644 --- a/packages/server/src/server/agent/agent-prompt.ts +++ b/packages/server/src/server/agent/agent-prompt.ts @@ -1,6 +1,10 @@ import type { Logger } from "pino"; -import type { AgentPromptInput, AgentRunOptions } from "./agent-sdk-types.js"; +import type { + AgentPermissionRequest, + AgentPromptInput, + AgentRunOptions, +} from "./agent-sdk-types.js"; import type { AgentManager, ManagedAgent } from "./agent-manager.js"; import type { AgentStorage } from "./agent-storage.js"; import { ensureAgentLoaded } from "./agent-loading.js"; @@ -254,15 +258,36 @@ interface FinishNotificationBodyInput { title: string; reason: "finished" | "errored" | "needs permission"; lastAssistantMessage: string | null; + permissionRequest?: AgentPermissionRequest; } function formatFinishNotificationBody(params: FinishNotificationBodyInput): string { const statusLine = `Agent ${params.childAgentId} (${params.title}) ${params.reason}.`; - const lastAssistantMessage = params.lastAssistantMessage?.trim(); - if (!lastAssistantMessage) { - return statusLine; + const sections = [statusLine]; + if (params.reason === "needs permission" && params.permissionRequest) { + sections.push( + "Respond with `respond_to_permission` using the `agentId` and `requestId` below.", + `\n${JSON.stringify( + { + agentId: params.childAgentId, + requestId: params.permissionRequest.id, + request: params.permissionRequest, + }, + null, + 2, + )}\n`, + ); } - return `${statusLine}\n\n\n${lastAssistantMessage}\n`; + const lastAssistantMessage = params.lastAssistantMessage?.trim(); + if (lastAssistantMessage) { + sections.push(`\n${lastAssistantMessage}\n`); + } + return sections.join("\n\n"); +} + +interface NotifySafelyOptions { + terminal?: boolean; + permissionRequest?: AgentPermissionRequest; } export function setupFinishNotification(params: SetupFinishNotificationParams): void { @@ -275,16 +300,21 @@ export function setupFinishNotification(params: SetupFinishNotificationParams): logger, } = params; let hasSeenRunning = false; - let fired = false; + let stopped = false; + const notifiedPermissionRequestIds = new Set(); let unsubscribe: (() => void) | null = null; + let notificationQueue = Promise.resolve(); - async function notify(reason: "finished" | "errored" | "needs permission"): Promise { - if (fired) { - return; - } - fired = true; + function stop(): void { + if (stopped) return; + stopped = true; unsubscribe?.(); + } + async function notify( + reason: "finished" | "errored" | "needs permission", + permissionRequest?: AgentPermissionRequest, + ): Promise { const callerRecord = await agentStorage.get(callerAgentId); if (callerRecord?.archivedAt) { return; @@ -301,6 +331,7 @@ export function setupFinishNotification(params: SetupFinishNotificationParams): title, reason, lastAssistantMessage, + permissionRequest, }); await sendPromptToAgent({ @@ -313,24 +344,38 @@ export function setupFinishNotification(params: SetupFinishNotificationParams): }); } - function notifySafely(reason: "finished" | "errored" | "needs permission"): void { - void notify(reason).catch((error) => { - logger.error( - { err: error, childAgentId, callerAgentId, reason }, - "Failed to notify caller agent", - ); - }); + function notifySafely( + reason: "finished" | "errored" | "needs permission", + options: NotifySafelyOptions = {}, + ): void { + if (stopped) return; + if (options.terminal ?? true) stop(); + notificationQueue = notificationQueue + .then(() => notify(reason, options.permissionRequest)) + .catch((error) => { + logger.error( + { err: error, childAgentId, callerAgentId, reason }, + "Failed to notify caller agent", + ); + }); } unsubscribe = agentManager.subscribe( (event) => { - if (fired) { + if (stopped) { return; } if (event.type === "agent_state") { + for (const requestId of notifiedPermissionRequestIds) { + if (!event.agent.pendingPermissions.has(requestId)) { + notifiedPermissionRequestIds.delete(requestId); + } + } if (event.agent.lifecycle === "running") { - hasSeenRunning = true; + if (event.agent.pendingPermissions.size === 0) { + hasSeenRunning = true; + } return; } if (event.agent.lifecycle === "error") { @@ -342,15 +387,33 @@ export function setupFinishNotification(params: SetupFinishNotificationParams): return; } if (event.agent.lifecycle === "closed") { - fired = true; - unsubscribe?.(); + stop(); return; } return; } if (event.event.type === "permission_requested") { - notifySafely("needs permission"); + // A permission pause is an intermediate checkpoint. Forget the run + // observed before it so an idle state during follow-up startup cannot + // masquerade as the final completion. + hasSeenRunning = false; + if (!notifiedPermissionRequestIds.has(event.event.request.id)) { + notifiedPermissionRequestIds.add(event.event.request.id); + notifySafely("needs permission", { + terminal: false, + permissionRequest: event.event.request, + }); + } + return; + } + + if (event.event.type === "permission_resolved") { + notifiedPermissionRequestIds.delete(event.event.requestId); + const childAgent = agentManager.getAgent(childAgentId); + if (childAgent?.pendingPermissions.size === 0) { + hasSeenRunning = childAgent.lifecycle === "running"; + } } }, { agentId: childAgentId, replayState: false }, @@ -363,7 +426,7 @@ export function setupFinishNotification(params: SetupFinishNotificationParams): // transitioning to "running"). const childSnapshot = agentManager.getAgent(childAgentId); if (!childSnapshot || childSnapshot.lifecycle === "closed") { - unsubscribe(); + stop(); return; } if (childSnapshot.lifecycle === "running") {