fix: make hook registration idempotent#198
Conversation
jcfischer
left a comment
There was a problem hiding this comment.
Review: CodeQuality + Architecture
Lenses applied: CodeQuality, Architecture
Analysis
Clean, well-scoped fix for #183 (duplicate hook registrations from arc install/upgrade).
Core change: registerHooks shifts from append-with-skip to filter-then-push (replace-by-tag). Three small helpers (matcherKey, hookGroupHasCommand, shouldReplaceHookGroup) keep the predicate readable and testable.
Correctness verified:
shouldReplaceHookGroupmatches on(matcher, command, pkg)— replaces same-pkg entries AND legacy untagged entries (_pai_pkg === undefined), preserves entries from other packages. ✓- Existing "deduplicates identical hook commands" test still holds: filter removes the old entry, push adds fresh → net 1. Idempotent. ✓
- New test covers the exact scenario from #183: multiple untagged duplicates + one tagged + different matcher + different package → collapsed to 3 entries (OtherPkg/Bash, Cortex/Bash, Read). ✓
matcherKeynormalizesundefined→""via nullish coalescing (also handlesnullfrom malformed JSON). ✓
Minor behavior change (acceptable): Old code was a no-op on re-register (skip if found). New code always removes-then-pushes, rewriting the entry. End state is identical; settings.json gets touched on every install even when unchanged. Not a problem — arc install already rewrites settings.json unconditionally.
No findings. blockers=0 majors=0 nits=0 — recommend: approve ✅
Summary
Fixes #183.
Verification
rtk bun test test/unit/hooks.test.tsrtk bun test test/commands/remove-hooks-137.test.tsrtk bun test test/commands/install.test.tsrtk bunx tsc --noEmitrtk bun run lintrtk bun test