From 4dace117deea2f69c0c7695f1f9947cbbefe5965 Mon Sep 17 00:00:00 2001 From: Raj Nakarja Date: Thu, 20 Aug 2026 17:31:20 +0200 Subject: [PATCH 1/3] Fail when a part reaches sideways, or the table loses a command Go refuses an import cycle, not one part importing another, so the rule the layout exists for rested on discipline alone: fleet importing device builds, vets and tests clean. A test now walks internal and fails on any import the documented graph does not allow. Nothing bound a test to the table either, so deleting a wired command, or leaving one without its Run, passed the whole suite. One test pins the set that carries a Run, and another renders the real table rather than the synthetic one dispatch tests against. Each guard was checked against the violation it exists to catch, with the import made real enough to compile. --- main_test.go | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/main_test.go b/main_test.go index c4291e2..0baaf90 100644 --- a/main_test.go +++ b/main_test.go @@ -1,7 +1,16 @@ package main import ( + "bytes" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "slices" + "strings" "testing" + + "github.com/siliconwitchery/superstack-cli/internal/dispatch" ) func TestCommandTable(t *testing.T) { @@ -73,3 +82,124 @@ func TestOnlyPlannedCommandsAreUnimplemented(t *testing.T) { t.Errorf("%q is answered by dispatch but not in the table", name) } } + +func TestNoPartImportsAnother(t *testing.T) { + const module = "github.com/siliconwitchery/superstack-cli/internal/" + const fixtures = "api/apitest" + + // The graph docs/cli.md publishes: a part reaches api and nothing else, + // and only main reaches dispatch. + allowed := map[string][]string{ + "api": {}, + "api/apitest": {"api"}, + "dispatch": {"api"}, + "account": {"api"}, + "device": {"api"}, + "fleet": {"api"}, + "key": {"api"}, + "login": {"api"}, + "member": {"api"}, + } + + walk := func(path string, entry fs.DirEntry, err error) error { + if err != nil { + return err + } + + if entry.IsDir() || !strings.HasSuffix(path, ".go") { + return nil + } + + owner := filepath.ToSlash(strings.TrimPrefix(filepath.Dir(path), "internal"+string(filepath.Separator))) + + permitted, known := allowed[owner] + + if !known { + t.Errorf("%s is a package the graph does not mention, add it to docs/cli.md and to this test", owner) + return nil + } + + file, err := parser.ParseFile(token.NewFileSet(), path, nil, parser.ImportsOnly) + + if err != nil { + return err + } + + for _, imported := range file.Imports { + target := strings.Trim(imported.Path.Value, `"`) + + if !strings.HasPrefix(target, module) { + continue + } + + target = strings.TrimPrefix(target, module) + + if target == owner || slices.Contains(permitted, target) { + continue + } + + if target == fixtures && strings.HasSuffix(path, "_test.go") { + continue + } + + t.Errorf("%s imports %s, which the layout does not allow", path, target) + } + + return nil + } + + err := filepath.WalkDir("internal", walk) + + if err != nil { + t.Fatal(err) + } +} + +func TestTheTableWiresEveryCommandOffered(t *testing.T) { + wired := []string{ + "account balance", "account delete", "account topup", + "device claim", "device list", "device release", "device rename", + "fleet create", "fleet delete", "fleet list", "fleet rename", "fleet transfer", + "key create", "key list", "key revoke", + "login", "logout", + "member add", "member list", "member remove", + } + + offered := []string{} + + for _, section := range sections { + for _, entry := range section.Commands { + if entry.Run != nil { + offered = append(offered, entry.Name) + } + } + } + + slices.Sort(offered) + + if !slices.Equal(offered, wired) { + t.Errorf("the table wires %v, want %v", offered, wired) + } +} + +func TestHelpRendersTheRealTable(t *testing.T) { + out := &bytes.Buffer{} + + err := dispatch.Dispatch(sections, version, []string{"help"}, strings.NewReader(""), out) + + if err != nil { + t.Fatal(err) + } + + for _, section := range sections { + if !strings.Contains(out.String(), section.Title) { + t.Errorf("help leaves out the %q section", section.Title) + } + + for _, entry := range section.Commands { + if !strings.Contains(out.String(), entry.Name) { + t.Errorf("help leaves out %q", entry.Name) + } + } + } +} From 238020267da2f32f6233f3f363c184b9f1adcb2c Mon Sep 17 00:00:00 2001 From: Raj Nakarja Date: Thu, 20 Aug 2026 17:39:10 +0200 Subject: [PATCH 2/3] Stop probing the server before every command api.CheckServer sent GET / before all twenty implemented commands, threw the response away, and returned the same sentence every request site already returns on a failed Do. It bought a round trip for a message the real request produces a moment later, and it could not tell reachable from too old because it read neither the status nor X-Min-Version. Measured against a stub: fleet list made GET / then GET /fleets, and now makes only GET /fleets. It also put a network call in the router, so every dispatch test had to stand up a listener for cases like -h and --version that never touch the network. The test now imports no network package at all. --- internal/api/api_test.go | 33 ------------------------------ internal/api/client.go | 18 ---------------- internal/dispatch/dispatch.go | 6 ------ internal/dispatch/dispatch_test.go | 9 +------- 4 files changed, 1 insertion(+), 65 deletions(-) diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 4ca247b..b3bab3a 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -4,7 +4,6 @@ import ( "bytes" "fmt" "net/http" - "net/http/httptest" "os" "path/filepath" "runtime" @@ -108,38 +107,6 @@ func TestApiRequestBase(t *testing.T) { } } -func TestCheckServer(t *testing.T) { - reachable := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - t.Cleanup(reachable.Close) - - unreachable := httptest.NewServer(http.NotFoundHandler()) - unreachable.Close() - tests := []struct { - name string - base string - wantError string - }{ - {name: "reachable", base: reachable.URL}, - {name: "unreachable", base: unreachable.URL, wantError: "the server could not be reached, check your connection"}, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - session := api.NewSession(test.base, "test", strings.NewReader(""), &bytes.Buffer{}) - - err := api.CheckServer(session) - - if test.wantError != "" { - if err == nil || err.Error() != test.wantError { - t.Fatalf("error = %v, want %q", err, test.wantError) - } - } else if err != nil { - t.Fatal(err) - } - }) - } -} - func TestFetchFleetsFailures(t *testing.T) { tests := []struct { name string diff --git a/internal/api/client.go b/internal/api/client.go index 50f933b..aec8162 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -11,24 +11,6 @@ import ( "strings" ) -func CheckServer(session Session) error { - request, err := Request(session, http.MethodGet, "/", nil) - - if err != nil { - return err - } - - response, err := session.Client.Do(request) - - if err != nil { - return errors.New("the server could not be reached, check your connection") - } - - response.Body.Close() - - return nil -} - 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) diff --git a/internal/dispatch/dispatch.go b/internal/dispatch/dispatch.go index f0218d3..926c2f1 100644 --- a/internal/dispatch/dispatch.go +++ b/internal/dispatch/dispatch.go @@ -187,12 +187,6 @@ func Dispatch(sections []Section, version string, arguments []string, in io.Read return fmt.Errorf("%s is not available yet", entry.Name) } - err = api.CheckServer(session) - - if err != nil { - return err - } - err = entry.Run(session, rest) return err diff --git a/internal/dispatch/dispatch_test.go b/internal/dispatch/dispatch_test.go index 237f270..628661a 100644 --- a/internal/dispatch/dispatch_test.go +++ b/internal/dispatch/dispatch_test.go @@ -3,8 +3,6 @@ package dispatch import ( "bytes" "fmt" - "net/http" - "net/http/httptest" "slices" "strings" "testing" @@ -164,10 +162,6 @@ func TestResolve(t *testing.T) { } func TestDispatch(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - - t.Cleanup(server.Close) - sections := []Section{ {Title: "Things", Commands: []Command{ {Name: "thing list", Arguments: "", Summary: "List a thing", Run: func(session api.Session, arguments []string) error { @@ -211,10 +205,9 @@ func TestDispatch(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - arguments := append([]string{"--server", server.URL}, test.arguments...) out := &bytes.Buffer{} - err := Dispatch(sections, "1.2.3", arguments, strings.NewReader(""), out) + err := Dispatch(sections, "1.2.3", test.arguments, strings.NewReader(""), out) if test.wantError != "" { if err == nil || err.Error() != test.wantError { From 07fea0e7f9e6d320c09673c3acf80966e57181f1 Mon Sep 17 00:00:00 2001 From: Raj Nakarja Date: Thu, 20 Aug 2026 17:43:30 +0200 Subject: [PATCH 3/3] Check the server before asking to delete an account account delete is the only command that puts a question before it acts, so it was the only one the removed probe was doing any good for: without it the user confirms deleting everything and then learns the server is unreachable. The check is written where it runs rather than behind a shared function with one caller. It is authenticated, unlike the old probe, which also means someone not logged in is told so before the question rather than after it. Measured against a stub: fleet list makes one request, key list makes its two real ones, and account delete is alone in making GET / first. --- internal/account/account.go | 18 ++++++++++++++++-- internal/account/account_test.go | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/internal/account/account.go b/internal/account/account.go index fda4769..7b49f9e 100644 --- a/internal/account/account.go +++ b/internal/account/account.go @@ -156,6 +156,20 @@ func Delete(session api.Session, arguments []string) error { return errors.New("account delete takes no arguments") } + request, err := api.AuthenticatedRequest(session, http.MethodGet, "/", nil) + + if err != nil { + return err + } + + response, err := session.Client.Do(request) + + if err != nil { + return errors.New("the server could not be reached, check your connection") + } + + 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') @@ -167,13 +181,13 @@ func Delete(session api.Session, arguments []string) error { return nil } - request, err := api.AuthenticatedRequest(session, http.MethodDelete, "/account", nil) + request, err = api.AuthenticatedRequest(session, http.MethodDelete, "/account", nil) if err != nil { return err } - response, err := session.Client.Do(request) + response, err = session.Client.Do(request) if err != nil { return errors.New("the server could not be reached, check your connection") diff --git a/internal/account/account_test.go b/internal/account/account_test.go index eee7575..32cc467 100644 --- a/internal/account/account_test.go +++ b/internal/account/account_test.go @@ -3,6 +3,7 @@ package account import ( "fmt" "net/http" + "net/http/httptest" "os" "strings" "testing" @@ -374,3 +375,22 @@ func TestAccountDelete(t *testing.T) { }) } } + +func TestAccountDeleteAsksNothingWhenTheServerIsGone(t *testing.T) { + session, out := apitest.LoggedInSession(t, http.NewServeMux()) + + gone := httptest.NewServer(http.NotFoundHandler()) + gone.Close() + + session.Base = gone.URL + + err := Delete(session, nil) + + if err == nil || !strings.Contains(err.Error(), "could not be reached") { + t.Fatalf("error = %v, want it to mention the server could not be reached", err) + } + + if out.String() != "" { + t.Errorf("it asked %q before finding the server was gone", out.String()) + } +}