fix(skill-install): resolve hook command at install time so npx flow works#20
Merged
fix(skill-install): resolve hook command at install time so npx flow works#20
Conversation
…works The README's recommended install path is `npx @keeperhub/wallet skill install`, which does not put the bin on the system PATH. Previously the installer wrote a bare `keeperhub-wallet-hook` command into ~/.claude/settings.json, so every subsequent tool use fired `command not found` because /bin/sh could not find the bin in the npx cache. Resolve at install time: - If keeperhub-wallet-hook is on PATH (global install or `npm link`), keep the bare command for lowest startup latency. - Otherwise write `npx -y -p @keeperhub/wallet keeperhub-wallet-hook` so the hook resolves regardless of where future shells run. The KEEPERHUB_WALLET_HOOK_COMMAND env var overrides resolution for tests, monorepo bin paths, and unusual deployments. Idempotency: the de-dup marker (`keeperhub-wallet-hook` substring) survives both forms, so re-running skill install across a global -> npx transition (or vice versa) replaces the existing entry rather than appending. Tests: - Existing registerClaudeCodeHook tests now inject hookCommand explicitly so assertions stay deterministic regardless of whether host PATH happens to carry the bin. - New tests cover the npx-form write path and bare-vs-npx de-dup transition. - New resolveHookCommand suite covers env override and PATH-probe fallback. - e2e suite pins KEEPERHUB_WALLET_HOOK_COMMAND in beforeEach so CI runs identically whether or not the test host has @keeperhub/wallet linked. README: documents the bare-vs-npx decision and the env override. index.ts: re-exports resolveHookCommand and RegisterClaudeCodeHookOptions. Repro: cleared global node_modules, ran npx @keeperhub/wallet skill install, saw `PreToolUse:Read hook error: keeperhub-wallet-hook: command not found` on every Claude Code tool call until the entry was hand-edited to use npx.
…feedback) Address two findings from the PR #20 security/edge-case review: 1. Pin the npx fallback to the installer's own version `npx -y -p @keeperhub/wallet keeperhub-wallet-hook` would pull `latest` on every PreToolUse hook fire. Any future compromise of the `@keeperhub/wallet` scope on the npm registry would then be executed on every tool call by every npx-installed user. At install time, read the package's own version from package.json and emit `npx -y -p @keeperhub/wallet@<version> keeperhub-wallet-hook`. Pinning bounds the supply-chain blast radius to "code that was already trusted enough to install" and makes upgrades explicit (re-run skill install from a fresh `npx @keeperhub/wallet@<new>`). Falls back to `latest` only if package.json cannot be located, which should never happen in published builds (dist/ sits next to package.json via pkg.files). 2. Narrow the idempotency marker to the `command` field Previously the de-dup match used JSON.stringify(entry).includes(marker), so a foreign hook whose `matcher` or args mentioned `keeperhub-wallet-hook` (a logger, wrapper script, audit tag) would be silently dropped on re-install. Now it inspects only `command`, which is the only place we ever write the marker. Both forms (bare bin and pinned npx) include the marker in `command`, so de-dup still survives global ↔ npx ↔ version-bump transitions. Tests - Pin: forces npx fallback by pointing PATH at /var/empty and asserts the emitted command contains @keeperhub/wallet@<semver>, not @latest. - Marker: a hook whose `matcher` mentions the bin but whose `command` is unrelated must survive re-install (length=2, both commands present). - Marker inverse: a hook whose `command` references the bin under any matcher must still be replaced (length=1). README - Document the version-pin rationale and the upgrade path. - Document that the de-dup matcher only inspects `command`. - Note the env override is trusted input (written verbatim to settings).
3 tasks
Reviewer follow-up on PR #20: the previous de-dup compared JSON.stringify(entry).includes(marker), then stopped looking. The follow-up commit narrowed the inspection to entry.hooks[].command but still dropped the WHOLE PreToolUse element if any of its hooks[] siblings referenced the keeperhub bin. That edge case bites a user who hand-merged our hook into one of their own PreToolUse elements: { matcher: "*", hooks: [ { type: "command", command: "/usr/local/bin/audit-logger" }, { type: "command", command: "keeperhub-wallet-hook" } ] } On re-install, the audit-logger sibling vanished silently. Probability is low (most users let the installer manage its own element) but the failure mode is data loss. Fix - Replace entryReferencesKeeperhubHook(entry): boolean with filterKeeperhubHooksFromEntry(entry): unknown that: - Returns the entry byte-identical when no hooks[] item references the bin (no churn for unrelated entries). - Returns null when every hooks[] item was ours (drops the whole element, no empty {matcher, hooks: []} shell). - Returns a shallow-cloned entry with the matching hooks[] items filtered out otherwise (preserves matcher and any other fields). - The de-dup loop now pushes the survivor (or skips when null) instead of all-or-nothing on the original entry. Tests - "preserves foreign sibling hooks[] inside the same PreToolUse element on re-install" — seed an element containing audit-logger + old kh hook, register, assert audit-logger survives in element[0] and the refreshed kh hook lands in element[1]. - "drops the whole PreToolUse element only when every hooks[] item was ours" — seed an all-keeperhub element with mixed bare + npx forms, register, assert no leftover {matcher, hooks: []} shell. - All 134 prior tests still pass: 136/136. Idempotency, transition coverage, and version pinning from earlier commits all unchanged.
This was referenced May 2, 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
1 participant
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.
Summary
npx @keeperhub/wallet skill install, which doesn't put the bin on PATH; the installer was writing a barekeeperhub-wallet-hookcommand into~/.claude/settings.json, so every tool call afterwards firedcommand not found.npm link), elsenpx -y -p @keeperhub/wallet keeperhub-wallet-hook.KEEPERHUB_WALLET_HOOK_COMMANDfor tests, monorepo bin paths, and unusual deployments.skill installacross a global ↔ npx transition replaces the existing entry rather than duplicating.Repro
@keeperhub/walletis NOT installed globally (or run with a cleanPATH).npx -y @keeperhub/wallet skill install— installer writes barekeeperhub-wallet-hookinto~/.claude/settings.json.PreToolUse:* hook error: /bin/sh: keeperhub-wallet-hook: command not found.Test plan
pnpm type-checkcleanpnpm test— 131/131 pass (5 new unit tests + 1 patched e2e setup)node -e "import('./dist/index.js').then(m => console.log(m.resolveHookCommand()))"returns the npx form on a host without global installTest additions
tests/unit/skill-install.test.tshookCommandexplicitly so assertions stay stable across host PATH statedescribe("resolveHookCommand()")block covering env override and PATH-probe fallbacktests/integration/skill-install-e2e.test.ts— pinsKEEPERHUB_WALLET_HOOK_COMMANDinbeforeEachfor deterministic e2e assertionsNotes
Discovered while wiring the wallet into a Claude Code session to demo paying for a marketplace workflow. Once the hook command was patched to the npx form by hand, the rest of the auto-pay loop worked end-to-end (paid $0.05 USDC on Base, balance dropped exactly $0.05).