From 1a6f026d8131812763aa56207b6a856bc107d75b Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 7 Jul 2026 16:55:36 +0200 Subject: [PATCH] fix(cli): delete hard-fails on 426 guard; flags degraded when --keep-data pointer save fails Addresses the two Cursor Bugbot findings still live on develop (raised on the develop->main PR #164 at commit 6cfb99e). The other three findings on #164 were already fixed on develop: --kubeconfig/--context threading and the --keep-data pointer clear by #165, the stale pointer after a failed wipe by #168. 1. HTTP 426 in the pre-offboard online guard is now a HARD failure. lookupClientStatus returning *api.UpgradeRequiredError was swallowed as a soft "couldn't check - continuing", proceeding into a destructive offboard on a CLI too old to talk to the backend. It now returns the upgrade signal immediately (mirrors `client status --wait`). --force still skips the whole guard by design; there the revoke call surfaces the same 426 as a hard error. 2. Under --keep-data, a failed cfg.Save() of the cleared active-client pointer now marks the offboard `degraded`, so the closing line no longer claims a clean offboard while the on-disk config still names the revoked client (parity with the wipe path, which already flagged it). Tests: TestDelete_Guard426_HardFails (426 -> hard error, no revoke, no teardown) and TestDelete_KeepData_SaveFails_HonestClosing (save-fail -> degraded closing, no clean-success line); both verified to fail without the fix. Full internal suite + go vet green. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/cli/delete.go | 18 ++++++++- internal/cli/delete_test.go | 77 +++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/internal/cli/delete.go b/internal/cli/delete.go index d84b901b..de4a99be 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -137,6 +137,16 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // unreachable precisely because it's being retired); warn and continue. if !o.force { if st, found, lerr := lookupClientStatus(ctx, client, prof.ActiveClientID); lerr != nil { + // A 426 (CLI too old) is NOT a soft "couldn't check" — the CLI can't + // speak to tracebloc at all, and waiting/continuing won't help. Fail + // immediately with the upgrade signal rather than proceeding into a + // destructive offboard on a softened guard (mirrors `client status + // --wait`). --force skips this whole block by design; there the revoke + // call below surfaces the same 426 as a hard error. + var ue *api.UpgradeRequiredError + if errors.As(lerr, &ue) { + return &exitError{code: 1, err: lerr} + } p.Hintf("Couldn't check whether this client is still online (%v) — continuing; pass --force to skip this check.", lerr) } else if !found { // The stored id isn't among this account's clients — likely a stale @@ -233,7 +243,13 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er prof.ActiveClientID, prof.ActiveClientName, prof.ActiveClientNamespace = "", "", "" if o.keepData { if serr := cfg.Save(); serr != nil { - p.Warnf("Kept local data, but couldn't clear the active-client pointer (%v).", serr) + // The in-memory clear didn't persist: on-disk config still names the + // now-revoked client, so the host still looks enrolled. Mark degraded so + // the closing line doesn't claim a clean offboard (parity with the wipe + // path below, which already flags a failed Save). + degraded = true + p.Warnf("Kept local data, but couldn't clear the active-client pointer (%v) — "+ + "the on-disk config still names the revoked client; re-run offboard or clear it by hand.", serr) } else { p.Infof("Kept local data and config (~/.tracebloc); cleared the active-client pointer — --keep-data.") } diff --git a/internal/cli/delete_test.go b/internal/cli/delete_test.go index 2593ef0b..bdd57501 100644 --- a/internal/cli/delete_test.go +++ b/internal/cli/delete_test.go @@ -10,6 +10,7 @@ import ( "strings" "testing" + "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/config" "github.com/tracebloc/cli/internal/nodeboot" "github.com/tracebloc/cli/internal/ui" @@ -326,6 +327,82 @@ func TestDelete_KubeconfigContext_ReachHelm(t *testing.T) { } } +// A 426 (CLI too old) from the pre-offboard online guard must be a HARD failure +// with the upgrade signal — NOT a softened "couldn't check, continuing" that then +// proceeds into a destructive offboard. (Bugbot: "Delete guard softens HTTP 426".) +func TestDelete_Guard426_HardFails(t *testing.T) { + revoked := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke") { + revoked = true + } + // The guard's status lookup (GET /edge-device/) returns 426 Upgrade Required. + w.WriteHeader(http.StatusUpgradeRequired) + _, _ = w.Write([]byte(`{"error":"upgrade_required","min_version":"0.9.0"}`)) + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + + var out bytes.Buffer + err := runDelete(context.Background(), ui.New(&out), typedNamePrompter{reply: "gpu-box-01"}, deleteOpts{}) + if err == nil { + t.Fatal("a 426 from the online guard must fail the offboard, got nil") + } + var ue *api.UpgradeRequiredError + if !errors.As(err, &ue) { + t.Errorf("want *api.UpgradeRequiredError, got %v", err) + } + if revoked { + t.Error("must NOT revoke after a 426 guard failure") + } + if len(fn.calls) != 0 { + t.Errorf("no teardown after a 426 guard failure, got: %v", fn.calls) + } +} + +// Under --keep-data, if persisting the cleared active-client pointer fails, the +// offboard must NOT print a clean-success closing — the on-disk config still names +// the revoked client, so the closing line must flag the incomplete cleanup. +// (Bugbot: "Keep-data save leaves stale pointer".) +func TestDelete_KeepData_SaveFails_HonestClosing(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":0}]`)) + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke"): + w.WriteHeader(http.StatusOK) + } + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + // Make cfg.Save() fail while cfg.Load() still succeeds: chmod the config dir + // read-only. Load reads dir/config.json fine (dir is traversable); Save's + // os.CreateTemp(dir, …) can't create the temp file in a non-writable dir. + // Restore perms so t.TempDir cleanup can remove it. + dataDir := os.Getenv("TRACEBLOC_CONFIG_DIR") + if err := os.Chmod(dataDir, 0o500); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(dataDir, 0o700) }) + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + + var out bytes.Buffer + if err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true, keepData: true}); err != nil { + t.Fatalf("offboard should still return nil (revoke succeeded): %v", err) + } + s := out.String() + if !strings.Contains(s, "couldn't clear the active-client pointer") { + t.Errorf("expected a pointer-clear-failed warning, got:\n%s", s) + } + if !strings.Contains(s, "some cleanup above didn't complete") { + t.Errorf("a failed pointer save under --keep-data must give the degraded closing, got:\n%s", s) + } + if strings.Contains(s, "no longer connected to tracebloc") { + t.Errorf("must not print the clean-success closing when the pointer save failed:\n%s", s) + } +} + // (d) A running/online client → refuse unless --force. func TestDelete_RunningJob_RefusesUnlessForce(t *testing.T) { newHandler := func() http.HandlerFunc {