You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fixes a bug where Stop hook's child process doesn't exit when user exits CLI (main process closes), causing HookRunner subprocesses to continue running indefinitely.
Key Changes:
Added AbortSignal support to HookRunner to terminate child processes on abort
Propagated AbortSignal through the entire hook call chain (HookSystem → HookEventHandler → HookRunner)
MessageBus.request() now supports abort cancellation
Added optional signal parameter to all 12 hook event methods
Screenshots / Video Demo
Dive Deeper
Root Cause:
When user exits CLI via Ctrl+C or other means, the main process exits but Stop hook's child processes (spawned via child_process.spawn) don't automatically terminate since they are independent processes.
Solution:
Use AbortSignal pattern to propagate cancellation signal
Listen for abort event in HookRunner.executeCommandHook()
Call child.kill() to terminate child process when abort is triggered
Modified Call Chain:
client.ts
↓ signal
config.ts (MessageBus handler)
↓ signal
message-bus.ts (request method)
↓ signal
hookSystem.ts (fire*Event methods)
↓ signal
hookEventHandler.ts (fire*Event methods)
↓ signal
hookRunner.ts (executeHooksParallel/Sequential)
↓ signal
executeCommandHook() → child.kill() on abort
This PR addresses a critical bug where Stop hook child processes don't terminate when the user exits the CLI, causing orphaned HookRunner subprocesses. The implementation propagates AbortSignal through the entire hook call chain (Client → MessageBus → HookSystem → HookEventHandler → HookRunner) to enable proper cancellation of child processes. The changes are well-structured, comprehensive, and include appropriate test updates.
🔍 General Feedback
Strong architectural approach: The AbortSignal pattern is correctly propagated through all layers of the hook execution chain, maintaining consistency with Node.js best practices for cancellation.
Comprehensive coverage: All 12 hook event methods now support the optional signal parameter, ensuring uniform cancellation support across the system.
Good test coverage: Test files have been updated to reflect the new signal parameter, though new tests specifically for abort behavior would strengthen the PR.
Clean diff organization: Changes follow a logical flow from the top-level (client.ts) down to the implementation (hookRunner.ts).
Positive: The graceful cleanup pattern with SIGTERM followed by SIGKILL after a timeout is well-implemented.
🎯 Specific Feedback
🔴 Critical
File: packages/core/src/hooks/hookRunner.ts:300-307 - The abort handler cleanup removes the listener but there's a potential race condition: if abort fires between the signal.aborted check at line 297 and adding the listener at line 303, the child process may not be killed. Consider checking signal.aborted again after adding the listener or use signal.addEventListener('abort', abortHandler, { once: true }) with immediate abort check.
File: packages/core/src/config/config.ts:813-819 - The abort check happens after the message bus handler is invoked. If the request was already aborted before this check, the hook system methods will still be called (they'll check signal internally, but this is wasted work). Consider moving the abort check earlier or ensuring the signal is passed to all downstream calls consistently (which appears to be done, but the early return optimization could be clearer).
🟡 High
File: packages/core/src/hooks/hookRunner.ts:283-293 - The killChild helper uses a 2-second timeout before SIGKILL, but the original timeout logic (line 244-251) uses 5 seconds. This inconsistency could lead to unpredictable behavior. Consider extracting the kill logic to a shared utility function or using consistent timeout values.
File: packages/core/src/core/client.ts:715-719 - After the Stop hook execution, there's an abort check, but the code continues to check stopOutput?.isBlockingDecision() at line 724 without verifying the signal wasn't aborted in between. While the abort check at line 717 helps, consider adding another check before processing the hook output to be extra safe.
File: packages/core/src/confirmation-bus/message-bus.ts:109-120 - The abortHandler calls cleanup() which removes the listener, but if both timeout and abort fire simultaneously, there could be a double-rejection scenario. The cleanup() function should ideally track whether the promise has already been settled to prevent this edge case.
🟢 Medium
File: packages/core/src/hooks/hookRunner.ts:268 - The aborted flag is used to track abort state, but this could be simplified by checking signal.aborted directly in the close handler. The flag adds state complexity without significant benefit.
File: packages/core/src/config/config.ts:826-921 - The switch statement passes signal to all hook methods, but several cases don't include it in the original code (now added). Consider using a helper function or object mapping to reduce repetition and ensure all cases consistently include the signal parameter.
File: packages/cli/src/ui/AppContainer.tsx:312-317, clearCommand.ts:73-76, useResumeCommand.ts:79-82 - The fireSessionStartEvent calls now include String(config.getApprovalMode()) as PermissionMode. This type assertion pattern appears in multiple places. Consider creating a helper method on Config to return the properly typed permission mode to reduce repetition and type assertions.
File: packages/core/src/services/chatCompressionService.ts:281-291 - The fireSessionStartEvent call passes undefined for the agentType parameter (5th position) and signal as the 6th parameter. This is inconsistent with other calls that pass signal as the last parameter when agentType is undefined. Consider reordering parameters or using an options object for better clarity.
🔵 Low
File: packages/core/src/hooks/hookRunner.ts:252 - Comment says "Helper to kill child process" but the function is defined inline. Consider extracting this to a private method with a more descriptive name like terminateChildProcess for better testability and reusability.
File: packages/core/src/confirmation-bus/types.ts:112-113 - The JSDoc comment for signal says "Optional AbortSignal to cancel hook execution" which is good, but consider adding an example or linking to MDN documentation for developers unfamiliar with the AbortSignal pattern.
File: packages/core/src/hooks/hookRunner.ts:53-57 - The executeHook method signature now includes signal?: AbortSignal, but the JSDoc comment above the method (lines 46-52) doesn't document this new parameter. Update the JSDoc to include @param signal Optional AbortSignal to cancel hook execution.
Test files - While existing tests were updated to pass undefined for the new signal parameter, consider adding specific test cases for:
Hook execution cancellation mid-execution
MessageBus request abort before timeout
Multiple hooks with abort (parallel vs sequential behavior)
Abort during Stop hook specifically (the main bug scenario)
✅ Highlights
Excellent bug fix approach: Using the standard AbortSignal pattern instead of custom cancellation logic makes the code more maintainable and familiar to Node.js developers.
Comprehensive propagation: The signal is correctly threaded through all layers: GeminiClient → MessageBus.request() → HookSystem → HookEventHandler → HookRunner → executeCommandHook().
Graceful process termination: The two-stage kill approach (SIGTERM → wait → SIGKILL) is well-implemented and prevents zombie processes.
Good defensive programming: Multiple abort checks throughout the call chain (in MessageBus, HookRunner loop, client.ts) ensure cancellation is respected at all stages.
Test updates: All affected test files were properly updated to include the new signal parameter, maintaining test validity.
Type safety: Proper TypeScript typing with optional signal?: AbortSignal throughout the call chain without breaking existing functionality.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Fixes a bug where Stop hook's child process doesn't exit when user exits CLI (main process closes), causing HookRunner subprocesses to continue running indefinitely.
Key Changes:
MessageBus.request()now supports abort cancellationScreenshots / Video Demo
Dive Deeper
Root Cause:
When user exits CLI via Ctrl+C or other means, the main process exits but Stop hook's child processes (spawned via child_process.spawn) don't automatically terminate since they are independent processes.
Solution:
HookRunner.executeCommandHook()child.kill()to terminate child process when abort is triggeredModified Call Chain:
Reviewer Test Plan
Testing Matrix
Linked issues / bugs