feat: support additionalContext in PreCompact hook#308297
feat: support additionalContext in PreCompact hook#308297roblourens wants to merge 2 commits intomainfrom
Conversation
Allow PreCompact hook to inject additional context into the summarization prompt, matching the pattern used by other hooks (PreToolUse, PostToolUse, SessionStart, SubagentStart, UserPromptSubmit). Changes: - Add IPreCompactHookSpecificCommandOutput type in hookCommandTypes.ts - Add PreCompactHookOutput interface in chatHookService.ts - Modify executePreCompactHook() to collect additionalContext from hook results - Merge hook additionalContext into summarizationInstructions in summarizeHistory() - Add unit tests for prompt rendering with summarizationInstructions - Add integration tests for PreCompact hook flow in preCompactHook.spec.tsx
There was a problem hiding this comment.
Pull request overview
Adds support for PreCompact hooks to contribute additionalContext that is merged into the conversation compaction/summarization prompt, aligning PreCompact with other hook types that can inject prompt context.
Changes:
- Introduces PreCompact hook-specific output typing (
additionalContext) for hook command results. - Executes PreCompact hooks before summarization and merges returned
additionalContextintosummarizationInstructions. - Adds unit and integration tests validating prompt rendering and PreCompact hook flow.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/platform/chat/common/hookCommandTypes.ts | Adds IPreCompactHookSpecificCommandOutput type to the hook command JSON contract types. |
| extensions/copilot/src/platform/chat/common/chatHookService.ts | Adds PreCompactHookOutput interface describing nested hookSpecificOutput.additionalContext. |
| extensions/copilot/src/extension/prompts/node/agent/summarizedConversationHistory.tsx | Runs PreCompact hooks before summarization and merges hook-provided context into summarization instructions; updates prompt heading text. |
| extensions/copilot/src/extension/prompts/node/agent/test/summarization.spec.tsx | Adds tests for rendering the “additional summarization instructions” section when provided/omitted. |
| extensions/copilot/src/extension/prompts/node/agent/test/preCompactHook.spec.tsx | Adds integration tests ensuring PreCompact is invoked and its additionalContext influences the summarization prompt. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 2
| import { LanguageModelTextPart, LanguageModelToolResult } from '../../../../../vscodeTypes'; | ||
| import { MockChatHookService } from '../../../../intents/test/node/toolCallingLoopHooks.spec'; | ||
| import { ChatVariablesCollection } from '../../../../prompt/common/chatVariablesCollection'; |
There was a problem hiding this comment.
Avoid importing MockChatHookService from another *.spec.ts file. Importing a spec as a module can cause that spec’s tests to be registered/executed as a dependency (and then again when the runner loads it as a test file), which can lead to duplicated suites and flaky ordering. Move MockChatHookService into a dedicated test helper module (non-*.spec.*) and import it from both specs.
| if (result.resultKind === 'error') { | ||
| const errorMessage = typeof result.output === 'string' ? result.output : 'Unknown error'; | ||
| this.logService.error(`[ConversationHistorySummarizer] PreCompact hook error: ${errorMessage}`); | ||
| } else if (result.resultKind === 'success' && result.output && typeof result.output === 'object') { | ||
| const output = result.output as PreCompactHookOutput; | ||
| if (output.hookSpecificOutput?.additionalContext) { | ||
| allAdditionalContext.push(output.hookSpecificOutput.additionalContext); | ||
| } | ||
| } |
There was a problem hiding this comment.
executePreCompactHook manually iterates hook results and only logs resultKind === 'error'. This drops warning results (and any stopReason/warningMessage) on the floor, unlike other hook call sites (e.g. SessionStart/SubagentStart) that use processHookResults(..., ignoreErrors: true) to consistently handle warnings/errors while still proceeding. Consider switching to processHookResults here (with outputStream: undefined and ignoreErrors: true) and collecting hookSpecificOutput.additionalContext in onSuccess.
| if (result.resultKind === 'error') { | |
| const errorMessage = typeof result.output === 'string' ? result.output : 'Unknown error'; | |
| this.logService.error(`[ConversationHistorySummarizer] PreCompact hook error: ${errorMessage}`); | |
| } else if (result.resultKind === 'success' && result.output && typeof result.output === 'object') { | |
| const output = result.output as PreCompactHookOutput; | |
| if (output.hookSpecificOutput?.additionalContext) { | |
| allAdditionalContext.push(output.hookSpecificOutput.additionalContext); | |
| } | |
| } | |
| if (result.resultKind === 'success' && result.output && typeof result.output === 'object') { | |
| const output = result.output as PreCompactHookOutput; | |
| if (output.hookSpecificOutput?.additionalContext) { | |
| allAdditionalContext.push(output.hookSpecificOutput.additionalContext); | |
| } | |
| } | |
| const warningMessage = 'warningMessage' in result && typeof result.warningMessage === 'string' ? result.warningMessage : undefined; | |
| const stopReason = 'stopReason' in result && typeof result.stopReason === 'string' ? result.stopReason : undefined; | |
| if (result.resultKind === 'error') { | |
| const errorMessage = typeof result.output === 'string' ? result.output : 'Unknown error'; | |
| this.logService.error(`[ConversationHistorySummarizer] PreCompact hook error: ${errorMessage}`); | |
| if (warningMessage) { | |
| this.logService.warn(`[ConversationHistorySummarizer] PreCompact hook warning: ${warningMessage}`); | |
| } | |
| if (stopReason) { | |
| this.logService.warn(`[ConversationHistorySummarizer] PreCompact hook stop reason: ${stopReason}`); | |
| } | |
| } else if (result.resultKind === 'warning') { | |
| const resultWarningMessage = typeof result.output === 'string' ? result.output : warningMessage ?? 'Unknown warning'; | |
| this.logService.warn(`[ConversationHistorySummarizer] PreCompact hook warning: ${resultWarningMessage}`); | |
| if (stopReason) { | |
| this.logService.warn(`[ConversationHistorySummarizer] PreCompact hook stop reason: ${stopReason}`); | |
| } | |
| } |
Allow PreCompact hook to inject additional context into the summarization prompt, matching the pattern used by other hooks (PreToolUse, PostToolUse, SessionStart, SubagentStart, UserPromptSubmit).
Changes: