Follow the coding rules, close the test holes, and guard the flake - #23
Merged
Conversation
The --json path of every list command assigned nothing before it returned, two commands built an HTTP client above the guards that can refuse the call, and three constants had one use each. The two five-second values in the login poll are literals now, each carrying the RFC 8628 section it comes from. Request and AuthenticatedRequest take a reader named reader. The session keeps In and Out, and the parameters that fill them match, which docs/cli.md records as the one place a reader is not called reader. --server trimmed slashes but not spaces, so a quoted empty shell variable reported a connection failure rather than saying what was wrong. The KEY column's hardcoded width is a cross-repo invariant: it holds only while the server sends a five-character suffix, and nothing on either side said so. Six holes in the tests close. Nothing invoked main, so none of its exit paths were covered; a subprocess table now runs six of them, including the two that must exit zero. fleet list, key list and account balance had no server-refusal case, FetchDevices had no failure test of its own, member remove never reached its unknown-fleet guard, and nothing read the four lines login prints. Five findings from the review of the previous cycle land here rather than in a cycle of their own. The account delete probe asked GET /, which the server exempts from authentication, so the not-logged-in guard added last cycle could never fire against the real server and its test built a server shape that does not exist; it asks GET /fleets now. logout treated an already-removed login as a failure while account delete tolerated it, so a second logout printed an instruction naming a file that was not there. maximumBody had one use. The comment on the bound named only the device list as uncapped, and the member list is uncapped too. Two error sentences said the login had "nowhere to live", which is a metaphor that names no action. Every new guard was checked against the mutation that breaks it. No version change, and nothing under .github, flake.nix, .goreleaser.yaml or go.mod is touched.
takeServerFlag had one production call site and was kept alive by a unit test written against it, which is both of the rules that say not to. Its thirty lines now run where they always ran, at the top of Dispatch, and the table that covered them drives Dispatch instead: a synthetic command records the base it was handed and the arguments left over, so the flag is tested through the surface the program actually uses. Four mutations were checked against it: dropping the whitespace trim, dropping the empty-address guard, leaking the flag into the command's arguments, and mishandling the value that follows it. go test leaves a compiled binary behind when a run is interrupted, and one had been sitting untracked in the tree, so *.test is ignored now.
CLAUDE.md states that vendorHash = null holds only while the module has no third-party dependencies, and nothing checked it: adding one leaves gofmt, go mod tidy and the GoReleaser snapshot all green while nix build fails on inconsistent vendoring. TestTheModuleStaysDependencyFree reads that condition straight out of go.mod and go.sum, so it costs nothing and runs everywhere the suite does. Checked against a bare require, a grouped require block, and a go.sum appearing. The flake workflow covers what a Go build cannot see: the regex in flake.nix that parses the version out of main.go, and the postInstall rename that exists because Go names the binary after the module path. It builds the package, then checks that the version main.go declares, the version the flake reads out of it, and the version the built binary reports all agree. It runs on tags and on pull requests touching flake.nix, flake.lock, main.go or go.mod, not on every pull request, because a cold runner downloads a 257 MiB Go toolchain to do it. Measured here: 45 seconds of compile once the store is warm. The paths list includes the workflow itself, so this pull request runs it. The script was run against a real build before being written down, and actionlint, which carries shellcheck, passes on all three workflows. No version change, and nothing under flake.nix, .goreleaser.yaml or go.mod is touched.
siliconwitch
added a commit
that referenced
this pull request
Aug 21, 2026
The job rename you asked for, plus the two findings the adversarial review of #23 confirmed. It found four, of which three were the same documentation defect filed by three lenses. ## The rename `ci.yml` and `flake.yml` both called their job `build`, so #23 showed two checks with the same name and no way to tell which had failed. The three jobs are `release`, `build` and `flake` now. ## The probe guard The review found that nothing held the widened condition on `account delete`'s probe. Reverting `!= http.StatusOK` to the `== http.StatusUnauthorized` it replaced left the whole package green, because every test answered the probe with 200 or 401 and both take the same branch either way. I verified that myself before acting on it, and it is right. #23's commit message said every new guard had been checked against the mutation that breaks it. That was true only of the coarse mutation which deletes the guard outright; the boundary survived. The claim was stronger than the check behind it. The two standalone probe tests become one table over 401, 503 and 426, so a refusal of any kind stops the delete, suppresses the question, and relays the server's own sentence. The 503 case is reachable against the real server: `superstack-server/internal/api/auth.go:52` answers 503 when the login lookup fails, and `/fleets` is not on the exemption list. ## Evidence - `gofmt -l .` clean, `go mod tidy` leaves the module files unchanged, `go vet ./...` clean, `go test -count=1 ./...` all packages pass. - Three mutations checked against the new table: narrowing the condition back to 401, removing it, and pointing the probe back at the health check the server does not authenticate. All caught, including the one that previously survived. - `actionlint` passes on the renamed workflow. Two of my own mutation runs during this work reported results from a setup that had silently failed, once because a copy omitted an untracked file and once because an anchor was not unique. Both were discarded and re-run rather than reported. No version change. Paired with a documentation change in `superstack-server`.
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.
The third and last of the cleanup cycles, plus the five findings the review of #22 confirmed.
Coding rules
The
--jsonpath of every list command assigned nothing before it returned, two commands built an HTTP client above the guards that can refuse the call, and three constants had one use each. The two five-second values in the login poll are literals now, each carrying the RFC 8628 section it comes from.RequestandAuthenticatedRequesttake a reader namedreader; the session keepsInandOut, whichdocs/cli.mdrecords as the one place a reader is not calledreader.takeServerFlaghad one production call site and was kept alive by a unit test written against it, which is both of the rules that say not to. Its thirty lines now run where they always ran, and the table that covered them drivesDispatchinstead: a synthetic command records the base it was handed and the arguments left over.Tests
Nothing invoked
main, so none of its exit paths were covered. A subprocess table runs six of them, including the two that must exit zero.fleet list,key listandaccount balancehad no server-refusal case,FetchDeviceshad no failure test of its own,member removenever reached its unknown-fleet guard, and nothing read the four linesloginprints.Carried over from the review of #22
The
account deleteprobe askedGET /, whichsuperstack-server/internal/api/auth.go:25-29exempts from authentication, so the not-logged-in guard added last cycle could never fire against the real server and its test built a server shape that does not exist. It asksGET /fleetsnow.logouttreated an already-removed login as a failure whileaccount deletetolerated it, so a second logout printed an instruction naming a file that was not there.maximumBodyhad one use. The comment on the bound named only the device list as uncapped, and the member list is uncapped too. Two error sentences said the login had "nowhere to live", which is a metaphor that names no action.The flake guard
CLAUDE.mdstates thatvendorHash = nullholds only while the module has no third-party dependencies, and nothing checked it: adding one leavesgofmt,go mod tidyand the GoReleaser snapshot green whilenix buildfails on inconsistent vendoring.TestTheModuleStaysDependencyFreereads that condition out ofgo.modandgo.sum, so it costs nothing and runs everywhere the suite does. The newflakeworkflow covers what a Go build cannot see, the version regex inflake.nixand thepostInstallrename, by building the package and checking that the versionmain.godeclares, the version the flake reads out of it, and the version the built binary reports all agree.It runs on tags and on pull requests touching
flake.nix,flake.lock,main.goorgo.mod, not on every pull request, because a cold runner downloads a 257 MiB Go toolchain to do it against abuildjob that finishes in about a minute. Measured locally: 45 seconds of compile once the store is warm. The paths list includes the workflow itself, so this pull request runs it rather than merging it untested.Evidence
gofmt -l .clean,go mod tidyleaves the module files unchanged,go vet ./...clean,go test -count=1 ./...all packages pass with no skips.fs.ErrNotExisttolerance inlogouthad no test, and the member-remove row served an empty fleet list so the loop never ran under mutation.bin/superstack, which is the rename working.gh api, not guessed.actionlint, which carries shellcheck, passes on all three workflows.api.testbinary was sitting untracked in the tree, so*.testis ignored now.An adversarial review over this diff is still running. If it confirms anything, the fix lands as a further commit on this branch before merge.
No version change, and nothing under
flake.nix,.goreleaser.yamlorgo.modis touched. Paired with a documentation change insuperstack-server.