feat(cli): typed error taxonomy and documented exit codes - #358
Merged
blockchain-maxis merged 8 commits intoSep 2, 2026
Merged
Conversation
Every CLI issue is blocked on this module existing. Add cli/ as a standalone Go module (go 1.25, no cgo) alongside the pnpm workspace, building a signet binary via cobra: cmd/signet/main.go is the entrypoint, internal/cmd holds the command tree, and internal/link, internal/keys, internal/spec are placeholder packages for the wallet-linking, key-management, and API-spec work that follows. signet --version carries a version and commit string injected via ldflags. signet --help and a bare `signet` invocation both print real usage text — cobra only renders the Usage: section when a command is Runnable() or has subcommands, so the root command needed an explicit RunE (falling through to cmd.Help()) to show it before any subcommand exists. golangci-lint is configured via cli/.golangci.yml, and a new CI job builds, vets, tests, and lints the module. Verified go build/go vet/go test all pass, and the binary cross-compiles with CGO_ENABLED=0 for linux/amd64 and darwin/arm64.
…entity Self-hosters and testnet users need to point the CLI at a deployment other than the default, and repeat runs shouldn't re-ask which identity to use. Every invocation needing --url and --source made self-hosted deployments second-class. Add internal/config: Load/Save a JSON file under os.UserConfigDir()/ signet (baseUrl, source), and Resolve combines flag > env (SIGNET_URL, baseUrl only) > config file > built-in default per field. Wire --url/ --source as persistent flags on the root command, resolved in PersistentPreRunE and attached to the command context via config.WithResolved/FromContext for subcommands to read once they exist. An explicit --source is saved back to the config file (RememberSource) so the next run without it picks the same identity back up. No config file is required for the default deployment. Document the precedence and file location in cli/README.md, and add SIGNET_URL to check-docs.mjs's ENV_ALLOW list — it's the cli/ module's own shell env, not part of the pnpm workspace's .env.
CI pipelines that link a deploy wallet need to parse the result, and
without structured output automation scrapes human-formatted text
that's free to change between releases.
Add internal/link.Link(handle, publicKey, network), validating the
handle (mirroring HANDLE_PATTERN from packages/types) and the public
key's charset/length, returning a Result{Handle, PublicKey, Network,
Status}. This doesn't yet perform a real on-chain claim or call a
deployment's HTTP API — internal/keys and internal/spec, which it
would need for that, are themselves still scaffolded — but the output
contract is real and stable now.
Wire it up as `signet link <handle> --public-key ... [--network]
[--json]`. Without --json, prints a human summary to stdout. With
--json, encodes exactly one JSON object to stdout via encoding/json
and nothing else; on a validation error, stdout stays empty in both
modes and the error reaches the user via stderr (main.go's existing
error handling), never stdout.
Covered by cmd/link_test.go (asserting --json's stdout decodes as the
expected JSON and is exactly one line, non-json output is not JSON,
and an invalid input leaves stdout empty) and link/link_test.go (the
validation logic in isolation).
… hygiene The CLI orchestrates the developer's deploy identity and ships as a prebuilt binary with no preview deploy or browser console — a regression surfaces on a user's machine, not in a PR check. - internal/keys.ResolvePublicKey(binary, source) resolves a named local identity to its public key by shelling out to `stellar keys address <source>`, rather than the CLI owning key storage or signing itself. Tested against a real faked `stellar` binary (internal/keys/testdata /fakestellar, compiled on first use — the standard Go helper-binary pattern), not just a swapped-out Go function, so the actual exec.Command wiring is exercised. - internal/exitcode + internal/cmd.ExitCode(err) map an error to a process exit code (0 ok, 1 generic, 2 invalid input) via an ExitCoder interface — link.ValidationError is the first implementer. exitcode lives in its own leaf package so internal/link's error type and internal/cmd's classifier can both depend on it without a cycle (cmd already imports link for the `link` command). - secrets_test.go asserts no secret-shaped value (a Stellar S... key) ever reaches stdout, stderr, or an error string, across --json and non-json runs — which caught a real issue while writing it: the invalid-public-key error echoed the raw value back, which would leak a real secret key a user passed to the wrong flag by mistake. Fixed in link.go to report the shape problem without repeating the value. - go test -race now runs in CI (needs cgo — a C toolchain, which ubuntu-latest has — unrelated to the production binary's CGO_ENABLED=0 requirement, since the test binary is never shipped). Not runnable in this environment for lack of a local C compiler. - Config precedence (flag > env > config > default) and --json stdout purity were already covered by blockchain-maxis#262/blockchain-maxis#264's tests; unchanged here. Scoping note: this does not cover "loopback single-use and timeout behaviour" or a "state mismatch" — no loopback HTTP server exists in the CLI's architecture today (neither blockchain-maxis#269 nor blockchain-maxis#263 needed one; the CLI-link challenge exchange is a plain two-request HTTP round trip, not a browser callback flow), so there is nothing yet to write that test against. Documented here rather than fabricated.
The cli lane already ran go build/vet/test -race/golangci-lint on the pinned Go version (go-version-file: cli/go.mod), but the CLI is consumed as a prebuilt binary from a release, not from this repo — a build break for a platform nobody's local dev machine matches (e.g. linux/arm64 built from an amd64 box) would otherwise stay invisible until it broke on a stranger's machine. Add a smoke-build step covering linux/amd64, linux/arm64, darwin/arm64, and windows/amd64 — no cgo (per go.mod's requirement), so each target needs only GOOS/GOARCH env vars on the one ubuntu-latest runner already running the rest of the lane, no per-platform runner or cross toolchain. A deliberately broken build fails the step (and the workflow) same as any other `go build`. Verified the same four-target loop locally before adding it to CI.
The CLI delegates identity resolution and signing to the stellar CLI (keys ls, keys public-key, tx sign --sign-with-key), but assumed all of it silently. A missing or old stellar CLI produced a raw exec error or an unrecognized-flag message pointing at the wrong tool entirely — the dependency is free (a developer with no stellar has deployed no contracts and has nothing to link), but the failure has to say so. Add keys.CheckStellarCLI(binary): checks PATH via exec.LookPath, then parses `<binary> --version` against keys.MinimumStellarVersion (25.2.0 — where tx sign gained --sign-with-key and stdin support) via a resilient X.Y.Z extraction rather than an exact-format match. A missing binary (ErrStellarNotFound) and a too-old version (ErrStellarTooOld) are distinct sentinels, each producing an actionable message naming keys.StellarInstallURL and the required version. Extended the fake stellar binary (internal/keys/testdata/fakestellar, from blockchain-maxis#290) to also answer --version, controlled by a FAKESTELLAR_VERSION env var the tests set per case, so the checks run against a real subprocess rather than a stubbed function. Fixed a real, latent flakiness bug while adding more tests through the same helper: buildFakeStellar's cached binary path lived in a t.TempDir(), whose cleanup fires when the *first* test that built it finishes — every later test reusing the cached path was pointed at an already-deleted file. Moved the shared build directory to a plain os.MkdirTemp, cleaned up once in TestMain instead.
CLI failures were only distinguishable by scraping message text: no sentinel errors, no exit code beyond 0/1/2 (success/generic/invalid input). Scripts and CI wrapping this command need to branch on genuine failure classes without parsing output that's free to change between releases — the same principle blockchain-maxis#264's --json mode already applies to stdout. Extend internal/exitcode with seven sentinel errors — configuration, no identity, signing failure, network, timeout, approval rejected, already-linked conflict — each wrapped with %w at the point something actually fails and matched with errors.Is via exitcode.CodeFor, which internal/cmd.ExitCode now consults before falling back to the ExitCoder interface (link.ValidationError) and finally Generic. Configuration/NoIdentity/SigningFailure/Network/Timeout/ ApprovalRejected/AlreadyLinked map to codes 3-9; all are defined now so the commands that will raise the latter five (signing, talking to a deployment, an interactive approval flow) have a stable, already-documented code from day one rather than retrofitting the taxonomy once those land. Wired the two categories with a real caller today: - root.go's config-file read/save failures now wrap ErrConfiguration - keys.ResolvePublicKey's "identity not found" path wraps ErrNoIdentity; its "binary not on PATH" path, and both of check.go's ErrStellarNotFound/ErrStellarTooOld paths (blockchain-maxis#297), wrap ErrConfiguration too — a missing or wrong-version external dependency is an environment problem, not a property of the input. Codes documented in cli/README.md's Exit codes table. Covered by exitcode_test.go (each sentinel's code, matching through multiple %w layers, unrelated errors correctly excluded, no two sentinels sharing a code), extended cmd/exitcode_test.go (the taxonomy is consulted first; a real root.go config failure carries the documented code), and extended keys tests (ResolvePublicKey/CheckStellarCLI's errors carry the right sentinel). Verified end-to-end via the built binary: a real config-read failure exits 3.
|
@Otfrugger Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
✅ Deploy Preview for stellar-signet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@Otfrugger is attempting to deploy a commit to the blockchainmaxis-8449's projects Team on Vercel. A member of the Team first needs to authorize it. |
This was referenced Sep 1, 2026
blockchain-maxis
added a commit
to Otfrugger/signet
that referenced
this pull request
Sep 2, 2026
Conflict resolution against the squashed CLI PRs: took main's version of every cherry-picked file, and this branch's for the three it genuinely changes (cli/README.md, keys_test.go, testdata/fakestellar). CheckStellarCLI was implemented and well tested, but nothing called it — not this PR, and not blockchain-maxis#358, blockchain-maxis#359 or blockchain-maxis#369 either. Issue blockchain-maxis#297 asks for the check to run 'before doing any work', so as it stood the acceptance was unmet: a missing or too-old stellar would still surface as a raw exec error. Wired it into ResolvePublicKey, which is the function that shells out to `stellar`, so every caller inherits the guard by construction — including the identity resolution still to land in blockchain-maxis#369. That also made ResolvePublicKey's own ad-hoc LookPath fallback redundant, since CheckStellarCLI reports the missing-binary case with a better message (it names the install URL and the required version), so the fallback is gone. Added three tests for the wiring rather than the function: a too-old stellar fails ResolvePublicKey with an error wrapping ErrStellarTooOld even for an identity the fake resolves happily, a missing binary wraps ErrStellarNotFound, and a supported version still resolves normally. Without these, deleting the call would leave every existing test passing. go build/vet, gofmt, golangci-lint v2.13.2 (0 issues), go test -race ./... and check-docs all clean.
blockchain-maxis#357 landed while this was open, and this branch's cherry-pick of it predates the change made there — CheckStellarCLI is now actually called from ResolvePublicKey. Resolving by picking a side would have silently reverted that, so keys.go, check_test.go and keys_test.go combine both: - keys.go keeps main's CheckStellarCLI guard and gains this branch's exitcode.ErrNoIdentity wrapping on the paths that remain. - check_test.go / keys_test.go keep main's wiring tests and add this branch's sentinel-code assertions. Everything else is a clean pick: this branch's for the taxonomy itself (internal/exitcode, cmd/exitcode.go, cmd/root.go, keys/check.go, cli/README.md), main's for the files it only carried as cherry-picks. Verified the documented table against the binary rather than trusting it — drove cmd.ExitCode with each sentinel wrapped in an outer error: configuration 3, no identity 4, signing failure 5, network 6, timeout 7, approval rejected 8, already linked 9, nil 0, unclassified 1 which matches cli/README.md's table exactly. Also confirmed end to end that the built binary still exits 2 on invalid input and 0 on success. go build/vet, gofmt, golangci-lint v2.13.2 (0 issues), go test -race ./... and check-docs all clean.
blockchain-maxis
merged commit Sep 2, 2026
f75b3e2
into
blockchain-maxis:main
10 of 11 checks passed
blockchain-maxis
added a commit
to Otfrugger/signet
that referenced
this pull request
Sep 2, 2026
Everything except cli/internal/browser/ and its docs was a cherry-pick of blockchain-maxis#335/blockchain-maxis#336/blockchain-maxis#342/blockchain-maxis#345/blockchain-maxis#356/blockchain-maxis#357/blockchain-maxis#358, all now on main — took main's for all of those. scripts/check-docs.mjs keeps all three allowlist entries rather than either side's pair (WAYLAND_DISPLAY from this branch, CLI_RELEASE_ENABLED and NPM_TOKEN from blockchain-maxis#346). Verified the fallback behaves as issue blockchain-maxis#257 requires, by driving OpenOrPrint with DISPLAY and WAYLAND_DISPLAY unset: headless, browser allowed -> prints 'Open this URL to continue: <url>', returns nil --no-browser -> same so the opener's failure is absorbed rather than propagated, and both paths reach the developer at the same URL. Left unwired deliberately, unlike blockchain-maxis#357's CheckStellarCLI: there is no call site to attach it to yet — `signet link` performs no approval flow (that lands with the loopback server and the pairing endpoints), so wiring it now would mean inventing the flow rather than connecting to one. Both the package doc and cli/README say so plainly. go build/vet, gofmt, golangci-lint v2.13.2 (0 issues), go test -race ./... and check-docs all clean.
blockchain-maxis
added a commit
to manchesternews98-jpg/signet
that referenced
this pull request
Sep 2, 2026
This branch introduced its own internal/keys design (a Runner interface plus context-taking List/PublicKey/Resolve) while blockchain-maxis#345/blockchain-maxis#357/blockchain-maxis#358 built a different one on main — a package-level `run` seam, CheckStellarCLI version guard, exitcode sentinels, and secret redaction. Rather than pick a side and lose work, kept main's package and ported this PR's new capability onto it: - keys.List(binary) shells `stellar keys ls` through the same `run` seam and the same CheckStellarCLI guard, so a missing or too-old stellar is reported identically here. - keys.Resolve(binary, explicit, prompt) picks --source, else the sole identity, else the prompt; a nil prompt (CI, --json) yields ErrAmbiguousIdentity instead of hanging on stdin. Both new sentinels wrap exitcode.ErrNoIdentity, so they map to the documented exit code 4. - identity.go keeps its command and interactive selector, rewired to keys.Resolve/keys.ResolvePublicKey and registered on main's root (which carries blockchain-maxis#336's config resolution). - execrunner_test.go is dropped with the ExecRunner type it tested; it also declared a second TestMain, which broke the package build. - Ported six tests onto main's fixtures: List returns every identity, an explicit source never shells out, the sole identity is auto-picked, no identities and an ambiguous set each report their sentinel, and the prompt is offered the full list. Extended the fake stellar to answer `keys ls` (via FAKESTELLAR_IDENTITIES) and to resolve a second identity. golangci-lint flagged four unchecked writes in identity.go (errcheck); now checked, matching how link.go already returns its write error. Verified against the fake binary end to end: one identity resolves silently, several with no tty refuses rather than hanging, none reports the actionable message, and piping a selection picks that identity and prints its public key. Also grepped the whole cli/ tree — no `keys show`, no `--sign-with-key`, no secret parsing anywhere, which is what issue blockchain-maxis#253 is actually about. All 6 Go packages pass go test -race; gofmt, golangci-lint v2.13.2 (0 issues) and check-docs clean.
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
5 participants
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.
closes #259
Stacked on #335, #336, #342, #345, #356, #357 — cherry-picked (authorship preserved) since this needs the
cli/module, its exit-code mechanism (#290), and thestellarCLI checks (#297) to build on. Top commit (f0000cc) is the one to review.Summary
CLI failures were only distinguishable by scraping message text: no sentinel errors, and only two real exit codes beyond success (
1generic,2invalid input, from #290). Scripts and CI wrapping this command need to branch on genuine failure classes without parsing output that's free to change between releases — the same principle--jsonalready applies to stdout (#264).Changes
cli/internal/exitcode/exitcode.go: extended with seven sentinel errors covering the acceptance criteria's list —ErrConfiguration,ErrNoIdentity,ErrSigningFailure,ErrNetwork,ErrTimeout,ErrApprovalRejected,ErrAlreadyLinked— mapped to codes3–9viaCodeFor(err) (int, bool), which walks the table witherrors.Isso it matches regardless of how many times an error was wrapped with%won its way up.cli/internal/cmd/exitcode.go:ExitCodenow consultsexitcode.CodeForfirst, then the existingExitCoderinterface (link.ValidationError→2), thenGeneric.SigningFailure/Network/Timeout/ApprovalRejected/AlreadyLinked) are defined now with stable codes so the commands that will raise them (signing, talking to a deployment, an interactive approval flow) don't need the taxonomy retrofitted later, matching howinternal/keys/internal/specare already scaffolded ahead of their real implementation:cli/internal/cmd/root.go: config-file read/save failures now wrapErrConfiguration.cli/internal/keys/keys.go:ResolvePublicKey's "identity not found" path wrapsErrNoIdentity; its "binary not on PATH" path wrapsErrConfiguration.cli/internal/keys/check.go: bothErrStellarNotFound/ErrStellarTooOld(CLI must verify thestellarbinary is present and new enough #297) now also wrapErrConfiguration— a missing or wrong-version external dependency is an environment problem, not a property of what the user typed.cli/README.md: documents all 10 codes (0–9) and how the classification works (sentinel taxonomy →ExitCoder→ generic fallback).internal/exitcode/exitcode_test.go(new) covers each sentinel's code, matching through multiple%wwrap layers, an unrelated error correctly excluded from the taxonomy, and no two sentinels sharing a code. Extendedcmd/exitcode_test.go(the taxonomy is consulted, and a realroot.goconfig-file failure — not a synthetic error — carries the documented code) and theinternal/keystests (ResolvePublicKey/CheckStellarCLI's errors carry the right sentinel viaerrors.Is).Verification
go build ./.../go vet ./.../go test ./...— all pass (54/54 tests across the module, +10 new).golangci-lint run ./...— 0 issues.node scripts/check-docs.mjs— passes.signet link ...exits3withconfiguration error: reading config file: ...on stderr.