feat(core): add post tool batch hooks - #4454
Conversation
📋 Review SummaryThis PR adds a 🔍 General Feedback
🎯 Specific Feedback🔵 Low
✅ Highlights
|
| ); | ||
|
|
||
| if (batchHookResult.shouldStop) { | ||
| completedCalls = withPostToolBatchStop( |
There was a problem hiding this comment.
[Critical] withPostToolBatchStop silently discards additionalContext.
When the hook returns both additionalContext and shouldStop: true, the code first appends context to the last call's response (line 3326), then withPostToolBatchStop replaces that same response with a fresh createErrorResponse(), destroying the appended context. Hook authors who return both fields will find their additionalContext silently lost.
| completedCalls = withPostToolBatchStop( | |
| if (batchHookResult.shouldStop) { | |
| completedCalls = withPostToolBatchStop( | |
| completedCalls, | |
| batchHookResult.stopReason || | |
| 'Execution stopped by PostToolBatch hook', | |
| ); | |
| // Re-apply additionalContext to the (now-error) last response | |
| completedCalls = withPostToolBatchAdditionalContext( | |
| completedCalls, | |
| batchHookResult.additionalContext, | |
| ); | |
| } |
— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| const response = part.functionResponse.response ?? {}; | ||
| const output = response['output']; | ||
| const error = response['error']; | ||
| const key = typeof output === 'string' ? 'output' : 'error'; |
There was a problem hiding this comment.
[Critical] appendContextToResponsePart writes to 'error' field when output is non-string.
When output is an object (e.g., structured JSON from MCP tools), typeof output !== 'string', so key becomes 'error'. Then currentText falls through to JSON.stringify(response) (stringifying the entire response), creating a spurious error field on a success response. The model may interpret this as a tool failure.
| const key = typeof output === 'string' ? 'output' : 'error'; | |
| const key = | |
| typeof output === 'string' ? 'output' | |
| : typeof error === 'string' ? 'error' | |
| : 'output'; |
— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| const batchHookResult = await this.withHookSpan( | ||
| { hookEvent: 'PostToolBatch', toolName: 'batch' }, | ||
| () => | ||
| firePostToolBatchHook( |
There was a problem hiding this comment.
[Critical] firePostToolBatchHook is called without an AbortSignal.
The function signature accepts signal?: AbortSignal (toolHookTriggers.ts:388) and the messageBus request forwards it, but no signal is passed here. A misbehaving or slow PostToolBatch hook (e.g., an HTTP hook with no timeout) can block the tool-completion path indefinitely with no way for the user to cancel.
| firePostToolBatchHook( | |
| () => | |
| firePostToolBatchHook( | |
| messageBus, | |
| completedCalls.map(toPostToolBatchToolCall), | |
| this.currentSignal, | |
| ), |
— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| expect(lastResponse?.functionResponse?.response?.['output']).toContain( | ||
| 'batch context', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
[Critical] Missing integration test for shouldStop: true path.
The existing test only covers the additionalContext scenario. The shouldStop path (lines 3332-3338 of coreToolScheduler.ts) — which converts the last completed call into an ErroredToolCall with EXECUTION_DENIED — has no test coverage. This is the feature's primary control-flow action. At minimum, add a test that:
- Has the mock messageBus return
{ continue: false, stopReason: 'halt' }for PostToolBatch - Asserts the last completed call has
status === 'error' - Asserts
response.errorType === ToolErrorType.EXECUTION_DENIED - Asserts
response.error.messagecontains'halt'
— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| const batchOutput = createHookOutput('PostToolBatch', response.output); | ||
|
|
||
| return { | ||
| shouldStop: batchOutput.shouldStopExecution(), |
There was a problem hiding this comment.
[Suggestion] shouldStopExecution() is called twice. Cache the result for consistency with the existing firePostToolUseHook pattern (line 276) which uses early-return:
| shouldStop: batchOutput.shouldStopExecution(), | |
| const shouldStop = batchOutput.shouldStopExecution(); | |
| return { | |
| shouldStop, | |
| stopReason: shouldStop ? batchOutput.getEffectiveReason() : undefined, | |
| additionalContext: batchOutput.getAdditionalContext(), | |
| }; |
— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| requiresRestart: false, | ||
| default: [], | ||
| description: | ||
| 'Hooks that execute after every tool call in a batch resolves.', |
There was a problem hiding this comment.
[Suggestion] Description is misleading — "after every tool call in a batch resolves" implies per-call semantics. The hook fires once after all tool calls resolve.
| 'Hooks that execute after every tool call in a batch resolves.', | |
| 'Hooks that execute once after all tool calls in a batch resolve.', |
Also update the same string in packages/vscode-ide-companion/schemas/settings.schema.json (line 1607).
— qwen-latest-series-invite-beta-v38 via Qwen Code /review
|
|
||
| // PostToolBatch uses DefaultHookOutput intentionally: it only needs the | ||
| // common stop and additional-context helpers. | ||
| const batchOutput = createHookOutput('PostToolBatch', response.output); |
There was a problem hiding this comment.
[Critical] Exit code 2 (standard "block" pattern) does not stop PostToolBatch execution.
firePostToolBatchHook calls createHookOutput('PostToolBatch', ...) which falls to the default case returning DefaultHookOutput. Its shouldStopExecution() only checks continue === false, not decision === 'deny'.
When a hook exits with code 2, convertPlainTextToHookOutput produces {decision: 'deny', reason: text} — which does NOT set continue. The batch continues silently.
Other hooks (PreToolUse, PostToolUse) check isDenied() which DOES inspect decision === 'deny', so the exit-code-2 pattern works correctly for them.
| const batchOutput = createHookOutput('PostToolBatch', response.output); | |
| case HookEventName.PostToolBatch: | |
| return new PostToolBatchHookOutput(data); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| try { | ||
| const shouldFirePostToolBatch = | ||
| !this.config.getDisableAllHooks() && | ||
| // Default to firing when the hook registry cannot answer. This keeps |
There was a problem hiding this comment.
[Suggestion] hasHooksForEvent?.('PostToolBatch') ?? true defaults to true — should be false.
Config.hasHooksForEvent always exists and internally uses ?? false. If a test mock or future refactoring removes the method, the ?. returns undefined and ?? true fires PostToolBatch on every batch — the opposite of the intended optimization. This asymmetry means all other hooks silently break while PostToolBatch silently fires.
| // Default to firing when the hook registry cannot answer. This keeps | |
| (this.config.hasHooksForEvent('PostToolBatch')) |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| batchSignal, | ||
| ), | ||
| (r) => | ||
| r.hookError |
There was a problem hiding this comment.
[Suggestion] completedCalls.map(toPostToolBatchToolCall) allocates serialized tool response objects even when no PostToolBatch hooks are configured.
Move the .map() call into the if (messageBus) block (after line ~3317) so it only runs when hooks are actually registered. This avoids unnecessary allocations for the majority of users who don't use PostToolBatch hooks.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const response = part.functionResponse.response ?? {}; | ||
| const output = response['output']; | ||
| const error = response['error']; | ||
| const hasOutput = Object.prototype.hasOwnProperty.call(response, 'output'); |
There was a problem hiding this comment.
[Suggestion] appendContextToResponsePart — key and currentText decision trees diverge when both output (non-string) and error (string) are present.
When response = { output: 42, error: "some error" }: key evaluates to 'error' (because hasOutput && typeof error !== 'string' is false), but currentText evaluates to JSON.stringify(output) → "42" (because hasOutput is true). The stringified output overwrites the original error message.
While current code paths produce either { output } or { error } (never both), this utility function should handle the mixed case correctly. Derive both from a single boolean:
| const hasOutput = Object.prototype.hasOwnProperty.call(response, 'output'); | |
| const useOutputKey = | |
| typeof output === 'string' || (hasOutput && typeof error !== 'string'); | |
| const key = useOutputKey ? 'output' : 'error'; | |
| const currentText = useOutputKey | |
| ? typeof output === 'string' | |
| ? output | |
| : JSON.stringify(output) | |
| : typeof error === 'string' | |
| ? error | |
| : JSON.stringify(response); |
— glm-5.1 via Qwen Code /review
| completedCalls, | ||
| batchHookResult.stopReason || | ||
| 'Execution stopped by PostToolBatch hook', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Order matters here but is undocumented — add a comment.
withPostToolBatchStop replaces the last call's entire response with createErrorResponse. withPostToolBatchAdditionalContext appends to whatever response exists. Stop MUST run first — if reordered, additionalContext is silently discarded (this was the exact bug fixed in this PR's prior commit). A future maintainer could easily reorder them during a refactor since "context then stop" sounds more natural.
| ); | |
| // Order matters: stop MUST precede additionalContext. | |
| // withPostToolBatchStop replaces the last call's response entirely, | |
| // so context must be appended AFTER the stop decision is applied. | |
| if (batchHookResult.shouldStop) { |
— glm-5.1 via Qwen Code /review
| .calls as unknown as Array<[ToolCall[]]>; | ||
| const completedCalls = completionCalls[0]?.[0]; | ||
| const lastCompletedCall = completedCalls?.at(-1); | ||
| expect(lastCompletedCall?.status).toBe('error'); |
There was a problem hiding this comment.
[Suggestion] Add an assertion that non-last calls are unaffected by the stop decision.
The test only asserts properties of completedCalls.at(-1). If a regression incorrectly marks all calls in the batch as errors (not just the last), this test would not catch it. withPostToolBatchStop is designed to modify only the last call — a defensive assertion would guard against scope creep:
| expect(lastCompletedCall?.status).toBe('error'); | |
| const completedCalls = completionCalls[0]?.[0]; | |
| const lastCompletedCall = completedCalls?.at(-1); | |
| expect(completedCalls[0]?.status).toBe('success'); | |
| expect(lastCompletedCall?.status).toBe('error'); |
— glm-5.1 via Qwen Code /review
| | 'PreToolUse' | ||
| | 'PostToolUse' | ||
| | 'PostToolUseFailure' | ||
| | 'PostToolBatch'; |
There was a problem hiding this comment.
[Suggestion] Missing telemetry span test for PostToolBatch.
The HookEvent union was extended with 'PostToolBatch' and the scheduler creates PostToolBatch hook spans, but no test was added in session-tracing.test.ts. Every other hook event variant (PreToolUse, PostToolUse, PostToolUseFailure) has a dedicated telemetry test. PostToolBatch uses a synthetic toolName: 'batch' and carries should_stop + has_additional_context attributes — without a test, regressions in span attribute names would go undetected.
— glm-5.1 via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] hookAggregator.ts missing PostToolBatch in mergeWithOrLogic and createSpecificHookOutput (file not in this PR's diff, so cannot be anchored as inline comment).
PostToolBatch falls through to mergeSimple in the mergeOutputs switch (hookAggregator.ts:102-118), which uses last-writer-wins { ...merged, ...output }. With two PostToolBatch hooks where hook 1 returns continue: false and hook 2 returns continue: true, the stop decision is silently overridden.
Similarly, createSpecificHookOutput (hookAggregator.ts:357-377) lacks a PostToolBatch case, so the merged result is wrapped in DefaultHookOutput instead of PostToolBatchHookOutput — the shouldStopExecution() override that checks isBlockingDecision() is not applied.
All other stop-capable events (PreToolUse, PostToolUse, Stop, SubagentStop, etc.) use mergeWithOrLogic.
// In mergeOutputs switch, add:
case HookEventName.PostToolBatch:
merged = this.mergeWithOrLogic(outputs, eventName);
break;
// In createSpecificHookOutput switch, add:
case HookEventName.PostToolBatch:
return new PostToolBatchHookOutput(output);
— qwen3.7-max via Qwen Code /review
| const completedCalls = [...this.toolCalls] as CompletedToolCall[]; | ||
| let completedCalls = [...this.toolCalls] as CompletedToolCall[]; | ||
| this.toolCalls = []; | ||
| const batchSignal = completedCalls |
There was a problem hiding this comment.
[Critical] batchSignal is always undefined — abort signal forwarding is dead code.
releaseBatchListenerIfDrained deletes entries from this.callIdToBatch in finalizeToolSpan (called for each tool call as it reaches terminal state), which runs before checkAndNotifyCompletion reaches this line. By the time all calls are terminal and this code executes, every callIdToBatch entry for the batch has already been removed. The .map(...).find(...) always returns undefined, so firePostToolBatchHook never receives an AbortSignal.
Suggested fix: Capture the AbortSignal at batch-creation time (in _schedule, where BatchAbortState is constructed) and store it alongside the tool calls — for example, on each CompletedToolCall or as a scheduler field keyed by batch. Retrieve it directly here instead of depending on callIdToBatch map state that has already been cleaned up.
— qwen3.7-max via Qwen Code /review
| /** | ||
| * PostToolBatch hook output | ||
| */ | ||
| export interface PostToolBatchOutput extends HookOutput { |
There was a problem hiding this comment.
[Suggestion] PostToolBatchOutput interface is declared and exported but never imported anywhere in the codebase. Every other hook output interface (PostToolUseOutput, StopOutput, etc.) is consumed somewhere.
Either remove this interface or consume it where appropriate (e.g., as the return type annotation in hookSystem.firePostToolBatchEvent instead of the widened DefaultHookOutput).
— qwen3.7-max via Qwen Code /review
| stopReason: shouldStop ? batchOutput.getEffectiveReason() : undefined, | ||
| additionalContext: batchOutput.getAdditionalContext(), | ||
| }; | ||
| } catch (error) { |
There was a problem hiding this comment.
[Suggestion] Missing test for messageBus.request rejection path.
The catch block at lines 428-431 returns { shouldStop: false, hookError: message } when messageBus.request throws, but no test exercises this path. Every other fire*Hook function in this file has a corresponding mockRejectedValue test:
firePreToolUseHook(test line 238)firePostToolUseHook(test line 390)firePostToolUseFailureHook(test line 602)
Add a test to follow the established pattern:
it('should return hookError when messageBus.request throws', async () => {
const mockMessageBus = createMockMessageBus();
(mockMessageBus.request as ReturnType<typeof vi.fn>).mockRejectedValue(
new Error('bus timeout'),
);
const result = await firePostToolBatchHook(mockMessageBus, []);
expect(result.shouldStop).toBe(false);
expect(result.hookError).toContain('bus timeout');
});— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
All Round 3 Critical/Suggestion findings are well-addressed in this push:
- batchSignal dead code → New
callIdToPostToolBatchSignalMap with explicit lifecycle and cleanup, documented with inline comment explaining whycallIdToBatchcannot be reused. - hookAggregator missing PostToolBatch → Added to both
mergeWithOrLogicandcreateSpecificHookOutputswitches, with tests for stop and deny decisions across multiple hooks. - firePostToolBatchHook catch block → Test added for
messageBus.requestrejection path. - appendContextToResponsePart key/currentText divergence → Unified with
useOutputKeyvariable. - Allocation on no-hook path →
.map(toPostToolBatchToolCall)moved insideif (messageBus)block. - shouldStop integration test → Added with non-last-call assertion.
Build passes, 263 focused tests pass (coreToolScheduler, toolHookTriggers, hookAggregator). Full review with 9 parallel agents + reverse audit found no new issues.
— qwen3.7-max via Qwen Code /review
| // Keep the scheduling signal until the all-calls-complete hook fires. | ||
| // callIdToBatch is drained earlier when spans end, so it cannot be used | ||
| // to recover the PostToolBatch AbortSignal reliably. | ||
| private callIdToPostToolBatchSignal = new Map<string, AbortSignal>(); |
There was a problem hiding this comment.
[Suggestion] callIdToPostToolBatchSignal has a single cleanup path (line 3300 in checkAndNotifyCompletion), while the parallel map callIdToBatch is also cleaned up in releaseBatchListenerIfDrained (called from finalizeToolSpan). If a tool call gets stuck and never reaches terminal state (e.g., an awaiting_approval call that is never resolved), the AbortSignal reference persists for the scheduler's lifetime.
In a long-lived daemon session, orphaned entries hold references to AbortSignal objects and their listener chains. Consider adding a defensive cleanup in drainSpansForBatch (the abort path) for callIds whose tool call has already reached terminal state, or document the accepted risk.
— qwen3.7-max via Qwen Code /review
| expect(lastResponse?.['error']).toContain('halt'); | ||
| expect(lastResponse?.['error']).toContain('batch context'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Two PostToolBatch code paths lack scheduler-level integration tests:
(1) hookError path: When firePostToolBatchHook returns hookError (e.g., messageBus.request returns success: false), the withHookSpan callback maps it to { success: false, error, shouldStop: false }. No test verifies that tool calls pass through unmodified and onAllToolCallsComplete still fires.
(2) Map cleanup: The callIdToPostToolBatchSignal.delete() loop at line 3300 is untested. A post-condition assertion like (scheduler as any).callIdToPostToolBatchSignal.size === 0 in existing tests would cover the cleanup path.
— qwen3.7-max via Qwen Code /review
| * Fired once after every tool call in a batch has resolved. | ||
| */ | ||
| export interface PostToolBatchInput extends HookInput { | ||
| tool_calls: PostToolBatchToolCall[]; |
There was a problem hiding this comment.
[Suggestion] PostToolBatchInput omits permission_mode, while PostToolUseInput (line 640) and PostToolUseFailureInput (line 665) both include it. Hook authors who write both PostToolUse and PostToolBatch hooks will find the batch input missing a field they rely on for permission-context-aware stop decisions (e.g., "only stop in default mode, not in yolo").
| tool_calls: PostToolBatchToolCall[]; | |
| export interface PostToolBatchInput extends HookInput { | |
| permission_mode: PermissionMode; | |
| tool_calls: PostToolBatchToolCall[]; | |
| } |
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] Test coverage gaps at the scheduler integration level: (1) callIdToPostToolBatchSignal cleanup is never verified across multiple sequential batches — stale signals could leak without detection; (2) no scheduler test verifies that tool calls pass through unchanged when firePostToolBatchHook returns hookError; (3) firePostToolBatchHook(undefined, ...) early-return guard is missing its standard test (every sibling hook trigger has one); (4) appendContextToResponsePart non-string output branch is untested (both integration tests use string outputs only).
[Suggestion] firePostToolBatchHook and PostToolBatchHookResult are not exported from the core package's public barrel (index.ts), while firePostToolUseHook and firePostToolUseFailureHook are. This means the ACP/daemon/VS Code integration path (Session.runToolCalls) cannot fire PostToolBatch hooks — the event works in CLI mode only.
| ); | ||
| } | ||
| if (messageBus) { | ||
| const batchToolCalls = completedCalls.map(toPostToolBatchToolCall); |
There was a problem hiding this comment.
[Critical] isRunning() returns false during the PostToolBatch hook await window.
this.toolCalls = [] at line 3293 empties the array. isFinalizingToolCalls is only set to true at line 3362 — after the await this.withHookSpan(...) completes. During this await, isRunning() (line 1195: this.isFinalizingToolCalls || this.toolCalls.some(...)) returns false.
A concurrent schedule() call during this window bypasses the request queue (since isRunning() returns false) and starts execution while the previous batch's completion processing is still in flight. This can cause batch interleaving and corrupted state.
| const batchToolCalls = completedCalls.map(toPostToolBatchToolCall); | |
| this.toolCalls = []; | |
| this.isFinalizingToolCalls = true; | |
| try { | |
| const batchSignal = completedCalls |
Then wrap the rest of the completion logic in a finally { this.isFinalizingToolCalls = false; } block (replacing the existing set/unset around onAllToolCallsComplete).
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Round 7 review: permission_mode threading through the PostToolBatch hook chain is correct — consistent signatures, defaults, and test updates at every layer. The new hookError test and callIdToPostToolBatchSignal.size === 0 assertions are solid additions.
The R6 Critical (isRunning() returning false during the PostToolBatch hook await window — isFinalizingToolCalls set after the await at line 3320) remains unresolved and still blocks merge.
All prior open inline comments (4 Critical + 11 Suggestion) are still outstanding. tsc + eslint clean, 542/542 tests pass, CI green (8/8).
— qwen3.7-max via Qwen Code /review
| } | ||
| this.notifyToolCallsUpdate(); | ||
| // After completion, process the next item in the queue. | ||
| if (this.requestQueue.length > 0) { |
There was a problem hiding this comment.
[Suggestion] Queue drain sits outside the try/finally — queued schedule() promises hang forever if the try block throws.
The try/finally correctly resets isFinalizingToolCalls on error, but the requestQueue draining at lines 3379-3384 is after the finally block. If recordToolResults, onAllToolCallsComplete, logToolCall, or notifyToolCallsUpdate throws:
finallyresetsisFinalizingToolCalls = false✓- The exception propagates out of
checkAndNotifyCompletion - Both call sites (lines 1157, 2186) are fire-and-forget — the error becomes an unhandled rejection
- Queue draining is never reached — any queued
schedule()promises hang indefinitely (never resolve, never reject)
This commit expanded the at-risk surface: the entire PostToolBatch hook, logToolCall, and recordToolResults are now inside the try — more throw-points than before, same unprotected drain.
| if (this.requestQueue.length > 0) { | |
| } finally { | |
| this.isFinalizingToolCalls = false; | |
| // Always drain the queue — queued schedule() callers must not hang. | |
| if (this.requestQueue.length > 0) { | |
| const next = this.requestQueue.shift()!; | |
| this._schedule(next.request, next.signal) | |
| .then(next.resolve) | |
| .catch(next.reject); | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
| ).toBe(0); | ||
| }); | ||
|
|
||
| it('queues new tool calls while a PostToolBatch hook is still running', async () => { |
There was a problem hiding this comment.
[Suggestion] The new queueing test covers the happy path but not the error path the try/finally was designed for.
The test correctly verifies that schedule() during a pending PostToolBatch hook queues rather than re-entering _schedule(). However, no test exercises the scenario where the hook or completion callback throws while items are queued — which is exactly the failure mode that motivated the try/finally structure.
A test where onAllToolCallsComplete rejects (or the PostToolBatch hook returns success: false causing an internal throw) while a second schedule() is queued would verify that:
isFinalizingToolCallsis properly reset viafinally- The queue is still drained after the error
- The scheduler remains usable for subsequent batches
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R9 changes look good. Both R8 Suggestions are properly addressed: queue drain moved inside finally prevents hung promises on error paths, and the new test correctly validates the error recovery flow. The .catch() handlers on fire-and-forget checkAndNotifyCompletion() calls are a solid defensive improvement. 544/544 tests pass, tsc and eslint clean. LGTM! ✅ — qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
|
|
||
| describe('firePostToolBatchHook', () => { |
There was a problem hiding this comment.
[Suggestion] Missing test branches for firePostToolBatchHook edge cases.
-
No messageBus early-return: every other hook trigger function (
firePreToolUseHook,firePostToolUseHook,firePostToolUseFailureHook, etc.) has a'should return ... when no messageBus is provided'test. PostToolBatch is the only one missing it. -
success:true, output:undefined: the function attoolHookTriggers.ts:413checks!response.success || !response.outputand produces the message'hook runner returned no output without error detail'. Only thesuccess:falsebranch is tested (line 498). The distinctsuccess:true, output:undefinedpath is never exercised.
// Suggested tests:
it('should return shouldStop: false when no messageBus is provided', async () => {
const result = await firePostToolBatchHook(undefined, []);
expect(result).toEqual({ shouldStop: false });
});
it('should return hookError when hook returns success without output', async () => {
const mockMessageBus = createMockMessageBus();
(mockMessageBus.request as ReturnType<typeof vi.fn>).mockResolvedValue({
success: true,
output: undefined,
});
const result = await firePostToolBatchHook(mockMessageBus, []);
expect(result.shouldStop).toBe(false);
expect(result.hookError).toMatch(/no output/);
});— claude-opus-4-7 via Claude Code /qreview
| contentLength: error.message.length, | ||
| }); | ||
|
|
||
| function serializeToolResponse( |
There was a problem hiding this comment.
[Suggestion] serializeToolResponse produces a tool_response schema that differs from PostToolUse's without documentation.
PostToolBatch: { response_parts, result_display, error, error_type, content_length }
PostToolUse: { llmContent, returnDisplay } (via toolHookTriggers.ts:247)
Hook authors working with both events will encounter different field names for similar data. The PostToolBatchToolCall.tool_response type is Record<string, unknown> with no JSDoc listing the fields.
Consider adding JSDoc on PostToolBatchToolCall.tool_response documenting the shape, or aligning field names where possible (e.g., include result_display alongside response_parts).
— claude-opus-4-7 via Claude Code /qreview
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R10 (14c1fd7): No issues found. LGTM! ✅
The R9→R10 delta adds well-targeted test coverage: two new tests for firePostToolBatchHook (no-messageBus early return, success-without-output hookError) and a PostToolBatch telemetry span test. All three new tests are correct and match the implementation. JSDoc and grammar improvements in types.ts are accurate.
9 review agents + 1 reverse audit round → 0 findings. tsc/eslint clean. 128/128 relevant tests pass. CI green 8/8.
— qwen3.7-max via Qwen Code /review
Local Real-Test Verification ReportRe-validating after the round 5 sequence ( Test environment
Suite results
Per-file breakdown:
The four
All four green. Live e2e — real shell-command hook against a real modelA real E1. Contract — payload shape{
"hook_event_name": "PostToolBatch",
"permission_mode": "yolo",
"has_session_id": true,
"has_transcript_path": true,
"has_cwd": true,
"tool_call_count": 1,
"first_call_keys": ["tool_input", "tool_name", "tool_response", "tool_use_id"],
"first_call_response_keys": ["content_length", "response_parts", "result_display"]
}Exact match with the documented interfaces:
E2. Batch-fire semantics — once per batch, not once per toolDriven the model with "Read alpha.txt, beta.txt, gamma.txt". The model serialized into 3 single-tool batches, and the hook fired 3 times ( E3.
|
# Conflicts: # packages/cli/src/ui/components/hooks/constants.test.ts # packages/core/src/core/coreToolScheduler.test.ts
There was a problem hiding this comment.
CI is failing on this PR — one test fails across all three OS runners:
packages/cli/src/ui/components/hooks/HooksManagementDialog.test.tsx → should navigate directly from a non-matcher hook to config detail
AssertionError: expected '╭───…' to contain '❯ 7. Stop'
Could you take a look?
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R11 (d6ab91a): No issues found. LGTM! ✅ The R10→R11 delta fixes the CI test failure by replacing hardcoded loop count and assertion index with dynamic DISPLAY_HOOK_EVENTS.indexOf(HookEventName.Stop) lookup. All 8 HooksManagementDialog tests pass, 37 constants tests pass, tsc clean. — qwen3.7-max via Qwen Code /review
…hook # Conflicts: # packages/core/src/core/coreToolScheduler.test.ts
| if (messageBus) { | ||
| const batchToolCalls = completedCalls.map(toPostToolBatchToolCall); | ||
| const permissionMode = this.config.getApprovalMode(); | ||
| const batchHookResult = await this.withHookSpan( |
There was a problem hiding this comment.
[Critical] Telemetry split-brain on PostToolBatch stop.
The tool's OTel span is finalized as OK in executeSingleToolCall's finally block (line 2790) before checkAndNotifyCompletion fires. When the PostToolBatch hook returns shouldStop: true, withPostToolBatchStop retroactively converts the last tool call to status: 'error' with EXECUTION_DENIED. Three systems now disagree: the OTel span says SUCCESS, the chat recording says ERROR, and the model sees ERROR.
The PostToolUse stop path handles this correctly — it calls setToolSpanFailure(span, TOOL_FAILURE_KIND_POST_HOOK_STOPPED, ...) before the span is finalized (line 3088). PostToolBatch cannot do this because the span is already closed.
Impact: Incident responders investigating a security event via OTel traces would see a tool execution as successful, while the chat log and model context show it as denied. This undermines the reliability of audit trails.
Suggested fix: Either (a) keep the OTel span open until after the PostToolBatch hook fires (move finalizeToolSpan from executeSingleToolCall's finally to checkAndNotifyCompletion), or (b) accept that spans cannot be corrected and add a PostToolBatch-level span attribute (e.g., post_batch_stop: true) on the existing hook span so operators can correlate.
— qwen3.7-max via Qwen Code /review
| MessageBusType.HOOK_EXECUTION_RESPONSE, | ||
| ); | ||
|
|
||
| if (!response.success || !response.output) { |
There was a problem hiding this comment.
[Critical] firePostToolBatchHook is silent when messageBus.request() returns success: false.
The !response.success branch returns { hookError: message } without any debugLogger call. The catch branch at line 429 does log (debugLogger.warn(...)), but this far more common failure path (hook script exits non-zero or times out) is completely silent. At the scheduler level, the hookError is only written to the OTel span's toEndMeta callback.
Impact: If a PostToolBatch hook is misconfigured or the hook script has a bug, the operator sees zero log evidence. The model continues executing without batch-level enforcement, and the only trace is OTel span attributes that most runbooks don't instruct engineers to check.
| if (!response.success || !response.output) { | |
| if (!response.success || !response.output) { | |
| const message = | |
| response.error?.message || | |
| `hook runner returned ${response.success ? 'no output' : 'success: false'} without error detail`; | |
| debugLogger.warn(`PostToolBatch hook returned failure: ${message}`); | |
| return { shouldStop: false, hookError: message }; | |
| } |
— qwen3.7-max via Qwen Code /review
| await this.onAllToolCallsComplete(completedCalls); | ||
| // Order matters: stop replaces the last response, so append | ||
| // additionalContext only after the stop decision is applied. | ||
| if (batchHookResult.shouldStop) { |
There was a problem hiding this comment.
[Critical] No debugLogger output when PostToolBatch hook returns shouldStop: true.
When batchHookResult.shouldStop is true, withPostToolBatchStop replaces the last tool call's response with an EXECUTION_DENIED error, but emits zero debugLogger output. The stop reason string is only embedded in the synthetic error response of the last tool call; earlier calls in the batch carry no trace of the stop decision.
Compare with the PostToolUse stop path (line 3078-3088) which calls setToolSpanFailure to update OTel spans with the stop reason.
Impact: An oncall engineer sees tool calls ending with EXECUTION_DENIED errors but no log line connecting that to "PostToolBatch hook requested stop with reason: X". Under sleep deprivation, the engineer would likely misdiagnose it as a permission system issue.
| if (batchHookResult.shouldStop) { | |
| if (batchHookResult.shouldStop) { | |
| debugLogger.info( | |
| `PostToolBatch hook stopped batch (${completedCalls.length} calls): ${ | |
| batchHookResult.stopReason || 'no reason given' | |
| }`, | |
| ); | |
| completedCalls = withPostToolBatchStop( |
— qwen3.7-max via Qwen Code /review
| ); | ||
|
|
||
| return { | ||
| ...response, |
There was a problem hiding this comment.
[Suggestion] appendContextToToolResponse doesn't update contentLength after adding additionalContext.
The spread { ...response, responseParts } carries the original contentLength forward unchanged, even though the actual content has grown by additionalContext.length + 2 (for the \n\n separator). ToolCallEvent telemetry records this.content_length = call.response.contentLength, so the logged content_length attribute will be stale.
Suggested fix:
| ...response, | |
| return { | |
| ...response, | |
| responseParts, | |
| contentLength: (response.contentLength ?? 0) + additionalContext.length + 2, | |
| }; |
— qwen3.7-max via Qwen Code /review
| additionalContext: string, | ||
| ): Part { | ||
| if (!part.functionResponse) { | ||
| return part; |
There was a problem hiding this comment.
[Suggestion] appendContextToResponsePart silently drops additionalContext when the last response part has no functionResponse.
When the last Part in a tool's output is not a functionResponse (e.g., inlineData, text), this function returns the part unchanged. The hook's additionalContext is silently lost with no warning or fallback.
Suggested fix: Add a debug log when context is dropped:
| return part; | |
| if (!part.functionResponse) { | |
| debugLogger.warn( | |
| 'appendContextToResponsePart: no functionResponse on part, additionalContext dropped', | |
| ); | |
| return part; | |
| } |
— qwen3.7-max via Qwen Code /review
| ): Record<string, unknown> { | ||
| // Keep this payload aligned with the persisted ToolCallResponseInfo fields | ||
| // hook authors need for batch-level auditing. | ||
| return { |
There was a problem hiding this comment.
[Suggestion] serializeToolResponse passes raw responseParts (including InlineData with Uint8Array) in the IPC payload.
For tools that return binary data (e.g., image readers), InlineData.data as a Uint8Array is serialized by JSON.stringify as {"0":123,"1":45,...} — an object with numeric keys that is both enormous and useless. A batch containing 10 file reads of 50 KB each produces an O(N × responseSize) IPC payload, unlike PostToolUse which fires per-call.
Suggested fix: Strip or summarize InlineData from response_parts:
| return { | |
| response_parts: response.responseParts.map((part) => | |
| part.inlineData | |
| ? { ...part, inlineData: { mimeType: part.inlineData.mimeType, data: '<binary omitted>' } } | |
| : part, | |
| ), |
— qwen3.7-max via Qwen Code /review
| signal, | ||
| }, | ||
| MessageBusType.HOOK_EXECUTION_RESPONSE, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] firePostToolBatchHook uses the 60-second default timeout from messageBus.request.
PostToolBatch fires inside checkAndNotifyCompletion while isFinalizingToolCalls = true, blocking the scheduler from dispatching queued tool requests and blocking onAllToolCallsComplete from delivering results to the model. Unlike PostToolUse (per-call, can overlap), PostToolBatch is the single gate between "all tools done" and "model receives results."
Suggested fix: Pass an explicit shorter timeout (e.g., 10-15s):
| ); | |
| MessageBusType.HOOK_EXECUTION_RESPONSE, | |
| 15_000, |
— qwen3.7-max via Qwen Code /review
| ToolErrorType.EXECUTION_DENIED, | ||
| ), | ||
| durationMs: lastCall.durationMs, | ||
| outcome: lastCall.outcome, |
There was a problem hiding this comment.
[Suggestion] EXECUTION_DENIED error type misrepresents post-execution stops.
The tool already executed successfully — its side effects (file writes, shell commands, network requests) are permanent and irreversible. EXECUTION_DENIED semantically means "the tool was prevented from executing." The model receives a response saying execution was "denied," when in reality the tool ran to completion and only the response was retroactively overwritten. This can cause the model to retry the operation (believing it was blocked), resulting in duplicate side effects.
Suggested fix: Use a distinct error type like ToolErrorType.POST_EXECUTION_STOP or ToolErrorType.HOOK_STOPPED that communicates "the tool ran but the hook requested execution to stop here."
— qwen3.7-max via Qwen Code /review
| tool_name: call.request.name, | ||
| tool_input: call.request.args, | ||
| tool_use_id: call.request.callId, | ||
| tool_response: serializeToolResponse(call.response), |
There was a problem hiding this comment.
[Suggestion] No scheduler-level integration test covers PostToolBatch firing when a tool in the batch failed during execution.
All existing PostToolBatch tests use tools that return successful results. serializeToolResponse populates error and error_type fields only for failed tools, but the serialized error-path payload is never verified end-to-end. If serializeToolResponse mishandles the ErroredToolCall.response shape, no test would catch it.
Suggested fix: Add a test where one tool throws during execution and the batch still fires PostToolBatch. Assert the hook input includes error and error_type for the failed tool.
— qwen3.7-max via Qwen Code /review
| if (batchHookResult.shouldStop) { | ||
| completedCalls = withPostToolBatchStop( | ||
| completedCalls, | ||
| batchHookResult.stopReason || |
There was a problem hiding this comment.
[Suggestion] No test for the merge interaction between AUTO-mode denial tracking (from #4476) and PostToolBatch shouldStop.
When AUTO mode blocks a tool (creating an ErroredToolCall with EXECUTION_DENIED) and PostToolBatch also returns shouldStop: true, withPostToolBatchStop replaces the last call entirely — the original AUTO-block error message and errorType are overwritten. recordToolResults then records the overwritten error, losing the original AUTO denial reason. No test covers this two-error-on-same-call interaction.
Suggested fix: Add a test where AUTO mode blocks the last tool in a batch and PostToolBatch returns shouldStop. Assert that denial counters are unchanged by PostToolBatch and document whether the error overwrite is intentional.
— qwen3.7-max via Qwen Code /review
| return { | ||
| ...response, | ||
| responseParts, | ||
| contentLength: (response.contentLength ?? 0) + additionalContext.length + 2, |
There was a problem hiding this comment.
[Suggestion] (response.contentLength ?? 0) produces misleading telemetry for binary tool outputs. When the tool returns inlineData (e.g., images), contentLength is set to undefined (since typeof content !== 'string'). The ?? 0 fallback converts this to 0 + additionalContext.length + 2, making telemetry report a ~20-byte response for what may be a 50MB image.
Consider propagating undefined when the original is undefined:
| contentLength: (response.contentLength ?? 0) + additionalContext.length + 2, | |
| contentLength: response.contentLength !== undefined | |
| ? response.contentLength + additionalContext.length + 2 | |
| : undefined, |
— qwen3.7-max via Qwen Code /review
| calls[calls.length - 1] = { | ||
| status: 'error', | ||
| request: lastCall.request, | ||
| tool: 'tool' in lastCall ? lastCall.tool : undefined, |
There was a problem hiding this comment.
[Suggestion] 'tool' in lastCall is dead code — all CompletedToolCall variants (SuccessfulToolCall, CancelledToolCall, ErroredToolCall) declare a tool property. The in check always returns true, so the undefined branch is unreachable. Simplify to tool: lastCall.tool.
Additionally, withPostToolBatchStop discards the original responseParts and resultDisplay entirely. While this is correct for the model (it should see the stop error), logToolCall and recordToolResults run after this replacement, so audit/replay data for the last call loses the original successful output. Consider preserving the original response in an adjacent field if audit fidelity matters.
— qwen3.7-max via Qwen Code /review
| shouldStop: r.shouldStop, | ||
| hasAdditionalContext: !!r.additionalContext, | ||
| blockType: r.shouldStop ? 'stop' : undefined, | ||
| postBatchStop: r.shouldStop, |
There was a problem hiding this comment.
[Suggestion] Two telemetry diagnostic gaps in this block:
-
stopReasonnot captured — whenshouldStop: true, the reason string is available inbatchHookResult.stopReasonbut is only used for the error message, not written to the span. Oncall engineers seepost_batch_stop: truewith no way to distinguish "PII detected" from "timeout". -
postBatchStopmissing onhookError— when the hook fails (timeout, invalid output),toEndMetamaps to{ success: false, error: r.hookError, shouldStop: false }without settingpostBatchStop. Queryingpost_batch_stop = truemisses all hook failures entirely.
Consider adding postBatchStopReason as a span attribute and always setting postBatchStop (even when false) so the attribute's presence indicates the hook ran.
— qwen3.7-max via Qwen Code /review
| this.checkAndNotifyCompletion(); | ||
| void this.checkAndNotifyCompletion().catch((error: unknown) => { | ||
| debugLogger.warn( | ||
| `Tool completion notification failed: ${ |
There was a problem hiding this comment.
[Suggestion] Two .catch() handlers (here and at line ~2290) print identical "Tool completion notification failed" messages. When both paths fail simultaneously, production logs show two indistinguishable errors. Add a site identifier to each message (e.g., "setStatusInternal completion failed" / "_schedule completion failed") for faster root-cause identification.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R14 (2639a5337): No new issues in this delta. The R13→R14 push cleanly addresses the R13 findings:
- contentLength undefined preservation — correctly keeps
undefinedfor binary outputs (inlineData) instead of fabricating a count via?? 0.ToolCallResponseInfo.contentLengthisnumber | undefinedand downstream consumers (telemetry/types.ts:217,serializeToolResponse) simply forward it. - Removal of dead
'tool' in lastCallguard — all threeCompletedToolCallvariants declare atoolfield; theincheck was alwaystrue. .catch()message disambiguation —setStatusInternalvs_scheduleprefixes now distinguish the two failure sites in production logs.postBatchStop: falseon hookError +postBatchStopReasonon success — closes the R13 telemetry gaps.truncateSpanErroracceptsstring; the|| 'no reason given'fallback correctly handles bothundefinedand emptyHookOutput.stopReason.- Tests: 256/256 pass in focused scheduler + session-tracing files (11 new assertions for
postBatchStop/postBatchStopReason); ESLint clean;tsc --noEmitclean acrosspackages/core.
Downgrading from Approve to Comment because ~26 inline comments from prior rounds remain open on the current commit (including several Critical: batchSignal dead code, exit-code-2 not stopping, firePostToolBatchHook silent on success: false, no debugLogger on shouldStop: true, telemetry split-brain, isRunning() false during hook await, appendContextToResponsePart 'error'-field write, withPostToolBatchStop discarding additionalContext, firePostToolBatchHook missing AbortSignal). Addressing those is a separate concern from the clean R14 delta.
— qwen3.7-max via Qwen Code /review
Verification ReportEnvironment: macOS Darwin 25.4.0 (Apple Silicon), Node.js, tmux parallel execution Test Results
Code Review Observations
Verdict✅ Ready to merge — All tests pass, types check cleanly, builds succeed, and the implementation follows established hook patterns with proper fast-path optimization. Verified by: wenshao |
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
LGTM. CI is green on 2639a53, and the post-merge hardening (binary omission in batch payloads, per-call status, 15s hook timeout + abort signal, and stop telemetry) is well covered by tests. Thanks for the quick turnaround on the fixes.
3 participants
Summary
Validation
Scope / Risk
Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Related #4343