From 5c3cd224c47068beac62bc85aa52021780086e59 Mon Sep 17 00:00:00 2001 From: Raj Nakarja Date: Fri, 21 Aug 2026 11:16:57 +0200 Subject: [PATCH 1/3] Follow the coding rules the last two cycles left behind 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. --- README.md | 23 +++-- internal/account/account.go | 16 ++-- internal/account/account_test.go | 45 +++++++++- internal/api/api_test.go | 30 +++++++ internal/api/client.go | 19 ++--- internal/api/session.go | 6 +- internal/device/device.go | 8 +- internal/dispatch/dispatch.go | 2 +- internal/dispatch/dispatch_test.go | 11 +++ internal/fleet/fleet.go | 4 +- internal/fleet/fleet_test.go | 11 +++ internal/key/key.go | 6 +- internal/key/key_test.go | 18 ++++ internal/login/login.go | 11 ++- internal/login/login_test.go | 131 +++++++++++++++++++++++++++++ internal/member/member.go | 4 +- internal/member/member_test.go | 10 ++- main_test.go | 66 +++++++++++++++ 18 files changed, 375 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 33e9c5c..a68a36b 100644 --- a/README.md +++ b/README.md @@ -42,19 +42,19 @@ Uploading Lua code and streaming logs are not available yet. ## Local development -1. Install the toolchain: - - - **Any platform:** [Go](https://go.dev) 1.25 or newer. - - **Nix:** `nix develop`, or `direnv allow` once with - [direnv](https://direnv.net) hooked into your shell. - 1. Clone the repository: ```sh - git clone git@github.com:siliconwitchery/superstack-cli.git ~/projects/superstack-cli + git clone https://github.com/siliconwitchery/superstack-cli.git ~/projects/superstack-cli cd ~/projects/superstack-cli ``` +1. Install the toolchain: + + - **Any platform:** [Go](https://go.dev) 1.25 or newer. + - **Nix:** `nix develop` from inside the clone, or `direnv allow` once with + [direnv](https://direnv.net) hooked into your shell. + 1. Build and run: ```sh @@ -88,7 +88,9 @@ Uploading Lua code and streaming logs are not available yet. git switch -C dev origin/main ``` -1. Change `version` in `main.go`, run the checks, commit, and push: +1. Change `version` in `main.go`. + +1. Run every check: ```sh gofmt -l . @@ -97,6 +99,11 @@ Uploading Lua code and streaming logs are not available yet. CGO_ENABLED=0 go vet ./... CGO_ENABLED=0 go test ./... git diff --check + ``` + +1. Commit and push: + + ```sh git add main.go git commit -m "Version " git push -u origin dev diff --git a/internal/account/account.go b/internal/account/account.go index d9419d0..2a04a92 100644 --- a/internal/account/account.go +++ b/internal/account/account.go @@ -66,7 +66,9 @@ func Balance(session api.Session, arguments []string) error { } if jsonOutput { - return json.NewEncoder(session.Out).Encode(balances) + err = json.NewEncoder(session.Out).Encode(balances) + + return err } if len(balances) == 0 { @@ -166,7 +168,7 @@ func Delete(session api.Session, arguments []string) error { return errors.New("account delete takes no arguments") } - request, err := api.AuthenticatedRequest(session, http.MethodGet, "/", nil) + request, err := api.AuthenticatedRequest(session, http.MethodGet, "/fleets", nil) if err != nil { return err @@ -178,12 +180,16 @@ func Delete(session api.Session, arguments []string) error { return errors.New("the server could not be reached, check your connection") } - response.Body.Close() + if response.StatusCode != http.StatusOK { + refusal := api.ServerError(response) - if response.StatusCode == http.StatusUnauthorized { - return errors.New("you are not logged in, run login first") + response.Body.Close() + + return refusal } + response.Body.Close() + fmt.Fprint(session.Out, "Delete your account, its logins, and your access to every fleet? This cannot be undone. [y/N] ") answer, _ := bufio.NewReader(session.In).ReadString('\n') diff --git a/internal/account/account_test.go b/internal/account/account_test.go index fdf7924..9dffb63 100644 --- a/internal/account/account_test.go +++ b/internal/account/account_test.go @@ -18,6 +18,7 @@ func TestAccountBalance(t *testing.T) { arguments []string fleets string balances string + refusal string wantLines []string wantAbsent []string wantExact string @@ -81,6 +82,13 @@ func TestAccountBalance(t *testing.T) { balances: `[]`, wantExact: "No credit on that fleet yet.\n", }, + { + name: "a fleet the list does not name", + arguments: []string{}, + fleets: `[{"id":1,"name":"crew","owner":true}]`, + balances: `[{"fleet":99,"balance":"15.000000","currency":"eur"}]`, + wantLines: []string{"99 -"}, + }, { name: "an unknown fleet", arguments: []string{"9"}, @@ -88,6 +96,13 @@ func TestAccountBalance(t *testing.T) { balances: `[]`, wantError: "no such fleet", }, + { + name: "server refusal", + arguments: []string{}, + fleets: `[{"id":1,"name":"crew","owner":true}]`, + refusal: "balances unavailable", + wantError: "balances unavailable", + }, { name: "a wordy id", arguments: []string{"crew"}, @@ -109,6 +124,11 @@ func TestAccountBalance(t *testing.T) { }) mux.HandleFunc("GET /balance", func(w http.ResponseWriter, r *http.Request) { + if test.refusal != "" { + http.Error(w, test.refusal, http.StatusServiceUnavailable) + return + } + fmt.Fprint(w, test.balances) }) @@ -330,6 +350,12 @@ func TestAccountDelete(t *testing.T) { mux := http.NewServeMux() + // The command asks the server whether the stored login still works + // before it puts the question, so every case has to answer this. + mux.HandleFunc("GET /fleets", func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, `[]`) + }) + mux.HandleFunc("DELETE /account", func(w http.ResponseWriter, r *http.Request) { deleted = true @@ -404,10 +430,17 @@ func TestAccountDeleteAsksNothingWhenTheServerIsGone(t *testing.T) { } func TestAccountDeleteStopsWhenTheStoredLoginIsNoLongerValid(t *testing.T) { + deleted := false + mux := http.NewServeMux() - mux.HandleFunc("GET /", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusUnauthorized) + // What the server's own auth middleware answers for a revoked login. + mux.HandleFunc("GET /fleets", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "the login is no longer valid, log in again", http.StatusUnauthorized) + }) + + mux.HandleFunc("DELETE /account", func(w http.ResponseWriter, r *http.Request) { + deleted = true }) session, out := apitest.LoggedInSession(t, mux) @@ -416,11 +449,15 @@ func TestAccountDeleteStopsWhenTheStoredLoginIsNoLongerValid(t *testing.T) { err := Delete(session, nil) - if err == nil || !strings.Contains(err.Error(), "not logged in") { - t.Fatalf("error = %v, want it to say the login is not valid", err) + if err == nil || !strings.Contains(err.Error(), "no longer valid") { + t.Fatalf("error = %v, want the server's own refusal", err) } if out.String() != "" { t.Errorf("output = %q, want the question never to be put", out.String()) } + + if deleted { + t.Error("the server saw the account deleted although the login was refused") + } } diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 70bbe1d..10be098 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -165,6 +165,36 @@ func TestFetchFleetsFailures(t *testing.T) { } } +func TestFetchDevicesFailures(t *testing.T) { + tests := []struct { + name string + status int + body string + wantError string + }{ + {name: "server refusal", status: http.StatusServiceUnavailable, body: "devices unavailable", wantError: "devices unavailable"}, + {name: "undecodable body", status: http.StatusOK, body: `[{`, wantError: "could not be read"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("GET /devices", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(test.status) + fmt.Fprint(w, test.body) + }) + + session, _ := apitest.LoggedInSession(t, mux) + + _, err := api.FetchDevices(session) + + if err == nil || !strings.Contains(err.Error(), test.wantError) { + t.Fatalf("error = %v, want it to mention %q", err, test.wantError) + } + }) + } +} + func TestFetchKeysFailures(t *testing.T) { tests := []struct { name string diff --git a/internal/api/client.go b/internal/api/client.go index 1fbe2b8..8485720 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -12,12 +12,8 @@ import ( "strings" ) -// Larger than any answer the server can produce: it caps fleets at 100 per -// user and keys at 100 per fleet, and only the device list is uncapped. -const maximumBody = 32 << 20 - -func Request(session Session, method string, path string, body io.Reader) (*http.Request, error) { - request, err := http.NewRequest(method, strings.TrimSuffix(session.Base, "/")+path, body) +func Request(session Session, method string, path string, reader io.Reader) (*http.Request, error) { + request, err := http.NewRequest(method, strings.TrimSuffix(session.Base, "/")+path, reader) if err != nil { return nil, err @@ -28,7 +24,7 @@ func Request(session Session, method string, path string, body io.Reader) (*http return request, nil } -func AuthenticatedRequest(session Session, method string, path string, body io.Reader) (*http.Request, error) { +func AuthenticatedRequest(session Session, method string, path string, reader io.Reader) (*http.Request, error) { storedKeyPath, err := KeyPath() if err != nil { @@ -51,7 +47,7 @@ func AuthenticatedRequest(session Session, method string, path string, body io.R return nil, errors.New("you are not logged in, run login first") } - request, err := Request(session, method, path, body) + request, err := Request(session, method, path, reader) if err != nil { return nil, err @@ -75,7 +71,8 @@ func ServerError(response *http.Response) error { } func Decode(response *http.Response, value any) error { - err := json.NewDecoder(io.LimitReader(response.Body, maximumBody)).Decode(value) + // 32 MiB is past every capped list and is the ceiling on the uncapped ones. + err := json.NewDecoder(io.LimitReader(response.Body, 32<<20)).Decode(value) if err != nil { return errors.New("the server's answer could not be read, try again in a moment") @@ -93,7 +90,7 @@ func KeyPath() (string, error) { home, err := os.UserHomeDir() if err != nil { - return "", errors.New("your home folder could not be found, so the login has nowhere to live") + return "", errors.New("your home folder could not be found, so the login cannot be read or saved") } stateHome = filepath.Join(home, ".local", "state") @@ -105,7 +102,7 @@ func KeyPath() (string, error) { configDirectory, err := os.UserConfigDir() if err != nil { - return "", errors.New("your settings folder could not be found, so the login has nowhere to live") + return "", errors.New("your settings folder could not be found, so the login cannot be read or saved") } return filepath.Join(configDirectory, "superstack", "key"), nil diff --git a/internal/api/session.go b/internal/api/session.go index 9f18277..5852251 100644 --- a/internal/api/session.go +++ b/internal/api/session.go @@ -10,8 +10,6 @@ import ( ) const DefaultBase = "https://supernext.siliconwitchery.com" -const defaultGithubBase = "https://github.com" -const defaultGitlabBase = "https://gitlab.com" type Session struct { Base string @@ -27,8 +25,8 @@ type Session struct { func NewSession(base string, version string, in io.Reader, out io.Writer) Session { return Session{ Base: base, - GithubBase: defaultGithubBase, - GitlabBase: defaultGitlabBase, + GithubBase: "https://github.com", + GitlabBase: "https://gitlab.com", Version: version, Client: &http.Client{Timeout: 30 * time.Second}, In: in, diff --git a/internal/device/device.go b/internal/device/device.go index 5c8cee8..fbba942 100644 --- a/internal/device/device.go +++ b/internal/device/device.go @@ -15,8 +15,6 @@ import ( ) func Claim(session api.Session, arguments []string) error { - claimClient := &http.Client{Timeout: 90 * time.Second} - if len(arguments) != 2 && len(arguments) != 3 { return errors.New("device claim takes an IMEI, a fleet id, and an optional name") } @@ -74,6 +72,8 @@ func Claim(session api.Session, arguments []string) error { request.Header.Set("Content-Type", "application/json") + claimClient := &http.Client{Timeout: 90 * time.Second} + response, err := claimClient.Do(request) if err != nil { @@ -143,7 +143,9 @@ func List(session api.Session, arguments []string) error { } if jsonOutput { - return json.NewEncoder(session.Out).Encode(filtered) + err = json.NewEncoder(session.Out).Encode(filtered) + + return err } if len(filtered) == 0 { diff --git a/internal/dispatch/dispatch.go b/internal/dispatch/dispatch.go index 926c2f1..c786713 100644 --- a/internal/dispatch/dispatch.go +++ b/internal/dispatch/dispatch.go @@ -33,7 +33,7 @@ func takeServerFlag(arguments []string) ([]string, string, error) { } } - base = strings.TrimRight(base, "/") + base = strings.TrimRight(strings.TrimSpace(base), "/") if base == "" { return nil, "", errors.New("--server needs an address") diff --git a/internal/dispatch/dispatch_test.go b/internal/dispatch/dispatch_test.go index 628661a..88eb09d 100644 --- a/internal/dispatch/dispatch_test.go +++ b/internal/dispatch/dispatch_test.go @@ -57,6 +57,17 @@ func TestTakeServerFlag(t *testing.T) { arguments: []string{"login", "--server="}, wantError: "needs an address", }, + { + name: "a value that is only spaces", + arguments: []string{"login", "--server", " "}, + wantError: "needs an address", + }, + { + name: "surrounding spaces are trimmed", + arguments: []string{"--server", " http://localhost:8080/ ", "login"}, + wantRemaining: "login", + wantBase: "http://localhost:8080", + }, { name: "an empty value from an unset shell variable", arguments: []string{"--server", "", "login"}, diff --git a/internal/fleet/fleet.go b/internal/fleet/fleet.go index 44bf6b1..5fbca86 100644 --- a/internal/fleet/fleet.go +++ b/internal/fleet/fleet.go @@ -74,7 +74,9 @@ func List(session api.Session, arguments []string) error { } if jsonOutput { - return json.NewEncoder(session.Out).Encode(fleets) + err = json.NewEncoder(session.Out).Encode(fleets) + + return err } if len(fleets) == 0 { diff --git a/internal/fleet/fleet_test.go b/internal/fleet/fleet_test.go index 031a42b..fd8f3e5 100644 --- a/internal/fleet/fleet_test.go +++ b/internal/fleet/fleet_test.go @@ -90,11 +90,17 @@ func TestFleetList(t *testing.T) { name string arguments []string fleets string + refusal string wantShown []string wantAbsent []string wantExact string wantError string }{ + { + name: "server refusal", + refusal: "fleets unavailable", + wantError: "fleets unavailable", + }, { name: "some fleets", fleets: `[{"id":1,"name":"field trial","owner":true},{"id":2,"name":"rooftop","owner":false}]`, @@ -130,6 +136,11 @@ func TestFleetList(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc("GET /fleets", func(w http.ResponseWriter, r *http.Request) { + if test.refusal != "" { + http.Error(w, test.refusal, http.StatusServiceUnavailable) + return + } + fmt.Fprint(w, test.fleets) }) diff --git a/internal/key/key.go b/internal/key/key.go index 830fb61..81ce208 100644 --- a/internal/key/key.go +++ b/internal/key/key.go @@ -123,7 +123,9 @@ func List(session api.Session, arguments []string) error { } if jsonOutput { - return json.NewEncoder(session.Out).Encode(keys) + err = json.NewEncoder(session.Out).Encode(keys) + + return err } if len(keys) == 0 { @@ -158,6 +160,8 @@ func List(session api.Session, arguments []string) error { fleetNameWidth = max(fleetNameWidth, len(fleetNameValues[index])) } + // KEY is fixed at eight: the server sends a five-character suffix and the + // cell prefixes it with three dots, so a shorter suffix would skew LABEL. fmt.Fprintf(session.Out, "%-*s %-*s %-*s %-8s %s\n", idWidth, "ID", fleetIdWidth, "FLEET", fleetNameWidth, "FLEET NAME", "KEY", "LABEL") diff --git a/internal/key/key_test.go b/internal/key/key_test.go index 69dac1c..1ac1ba9 100644 --- a/internal/key/key_test.go +++ b/internal/key/key_test.go @@ -153,7 +153,14 @@ func TestKeyList(t *testing.T) { wantHidden []string wantError string keys string + refusal string }{ + { + name: "server refusal", + arguments: []string{}, + refusal: "keys unavailable", + wantError: "keys unavailable", + }, { name: "every fleet's keys", arguments: []string{}, @@ -196,6 +203,12 @@ func TestKeyList(t *testing.T) { wantShown: []string{`"id":1`}, wantHidden: []string{`"id":2`, "ID FLEET"}, }, + { + name: "a fleet the list does not name", + arguments: []string{}, + keys: `[{"id":1,"fleet":99,"label":"orphan","suffix":"ab2de"}]`, + wantShown: []string{"99 -"}, + }, { name: "a fleet out of reach", arguments: []string{"9"}, @@ -228,6 +241,11 @@ func TestKeyList(t *testing.T) { }) mux.HandleFunc("GET /keys", func(w http.ResponseWriter, r *http.Request) { + if test.refusal != "" { + http.Error(w, test.refusal, http.StatusServiceUnavailable) + return + } + fmt.Fprint(w, servedKeys) }) diff --git a/internal/login/login.go b/internal/login/login.go index 0741002..5c135c7 100644 --- a/internal/login/login.go +++ b/internal/login/login.go @@ -18,8 +18,6 @@ import ( ) func Login(session api.Session, arguments []string) error { - oauthClient := &http.Client{Timeout: 30 * time.Second} - if len(arguments) != 1 || (arguments[0] != "github" && arguments[0] != "gitlab") { return errors.New("login takes a provider: github or gitlab") } @@ -90,6 +88,8 @@ func Login(session api.Session, arguments []string) error { codeRequest.Header.Set("Content-Type", "application/x-www-form-urlencoded") codeRequest.Header.Set("Accept", "application/json") + oauthClient := &http.Client{Timeout: 30 * time.Second} + codeResponse, err := oauthClient.Do(codeRequest) if err != nil { @@ -138,11 +138,10 @@ func Login(session api.Session, arguments []string) error { deadline := time.Now().Add(time.Duration(code.ExpiresIn) * time.Second) - const defaultPollInterval = 5 interval := code.Interval if interval <= 0 { - interval = defaultPollInterval + interval = 5 // RFC 8628 section 3.2: the default when a provider omits it } accessToken := "" @@ -200,7 +199,7 @@ func Login(session api.Session, arguments []string) error { case "authorization_pending": case "slow_down": - interval += 5 + interval += 5 // RFC 8628 section 3.5: each slow_down adds five seconds case "expired_token": return errors.New("the code expired before it was entered, run login again") @@ -356,7 +355,7 @@ func Logout(session api.Session, arguments []string) error { err = os.Remove(path) - if err != nil { + if err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("the login stored at %s is no longer valid but could not be removed, delete it yourself", path) } diff --git a/internal/login/login_test.go b/internal/login/login_test.go index cabbe96..341c47a 100644 --- a/internal/login/login_test.go +++ b/internal/login/login_test.go @@ -643,3 +643,134 @@ func TestLoginReplacesAStoredLoginLeftTooOpen(t *testing.T) { t.Errorf("the login folder holds %d files, want only the stored login", len(entries)) } } + +func TestLoginShowsTheCodeAndWhereToEnterIt(t *testing.T) { + apitest.IsolateKeyStorage(t) + + _, providerBase := fakeProviderForLogin(t, "gitlab", 1, "", []string{`{"access_token": "glpat-test"}`}) + session, out := fakeSuperstack(t, "", "") + session.GitlabBase = providerBase + + err := Login(session, []string{"gitlab"}) + + if err != nil { + t.Fatal(err) + } + + want := "Copy your one-time code: WDJB-MJHT\n" + + "Then enter it at https://gitlab.com/-/user_settings/device?user_code=WDJB-MJHT\n" + + "Press enter to open the browser.\n" + + "Logged in as someone@example.com.\n" + + if out.String() != want { + t.Errorf("output = %q, want %q", out.String(), want) + } +} + +func TestLoginKeepsAWorkingLoginWhenTheNewOneCannotBeSaved(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores the folder mode this test rests on") + } + + apitest.IsolateKeyStorage(t) + + path, err := api.KeyPath() + + if err != nil { + t.Fatal(err) + } + + directory := filepath.Dir(path) + + err = os.MkdirAll(directory, 0o700) + + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(path, []byte("ssk_working\n"), 0o600) + + if err != nil { + t.Fatal(err) + } + + err = os.Chmod(directory, 0o500) + + if err != nil { + t.Fatal(err) + } + + t.Cleanup(func() { os.Chmod(directory, 0o700) }) + + _, providerBase := fakeProviderForLogin(t, "gitlab", 1, "", []string{`{"access_token": "glpat-test"}`}) + session, out := fakeSuperstack(t, "", "") + session.GitlabBase = providerBase + + err = Login(session, []string{"gitlab"}) + + if err == nil || !strings.Contains(err.Error(), "could not be saved") { + t.Fatalf("error = %v, want it to say the login could not be saved", err) + } + + stored, err := os.ReadFile(path) + + if err != nil { + t.Fatal(err) + } + + if strings.TrimSpace(string(stored)) != "ssk_working" { + t.Errorf("the stored login is %q, want the working one left untouched", strings.TrimSpace(string(stored))) + } + + if strings.Contains(out.String(), "Logged in as") { + t.Errorf("output = %q, want no claim that the login succeeded", out.String()) + } +} + +func TestLogoutToleratesALoginAlreadyRemoved(t *testing.T) { + apitest.IsolateKeyStorage(t) + + path, err := api.KeyPath() + + if err != nil { + t.Fatal(err) + } + + err = os.MkdirAll(filepath.Dir(path), 0o700) + + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(path, []byte("ssk_test\n"), 0o600) + + if err != nil { + t.Fatal(err) + } + + mux := http.NewServeMux() + + // Another shell logs out, or deletes the account, during the round trip. + mux.HandleFunc("POST /logout", func(w http.ResponseWriter, r *http.Request) { + os.Remove(path) + + w.WriteHeader(http.StatusNoContent) + }) + + server := httptest.NewServer(mux) + + defer server.Close() + + out := &bytes.Buffer{} + session := api.NewSession(server.URL, "test", strings.NewReader(""), out) + + err = Logout(session, nil) + + if err != nil { + t.Fatalf("error = %v, want a login already removed to be no failure", err) + } + + if out.String() != "Logged out.\n" { + t.Errorf("output = %q, want %q", out.String(), "Logged out.\n") + } +} diff --git a/internal/member/member.go b/internal/member/member.go index 37e8800..4c85dff 100644 --- a/internal/member/member.go +++ b/internal/member/member.go @@ -103,7 +103,9 @@ func List(session api.Session, arguments []string) error { } if jsonOutput { - return json.NewEncoder(session.Out).Encode(people) + err = json.NewEncoder(session.Out).Encode(people) + + return err } owner := api.Printable(people.Owner) diff --git a/internal/member/member_test.go b/internal/member/member_test.go index faa0036..442b76c 100644 --- a/internal/member/member_test.go +++ b/internal/member/member_test.go @@ -217,6 +217,7 @@ func TestMemberRemove(t *testing.T) { name string email string answer string + fleets string refusal string wantRemoved bool wantShown string @@ -230,6 +231,7 @@ func TestMemberRemove(t *testing.T) { {name: "declined with n", email: "member@example.com", answer: "n\n", wantShown: "Nothing removed"}, {name: "closed input", email: "member@example.com", wantShown: "Nothing removed"}, {name: "server refusal", email: "member@example.com", answer: "y\n", refusal: "only an owner can remove members", wantRemoved: true, wantError: "only an owner can remove members"}, + {name: "a fleet that is not yours", email: "member@example.com", answer: "y\n", fleets: `[{"id":7,"name":"someone else's","owner":true}]`, wantError: "no such fleet"}, } for _, test := range tests { @@ -239,8 +241,14 @@ func TestMemberRemove(t *testing.T) { mux := http.NewServeMux() + fleets := test.fleets + + if fleets == "" { + fleets = `[{"id":3,"name":"pilot","owner":true}]` + } + mux.HandleFunc("GET /fleets", func(w http.ResponseWriter, r *http.Request) { - fmt.Fprint(w, `[{"id":3,"name":"pilot","owner":true}]`) + fmt.Fprint(w, fleets) }) mux.HandleFunc("DELETE /fleets/{id}/members/{email}", func(w http.ResponseWriter, r *http.Request) { diff --git a/main_test.go b/main_test.go index 0baaf90..9af130e 100644 --- a/main_test.go +++ b/main_test.go @@ -5,6 +5,8 @@ import ( "go/parser" "go/token" "io/fs" + "os" + "os/exec" "path/filepath" "slices" "strings" @@ -203,3 +205,67 @@ func TestHelpRendersTheRealTable(t *testing.T) { } } } + +// Re-runs this test binary as a child so main's own exit codes are observed +// rather than the error its command returned. +func TestMainReportsFailureWithANonZeroExit(t *testing.T) { + arguments, isChild := os.LookupEnv("SUPERSTACK_MAIN_ARGUMENTS") + + if isChild { + os.Args = append([]string{"superstack"}, strings.Fields(arguments)...) + + main() + + return + } + + tests := []struct { + name string + arguments string + wantCode int + wantSays string + }{ + {name: "no arguments", arguments: " ", wantCode: 0, wantSays: "Usage: superstack"}, + {name: "the version", arguments: "version", wantCode: 0}, + {name: "an unknown command", arguments: "nonsense", wantCode: 1, wantSays: "unknown command"}, + {name: "a command nobody has built yet", arguments: "tail 111111111111111", wantCode: 1, wantSays: "not available yet"}, + {name: "a command that needs a login", arguments: "fleet list", wantCode: 1, wantSays: "not logged in"}, + {name: "a flag with no value", arguments: "--server", wantCode: 1, wantSays: "needs an address"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + home := t.TempDir() + + command := exec.Command(os.Args[0], "-test.run=TestMainReportsFailureWithANonZeroExit") + + command.Env = append(os.Environ(), + "SUPERSTACK_MAIN_ARGUMENTS="+test.arguments, + "HOME="+home, + "XDG_STATE_HOME="+home, + "AppData="+home, + ) + + output, err := command.CombinedOutput() + + code := 0 + exitError, wasExit := err.(*exec.ExitError) + + switch { + case wasExit: + code = exitError.ExitCode() + + case err != nil: + t.Fatal(err) + } + + if code != test.wantCode { + t.Errorf("superstack %s exited %d, want %d, having said %q", test.arguments, code, test.wantCode, output) + } + + if test.wantSays != "" && !strings.Contains(string(output), test.wantSays) { + t.Errorf("superstack %s said %q, want it to mention %q", test.arguments, output, test.wantSays) + } + }) + } +} From ab09eea441fd7d18ef72c040b4ac88e01c9704e4 Mon Sep 17 00:00:00 2001 From: Raj Nakarja Date: Fri, 21 Aug 2026 11:24:35 +0200 Subject: [PATCH 2/3] Inline the server flag, and keep test binaries out of the tree 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. --- .gitignore | 3 + internal/dispatch/dispatch.go | 65 +++++++--------- internal/dispatch/dispatch_test.go | 117 ++++++++++++++++++----------- 3 files changed, 103 insertions(+), 82 deletions(-) diff --git a/.gitignore b/.gitignore index 33b981f..7023785 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,9 @@ /superstack /superstack-cli +# Compiled test binaries, which go test leaves behind when a run is interrupted +*.test + # GoReleaser output /dist/ diff --git a/internal/dispatch/dispatch.go b/internal/dispatch/dispatch.go index c786713..21d5eac 100644 --- a/internal/dispatch/dispatch.go +++ b/internal/dispatch/dispatch.go @@ -9,39 +9,6 @@ import ( "github.com/siliconwitchery/superstack-cli/internal/api" ) -func takeServerFlag(arguments []string) ([]string, string, error) { - remaining := []string{} - - base := api.DefaultBase - - for index := 0; index < len(arguments); index++ { - switch { - case arguments[index] == "--server": - if index+1 == len(arguments) { - return nil, "", errors.New("--server needs an address") - } - - index++ - - base = arguments[index] - - case strings.HasPrefix(arguments[index], "--server="): - base = strings.TrimPrefix(arguments[index], "--server=") - - default: - remaining = append(remaining, arguments[index]) - } - } - - base = strings.TrimRight(strings.TrimSpace(base), "/") - - if base == "" { - return nil, "", errors.New("--server needs an address") - } - - return remaining, base, nil -} - type Command struct { Name string Arguments string @@ -127,12 +94,36 @@ func printHelp(session api.Session, sections []Section) { } func Dispatch(sections []Section, version string, arguments []string, in io.Reader, out io.Writer) error { - arguments, base, err := takeServerFlag(arguments) + remaining := []string{} + base := api.DefaultBase + + for index := 0; index < len(arguments); index++ { + switch { + case arguments[index] == "--server": + if index+1 == len(arguments) { + return errors.New("--server needs an address") + } + + index++ + + base = arguments[index] + + case strings.HasPrefix(arguments[index], "--server="): + base = strings.TrimPrefix(arguments[index], "--server=") - if err != nil { - return err + default: + remaining = append(remaining, arguments[index]) + } } + base = strings.TrimRight(strings.TrimSpace(base), "/") + + if base == "" { + return errors.New("--server needs an address") + } + + arguments = remaining + session := api.NewSession(base, version, in, out) if len(arguments) == 0 { @@ -187,7 +178,7 @@ func Dispatch(sections []Section, version string, arguments []string, in io.Read return fmt.Errorf("%s is not available yet", entry.Name) } - err = entry.Run(session, rest) + err := entry.Run(session, rest) return err } diff --git a/internal/dispatch/dispatch_test.go b/internal/dispatch/dispatch_test.go index 88eb09d..2f2c620 100644 --- a/internal/dispatch/dispatch_test.go +++ b/internal/dispatch/dispatch_test.go @@ -10,42 +10,57 @@ import ( "github.com/siliconwitchery/superstack-cli/internal/api" ) -func TestTakeServerFlag(t *testing.T) { +func TestDispatchTakesTheServerFlag(t *testing.T) { tests := []struct { - name string - arguments []string - wantRemaining string - wantBase string - wantError string + name string + arguments []string + wantCommand string + wantRest string + wantBase string + wantError string }{ { - name: "no flag", - arguments: []string{"login"}, - wantRemaining: "login", + name: "no flag", + arguments: []string{"login"}, + wantCommand: "login", + wantBase: api.DefaultBase, }, { - name: "a url", - arguments: []string{"--server", "http://localhost:8080", "login"}, - wantRemaining: "login", - wantBase: "http://localhost:8080", + name: "a url", + arguments: []string{"--server", "http://localhost:8080", "login"}, + wantCommand: "login", + wantBase: "http://localhost:8080", }, { - name: "a url in equals form", - arguments: []string{"logout", "--server=https://staging.example.com"}, - wantRemaining: "logout", - wantBase: "https://staging.example.com", + name: "a url in equals form", + arguments: []string{"login", "--server=https://staging.example.com"}, + wantCommand: "login", + wantBase: "https://staging.example.com", }, { - name: "a trailing slash is trimmed", - arguments: []string{"--server", "http://localhost:8080/", "login"}, - wantRemaining: "login", - wantBase: "http://localhost:8080", + name: "a trailing slash is trimmed", + arguments: []string{"--server", "http://localhost:8080/", "login"}, + wantCommand: "login", + wantBase: "http://localhost:8080", }, { - name: "the flag between command words", - arguments: []string{"fleet", "--server=http://localhost:9999", "list"}, - wantRemaining: "fleet list", - wantBase: "http://localhost:9999", + name: "surrounding spaces are trimmed", + arguments: []string{"--server", " http://localhost:8080/ ", "login"}, + wantCommand: "login", + wantBase: "http://localhost:8080", + }, + { + name: "the flag between command words", + arguments: []string{"fleet", "--server=http://localhost:9999", "list"}, + wantCommand: "fleet list", + wantBase: "http://localhost:9999", + }, + { + name: "the flag never reaches the command", + arguments: []string{"fleet", "list", "--server=http://localhost:9999", "3"}, + wantCommand: "fleet list", + wantRest: "3", + wantBase: "http://localhost:9999", }, { name: "a missing value", @@ -62,17 +77,6 @@ func TestTakeServerFlag(t *testing.T) { arguments: []string{"login", "--server", " "}, wantError: "needs an address", }, - { - name: "surrounding spaces are trimmed", - arguments: []string{"--server", " http://localhost:8080/ ", "login"}, - wantRemaining: "login", - wantBase: "http://localhost:8080", - }, - { - name: "an empty value from an unset shell variable", - arguments: []string{"--server", "", "login"}, - wantError: "needs an address", - }, { name: "an address of nothing but slashes", arguments: []string{"--server", "//", "login"}, @@ -82,13 +86,38 @@ func TestTakeServerFlag(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - remaining, base, err := takeServerFlag(test.arguments) + ranCommand := "" + seenRest := []string{} + seenBase := "" + + record := func(name string) func(api.Session, []string) error { + return func(session api.Session, arguments []string) error { + ranCommand = name + seenRest = arguments + seenBase = session.Base + + return nil + } + } + + sections := []Section{ + {Title: "Things", Commands: []Command{ + {Name: "login", Summary: "Log in", Run: record("login")}, + {Name: "fleet list", Summary: "List fleets", Run: record("fleet list")}, + }}, + } + + err := Dispatch(sections, "1.2.3", test.arguments, strings.NewReader(""), &bytes.Buffer{}) if test.wantError != "" { if err == nil || !strings.Contains(err.Error(), test.wantError) { t.Fatalf("error = %v, want it to mention %q", err, test.wantError) } + if ranCommand != "" { + t.Errorf("%q ran although the address was refused", ranCommand) + } + return } @@ -96,18 +125,16 @@ func TestTakeServerFlag(t *testing.T) { t.Fatal(err) } - if strings.Join(remaining, " ") != test.wantRemaining { - t.Errorf("remaining = %q, want %q", strings.Join(remaining, " "), test.wantRemaining) + if ranCommand != test.wantCommand { + t.Errorf("%q ran, want %q", ranCommand, test.wantCommand) } - wantBase := test.wantBase - - if wantBase == "" { - wantBase = api.DefaultBase + if strings.Join(seenRest, " ") != test.wantRest { + t.Errorf("the command was handed %q, want %q", strings.Join(seenRest, " "), test.wantRest) } - if base != wantBase { - t.Errorf("base = %q, want %q", base, wantBase) + if seenBase != test.wantBase { + t.Errorf("the session base is %q, want %q", seenBase, test.wantBase) } }) } From aeffaf2154cc49c617d49185c73900582f79b203 Mon Sep 17 00:00:00 2001 From: Raj Nakarja Date: Fri, 21 Aug 2026 11:34:28 +0200 Subject: [PATCH 3/3] Guard the flake's vendorHash, cheaply on every change and fully on a tag 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. --- .github/workflows/flake.yml | 58 +++++++++++++++++++++++++++++++++++++ main_test.go | 21 ++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 .github/workflows/flake.yml diff --git a/.github/workflows/flake.yml b/.github/workflows/flake.yml new file mode 100644 index 0000000..44a56b1 --- /dev/null +++ b/.github/workflows/flake.yml @@ -0,0 +1,58 @@ +name: flake + +on: + push: + tags: ["v*"] + pull_request: + paths: + - .github/workflows/flake.yml + - flake.lock + - flake.nix + - go.mod + - main.go + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: flake-${{ github.ref }} + cancel-in-progress: true + +jobs: + build: + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - uses: cachix/install-nix-action@13d8dd58da0234aa297dedd986986ccb8e7f3e24 # v31.11.1 + with: + extra_nix_config: | + experimental-features = nix-command flakes + + - name: Build the package the flake defines + run: nix build --print-build-logs .#superstack + + - name: Verify the flake and the binary it built agree with main.go + run: | + declared=$(sed -n 's/^const version = "\(.*\)"$/\1/p' main.go) + + if [ -z "$declared" ]; then + echo "could not read 'const version' from main.go" + exit 1 + fi + + evaluated=$(nix eval --raw .#superstack.version) + + if [ "$evaluated" != "$declared" ]; then + echo "main.go declares $declared, but flake.nix reads $evaluated" + exit 1 + fi + + reported=$(./result/bin/superstack --version) + + if [ "$reported" != "$declared" ]; then + echo "main.go declares $declared, but the built binary reports $reported" + exit 1 + fi diff --git a/main_test.go b/main_test.go index 9af130e..845ed07 100644 --- a/main_test.go +++ b/main_test.go @@ -2,6 +2,7 @@ package main import ( "bytes" + "errors" "go/parser" "go/token" "io/fs" @@ -269,3 +270,23 @@ func TestMainReportsFailureWithANonZeroExit(t *testing.T) { }) } } + +func TestTheModuleStaysDependencyFree(t *testing.T) { + module, err := os.ReadFile("go.mod") + + if err != nil { + t.Fatal(err) + } + + for _, line := range strings.Split(string(module), "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "require") { + t.Errorf("go.mod says %q, so flake.nix cannot keep vendorHash = null", strings.TrimSpace(line)) + } + } + + _, err = os.Stat("go.sum") + + if !errors.Is(err, fs.ErrNotExist) { + t.Error("go.sum exists, so something is vendored and flake.nix needs a real vendorHash") + } +}