Skip to content

Confirm every destructive verb, add account delete, and close the test holes - #9

Merged
siliconwitch merged 4 commits into
mainfrom
dev
Aug 13, 2026
Merged

Confirm every destructive verb, add account delete, and close the test holes#9
siliconwitch merged 4 commits into
mainfrom
dev

Conversation

@siliconwitch

Copy link
Copy Markdown
Member

Ran the same audit over the CLI that the server got. The code was in good shape; the tests were not.

Destructive verbs

CLAUDE.md says "the destructive verbs ask for interactive confirmation before acting", but only fleet delete did. key revoke and member remove now confirm too, and both name what they are about to do rather than echoing back the id that was typed:

Revoke "production"? Anything still using it stops reaching the fleet. [y/N]
Take away member@example.com's access to "pilot"? [y/N]

Catching a mistyped id is the point — revoking the wrong key kills a live integration and the secret is only ever shown once — so naming the key needed the key fetch lifted out of key list into keys.go, matching fleets.go and balances.go. That block was a verbatim copy anyway.

account delete

Confirms, relays the server's refusal while a fleet is still owned, and removes the stored login once the account is gone. Driven end to end against a live server: blocked with an actionable message, login survives the refusal, declining changes nothing, confirming cascades server-side and clears the local key.

Test holes

Each of these is a mutation that passed the suite before and fails now:

Mutation
Nine commands drop the credential entirely nothing asserted Authorization travelled
fleet delete ignores a server refusal it would print Deleted "crew". after a 403
fleet list swaps owner/member labels output was never asserted
fleet list drops its header row "
member list ignores which fleet you asked for member list 7 could list fleet 1
resolve collapses all arguments into one compared after joining with a space
a built command loses its run wiring silently reverts to "not implemented yet"

The credential one was closed by wrapping loggedInTestServer, which covers all nine commands at once. The wiring one is now a list of deliberately-unimplemented commands, so building one forces the list to change.

Smaller fixes

  • --json was advertised globally but rejected by eleven of the fifteen implemented commands; fleet create demo --json errored with "takes one name", never mentioning the flag the help had just promised. Moved into each command's arguments so the table stays the single source of truth, and the usage line stops promising flags.
  • member list --json printed the server's raw bytes with no trailing newline, gluing consecutive outputs onto one line.
  • A data race, reproduced with -race: login's stdin goroutine outlives the read and touches os.Stdin after something else may have replaced it. No -race CI job added, since that needs cgo and the operational principles forbid it.
  • The AUR key was generated inside the checkout of a public repo whose CLAUDE.md opens with "never commit secrets". Now cd "$(mktemp -d)" first.
  • GoReleaser gains replace_existing_artifacts: true; mode: keep-existing only governs the release body, so the re-run CLAUDE.md explicitly contemplates died on duplicate asset upload.

Version

0.0.3, not 0.0.4. The const was bumped twice since v0.0.2 shipped and 0.0.3 was never tagged, so the next release above the latest tag is 0.0.3. The server gate matches.

Release ordering: this has to be tagged and published before the server, or a 0.0.3 gate refuses every existing v0.0.2 client on every route including /login.

…e test holes

key revoke and member remove now confirm on stdin like fleet delete, which
CLAUDE.md already required of every destructive verb. Both name what they are
about to do rather than echoing back the id that was typed, since catching a
mistyped id is the point: revoking the wrong key kills a live integration and
the secret is only ever shown once. Naming the key needed the key fetch
lifted out of key list into its own file, matching fleets.go and balances.go.

account delete is wired up: it confirms, relays the server's refusal while a
fleet is still owned, and removes the stored login once the account is gone.

The help advertised --json globally though eleven of the fifteen implemented
commands reject it, so it moves into each command's own arguments and the
usage line stops promising flags. member list --json printed the server's raw
bytes with no trailing newline, unlike the other three.

login's stdin goroutine outlived the read and touched os.Stdin after something
else could have replaced it, which the race detector reports.

The tests had holes wide enough to drive through: nothing asserted the stored
key travelled, so nine commands could drop it and stay green; no command drove
a non-success status; fleet list and member list asserted nothing about their
output, and member list never checked which fleet it asked for. resolve was
compared after joining, so collapsing every argument into one string passed.
The version const drops to 0.0.3, the next release above v0.0.2.
The version drifted to 0.0.4 while v0.0.2 was the newest release, because
CLAUDE.md carried two contradictory rules: an operational principle saying the
dev version is "the next 0.0.x above the latest release tag", which bumps once
per cycle, and this file's Releases section plus the README saying "bump it,
then tag the commit that bumped it", which bumps once per release. Following
both bumps twice per release, and two cycles shipped no release.

The single rule replaces both: the version moves one step per release, and is
the version the next release will carry. A "Development cycle" section, shared
verbatim with the server repo, writes the flow down, including that dev is
created fresh from main every cycle and never resumed. Merging is squash-only
and GitHub already deletes the head branch, so a resumed local dev keeps the
pre-squash commits and conflicts on every line; that is what happened.

ci.yml now refuses a version more than one step past the latest release tag,
which would have caught the 0.0.4 state. Prerelease tags are skipped so that
"latest release" means the same here as it does to the server's deploy guard,
which reads it from the releases API: without that filter an rc that never
became a real release lets the const drift past it. Tested against eleven
cases including the exact bug.

The README's ruleset step gains squash-only merging, which the server's
already had and which the never-resume rule depends on, and both READMEs now
enable automatically deleting head branches.

OPEN, for the next pass. The README's Releasing section lost its bump step,
because under the rule as written the bump has already happened at the start
of the cycle. That is internally consistent but it is the half of the design I
would revisit: bumping at release time instead, in a release pull request, is
driftless by construction, keeps the server's gate from ever leading the
published CLI, and lets server-only releases happen without a CLI release. If
that is taken, the bump step comes back here and the operational bullet moves
with it. Decided separately: the server keeps refusing outdated CLIs with 426
rather than sending an advisory header, because a wire break is hard to
predict. Polling GitHub for the latest release from the CLI was considered and
rejected: this binary has no external dependencies at all, and a second
hostname brings rate limits, proxy failures and a packaging objection.
@siliconwitch

Copy link
Copy Markdown
Member Author

Handoff: one open decision before merge

A third commit landed adding the version guard and the cycle docs. Reasoning below, including the part I'd change.

Why the version drifted to 0.0.4

Not a slip. CLAUDE.md carried two contradictory rules: an operational principle saying the dev version is "the next 0.0.x above the latest release tag" (bump per cycle), and this repo's Releases section plus README saying "bump it, then tag the commit that bumped it" (bump per release). Following both bumps twice per release. 7b1e251 opened 0.0.3, a7a952f opened 0.0.4, and 0.0.3 never shipped.

The open decision

The README's "Releasing" lost its bump step, because under the rule as written the bump already happened at the start of the cycle. Internally consistent, but it's the half I'd revisit.

Bumping at release time instead, in a release PR, is driftless by construction and stops the server's gate ever leading the published CLI. That collision is live in the server PR right now: the gate mirrors the const, the const is bumped at cycle start, so the gate sits above the newest published CLI for most of every cycle, and the server's new deploy guard correctly refuses to deploy in that state. If bump-at-release is taken, the bump step comes back into this README and the operational bullet moves with it.

I didn't just make the change because Raj wanted a second opinion on it.

Decisions already taken, don't re-litigate

  • The server keeps refusing outdated CLIs with 426, rather than sending an advisory header. A wire break is hard to predict, so a hard refusal is the safer default.
  • Polling GitHub from the CLI for the latest release was rejected. This binary has no go.sum at all — zero external dependencies, one hostname. A second host brings unauthenticated rate limits (60/hr per IP, which a shared NAT or CI runner burns through), proxy failures, a cache file and staleness policy, and a packaging objection: AUR and nixpkgs reviewers are specifically hostile to binaries that phone home.
  • Server and CLI versions stay separate. The server is deployed by date tag; this is distributed through Homebrew, Scoop, the AUR and the flake. Unifying pushes empty updates through all four on every server-only change.

On the drift check

It filters prerelease tags so "latest release" means the same here as it does to the server guard, which reads it from the releases API. Without the filter, an rc that never became a real release lets the const drift past it — verified. Tested against 11 cases including the 0.0.4 bug, using the script extracted from the YAML with yq rather than a hand transcription.

@siliconwitch
siliconwitch merged commit e8aa636 into main Aug 13, 2026
1 check passed
@siliconwitch
siliconwitch deleted the dev branch August 13, 2026 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant