docs: fix three onboarding friction points for Linux/headless installs - #88
Conversation
|
SSH prerequisite companion PR: KeeperHub/claude-plugins#5 |
|
@suisuss Both blockers were fair. null becoming "null" was a regression I introduced, and the else branch was wider than the bug. Fixed: null returns early as a no-op per the Unmarshaler contract; acceptance narrowed to numbers via json.Number, which restores *json.UnmarshalTypeError for objects, arrays and bools. Also corrected the wrong "panics" comment, moved the test to package wallet_test to match its neighbours, and extended the table with null, object, bool and array cases across both ChainBalance and Token — you were right that those would have caught both blockers. Left the strings import in place; it's still used elsewhere in the file. On the marshal question: I'd lean toward (a) — emit a bare number when the value is numeric. It restores parity with kh chain list --json for the case that actually occurs and closes the jq footgun without a breaking change to that command. The cost you name is real, output shape following input shape, but for chainId the input is numeric in every response I've seen, so the string branch is a compatibility path rather than a live one. Happy to add it here or keep this PR scoped to the decode fix — your call. |
34eee3f to
ea3910b
Compare
|
Verified the Linux install snippet on Ubuntu/WSL2 (x86_64, GNU tar 1.34) — amended to add |
- quickstart: add Linux binary install one-liner for no-Homebrew/headless environments; the existing section was macOS-first with no path for remote servers or WSL boxes without Homebrew installed - quickstart: correct auth description from "opens a browser window" to device-code flow; headless users need to know codes expire (~2 min) and the URL can be opened on any machine - kh_wallet_info: add Node.js/npx prerequisite; the command shells out to npx @keeperhub/wallet but this dependency was undocumented, causing "could not determine executable to run" with no actionable guidance Found during a fresh Linux/WSL2 onboarding run on v0.13.1 (2026-08-06).
ea3910b to
b98abd7
Compare
|
Amended — the original note in The fix is in #89 ( Related: #89 (root cause fix) |
joelorzet
left a comment
There was a problem hiding this comment.
Two of the three changes hold up. One will fail CI as written, and one states a number that is off by an order of magnitude.
docs/kh_wallet_info.md is generated, so this edit gets dropped
docs/generate.go runs doc.GenMarkdownTree over the cobra tree, and the docs-check job runs go generate ./docs/ followed by git diff --exit-code docs/. Running that against this branch deletes your line:
$ go generate ./docs/ && git diff --stat docs/
docs/kh_wallet_info.md | 2 --
Checks have not run on this PR yet because a first time contributor PR needs workflow approval, which is why this has not surfaced on its own.
The place for the note is the Long string on the cobra command, followed by go generate ./docs/ in the same commit. That also puts it in kh wallet info --help, which is where someone who just hit the error is actually looking. Worth attaching to the parent kh wallet command rather than info alone, since fund, add, link and feedback shell out the same way.
Note that #89 changes the picture here: the "could not determine executable to run" error was binary name inference, not a missing Node. Node remains a real prerequisite worth documenting, so keep the note, just as a prerequisite rather than as the explanation for that error.
The two minute expiry is not right
The server configures the device authorization flow with expiresIn: "15m", so a code is good for fifteen minutes. Please drop the two minute figure or replace it with fifteen.
The rest of that paragraph is correct and worth having. I read internal/auth/device.go and there is no browser opening code anywhere in the path: it requests a device code, prints the URL and the user code, then polls. The sentence you are replacing was simply wrong.
While you are there, kh auth login's own Long says "Opens a browser to confirm a one-time code", and cmd/auth/auth.go carries an example commented "Log in via browser". Both repeat the error this PR is correcting, and both feed docs/kh_auth_login.md. Fixing quickstart alone leaves the wrong description in the command help, which is the copy most people meet first.
The install one-liner works
Ran it verbatim against the current release. It resolves the asset, extracts, and produces a valid statically linked linux amd64 ELF binary. Asset naming and archive layout both check out, and kh sits at the archive root, so the single member extract is correct.
One thing worth adding: the release publishes checksums.txt, and this snippet pipes an unverified binary straight into tar. Since this would become the install path we recommend to people on exactly the machines that cannot use Homebrew, it is worth verifying the download, or at minimum pointing at the checksums file.
Minor and separate: the unauthenticated GitHub API is rate limited to 60 requests an hour per IP, which tends to bite on shared CI addresses and fails in a way that looks nothing like rate limiting.
Requesting changes for the generated file and the expiry figure. The quickstart install section and the flow correction are both things this repo needs.
- Move Node.js prerequisite from kh_wallet_info.md (generated, wrong place)
to cmd/wallet/wallet.go Long so it covers all agentic subcommands at once
and regenerates cleanly into kh_wallet.md
- Fix two browser-error sources: auth.go Example comment ("via browser" →
device code description) and login.go Long (clarify browser does not open
automatically; code TTL is 15 minutes, not "roughly two minutes")
- Replace unverified install pipe with checksum-verified snippet:
downloads checksums.txt alongside the tarball, runs sha256sum --ignore-
missing, then extracts; add GitHub API rate-limit note (60 req/hr/IP)
- Regenerate docs/kh_auth*.md, docs/kh_wallet*.md, docs/quickstart.md
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Revised — four changes in this push:
|
|
Heads up on merge order: #88 and #89 both touch the |
What changed since the last review
Two of the six PR comments (2026-08-06 08:31 and 08:38, re: BlockingNone. Mechanical - actionable as-isNone. Needs a decisionNone. VerdictApprove - all four items from the prior review are fixed and verified against source/base, the install snippet runs clean end to end, and the increment touches only help-text strings and docs, no auth logic. |
What and why
Three documentation gaps hit during a fresh Linux/WSL2 onboarding run on v0.13.1 (2026-08-06). Each change is self-contained and fixable in docs without a code change.
1. Linux binary install —
quickstart.mdThe install section leads with Homebrew and lists "Binary download: see GitHub Releases" as a one-liner with no command. Users on headless Linux boxes (remote servers, WSL, CI) without Homebrew have no actionable path. Added a
curlone-liner that fetches the latest release automatically and extracts to~/.local/bin.2. Auth flow description —
quickstart.mdThis is wrong for any headless or remote environment — and it's wrong about the mechanism.
kh auth loginuses the device code flow: it prints a URL and a code; the user visits the URL on any machine. On a remote box, the browser never opens. Codes expire in roughly two minutes, and there's no retry hint.Changed to accurately describe the flow and tell headless users what to do when the URL doesn't open automatically.
3. Node.js/npx prerequisite —
kh_wallet_info.mdkh wallet info(and otherkh walletsubcommands) shells out tonpx @keeperhub/wallet. This dependency is unmentioned, so users without Node.js see:with no actionable guidance. Added a Prerequisite callout pointing to nodejs.org.
Related
Companion PR for SSH prerequisite in
claude-plugins: KeeperHub/claude-plugins#Full onboarding teardown (to be published with the carrydesk repo): covers these three items plus the
kh w balancechainIdtype mismatch fixed in #87.