fix(hooks): narrow inject matchers — Mac CPU fix D - #1479
Conversation
The team settings.json hook config was wiring SessionStart, SessionEnd,
TeammateIdle, and TaskCompleted with matcher='*' even though zero handlers
exist for those events. PostToolUse was wired with '*' but only one
handler (runtime-emit-msg, matcher /^SendMessage$/) exists. Result: every
fire of any of these events caused a wasted bun cold-start to run the
hook-dispatch entrypoint that did nothing.
Fix:
- types.ts: replace flat DISPATCHED_EVENTS array with DISPATCHED_EVENT_MATCHERS
map. Only PreToolUse ('*') and PostToolUse ('SendMessage') are wired.
DISPATCHED_EVENTS retained as a derived array for backward-compat.
- inject.ts: buildHooksConfig() now uses per-event matchers. injectIntoFile()
refactored into helpers (readSettings, allEventsAlreadyInjected,
hasNoObsoleteGenieEntries, pruneObsoleteGenieEntries, refreshMatcherEntries,
upsertGenieEntry) — each with single responsibility and no cognitive
complexity warnings.
- Pruning: cleans up SessionStart/SessionEnd/TeammateIdle/TaskCompleted from
any pre-fix-D installed settings.json.
- Refresh: updates PostToolUse matcher '*' → 'SendMessage' on next inject.
- User-defined hooks under obsolete events are PRESERVED (only genie's
own dispatch entries are pruned).
Validation: 10/10 src/hooks/__tests__/inject.test.ts pass (6 prior + 4 new
covering: matcher map shape, PostToolUse SendMessage narrowing, obsolete
event pruning, user-hook preservation). tsc --noEmit clean. biome clean.
Fix D of the 5-step .19 Mac-CPU root-cause plan (A→E):
- A: shipped #1475 — drop runRetention from getConnection
- filewatch: shipped #1474 — chokidar replacement
- C: shipped #1476 — GENIE_SKIP_DB_BOOT for hook dispatch
- D: this PR
- B/E: pending — needsSeed cache (lower priority post-C); session-sync cache file
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request optimizes the hook injection process to mitigate performance issues on Mac-CPU environments by reducing unnecessary dispatcher invocations. It replaces the generic event list with a mapping that uses specific tool matchers and excludes events without handlers. The injection logic is refactored to support pruning obsolete entries and refreshing existing ones. Feedback indicates that the logic for skipping redundant injections is currently too lenient and should be strengthened to verify timeouts and prevent stale entries.
| function allEventsAlreadyInjected(existingHooks: HooksConfig, hooksConfig: HooksConfig): boolean { | ||
| return DISPATCHED_EVENTS.every((event) => { | ||
| const existing = existingHooks[event]; | ||
| const desiredCommand = hooksConfig[event][0].hooks[0].command; | ||
| const desiredMatcher = hooksConfig[event][0].matcher; | ||
| return existing?.some((m) => m.matcher === desiredMatcher && m.hooks?.some((h) => h.command === desiredCommand)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The short-circuit logic in allEventsAlreadyInjected is too lenient. It returns true if it finds at least one matching genie entry, but it does not verify that the timeout matches DISPATCH_TIMEOUT, nor does it ensure the absence of stale genie entries (e.g., an old * matcher remaining alongside a new SendMessage matcher). This could lead to the injection process skipping necessary updates or leaving redundant dispatcher invocations in settings.json if the configuration is in a partially updated state.
function allEventsAlreadyInjected(existingHooks: HooksConfig, hooksConfig: HooksConfig): boolean {
return DISPATCHED_EVENTS.every((event) => {
const existing = existingHooks[event] ?? [];
const desired = hooksConfig[event][0];
const desiredCommand = desired.hooks[0].command;
const desiredMatcher = desired.matcher;
const genieEntries = existing.filter((m) => m.hooks?.some((h) => isGenieDispatchCommand(h.command)));
if (genieEntries.length === 0) return false;
// Ensure all genie entries for this event match the desired configuration
return genieEntries.every((m) =>
m.matcher === desiredMatcher &&
m.hooks.every((h) =>
!isGenieDispatchCommand(h.command) ||
(h.command === desiredCommand && h.timeout === DISPATCH_TIMEOUT)
)
);
});
}There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48ef4ed5e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| function pruneObsoleteGenieEntries(mergedHooks: HooksConfig): void { | ||
| for (const event of Object.keys(mergedHooks)) { | ||
| if (DISPATCHED_EVENTS.includes(event as never)) continue; |
There was a problem hiding this comment.
Restrict pruning to deprecated hook events only
pruneObsoleteGenieEntries currently treats every event outside DISPATCHED_EVENTS as obsolete, so a re-inject removes genie hook dispatch entries from still-supported events like UserPromptSubmit/Stop (handlers are registered in src/hooks/index.ts). In practice, teams that manually enabled dispatch for those events will silently lose those hooks the next time injectTeamHooks runs, which is a functional regression introduced by this cleanup path.
Useful? React with 👍 / 👎.
| const hasGenieHook = matcher.hooks?.some((h) => isGenieDispatchCommand(h.command)); | ||
| return { | ||
| ...matcher, | ||
| matcher: hasGenieHook ? genieEntry.matcher : matcher.matcher, |
There was a problem hiding this comment.
Avoid rewriting shared matcher blocks in-place
When a matcher block contains both the genie dispatch hook and user-defined hooks, refreshMatcherEntries rewrites the entire block’s matcher to genieEntry.matcher. For PostToolUse, this changes * to SendMessage and unintentionally narrows user hooks in the same block, so their commands stop running for other tools after reinjection. The matcher update should be isolated to genie’s own entry instead of mutating shared entries.
Useful? React with 👍 / 👎.
Per-process Map<executorId, sessionId> in session-sync.ts caches NOTHING across `genie hook dispatch` bun forks (each fork starts with empty Map). Result: every cold-start hook fork did 3 DB round-trips (getAgentByName + getExecutor + audit) even when the (executorId, sessionId) pair was already-reconciled by a previous fork. Fix: - Disk-backed cache at ~/.genie/cache/session-sync.json (overridable via __GENIE_SESSION_SYNC_CACHE_FILE for tests) - loadDiskCache() runs once per fork BEFORE any DB call, replacing empty Map with previously-persisted (executorId, sessionId) pairs - persistDiskCache() writes after every cache mutation (atomic via write-temp + rename) - MAX_CACHE_ENTRIES=1000 with insertion-order trim — bounded growth - Concurrency: rename races can lose one fork's writes for OTHER executor entries (benign: at most occasional redundant DB syncs from later forks) - Corrupt cache file is tolerated — falls through to DB After this PR, hook forks have ZERO DB calls in the steady state for already-reconciled sessions (only on actual rotation or first capture). Validation: 18/18 src/hooks/__tests__/session-sync.test.ts pass (14 prior + 4 new: persist-after-reconcile, cold-start-loads-from-disk, cache-miss- falls-through, corrupt-file-tolerated). tsc --noEmit clean. biome clean. Fix E of the 5-step .19 Mac-CPU root-cause plan (A→E): - A: shipped #1475 — drop runRetention from getConnection - filewatch: shipped #1474 — chokidar replacement - C: shipped #1476 — GENIE_SKIP_DB_BOOT for hook dispatch - D: open #1479 — narrow inject matchers - E: this PR - B: in flight (twin) — needsSeed mtime cache (lower priority post-C) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
After rebasing fix E onto current dev (post #1479/#1482/#1483/#1484 merges), the pre-existing session-sync (Gap 2) tests started failing because loadDiskCache() on first call would read the production ~/.genie/cache/session-sync.json — populated by earlier real-mode runs of the handler. Stale (executorId, sessionId) entries caused the in-memory cache to short-circuit BEFORE the test fixtures' mocked DB calls were made, so audit-event emissions never fired. Fix: loadDiskCache() and persistDiskCache() now skip ALL disk I/O when: - ANY _deps field is non-null (test installed mocks), OR - NODE_ENV=test or BUN_ENV=test UNLESS the test explicitly set __GENIE_SESSION_SYNC_CACHE_FILE via _setCacheFileForTest (the new fix-E tests opt in this way). Mirrors the existing test-mode skip in shouldSkipSync. Validation: 18/18 src/hooks/__tests__/session-sync.test.ts pass. Both the 14 pre-existing tests and the 4 new fix-E tests work — pre-existing tests get clean cache state; fix-E tests use their isolated tmp-dir cache file. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Per-process Map<executorId, sessionId> in session-sync.ts caches NOTHING across `genie hook dispatch` bun forks (each fork starts with empty Map). Result: every cold-start hook fork did 3 DB round-trips (getAgentByName + getExecutor + audit) even when the (executorId, sessionId) pair was already-reconciled by a previous fork. Fix: - Disk-backed cache at ~/.genie/cache/session-sync.json (overridable via __GENIE_SESSION_SYNC_CACHE_FILE for tests) - loadDiskCache() runs once per fork BEFORE any DB call, replacing empty Map with previously-persisted (executorId, sessionId) pairs - persistDiskCache() writes after every cache mutation (atomic via write-temp + rename) - MAX_CACHE_ENTRIES=1000 with insertion-order trim — bounded growth - Concurrency: rename races can lose one fork's writes for OTHER executor entries (benign: at most occasional redundant DB syncs from later forks) - Corrupt cache file is tolerated — falls through to DB After this PR, hook forks have ZERO DB calls in the steady state for already-reconciled sessions (only on actual rotation or first capture). Validation: 18/18 src/hooks/__tests__/session-sync.test.ts pass (14 prior + 4 new: persist-after-reconcile, cold-start-loads-from-disk, cache-miss- falls-through, corrupt-file-tolerated). tsc --noEmit clean. biome clean. Fix E of the 5-step .19 Mac-CPU root-cause plan (A→E): - A: shipped #1475 — drop runRetention from getConnection - filewatch: shipped #1474 — chokidar replacement - C: shipped #1476 — GENIE_SKIP_DB_BOOT for hook dispatch - D: open #1479 — narrow inject matchers - E: this PR - B: in flight (twin) — needsSeed mtime cache (lower priority post-C) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
After rebasing fix E onto current dev (post #1479/#1482/#1483/#1484 merges), the pre-existing session-sync (Gap 2) tests started failing because loadDiskCache() on first call would read the production ~/.genie/cache/session-sync.json — populated by earlier real-mode runs of the handler. Stale (executorId, sessionId) entries caused the in-memory cache to short-circuit BEFORE the test fixtures' mocked DB calls were made, so audit-event emissions never fired. Fix: loadDiskCache() and persistDiskCache() now skip ALL disk I/O when: - ANY _deps field is non-null (test installed mocks), OR - NODE_ENV=test or BUN_ENV=test UNLESS the test explicitly set __GENIE_SESSION_SYNC_CACHE_FILE via _setCacheFileForTest (the new fix-E tests opt in this way). Mirrors the existing test-mode skip in shouldSkipSync. Validation: 18/18 src/hooks/__tests__/session-sync.test.ts pass. Both the 14 pre-existing tests and the 4 new fix-E tests work — pre-existing tests get clean cache state; fix-E tests use their isolated tmp-dir cache file. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(hooks): persist session-sync cache to disk — Mac CPU fix E Per-process Map<executorId, sessionId> in session-sync.ts caches NOTHING across `genie hook dispatch` bun forks (each fork starts with empty Map). Result: every cold-start hook fork did 3 DB round-trips (getAgentByName + getExecutor + audit) even when the (executorId, sessionId) pair was already-reconciled by a previous fork. Fix: - Disk-backed cache at ~/.genie/cache/session-sync.json (overridable via __GENIE_SESSION_SYNC_CACHE_FILE for tests) - loadDiskCache() runs once per fork BEFORE any DB call, replacing empty Map with previously-persisted (executorId, sessionId) pairs - persistDiskCache() writes after every cache mutation (atomic via write-temp + rename) - MAX_CACHE_ENTRIES=1000 with insertion-order trim — bounded growth - Concurrency: rename races can lose one fork's writes for OTHER executor entries (benign: at most occasional redundant DB syncs from later forks) - Corrupt cache file is tolerated — falls through to DB After this PR, hook forks have ZERO DB calls in the steady state for already-reconciled sessions (only on actual rotation or first capture). Validation: 18/18 src/hooks/__tests__/session-sync.test.ts pass (14 prior + 4 new: persist-after-reconcile, cold-start-loads-from-disk, cache-miss- falls-through, corrupt-file-tolerated). tsc --noEmit clean. biome clean. Fix E of the 5-step .19 Mac-CPU root-cause plan (A→E): - A: shipped #1475 — drop runRetention from getConnection - filewatch: shipped #1474 — chokidar replacement - C: shipped #1476 — GENIE_SKIP_DB_BOOT for hook dispatch - D: open #1479 — narrow inject matchers - E: this PR - B: in flight (twin) — needsSeed mtime cache (lower priority post-C) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks): skip disk cache I/O in test mode (post-rebase fix-up) After rebasing fix E onto current dev (post #1479/#1482/#1483/#1484 merges), the pre-existing session-sync (Gap 2) tests started failing because loadDiskCache() on first call would read the production ~/.genie/cache/session-sync.json — populated by earlier real-mode runs of the handler. Stale (executorId, sessionId) entries caused the in-memory cache to short-circuit BEFORE the test fixtures' mocked DB calls were made, so audit-event emissions never fired. Fix: loadDiskCache() and persistDiskCache() now skip ALL disk I/O when: - ANY _deps field is non-null (test installed mocks), OR - NODE_ENV=test or BUN_ENV=test UNLESS the test explicitly set __GENIE_SESSION_SYNC_CACHE_FILE via _setCacheFileForTest (the new fix-E tests opt in this way). Mirrors the existing test-mode skip in shouldSkipSync. Validation: 18/18 src/hooks/__tests__/session-sync.test.ts pass. Both the 14 pre-existing tests and the 4 new fix-E tests work — pre-existing tests get clean cache state; fix-E tests use their isolated tmp-dir cache file. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…lowup) dog-fooder-da66 verdict 2026-04-29 found a THIRD injection layer with the same wide-matcher leak after my first commit fixed two: 1. inject.ts:injectIntoFile (claude team settings.json) — fixed in #1479 2. provider-adapters.ts:buildSettingsObject (claude inline --settings) — fixed in this PRs first commit 3. codex-inject.ts:buildCodexHookFragment (~/.codex/config.toml) — STILL leaking SessionStart, PermissionRequest with matcher='*' even though no handlers exist Fix: - New CODEX_DISPATCHED_EVENT_MATCHERS in hooks/types.ts mirrors the claude DISPATCHED_EVENT_MATCHERS but is wider — codex has handlers for UserPromptSubmit (codex-inbox-deliver) and Stop (runtime-emit-assistant-response) that claude does not have. - Codex matchers: PreToolUse:*, PostToolUse:SendMessage, UserPromptSubmit:*, Stop:* - Dropped SessionStart + PermissionRequest (no handlers). - codex-inject.ts:CODEX_DISPATCHED_EVENTS now derived from the matcher map so list and matchers can never drift. - Test updated to assert the new contract + added matcher narrowing assertion. All 3 injection layers now sourced from the same matcher maps. Validation: 7/7 codex-inject tests pass. tsc clean. biome clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…S too (Fix D completion) (#1513) * fix(spawn): inline --settings hooks must use DISPATCHED_EVENT_MATCHERS too (Fix D completion) dog-fooder-da66 verdict 2026-04-29 surfaced that #1479 (Mac CPU Fix D — narrow inject matchers) only narrowed ONE of TWO injection layers. The team-level ~/.claude/teams/<team>/settings.json injection done by inject.ts:injectIntoFile was correctly narrowed. But provider-adapters.ts:buildSettingsObject — which emits an INLINE --settings JSON to the claude CLI command at spawn time — still hardcoded the wide matchers: PreToolUse: matcher='*' (correct, has handlers) PostToolUse: matcher='*' (WRONG, only SendMessage handler exists) UserPromptSubmit: wired (WRONG, no handlers in DISPATCHED_EVENT_MATCHERS) Stop: wired (WRONG, same) Net effect: every spawn-time --settings injection still wired the wasted matchers, so claude sessions still spawned bun-fork hook dispatchers for PostToolUse:Bash/Read/Write/Edit, UserPromptSubmit, and Stop — all going through the F1 fallback path with no useful work. Fix: source DISPATCHED_EVENT_MATCHERS from hooks/types.js (lazy require to avoid import cycles) and iterate it to build the hooks object. Both layers now stay aligned automatically when the matcher map is updated. Empirical (post-fix): bun run src/genie.ts spawn engineer --provider claude --model opus --no-interactive → emitted --settings now contains PostToolUse:[{matcher:"SendMessage"}] → UserPromptSubmit and Stop entries removed entirely Evidence (from da66 verdict): /home/genie/workspace/agents/genie/.genie/agents/dog-fooder/state/phaseb-413-20260429T143731Z/ Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks): codex inject also narrowed — Fix D third layer (#1513 followup) dog-fooder-da66 verdict 2026-04-29 found a THIRD injection layer with the same wide-matcher leak after my first commit fixed two: 1. inject.ts:injectIntoFile (claude team settings.json) — fixed in #1479 2. provider-adapters.ts:buildSettingsObject (claude inline --settings) — fixed in this PRs first commit 3. codex-inject.ts:buildCodexHookFragment (~/.codex/config.toml) — STILL leaking SessionStart, PermissionRequest with matcher='*' even though no handlers exist Fix: - New CODEX_DISPATCHED_EVENT_MATCHERS in hooks/types.ts mirrors the claude DISPATCHED_EVENT_MATCHERS but is wider — codex has handlers for UserPromptSubmit (codex-inbox-deliver) and Stop (runtime-emit-assistant-response) that claude does not have. - Codex matchers: PreToolUse:*, PostToolUse:SendMessage, UserPromptSubmit:*, Stop:* - Dropped SessionStart + PermissionRequest (no handlers). - codex-inject.ts:CODEX_DISPATCHED_EVENTS now derived from the matcher map so list and matchers can never drift. - Test updated to assert the new contract + added matcher narrowing assertion. All 3 injection layers now sourced from the same matcher maps. Validation: 7/7 codex-inject tests pass. tsc clean. biome clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Fix D of the 5-step .19 Mac-CPU root-cause plan. Eliminates wasted
buncold-starts caused by hook-event matchers that fire dispatchers for events with zero handlers.Root cause
src/hooks/inject.ts:buildHooksConfig()wired the teamsettings.jsonwith{ matcher: '*' }for every event inDISPATCHED_EVENTS:PreToolUse✓ — has 9 handlers, broad coverage justifiedPostToolUse✗ — only 1 handler (runtime-emit-msgwith matcher/^SendMessage$/); wiring*meant Bash/Read/Write/Edit post-uses each spawned a uselessbunforkSessionStart✗ — 0 handlersSessionEnd✗ — 0 handlersTeammateIdle✗ — 0 handlersTaskCompleted✗ — 0 handlersOn a busy Mac dev machine this multiplied the cold-start fanout that fixes A and C already addressed.
Fix
types.ts— replace flatDISPATCHED_EVENTSarray withDISPATCHED_EVENT_MATCHERSmap. OnlyPreToolUse: '*'andPostToolUse: 'SendMessage'are wired.DISPATCHED_EVENTSretained as a derived array for backward-compat (other consumers unchanged).inject.ts—buildHooksConfig()uses per-event matchers.injectIntoFile()refactored into single-responsibility helpers (readSettings,allEventsAlreadyInjected,hasNoObsoleteGenieEntries,pruneObsoleteGenieEntries,refreshMatcherEntries,upsertGenieEntry).SessionStart/SessionEnd/TeammateIdle/TaskCompletedfrom pre-fix-D installed settings.json. User-defined hooks under those events are PRESERVED — only genie's own dispatch entries are pruned.*→SendMessage).Validation
bun test src/hooks/__tests__/inject.test.ts— 10/10 pass (6 prior + 4 new):DISPATCHED_EVENT_MATCHERS only wires events that have handlersPostToolUse is wired with SendMessage matcher (not '*')injectIntoFile prunes obsolete genie entries on re-injectinjectIntoFile preserves user-defined hooks under obsolete eventsbun x tsc --noEmitcleanbun x biome checkclean (no complexity warnings, all formatting auto)Where this fits in the .19 Mac-CPU sprint
needsSeedmtime-marker cache (lower priority post-C since hooks now skip seed entirely)