fix(wallet): use -p flag so npx resolves the correct binary

[Makabeez]

Problem

Every kh wallet subcommand that wraps @keeperhub/wallet fails with:

could not determine executable to run

The error is reproducible on any machine with Node installed. Root cause: @keeperhub/wallet publishes three bins — keeperhub-wallet, keeperhub-wallet-hook, keeperhub-wallet-mcp — none named wallet. When npx receives only the package name (npx @keeperhub/wallet <subcmd>), it tries to infer the binary from the package name, finds no match, and exits.

This affects kh wallet info, kh wallet fund, kh wallet add, kh wallet link, and kh wallet feedback. The error message gives no indication that the issue is binary name inference rather than a missing Node installation.

Fix

// Before:
childArgs := append([]string{"@keeperhub/wallet", subcmd}, args...)

// After:
childArgs := append([]string{"-p", "@keeperhub/wallet", "keeperhub-wallet", subcmd}, args...)

-p @keeperhub/wallet names the package to install/use; keeperhub-wallet names the exact binary. npx resolves correctly and kh wallet info works.

Verification

Tested on Ubuntu/WSL2 (Node v24.13.0, npm 11.12.1):

$ kh wallet info
subOrgId: a4a168e7-08ff-44d7-af8a-87eb68e73d44
walletAddress: 0xe1dC20C4Ed70b281441FA9b8324168DF4bdC11be

All existing wallet tests pass. One-line diff, no behaviour change beyond fixing the broken invocation.

How it was found

Found while running kh wallet fund during a live onboarding session — not from reading code. The workaround (keeperhub-wallet fund via the global npm install) led to discovering that the binary names didn't match what npx was trying to infer.

Related

• Doc PR docs: fix three onboarding friction points for Linux/headless installs #88 added a Node.js prerequisite note to kh_wallet_info.md, treating this as an undocumented dependency. That diagnosis was wrong — the dependency is documented in the synopsis; the actual bug is binary name inference. docs: fix three onboarding friction points for Linux/headless installs #88 is still useful as a fallback note but this PR fixes the root cause.

[Makabeez]

Related: #88 (docs PR that initially misdiagnosed this error as a missing Node install — amended to drop the incorrect mapping now that the root cause is identified here).

[joelorzet]

Verified the root cause independently rather than taking the report at face value. npm view @keeperhub/wallet bin returns exactly the three you listed:

{
  'keeperhub-wallet': 'bin/keeperhub-wallet.js',
  'keeperhub-wallet-mcp': 'bin/keeperhub-wallet-mcp.js',
  'keeperhub-wallet-hook': 'bin/keeperhub-wallet-hook.js'
}

None is named wallet, so npx has nothing to infer from and the failure is unconditional rather than environment specific. npx -p <package> <binary> is the correct form, and the comment explaining why earns its place, because the next person to read that line would otherwise assume the package name alone was sufficient.

On your branch: builds, go vet clean, go test ./cmd/wallet/... passes, and go generate ./docs/ leaves no diff, so docs-check is satisfied.

Two changes requested before this merges.

There is no test asserting the argv. execCommand is already a package level var whose comment says it exists "so tests can override with a fake builder", but nothing overrides it, so the exact bug this fixes was never covered and a future regression would not be caught either. Worth adding while the context is fresh. The current tests live in package wallet_test and execCommand is unexported, so the override needs an in package test file declaring package wallet, capturing the args and asserting the first three are -p, @keeperhub/wallet, keeperhub-wallet.

Separately, Long on these commands still reads "Thin wrapper around npx @keeperhub/wallet info", which now describes the old invocation. If you touch that string, note that TestNewAddCmd_Help asserts npx @keeperhub/wallet appears in the help output, and docs/ is generated from those strings, so it needs go generate ./docs/ in the same commit.

The diagnosis holds up and this restores five broken subcommands, so the substance is not in question. The test is the one I would insist on: a one line argv fix with no argv assertion is exactly the shape that silently regresses.

[Makabeez]

Heads up on merge order: #88 and #89 both touch the Long on cmd/wallet/wallet.go (Node prerequisite here, invocation string there) and both regenerate docs/kh_wallet.md, so whichever lands second will conflict. Happy to rebase the loser onto main and re-push as soon as the first one merges — just say which order you'd prefer.

[suisuss]

What changed since the last review

• cmd/wallet/agentic_wrapper_internal_test.go added: package wallet (in-package, not wallet_test), overrides execCommand/lookPath, calls runNpxWallet directly, and asserts argv[0..2] == [-p, @keeperhub/wallet, keeperhub-wallet].
• Long strings updated in add.go, feedback.go, fund.go, info.go, link.go, wallet.go to read npx -p @keeperhub/wallet keeperhub-wallet <subcmd>.
• TestNewAddCmd_Help in agentic_wrapper_test.go split into two assert.Contains checks (@keeperhub/wallet, keeperhub-wallet) since the strings are no longer contiguous after the -p insertion.
• docs/kh_wallet*.md regenerated to match the new Long strings.

Previously raised items

• Missing argv assertion for the fix - addressed. agentic_wrapper_internal_test.go:TestAgenticWrapperInvokesExplicitBinary overrides execCommand/lookPath and asserts the first three argv entries.
• Long strings stale + TestNewAddCmd_Help / docs out of sync - addressed. All six command files updated, help-text test split correctly, docs/kh_wallet*.md regenerated in the same commit.

Blocking

None.

Mechanical - actionable as-is

• agentic_wrapper_internal_test.go:23 - the fake execCommand returns exec.Command("true") and runNpxWallet calls .Run() on it. true is not a real binary on Windows, and goreleaser (.goreleaser.yaml) ships a windows build. CI (ci.yml) only runs test on ubuntu-latest so this won't fail in CI, but go test ./... will fail for anyone running it on Windows. A cross-platform no-op (or skipping the .Run() failure path since only argv is being asserted) avoids the platform dependency.

Needs a decision

None.

Verdict

Approve. Both items from the prior review are resolved with no new issues introduced by these commits.