Deduplicate hooks from multiple discovery paths#303928
Open
lewing wants to merge 3 commits intomicrosoft:mainfrom
Open
Deduplicate hooks from multiple discovery paths#303928lewing wants to merge 3 commits intomicrosoft:mainfrom
lewing wants to merge 3 commits intomicrosoft:mainfrom
Conversation
When the same hook is defined in both .github/hooks/ and .claude/settings.json, computeHooks() collects both and fires them twice per tool call. This adds a deduplication step after all hooks are collected but before converting to immutable ChatRequestHooks. Two hooks are considered identical if all their command fields (command, windows, linux, osx), cwd, env, and timeout match. First occurrence is kept, preserving the path priority order from DEFAULT_HOOK_FILE_PATHS. Fixes microsoft#303926 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prevents chat hooks from firing twice when the same hook is discovered via multiple configuration paths (notably .github/hooks/ and .claude/settings.json) by deduplicating equivalent hook commands after collection in computeHooks().
Changes:
- Added
deduplicateHooks()(signature-based) to remove duplicateIHookCommands while preserving first-seen order. - Applied hook deduplication in
PromptsService.computeHooks()and added trace logging when duplicates are removed. - Added a dedicated
deduplicateHookstest suite with coverage for ordering and field-difference cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/promptSyntax/hookSchema.ts | Adds hook signature computation + deduplicateHooks() utility. |
| src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts | Runs deduplication on collected hooks before building ChatRequestHooks, with trace logging. |
| src/vs/workbench/contrib/chat/test/common/promptSyntax/hookSchema.test.ts | Adds unit tests validating dedup behavior across relevant fields and ordering. |
src/vs/workbench/contrib/chat/common/promptSyntax/hookSchema.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/common/promptSyntax/hookSchema.ts
Outdated
Show resolved
Hide resolved
- Use JSON.stringify instead of NUL-joined parts for hook signature encoding to avoid ambiguity from separator chars in env keys/values - Accept readonly IHookCommand[] in deduplicateHooks() to match ChatRequestHooks readonly arrays Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts
Outdated
Show resolved
Hide resolved
Use [...deduped] instead of 'as IHookCommand[]' cast to maintain mutable array for the collectedHooks Map without weakening the readonly return type of deduplicateHooks(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
pwang347
reviewed
Mar 23, 2026
| for (const [hookType, commands] of collectedHooks) { | ||
| const deduped = deduplicateHooks(commands); | ||
| if (deduped.length < commands.length) { | ||
| this.logger.trace(`[PromptsService] Deduplicated ${hookType} hooks: ${commands.length} → ${deduped.length}`); |
Member
There was a problem hiding this comment.
We need to also actually get the list of files that were deduped for debug events. I can take a look at that though because that flow needs some caching changes. Thanks!
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When the same hook is defined in both
.github/hooks/and.claude/settings.json,computeHooks()fires it twice because it collects from allDEFAULT_HOOK_FILE_PATHSwithout deduplication.This is a real problem for cross-tool projects that need hooks in both paths —
.github/hooks/for Copilot CLI/Gemini CLI and.claude/settings.jsonfor Claude Code. Since VS Code reads both, users get double-fired hooks.Changes
hookSchema.ts: AddeddeduplicateHooks()function that compares hooks by all their command fields (command,windows,linux,osx),cwd,env, andtimeout. Uses a signature-basedSetfor O(n) dedup. First occurrence is kept, preserving path priority order fromDEFAULT_HOOK_FILE_PATHS. Environment variables are sorted before comparison so{A:1, B:2}equals{B:2, A:1}.promptsServiceImpl.ts: CallsdeduplicateHooks()after all hooks are collected (from files + plugins) but before converting to immutableChatRequestHooks. Adds trace logging when duplicates are removed.hookSchema.test.ts: 11 test cases covering:Fixes #303926