From 9b728260785bf9c039d4af5ca38f495730b60052 Mon Sep 17 00:00:00 2001 From: Adam Weidman Date: Mon, 9 Mar 2026 00:14:48 -0400 Subject: [PATCH] Refine execution lifecycle facade follow-ups --- .../cli/src/ui/hooks/shellCommandProcessor.ts | 10 ++- .../cli/src/ui/hooks/useGeminiStream.test.tsx | 30 ++++++++ packages/cli/src/ui/hooks/useGeminiStream.ts | 73 ++++++++++--------- .../executionLifecycleService.test.ts | 15 ++++ .../src/services/executionLifecycleService.ts | 13 +--- 5 files changed, 93 insertions(+), 48 deletions(-) diff --git a/packages/cli/src/ui/hooks/shellCommandProcessor.ts b/packages/cli/src/ui/hooks/shellCommandProcessor.ts index 364b395876..3e4a6d54b1 100644 --- a/packages/cli/src/ui/hooks/shellCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/shellCommandProcessor.ts @@ -80,7 +80,7 @@ export const useShellCommandProcessor = ( setShellInputFocused: (value: boolean) => void, terminalWidth?: number, terminalHeight?: number, - activeToolPtyId?: number, + activeBackgroundExecutionId?: number, isWaitingForConfirmation?: boolean, ) => { const [state, dispatch] = useReducer(shellReducer, initialState); @@ -103,7 +103,8 @@ export const useShellCommandProcessor = ( } const m = manager.current; - const activePtyId = state.activeShellPtyId || activeToolPtyId; + const activePtyId = + state.activeShellPtyId ?? activeBackgroundExecutionId ?? undefined; useEffect(() => { const isForegroundActive = !!activePtyId || !!isWaitingForConfirmation; @@ -191,7 +192,8 @@ export const useShellCommandProcessor = ( ]); const backgroundCurrentShell = useCallback(() => { - const pidToBackground = state.activeShellPtyId || activeToolPtyId; + const pidToBackground = + state.activeShellPtyId ?? activeBackgroundExecutionId; if (pidToBackground) { ShellExecutionService.background(pidToBackground); m.backgroundedPids.add(pidToBackground); @@ -202,7 +204,7 @@ export const useShellCommandProcessor = ( m.restoreTimeout = null; } } - }, [state.activeShellPtyId, activeToolPtyId, m]); + }, [state.activeShellPtyId, activeBackgroundExecutionId, m]); const dismissBackgroundShell = useCallback( (pid: number) => { diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index 389b757e99..53afe5022b 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -96,6 +96,31 @@ const MockedUserPromptEvent = vi.hoisted(() => vi.fn().mockImplementation(() => {}), ); const mockParseAndFormatApiError = vi.hoisted(() => vi.fn()); +const mockIsBackgroundExecutionData = vi.hoisted( + () => (data: unknown): data is { executionId?: number; pid?: number } => { + if (typeof data !== 'object' || data === null) { + return false; + } + const value = data as { + executionId?: unknown; + pid?: unknown; + command?: unknown; + initialOutput?: unknown; + }; + return ( + (value.executionId === undefined || typeof value.executionId === 'number') && + (value.pid === undefined || typeof value.pid === 'number') && + (value.command === undefined || typeof value.command === 'string') && + (value.initialOutput === undefined || + typeof value.initialOutput === 'string') + ); + }, +); +const mockGetBackgroundExecutionId = vi.hoisted( + () => + (data: { executionId?: number; pid?: number }): number | undefined => + data.executionId ?? data.pid, +); const MockValidationRequiredError = vi.hoisted( () => @@ -121,6 +146,11 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { const actualCoreModule = (await importOriginal()) as any; return { ...actualCoreModule, + isBackgroundExecutionData: + actualCoreModule.isBackgroundExecutionData ?? + mockIsBackgroundExecutionData, + getBackgroundExecutionId: + actualCoreModule.getBackgroundExecutionId ?? mockGetBackgroundExecutionId, GitService: vi.fn(), GeminiClient: MockedGeminiClientClass, UserPromptEvent: MockedUserPromptEvent, diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index 4dd27f1f0b..8e768f1aea 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -37,6 +37,8 @@ import { buildUserSteeringHintPrompt, GeminiCliOperation, getPlanModeExitMessage, + getBackgroundExecutionId, + isBackgroundExecutionData, } from '@google/gemini-cli-core'; import type { Config, @@ -100,12 +102,12 @@ interface BackgroundedToolInfo { initialOutput: string; } -interface BackgroundExecutionData { +type BackgroundExecutionDataLike = { executionId?: number; pid?: number; command?: string; initialOutput?: string; -} +} & Record; enum StreamProcessingStatus { Completed, @@ -118,55 +120,58 @@ const SUPPRESSED_TOOL_ERRORS_NOTE = const LOW_VERBOSITY_FAILURE_NOTE = 'This request failed. Press F12 for diagnostics, or run /settings and change "Error Verbosity" to full for full details.'; -function isBackgroundExecutionData( - data: unknown, -): data is BackgroundExecutionData { - if (typeof data !== 'object' || data === null) { - return false; - } - - const executionId = 'executionId' in data ? data.executionId : undefined; - const pid = 'pid' in data ? data.pid : undefined; - const command = 'command' in data ? data.command : undefined; - const initialOutput = - 'initialOutput' in data ? data.initialOutput : undefined; - - return ( - (executionId === undefined || typeof executionId === 'number') && - (pid === undefined || typeof pid === 'number') && - (command === undefined || typeof command === 'string') && - (initialOutput === undefined || typeof initialOutput === 'string') - ); +function isBackgroundExecutionDataValidator( + candidate: unknown, +): candidate is (data: unknown) => data is BackgroundExecutionDataLike { + return typeof candidate === 'function'; } -function getBackgroundExecutionId( - data: BackgroundExecutionData, +function isBackgroundExecutionIdGetter( + candidate: unknown, +): candidate is (data: BackgroundExecutionDataLike) => number | undefined { + return typeof candidate === 'function'; +} + +function isBackgroundExecutionDataFromCore( + data: unknown, +): data is BackgroundExecutionDataLike { + const candidate: unknown = isBackgroundExecutionData; + if (isBackgroundExecutionDataValidator(candidate)) { + return candidate(data); + } + + return false; +} + +function getBackgroundExecutionIdFromCore( + data: BackgroundExecutionDataLike, ): number | undefined { - if (typeof data.executionId === 'number') { - return data.executionId; + const candidate: unknown = getBackgroundExecutionId; + if (isBackgroundExecutionIdGetter(candidate)) { + return candidate(data); } - if (typeof data.pid === 'number') { - return data.pid; - } - return undefined; + + return data.executionId ?? data.pid; } function getBackgroundedToolInfo( toolCall: TrackedCompletedToolCall | TrackedCancelledToolCall, ): BackgroundedToolInfo | undefined { const response = toolCall.response as ToolResponseWithParts; - const rawData = response?.data; - const data = isBackgroundExecutionData(rawData) ? rawData : undefined; - const executionId = data ? getBackgroundExecutionId(data) : undefined; + const rawData: unknown = response?.data; + if (!isBackgroundExecutionDataFromCore(rawData)) { + return undefined; + } + const executionId = getBackgroundExecutionIdFromCore(rawData); if (executionId === undefined) { return undefined; } return { executionId, - command: data.command ?? toolCall.request.name, - initialOutput: data.initialOutput ?? '', + command: rawData.command ?? toolCall.request.name, + initialOutput: rawData.initialOutput ?? '', }; } diff --git a/packages/core/src/services/executionLifecycleService.test.ts b/packages/core/src/services/executionLifecycleService.test.ts index facbdc9141..60a29a7f5c 100644 --- a/packages/core/src/services/executionLifecycleService.test.ts +++ b/packages/core/src/services/executionLifecycleService.test.ts @@ -133,6 +133,21 @@ describe('ExecutionLifecycleService', () => { expect(result.error?.message).toContain('Operation cancelled by user'); }); + it('does not probe OS process state for completed non-process execution IDs', async () => { + const handle = ExecutionLifecycleService.createExecution(); + if (handle.pid === undefined) { + throw new Error('Expected execution ID.'); + } + + ExecutionLifecycleService.completeExecution(handle.pid, { exitCode: 0 }); + await handle.result; + + const processKillSpy = vi.spyOn(process, 'kill'); + expect(ExecutionLifecycleService.isActive(handle.pid)).toBe(false); + expect(processKillSpy).not.toHaveBeenCalled(); + processKillSpy.mockRestore(); + }); + it('manages external executions through registration hooks', async () => { const writeInput = vi.fn(); const isActive = vi.fn().mockReturnValue(true); diff --git a/packages/core/src/services/executionLifecycleService.ts b/packages/core/src/services/executionLifecycleService.ts index ae71afd1aa..7d1384e3c5 100644 --- a/packages/core/src/services/executionLifecycleService.ts +++ b/packages/core/src/services/executionLifecycleService.ts @@ -312,16 +312,6 @@ export class ExecutionLifecycleService { this.settleExecution(executionId, result); } - /** - * @deprecated Use completeWithResult() for new call sites. - */ - static finalizeExecution( - executionId: number, - result: ExecutionResult, - ): void { - this.completeWithResult(executionId, result); - } - static background(executionId: number): void { const resolve = this.activeResolvers.get(executionId); if (!resolve) { @@ -423,6 +413,9 @@ export class ExecutionLifecycleService { static isActive(executionId: number): boolean { const execution = this.activeExecutions.get(executionId); if (!execution) { + if (executionId >= NON_PROCESS_EXECUTION_ID_START) { + return false; + } try { return process.kill(executionId, 0); } catch {