diff --git a/packages/web-shell/client/App.module.css b/packages/web-shell/client/App.module.css index 467d2290903..d8fe6778c37 100644 --- a/packages/web-shell/client/App.module.css +++ b/packages/web-shell/client/App.module.css @@ -748,13 +748,16 @@ pointer-events: auto; } +/* In-flow bottom sheet: the approval replaces the hidden composer, so the + message list shrinks above it instead of being covered by a floating + overlay. The relative positioning + z-index still keeps the sheet above + portal-hosted DialogShell modals (backdrop at z-50) without overlapping + the message list. */ .approvalOverlay { - position: absolute; - right: 20px; - bottom: calc(100% + 8px); - left: 20px; + position: relative; z-index: calc(var(--web-shell-dialog-backdrop-z-index, 50) + 10); - pointer-events: auto; + margin-bottom: 8px; + min-width: 0; } .approvalOverlay:focus, @@ -827,6 +830,10 @@ padding: 0; } +.composerHidden { + display: none; +} + .composerHeader { margin-bottom: 8px; } diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index 4030676c5f7..5f80dfdf310 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -10397,6 +10397,55 @@ describe('App session callbacks', () => { expect(testState.latestToolApprovalKeyboardActive).toBe(true); }); + it('hides the composer while a tool approval overlay is pending and restores it after resolution', async () => { + const { container, rerender } = renderApp(); + await flush(); + + const composerWrapper = () => + container.querySelector('[data-web-shell-composer]')?.parentElement; + expect(composerWrapper()?.className).not.toContain('composerHidden'); + + await act(async () => { + testState.blocks = [makePendingPermissionBlock()]; + rerender(); + await Promise.resolve(); + }); + expect( + document.querySelector('[data-testid="approval-overlay"]'), + ).not.toBeNull(); + expect(composerWrapper()?.className).toContain('composerHidden'); + + await act(async () => { + testState.blocks = []; + rerender(); + await Promise.resolve(); + }); + expect( + document.querySelector('[data-testid="approval-overlay"]'), + ).toBeNull(); + expect(composerWrapper()?.className).not.toContain('composerHidden'); + }); + + it('hides the composer while an ask-user question overlay is pending', async () => { + const { container, rerender } = renderApp(); + await flush(); + + await act(async () => { + testState.blocks = [ + makePendingPermissionBlock({ toolName: 'ask_user_question' }), + ]; + rerender(); + await Promise.resolve(); + }); + expect( + document.querySelector('[data-testid="approval-overlay"]'), + ).not.toBeNull(); + expect( + container.querySelector('[data-web-shell-composer]')?.parentElement + ?.className, + ).toContain('composerHidden'); + }); + it('does not show missing-session state for non-404/410 errors', async () => { mockConnection.status = 'disconnected'; mockConnection.sessionId = undefined; diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 13e74200c65..8e9a3422252 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -12384,7 +12384,17 @@ export function App({ /> )} -
+ {/* A pending approval overlay owns the footer: drop the + composer out of layout (kept mounted so the draft + survives) instead of leaving a live input below the + dialog. */} +
{streamingState !== 'idle' ? ( suppressFailedPromptRetryStreaming ? null : ( { expect(footerProps.at(-1)?.disabled).toBe(true); }); + it('hides the pane composer while an approval is pending', () => { + pendingPermission = { id: 'perm-1', toolName: 'write_file', rawInput: {} }; + render(); + expect(testid('pane-approval')).not.toBeNull(); + // The streaming status and the editor share the approval-hidden wrapper, + // so neither lingers below the dialog. + expect(testid('pane-streaming')?.parentElement?.className).toContain( + 'composerHidden', + ); + expect( + container!.querySelector('[data-web-shell-composer]')?.parentElement + ?.className, + ).toContain('composerHidden'); + }); + + it('restores the pane composer after the approval resolves', () => { + pendingPermission = { id: 'perm-1', toolName: 'write_file', rawInput: {} }; + render(); + expect( + container!.querySelector('[data-web-shell-composer]')?.parentElement + ?.className, + ).toContain('composerHidden'); + + pendingPermission = null; + rerender(); + expect(testid('pane-approval')).toBeNull(); + expect( + container!.querySelector('[data-web-shell-composer]')?.parentElement + ?.className, + ).not.toContain('composerHidden'); + }); + it('adds no composer footer DOM when omitted or returning null', () => { render(); const composer = container!.querySelector('[data-web-shell-composer]'); diff --git a/packages/web-shell/client/components/ChatPane.tsx b/packages/web-shell/client/components/ChatPane.tsx index 2ace8fe1a7c..be45220c0bb 100644 --- a/packages/web-shell/client/components/ChatPane.tsx +++ b/packages/web-shell/client/components/ChatPane.tsx @@ -1043,79 +1043,86 @@ export function ChatPane({ />
)} - {/* Panes keep the composer status compact: spinner + elapsed time + - token count + cancel hint, but no rotating "witty" loading phrase. */} - - - {unknownPromptAdmission && ( -
- {t('queue.admissionUnknown')} - {unknownPromptAdmission.payloadAvailable && ( - - - - - )} -
- )} - - {CustomComposerFooter && ( - + {/* Panes keep the composer status compact: spinner + elapsed time + + token count + cancel hint, but no rotating "witty" loading + phrase. */} + + + {unknownPromptAdmission && ( +
+ {t('queue.admissionUnknown')} + {unknownPromptAdmission.payloadAvailable && ( + + + + + )} +
+ )} + - )} + {CustomComposerFooter && ( + + )} +
); diff --git a/packages/web-shell/client/components/messages/AskUserQuestion.module.css b/packages/web-shell/client/components/messages/AskUserQuestion.module.css index 6d845635153..a7cf8caf5f0 100644 --- a/packages/web-shell/client/components/messages/AskUserQuestion.module.css +++ b/packages/web-shell/client/components/messages/AskUserQuestion.module.css @@ -3,7 +3,7 @@ container-type: inline-size; margin: 12px auto; width: 100%; - max-width: min(800px, var(--chat-content-width, 800px)); + max-width: min(100%, var(--chat-content-width, 1000px)); box-sizing: border-box; padding: 16px; border: 1.5px solid var(--border); diff --git a/packages/web-shell/client/components/messages/SystemMessage.module.css b/packages/web-shell/client/components/messages/SystemMessage.module.css index ea04d37ec6b..39ff7546448 100644 --- a/packages/web-shell/client/components/messages/SystemMessage.module.css +++ b/packages/web-shell/client/components/messages/SystemMessage.module.css @@ -57,7 +57,6 @@ display: flex; width: 100%; justify-content: flex-start; - margin: 12px 0; } .notificationBubbleColumn { diff --git a/packages/web-shell/client/components/messages/ToolApproval.module.css b/packages/web-shell/client/components/messages/ToolApproval.module.css index 5c275f8e42e..7d071550f7d 100644 --- a/packages/web-shell/client/components/messages/ToolApproval.module.css +++ b/packages/web-shell/client/components/messages/ToolApproval.module.css @@ -1,7 +1,7 @@ .approval { margin: 0 auto; width: 100%; - max-width: min(800px, var(--chat-content-width, 800px)); + max-width: min(100%, var(--chat-content-width, 1000px)); box-sizing: border-box; padding: 16px; border: 1.5px solid var(--border); diff --git a/packages/web-shell/client/components/messages/ToolApproval.test.tsx b/packages/web-shell/client/components/messages/ToolApproval.test.tsx index 315ceb3d98b..0266291198c 100644 --- a/packages/web-shell/client/components/messages/ToolApproval.test.tsx +++ b/packages/web-shell/client/components/messages/ToolApproval.test.tsx @@ -389,6 +389,68 @@ describe('ToolApproval accessibility', () => { expect(document.activeElement).toBe(optionButtons()[0]); }); + it('defaults an agent-launch dialog to the one-shot allow, not Reject', () => { + render(undefined, { + id: 'req-agent-launch', + toolName: 'agent', + title: 'Launch Explore agent', + content: [], + options: [ + { + id: 'proceed_always_project', + label: 'Always in project', + kind: 'allow_always', + }, + { + id: 'proceed_always_user', + label: 'Always for user', + kind: 'allow_always', + }, + { id: 'proceed_once', label: 'Allow', kind: 'allow_once' }, + { id: 'cancel', label: 'Reject', kind: 'reject_once' }, + ], + }); + const opts = optionButtons(); + expect(opts.map((o) => o.getAttribute('data-option-id'))).toEqual([ + 'cancel', + 'proceed_always_user', + 'proceed_always_project', + 'proceed_once', + ]); + // Launching the agent is the proposed next action: focus and initial + // selection land on the one-shot allow instead of the reject button. + expect(document.activeElement).toBe(opts[3]); + expect(opts[3]!.tabIndex).toBe(0); + }); + + it('defaults an agent-launch dialog to Reject when no one-shot allow exists', () => { + render(undefined, { + id: 'req-agent-no-once', + toolName: 'agent', + title: 'Launch Explore agent', + content: [], + options: [ + { + id: 'proceed_always_project', + label: 'Always in project', + kind: 'allow_always', + }, + { + id: 'proceed_always_user', + label: 'Always for user', + kind: 'allow_always', + }, + { id: 'cancel', label: 'Reject', kind: 'reject_once' }, + ], + }); + const opts = optionButtons(); + // No one-shot allow: the default must be the reject, never a permanent + // allow rule. + expect(opts[0]!.getAttribute('data-option-id')).toBe('cancel'); + expect(document.activeElement).toBe(opts[0]); + expect(opts[0]!.tabIndex).toBe(0); + }); + it('leaves Enter to native button activation (no double-press guard)', () => { render(undefined); const opts = optionButtons(); diff --git a/packages/web-shell/client/components/messages/ToolApproval.tsx b/packages/web-shell/client/components/messages/ToolApproval.tsx index ac97d2ba23e..124dba150f1 100644 --- a/packages/web-shell/client/components/messages/ToolApproval.tsx +++ b/packages/web-shell/client/components/messages/ToolApproval.tsx @@ -92,7 +92,23 @@ function getDescriptionText(request: PermissionRequest): string | undefined { return request.title; } -function getSafeDefaultIndex(options: PermissionRequest['options']): number { +function getSafeDefaultIndex( + options: PermissionRequest['options'], + isAgent = false, +): number { + if (isAgent) { + // Launching the agent is the model's proposed next action: default the + // selection to the one-shot allow instead of the reject button, and never + // to a permanent allow rule. + const allowOnceIdx = options.findIndex((o) => o.kind === 'allow_once'); + if (allowOnceIdx >= 0) return allowOnceIdx; + // No one-shot option: fall back to the reject (safe) rather than landing + // on a permanent allow rule. + const rejectIdx = options.findIndex( + (o) => o.kind === 'reject_once' || o.kind === 'reject_always', + ); + return rejectIdx >= 0 ? rejectIdx : 0; + } if ( options.length > 1 && (options[0].kind === 'allow_always' || options[0].kind === 'reject_always') @@ -206,13 +222,14 @@ export function ToolApproval({ planTodos = [], }: ToolApprovalProps) { const { t } = useI18n(); + const isAgent = isAgentTool(request.toolName); const displayOptions = useMemo( () => prepareDisplayOptions(request.options), [request.options], ); const safeDefaultIndex = useMemo( - () => getSafeDefaultIndex(displayOptions), - [displayOptions], + () => getSafeDefaultIndex(displayOptions, isAgent), + [displayOptions, isAgent], ); // Prefer the localized label. Known producers give every option a distinct // i18n key (plan mode's restore_previous has its own), so this normally @@ -368,7 +385,6 @@ export function ToolApproval({ ); const isExec = isExecKind(request); - const isAgent = isAgentTool(request.toolName); const command = getCommandFromRawInput(request); const showsCommandBlock = Boolean( (isExec && command) || (contentText && contentText !== request.title), diff --git a/packages/web-shell/client/components/messages/ToolGroup.test.tsx b/packages/web-shell/client/components/messages/ToolGroup.test.tsx index b0f721a97ac..5634992f14a 100644 --- a/packages/web-shell/client/components/messages/ToolGroup.test.tsx +++ b/packages/web-shell/client/components/messages/ToolGroup.test.tsx @@ -1392,6 +1392,89 @@ describe('tool row rendering', () => { expect(onOpen).toHaveBeenCalledWith(tool); }); + it('keeps the agent row static while its launch approval is pending', () => { + const onOpen = vi.fn(); + const tool = makeTool({ + toolName: 'agent', + status: 'pending', + args: { subagent_type: 'Explore' }, + }); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + + + , + ); + }); + mounted.push({ root, container }); + + // No expand affordance while the launch approval is unanswered: the row + // must not open details for an agent that has not started yet. + expect(container.querySelector('[class*="lineExpandable"]')).toBeNull(); + expect(container.querySelector('button[class*="lineButton"]')).toBeNull(); + act(() => { + (container.querySelector('[class*="lineButton"]') as HTMLElement).click(); + }); + expect(onOpen).not.toHaveBeenCalled(); + }); + + it('keeps the agent row openable while a sub-tool approval is pending', () => { + const onOpen = vi.fn(); + const tool = makeTool({ + toolName: 'agent', + status: 'in_progress', + args: { subagent_type: 'Explore' }, + subTools: [{ callId: 'sub-1', toolName: 'web_fetch', status: 'pending' }], + }); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + + + , + ); + }); + mounted.push({ root, container }); + + // A sub-tool approval is not the agent's own launch approval: the row + // stays openable so the pending sub-tool stays reachable in the details + // panel. + expect( + container.querySelector('button[class*="lineButton"]'), + ).not.toBeNull(); + act(() => { + ( + container.querySelector('button[class*="lineButton"]') as HTMLElement + ).click(); + }); + expect(onOpen).toHaveBeenCalledWith(tool); + }); + it('respects hideHeader for agent tools inside SubagentDetailsProvider', () => { const onOpen = vi.fn(); const tool = makeTool({ diff --git a/packages/web-shell/client/components/messages/ToolGroup.tsx b/packages/web-shell/client/components/messages/ToolGroup.tsx index 75d2fd83e2d..e83a59ae1ea 100644 --- a/packages/web-shell/client/components/messages/ToolGroup.tsx +++ b/packages/web-shell/client/components/messages/ToolGroup.tsx @@ -1181,36 +1181,53 @@ export const ToolLine = memo(function ToolLine({ ] .filter(Boolean) .join(' · '); - const showExpanded = - forceExpanded || expanded || !!hasApproval || !!hasSubToolApproval; + const showExpanded = forceExpanded || expanded || !!hasSubToolApproval; + // While the agent's own launch approval is pending there is nothing to + // show yet — keep the row compact and non-openable; the approval dialog + // is the single source of interaction. + const approvalPending = !!hasApproval; const panel = ( ); if (subagentDetails && !hideHeader) { + const rowContent = ( + <> + + + {displayName} + + + ); return (
- + {approvalPending ? ( +
+ {rowContent} +
+ ) : ( + + )}
); } @@ -1218,8 +1235,10 @@ export const ToolLine = memo(function ToolLine({
{!hideHeader && (
setExpanded(!expanded)} + className={`${styles.lineMain} ${ + approvalPending ? '' : styles.lineExpandable + }`} + onClick={approvalPending ? undefined : () => setExpanded(!expanded)} > @@ -1236,12 +1255,14 @@ export const ToolLine = memo(function ToolLine({ workspaceCwd, }} /> -
)} {showExpanded && ( diff --git a/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.module.css b/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.module.css index 6fc0934fc11..7e6d810f1cf 100644 --- a/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.module.css +++ b/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.module.css @@ -281,6 +281,17 @@ outline: none; } +/* Pending-approval rows are inert placeholders: no pointer affordance, no + hover highlight. */ +.row[aria-disabled='true'] { + cursor: default; +} + +.row[aria-disabled='true']:hover, +.row[aria-disabled='true']:focus-visible { + background: transparent; +} + .rowStatus { width: 14px; min-width: 14px; diff --git a/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.test.tsx b/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.test.tsx index b70c3cf74f4..88777ccdf71 100644 --- a/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.test.tsx +++ b/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.test.tsx @@ -1224,6 +1224,48 @@ describe('ParallelAgentsGroup activity rendering', () => { } }); + it('keeps the pending-approval row inert while siblings stay clickable', () => { + const approval: PermissionRequest = { + id: 'approval', + toolCallId: 'a1', + content: [], + options: [], + }; + const { container } = renderManagedGroup( + [ + agent({ callId: 'a1', status: 'pending' }), + agent({ callId: 'a2', status: 'completed' }), + ], + { autoManageExpansion: true, pendingApproval: approval }, + ); + + const pendingRow = container.querySelector( + 'div[aria-disabled="true"]', + ) as HTMLElement; + // R2-5: the inert placeholder keeps the status so the dot keeps its + // active color instead of falling back to the muted default. + expect(pendingRow).not.toBeNull(); + expect(pendingRow.getAttribute('data-agent-status')).toBe('active'); + // R2-6: the pending row must not advertise a running agent. + expect( + pendingRow + .querySelector('[class*="rowStatus"]') + ?.getAttribute('aria-label'), + ).toBe('pending'); + // The pending row is a plain div: no button, no click affordance. + expect( + container.querySelector('button[data-agent-status="active"]'), + ).toBeNull(); + expect(pendingRow.tagName).toBe('DIV'); + + // The sibling whose approval is not pending stays fully interactive. + const sibling = container.querySelector( + 'button[data-agent-status="completed"]', + ) as HTMLButtonElement; + expect(sibling).not.toBeNull(); + expect(sibling.hasAttribute('aria-disabled')).toBe(false); + }); + it('hands focus to the summary when the automatic exit starts under a focused row', () => { vi.useFakeTimers(); try { diff --git a/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.tsx b/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.tsx index 5cf2e5a27c2..ba696b8ce36 100644 --- a/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.tsx +++ b/packages/web-shell/client/components/messages/tools/ParallelAgentsGroup.tsx @@ -451,78 +451,106 @@ export function ParallelAgentsGroup({ ? t('subagent.failed') : t('subagent.completed'); const isExpanded = expandedId === agent.callId; + // While the agent's own launch approval is unanswered there + // is nothing to show yet — keep the row compact and + // non-interactive, mirroring ToolLine's pending guard. + const approvalPending = + pendingApproval?.toolCallId === agent.callId; + const statusLabel = approvalPending + ? t('subagent.pending') + : rowStatusLabel; const localizedAgentType = localizeAgentTypeName( agentType, t, ); const showAgentType = !!desc && !isDefaultAgentType(agentType); - return ( -
- + + {(stats.duration || stats.tokens) && ( + + {stats.duration && {stats.duration}} + {stats.duration && stats.tokens && ( + + )} + {stats.tokens && {stats.tokens}} + + )} + + ); + return ( +
+ {approvalPending ? ( +
+ {rowContent} +
+ ) : ( + + )} {!subagentDetails && isExpanded && (
diff --git a/packages/web-shell/client/hooks/useMessages.test.ts b/packages/web-shell/client/hooks/useMessages.test.ts index 200846e527b..3ecda1ff9e7 100644 --- a/packages/web-shell/client/hooks/useMessages.test.ts +++ b/packages/web-shell/client/hooks/useMessages.test.ts @@ -604,6 +604,442 @@ describe('background agent task reconciliation', () => { vi.useRealTimers(); }); + it('does not fail an agent while its launch approval is unanswered', async () => { + vi.useFakeTimers(); + // The agent call is pending with an unresolved permission request for the + // same callId; its subagent session cannot exist yet, so the + // reconciliation 404 probe must be skipped rather than accumulating + // missing-agent misses and painting a failure. + hookState.blocks = [ + baseBlock({ + id: 'perm-agent', + kind: 'permission', + requestId: 'req-1', + sessionId: 'session-1', + title: 'Launch agent', + options: [{ optionId: 'proceed_once', label: 'Allow', raw: {} }], + toolCall: { + toolCallId: 'agent-call', + kind: 'other', + status: 'pending', + title: 'Launch agent', + rawInput: { run_in_background: true }, + }, + preview: { kind: 'generic' as const }, + }), + baseBlock({ + id: 'agent-block-agent-call', + kind: 'tool', + toolCallId: 'agent-call', + title: 'Agent', + status: 'in_progress', + toolName: 'agent', + rawInput: { run_in_background: true }, + rawOutput: { type: 'task_execution', status: 'background' }, + }), + ]; + hookState.resolveSubagentSession.mockReset(); + hookState.resolveSubagentSession.mockRejectedValue( + new DaemonHttpError( + 404, + { code: 'session_not_found', toolCallId: 'agent-call' }, + 'not found', + ), + ); + const { container, render, unmount } = mountStatusConsumer(); + + await act(async () => render()); + // The pending-permission agent is excluded from reconciliation, so the + // probe never fires and the card cannot be marked failed. + expect(hookState.resolveSubagentSession).not.toHaveBeenCalled(); + expect(container.textContent).toBe('pending'); + + // Even after several retry windows of wall-clock time it stays active. + await act(async () => vi.advanceTimersByTimeAsync(120_000)); + expect(hookState.resolveSubagentSession).not.toHaveBeenCalled(); + expect(container.textContent).toBe('pending'); + + // Once the launch approval resolves, the reconciliation must resume + // probing: the subagent session may now register, and a missing session + // crosses the grace into a visible failure exactly like any other + // background agent. + hookState.blocks = [ + { + ...hookState.blocks[0], + resolved: 'selected:proceed_once', + }, + hookState.blocks[1], + ]; + await act(async () => render()); + await vi.waitFor(() => + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(1), + ); + // First 404 miss keeps the card pending. + expect(container.textContent).toBe('pending'); + // The retry's second miss crosses the missing-agent grace → failed. + await act(async () => vi.advanceTimersByTimeAsync(60_000)); + await vi.waitFor(() => { + expect(container.textContent).toBe('failed'); + }); + + await act(async () => unmount()); + vi.useRealTimers(); + }); + + it('probes only the healthy agent while a sibling launch approval is pending', async () => { + vi.useFakeTimers(); + hookState.blocks = [ + baseBlock({ + id: 'perm-agent-a', + kind: 'permission', + requestId: 'req-a', + sessionId: 'session-1', + title: 'Launch agent A', + options: [{ optionId: 'proceed_once', label: 'Allow', raw: {} }], + toolCall: { + toolCallId: 'agent-call-a', + kind: 'other', + status: 'pending', + title: 'Launch agent A', + rawInput: { run_in_background: true }, + }, + preview: { kind: 'generic' as const }, + }), + baseBlock({ + id: 'agent-a', + kind: 'tool', + toolCallId: 'agent-call-a', + title: 'Agent', + status: 'in_progress', + toolName: 'agent', + rawInput: { run_in_background: true }, + rawOutput: { type: 'task_execution', status: 'background' }, + }), + baseBlock({ + id: 'agent-b', + kind: 'tool', + toolCallId: 'agent-call-b', + title: 'Agent', + status: 'in_progress', + toolName: 'agent', + rawInput: { run_in_background: true }, + rawOutput: { type: 'task_execution', status: 'background' }, + }), + ]; + hookState.resolveSubagentSession.mockReset(); + hookState.resolveSubagentSession.mockResolvedValue({ + status: 'running', + sessionId: 'sub-agent-b', + }); + const { render, unmount } = mountStatusConsumer({ allTools: true }); + + await act(async () => render()); + await vi.waitFor(() => + expect(hookState.resolveSubagentSession).toHaveBeenCalled(), + ); + // Exclusion is per callId: the healthy sibling keeps probing while the + // approved agent is skipped. + const probed = hookState.resolveSubagentSession.mock.calls.map( + (call) => call[1], + ); + expect(probed).toContain('agent-call-b'); + expect(probed).not.toContain('agent-call-a'); + + await act(async () => unmount()); + vi.useRealTimers(); + }); + + it('resets accumulated missing-agent misses when an approval engages', async () => { + vi.useFakeTimers(); + // Phase 1: no permission yet — one probe fires and 404s (miss 1). + hookState.blocks = [ + baseBlock({ + id: 'agent-a', + kind: 'tool', + toolCallId: 'agent-call-a', + title: 'Agent', + status: 'in_progress', + toolName: 'agent', + rawInput: { run_in_background: true }, + rawOutput: { type: 'task_execution', status: 'background' }, + }), + ]; + hookState.resolveSubagentSession.mockReset(); + hookState.resolveSubagentSession.mockRejectedValue( + new DaemonHttpError( + 404, + { code: 'session_not_found', toolCallId: 'agent-call-a' }, + 'not found', + ), + ); + const { container, render, unmount } = mountStatusConsumer(); + + await act(async () => render()); + await vi.waitFor(() => + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(1), + ); + expect(container.textContent).toBe('pending'); + + // Phase 2: the launch approval arrives — exclusion engages and must reset + // the accumulated miss, so no further probe fires while it is open. + hookState.blocks = [ + baseBlock({ + id: 'perm-agent-a', + kind: 'permission', + requestId: 'req-a', + sessionId: 'session-1', + title: 'Launch agent A', + options: [{ optionId: 'proceed_once', label: 'Allow', raw: {} }], + toolCall: { + toolCallId: 'agent-call-a', + kind: 'other', + status: 'pending', + title: 'Launch agent A', + rawInput: { run_in_background: true }, + }, + preview: { kind: 'generic' as const }, + }), + hookState.blocks[0], + ]; + await act(async () => render()); + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(1); + + // Phase 3: the approval resolves — probing resumes with a fresh grace; + // the pre-exclusion miss must not carry over, so the second post-approval + // 404 still leaves the card pending, and a third marks it failed. + hookState.blocks = [ + { + ...hookState.blocks[0], + resolved: 'selected:proceed_once', + }, + hookState.blocks[1], + ]; + await act(async () => render()); + await vi.waitFor(() => + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(2), + ); + expect(container.textContent).toBe('pending'); + await act(async () => vi.advanceTimersByTimeAsync(60_000)); + await vi.waitFor(() => { + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(3); + expect(container.textContent).toBe('failed'); + }); + + await act(async () => unmount()); + vi.useRealTimers(); + }); + + it('ignores a permanent failure that settles after approval engages', async () => { + const probe = deferred(); + hookState.blocks = [backgroundAgentBlock('agent-call')]; + hookState.resolveSubagentSession.mockReset(); + hookState.resolveSubagentSession.mockReturnValueOnce(probe.promise); + const { container, render, unmount } = mountStatusConsumer(); + + await act(async () => render()); + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(1); + + hookState.blocks = [ + baseBlock({ + id: 'perm-agent', + kind: 'permission', + requestId: 'req-agent', + sessionId: 'session-1', + title: 'Launch agent', + options: [{ optionId: 'proceed_once', label: 'Allow', raw: {} }], + toolCall: { + toolCallId: 'agent-call', + kind: 'other', + status: 'pending', + title: 'Launch agent', + rawInput: { run_in_background: true }, + }, + preview: { kind: 'generic' as const }, + }), + backgroundAgentBlock('agent-call'), + ]; + await act(async () => render()); + await act(async () => { + probe.reject( + new DaemonHttpError( + 400, + { code: 'invalid_subagent_ref' }, + 'bad request', + ), + ); + }); + + expect(container.textContent).toBe('pending'); + await act(async () => unmount()); + }); + + it('does not count a late 404 after approval engages', async () => { + vi.useFakeTimers(); + const probe = deferred(); + const missing = new DaemonHttpError( + 404, + { code: 'session_not_found', toolCallId: 'agent-call' }, + 'not found', + ); + hookState.blocks = [backgroundAgentBlock('agent-call')]; + hookState.resolveSubagentSession.mockReset(); + hookState.resolveSubagentSession + .mockReturnValueOnce(probe.promise) + .mockRejectedValue(missing); + const { container, render, unmount } = mountStatusConsumer(); + + await act(async () => render()); + hookState.blocks = [ + baseBlock({ + id: 'perm-agent', + kind: 'permission', + requestId: 'req-agent', + sessionId: 'session-1', + title: 'Launch agent', + options: [{ optionId: 'proceed_once', label: 'Allow', raw: {} }], + toolCall: { + toolCallId: 'agent-call', + kind: 'other', + status: 'pending', + title: 'Launch agent', + rawInput: { run_in_background: true }, + }, + preview: { kind: 'generic' as const }, + }), + backgroundAgentBlock('agent-call'), + ]; + await act(async () => render()); + await act(async () => probe.reject(missing)); + expect(container.textContent).toBe('pending'); + + hookState.blocks = [ + { ...hookState.blocks[0], resolved: 'selected:proceed_once' }, + hookState.blocks[1], + ]; + await act(async () => render()); + await vi.waitFor(() => + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(2), + ); + expect(container.textContent).toBe('pending'); + + await act(async () => vi.advanceTimersByTimeAsync(3_000)); + await vi.waitFor(() => expect(container.textContent).toBe('failed')); + + await act(async () => unmount()); + vi.useRealTimers(); + }); + + it('paces transient-error attempts across permission churn', async () => { + vi.useFakeTimers(); + const agent = backgroundAgentBlock('agent-call'); + const permission = baseBlock({ + id: 'perm-file', + kind: 'permission', + requestId: 'req-file', + sessionId: 'session-1', + title: 'Write file', + options: [{ optionId: 'allow', label: 'Allow', raw: {} }], + toolCall: { + toolCallId: 'file-call', + kind: 'other', + status: 'pending', + title: 'Write file', + rawInput: {}, + }, + preview: { kind: 'generic' as const }, + }); + hookState.blocks = [agent]; + hookState.resolveSubagentSession.mockReset(); + hookState.resolveSubagentSession.mockRejectedValue( + new DaemonHttpError(500, { code: 'internal_error' }, 'server error'), + ); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const { container, render, unmount } = mountStatusConsumer(); + + await act(async () => render()); + for (let index = 0; index < 8; index += 1) { + hookState.blocks = index % 2 === 0 ? [permission, agent] : [agent]; + await act(async () => render()); + await vi.waitFor(() => + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes( + index + 2, + ), + ); + } + + expect(container.textContent).toBe('pending'); + expect(warnSpy).not.toHaveBeenCalledWith( + '[web-shell] background agent reconciliation retry budget exhausted; marking agents failed', + expect.anything(), + ); + + await act(async () => unmount()); + warnSpy.mockRestore(); + vi.useRealTimers(); + }); + + it('keeps the missing-agent grace when an unrelated permission re-probes', async () => { + vi.useFakeTimers(); + // Phase 1: a background agent probes and 404s once (miss 1). + hookState.blocks = [backgroundAgentBlock('agent-call')]; + hookState.resolveSubagentSession.mockReset(); + hookState.resolveSubagentSession.mockRejectedValue( + new DaemonHttpError( + 404, + { code: 'session_not_found', toolCallId: 'agent-call' }, + 'not found', + ), + ); + const { container, render, unmount } = mountStatusConsumer(); + + await act(async () => render()); + await vi.waitFor(() => + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(1), + ); + expect(container.textContent).toBe('pending'); + + // Phase 2: an unrelated permission appears inside the retry window. The + // effect re-runs and probes again immediately; that second 404 must not + // count toward the grace because the base backoff has not elapsed — the + // ladder is wall-clock paced, not round-paced. + hookState.blocks = [ + baseBlock({ + id: 'perm-file', + kind: 'permission', + requestId: 'req-file', + sessionId: 'session-1', + title: 'Write file', + options: [{ optionId: 'allow', label: 'Allow', raw: {} }], + toolCall: { + toolCallId: 'file-call', + kind: 'other', + status: 'pending', + title: 'Write file', + rawInput: {}, + }, + preview: { kind: 'generic' as const }, + }), + backgroundAgentBlock('agent-call'), + ]; + await act(async () => render()); + await vi.waitFor(() => + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(2), + ); + // Two immediate 404s inside the backoff window must leave the miss count + // at 1, so the card stays pending. + expect(container.textContent).toBe('pending'); + + // Phase 3: the next probe after the base backoff crosses the grace. + await act(async () => vi.advanceTimersByTimeAsync(60_000)); + await vi.waitFor(() => { + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(3); + expect(container.textContent).toBe('failed'); + }); + + await act(async () => unmount()); + vi.useRealTimers(); + }); + it('gives a session-level 404 the same grace as a missing agent', async () => { vi.useFakeTimers(); hookState.blocks = [backgroundAgentBlock('agent-call')]; @@ -1161,13 +1597,15 @@ describe('background agent task reconciliation', () => { ); expect(container.textContent).toBe('pending'); - // The double-count must not shorten the documented budget: failure - // still takes eight erroring rounds in total. - for (const delay of [6_000, 12_000, 24_000, 48_000, 60_000, 60_000]) { + // The immediate identity-triggered round is inside the pacing window, + // so only wall-clock-paced errors consume the documented budget. + for (const delay of [ + 6_000, 12_000, 24_000, 48_000, 60_000, 60_000, 60_000, + ]) { await act(async () => vi.advanceTimersByTimeAsync(delay)); } await vi.waitFor(() => { - expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(8); + expect(hookState.resolveSubagentSession).toHaveBeenCalledTimes(9); expect(container.textContent).toBe('failed'); }); } finally { diff --git a/packages/web-shell/client/hooks/useMessages.ts b/packages/web-shell/client/hooks/useMessages.ts index ce850357670..2c3ac76fd76 100644 --- a/packages/web-shell/client/hooks/useMessages.ts +++ b/packages/web-shell/client/hooks/useMessages.ts @@ -125,6 +125,36 @@ export function getPendingBackgroundAgentKey( return callIds.join('|'); } +type DaemonPermissionTranscriptBlock = Extract< + DaemonTranscriptBlock, + { kind: 'permission' } +>; + +/** + * CallIds whose permission request is still unanswered. Such an agent has not + * spawned yet, so its subagent session legitimately does not exist and the + * reconciliation 404 probe must not count toward the missing-agent grace. + */ +function getPendingPermissionCallIds( + blocks: readonly DaemonTranscriptBlock[], +): Set { + const ids = new Set(); + for (const block of blocks) { + if (block.kind !== 'permission') continue; + const perm = block as DaemonPermissionTranscriptBlock; + if (perm.resolved) continue; + const toolCall = getRecord(perm.toolCall); + const callId = + typeof toolCall?.['toolCallId'] === 'string' + ? toolCall['toolCallId'] + : typeof toolCall?.['id'] === 'string' + ? toolCall['id'] + : undefined; + if (callId) ids.add(callId); + } + return ids; +} + export function reconcileBackgroundAgentResolutions( messages: Message[], resolutions: ReadonlyMap, @@ -207,6 +237,13 @@ export function useMessagesFromBlocks( () => getPendingBackgroundAgentKey(reconciledMessages), [reconciledMessages], ); + // A stable primitive key for the effect dependency: the Set's identity + // changes on every transcript delta, which would re-run the reconciliation + // effect on each streamed update and defeat the retry backoff. + const pendingPermissionKey = useMemo( + () => [...getPendingPermissionCallIds(blocks)].sort().join('|'), + [blocks], + ); const backgroundAgentNotificationKey = useMemo( () => getBackgroundAgentNotificationKey(blocks), [blocks], @@ -234,6 +271,13 @@ export function useMessagesFromBlocks( errorAttempts: new Map(), }); const missingAgentMissesRef = useRef(new Map()); + // Last 404 timestamp per callId. The grace ladder is wall-clock paced: a + // miss only counts toward the grace once the base backoff has elapsed since + // the previous miss, so a re-probe triggered by an unrelated transcript + // change (for example another permission appearing) cannot collapse the + // retry ladder into two immediate misses. + const missTimestampsRef = useRef(new Map()); + const errorTimestampsRef = useRef(new Map()); const lastConnectionKeyRef = useRef(undefined); useEffect(() => { @@ -245,6 +289,8 @@ export function useMessagesFromBlocks( if (lastConnectionKeyRef.current !== connectionKey) { lastConnectionKeyRef.current = connectionKey; missingAgentMissesRef.current.clear(); + missTimestampsRef.current.clear(); + errorTimestampsRef.current.clear(); retryBackoffRef.current = { key: '', attempts: 0, @@ -272,12 +318,27 @@ export function useMessagesFromBlocks( const requestKey = `${sessionId}:${pendingBackgroundAgentKey}:${backgroundAgentNotificationKey}`; const retryScopeKey = `${sessionId}:${pendingBackgroundAgentKey}`; const cachedRound = reconciliationRequestRef.current; - const callIds = pendingBackgroundAgentKey.split('|'); + // Agents still under approval have not spawned their subagent session + // yet: exclude them so the 404 probe cannot accumulate missing-agent + // misses and paint a failure while the dialog is unanswered. Rebuild the + // membership from the stable key — the effect depends on the key, not the + // Set, so a transcript delta with unchanged permission content does not + // re-run this effect. + const pendingPermissionCallIds = new Set( + pendingPermissionKey ? pendingPermissionKey.split('|') : [], + ); + const callIds = pendingBackgroundAgentKey + .split('|') + .filter((callId) => !pendingPermissionCallIds.has(callId)); for (const callId of [...missingAgentMissesRef.current.keys()]) { if (!callIds.includes(callId)) { missingAgentMissesRef.current.delete(callId); + missTimestampsRef.current.delete(callId); } } + for (const callId of [...errorTimestampsRef.current.keys()]) { + if (!callIds.includes(callId)) errorTimestampsRef.current.delete(callId); + } const roundErrors: Array<{ callId: string; error: unknown }> = []; const roundNotFounds: string[] = []; // A settled round that was already processed must not be reused: a @@ -351,11 +412,26 @@ export function useMessagesFromBlocks( round.processed = true; // Grace-miss accounting lives in the active handler, not the per-call // closure: a superseded round's late 404 must not consume grace that - // belongs to the live round. + // belongs to the live round. The handler also re-checks the current + // probe set: a round that straddles a permission transition settles + // with misses for an agent that is now excluded, and those must not + // be counted (or re-added after the exclusion cleanup ran). for (const callId of succeeded) { missingAgentMissesRef.current.delete(callId); + missTimestampsRef.current.delete(callId); + errorTimestampsRef.current.delete(callId); + } + for (const callId of [...resolutions.keys()]) { + if (!callIds.includes(callId)) resolutions.delete(callId); } for (const callId of notFounds) { + if (!callIds.includes(callId)) continue; + const now = Date.now(); + const lastMiss = missTimestampsRef.current.get(callId) ?? 0; + if (now - lastMiss < BACKGROUND_AGENT_RECONCILIATION_RETRY_BASE_MS) { + continue; + } + missTimestampsRef.current.set(callId, now); const misses = (missingAgentMissesRef.current.get(callId) ?? 0) + 1; missingAgentMissesRef.current.set(callId, misses); if (misses >= MISSING_BACKGROUND_AGENT_GRACE_MISSES) { @@ -382,7 +458,18 @@ export function useMessagesFromBlocks( // never consume the budget. const errorAttempts = new Map(); for (const entry of errors) { - const count = (previous.errorAttempts.get(entry.callId) ?? 0) + 1; + if (!callIds.includes(entry.callId)) continue; + const now = Date.now(); + const lastError = errorTimestampsRef.current.get(entry.callId); + const previousCount = previous.errorAttempts.get(entry.callId) ?? 0; + const count = + lastError !== undefined && + now - lastError < BACKGROUND_AGENT_RECONCILIATION_RETRY_BASE_MS + ? previousCount + : previousCount + 1; + if (count !== previousCount) { + errorTimestampsRef.current.set(entry.callId, now); + } errorAttempts.set(entry.callId, count); if (count >= BACKGROUND_AGENT_RECONCILIATION_MAX_ATTEMPTS) { failedCallIds.push(entry.callId); @@ -459,6 +546,7 @@ export function useMessagesFromBlocks( connection.sessionId, connection.status, pendingBackgroundAgentKey, + pendingPermissionKey, reconciliationAttempt, workspace.client, ]);