Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the chat extension hooks implementation to support hook-specific output fields for different hook types beyond just preToolUse hooks. The changes aim to make the hook system more flexible by allowing all hooks to return custom output fields rather than filtering to only known preToolUse fields.
Changes:
- Simplified
_extractHookSpecificOutputto pass through all output fields instead of filtering to specific known fields - Removed input logging truncation to log full hook input including toolArgs
- Added test case to verify non-preToolUse hooks can return custom output fields
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/hooksExecutionService.ts | Simplified field extraction logic and removed input truncation in logging |
| src/vs/workbench/contrib/chat/test/common/hooksExecutionService.test.ts | Added test for passing through hook-specific fields for Stop hooks |
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/chat/common/hooksExecutionService.ts:330
- The simplified _extractHookSpecificOutput method now includes ALL fields from the result, including common fields like 'stopReason' and 'systemMessage'. This creates duplication - these common fields are already extracted separately and placed in the IHookResult as top-level properties (stopReason and messageForUser). Now they would also appear in the output property.
According to the existing test on line 193-215 of the test file, when a result contains both common fields (stopReason, systemMessage) and hook-specific fields (permissionDecision), only the hook-specific fields should be in the output property. The common fields should be excluded from output since they're already represented as separate properties in IHookResult.
The old implementation filtered to only include known hook-specific fields (permissionDecision, permissionDecisionReason for PreToolUse hooks). The new implementation should exclude the common fields to avoid duplication.
// Extract only known hook-specific fields for output
const resultObj = commandResult.result as Record<string, unknown>;
const hookOutput = this._extractHookSpecificOutput(resultObj);
return {
stopReason: commonFields.stopReason,
messageForUser: commonFields.systemMessage,
output: Object.keys(hookOutput).length > 0 ? hookOutput : undefined,
success: true,
src/vs/workbench/contrib/chat/common/hooksExecutionService.ts:265
- The removal of input truncation could lead to excessively large logs when toolArgs contains large objects (e.g., large file contents, base64 encoded data, or deeply nested structures). The original implementation truncated toolArgs to '...' specifically to avoid this issue.
While full logging may be useful for debugging, it could cause performance issues and make logs difficult to read when tool arguments are large. Consider either restoring the truncation, implementing a size limit on logged values, or making the full logging conditional (e.g., based on a debug flag or maximum size threshold).
cwd: hookCommand.cwd?.fsPath ?? '',
Allow output to flow through to chat extension