refactor(server): resolve hook provider by providerId - #388
Open
tbrandenburg wants to merge 2 commits into
Open
Conversation
added 2 commits
August 14, 2026 21:46
POST /api/hooks/:providerId accepted and validated a providerId in the URL, but HookEventHandler.handleEvent(_providerId, event) never read it -- every hook POST reached the same hardcoded Claude provider instance regardless of the id in the URL. This adds a real registry and makes providerId meaningful: - server/src/providers/index.ts: Map<string, HookProvider> + getProvider(id), falling back to the default (Claude) for any unregistered id so existing callers keep working exactly as before. getAllProviders() for future cross-provider concerns (e.g. capability unions). - HookEventHandler now resolves the provider per event by providerId instead of a constructor-captured singleton. protocolVersion mismatches are now checked per-provider-id with warn-once logging (was constructor-time, single-provider only). - AgentRuntime wires HookEventHandler through the shared providerRegistry; its own constructor still takes one 'primary' HookProvider for the Claude-specific file-fallback/Agent-Teams singleton wiring, which stays a single-provider concern until a provider with a file fallback or team extension actually exists. - Tests: providerRegistry.test.ts (resolution + fallback), and a 'provider registry routing' suite in hookEventHandler.test.ts proving two distinct providerIds reach two distinct provider instances end-to-end (via a minimal synthetic second provider, standing in until a real second provider lands) and that unknown-provider / protocol-mismatch cases are dropped with a single warning. No behavior change for the only provider that exists today (Claude): all 383 server tests pass, lint/types/build/asyncapi/e2e-inventory all clean with zero drift.
This was referenced Aug 14, 2026
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.
What
POST /api/hooks/:providerIdaccepts and validates aproviderIdin the URL, but nothing downstream ever reads it:HookEventHandler.handleEvent(_providerId, event)has an underscore-prefixed, never-consumed parameter, and every hook route reaches the same hardcoded Claude provider instance regardless of the id in the URL. Today,/api/hooks/claudeand/api/hooks/some-other-idare indistinguishable.This PR fixes that:
providerIdnow actually resolves to a provider.Changes
server/src/providers/index.ts: adds a realMap<string, HookProvider>registry,getProvider(id)(falls back to the default/Claude provider for any unregistered id, preserving today's behavior for callers that don't know about future providers), andgetAllProviders()for future cross-provider concerns. Existing named exports (claudeProvider,copyHookScript) are untouched, so no current importer breaks.HookEventHandler: now resolves the provider per event via the injected registry instead of a constructor-captured singleton. TheprotocolVersioncompatibility check moves from constructor-time (single provider) to per-provider-id, cached so an unsupported/unknown provider only logs once, not once per event.AgentRuntime: wiresHookEventHandlerthrough the shared registry. Its own constructor still takes one "primary"HookProviderfor the Claude-specific file-fallback/Agent-Teams singleton wiring (module-levelsetHookProvider/setTeamProvidercalls) — those are genuinely Claude-only concerns today (no other provider has a file fallback or ateamextension), so a single primary provider is still the right shape there. Only hook dispatch needed to become multi-provider-aware.providerRegistry.test.ts(resolution + fallback), and a new "provider registry routing" suite inhookEventHandler.test.tsproving two distinctproviderIds reach two distinct provider instances end-to-end (via a minimal synthetic second provider standing in until a real second provider exists), plus coverage for the unknown-provider and protocol-mismatch drop paths.Why this framing
This reads as a bugfix toward the design the code already implies (
SessionRouteralready storesproviderIdon buffered events and replays it — the buffer/flush path was already provider-aware; only the resolution step was missing), not a redesign. It's also the only piece of groundwork a genuinely new (non-Claude)HookProviderwould need before it could be added as an isolated, additive change underserver/src/providers/hook/<new>/.Testing
npm run lint/npm run check-types— clean (0 errors; 1 pre-existing unrelated webview warning)npm run asyncapi:validate/npm run asyncapi:generate— valid, zero diff (no protocol changes in this PR)npm run e2e:inventory— zero diff (no e2e specs added/changed)npm run build— succeedsnpm test(server + webview) — 383/383 server tests pass, 52/52 webview tests pass