From 550979a553755814ddb11a4ce7acc6895fc1a72d Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Mon, 27 Jul 2026 09:59:37 +0200 Subject: [PATCH 1/9] =?UTF-8?q?fix(prompts):=20a=20cancelled=20prompt=20is?= =?UTF-8?q?=20never=20silent=20=E2=80=94=20one=20helper,=20exit=200,=20a?= =?UTF-8?q?=20visible=20note=20(#410)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ctrl-C at `client create`'s "Provision this client?" confirm and at `delete`'s typed-name confirmation exited 0 having printed nothing about it: `mapClientErr` mapped errInteractiveCancelled straight to nil with no Printer call. A script could not tell an aborted run from a completed one, and the declined-answer branch sitting directly beside each one DID print. Centralise the convention instead of duplicating it a fifth and sixth time: - `cleanCancel(p, nothing, …)` is now the only place a cancellation is reported — it prints "Cancelled — ." and returns the clean exit. - `mapClientErr` becomes `mapPromptErr(p, err, nothing, …)`: the Printer and the note are in the signature, so the silent-return shape is unreachable. Non-cancel errors still map to exit 1. - All six prompt sites route their cancellation through it, including the declined-answer twins, so Ctrl-C and "no" cannot drift apart. Output is byte-identical at the four sites that were already correct; `client create`'s terse "Cancelled." becomes "Cancelled — nothing was provisioned." Exit code stays 0, which is what all six sites already did and what exitOK documents. exitInterrupted (130) is used only for a Ctrl-C that cuts short work already in flight (sign-in wait, status --wait, the seal suite, an installer re-run) — its comment claimed the prompt case too, contradicting exitOK and every call site, so fix that and the matching row in docs/troubleshooting.md. Tests: cancel_test.go is a table over every prompt a user can back out of, asserting the exit code, the user-visible line, and that the command did not act anyway. The copy-catalog harvester learns about the new copy helper so the assembled "Cancelled — …" lines stay in the catalog. Co-authored-by: Claude Fable 5 --- docs/troubleshooting.md | 4 +- internal/cli/cancel_test.go | 146 ++++++++++++++++++ internal/cli/client.go | 19 +-- internal/cli/copy_catalog_test.go | 23 ++- internal/cli/data_delete.go | 26 ++-- internal/cli/data_ingest_cluster.go | 7 +- internal/cli/data_ingest_local.go | 4 +- internal/cli/delete.go | 9 +- internal/cli/exitcodes.go | 20 ++- internal/cli/interactive.go | 47 +++++- internal/cli/pure_helpers_coverage_test.go | 24 ++- internal/cli/resources_set.go | 21 ++- .../cli/testdata/golden/zz-all-strings.golden | 4 +- 13 files changed, 292 insertions(+), 62 deletions(-) create mode 100644 internal/cli/cancel_test.go diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index bb9f5867..9943e9ee 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -115,7 +115,7 @@ produces that code. | Code | Meaning | Produced by | Constant | |------|---------|-------------|----------| -| `0` | Success — includes `--dry-run` completing, a guided run you cancelled cleanly, and `doctor` passing with warnings only | all commands | `exitOK` | +| `0` | Success — includes `--dry-run` completing, any prompt you declined or cancelled with Ctrl-C (nothing was started, and the CLI prints a `Cancelled — …` line saying so), and `doctor` passing with warnings only | all commands | `exitOK` | | `1` | Generic failure with no more specific bucket (also any error without an explicit code) | `login`, `client …`, `delete`, mistyped commands | `exitFailure` | | `2` | Your input didn't validate: schema validation failed (spec synthesized from flags, or your YAML), an unsupported/unknown `--task`, a task-scoped flag applied to the wrong task, an invalid dataset name, or a resource size that doesn't fit the machine | `data ingest`, `data validate`, `data delete`, `resources set` | `exitBadInput` | | `2` | One or more checks failed — for `client status --seal`: the environment is unsealed (a conformance check failed), or unknown (the chart ships no conformance checks, so the seal couldn't be verified) | `doctor`, `client status --seal` | `exitChecksFailed` | @@ -129,7 +129,7 @@ produces that code. | `7` | The cluster couldn't be queried for its datasets | `data list` | `exitQueryFailed` | | `8` | jobs-manager rejected the submitted run (a non-auth 4xx/5xx), or the port-forward to it couldn't be set up | `data ingest` | `exitSubmitFailed` | | `9` | The ingestion Job exited non-zero, completed with row-level failures the summary panel reports, or its outcome couldn't be determined / followed | `data ingest` | `exitIngestFailed` | -| `130` | You hit Ctrl-C at an interactive prompt (128+SIGINT) | interactive prompts | `exitInterrupted` | +| `130` | You hit Ctrl-C while something was already running — the sign-in wait, `client status --wait`, the seal check, or an installer re-run (128+SIGINT). Ctrl-C at a *question* is `0` instead: nothing had started | `login`, `client status --wait`, `client status --seal`, `upgrade`, `prepare-host` | `exitInterrupted` | ## Still stuck? diff --git a/internal/cli/cancel_test.go b/internal/cli/cancel_test.go new file mode 100644 index 00000000..d6e18e3f --- /dev/null +++ b/internal/cli/cancel_test.go @@ -0,0 +1,146 @@ +// The cancellation contract, in one table. Every interactive prompt in the CLI +// can be backed out of two ways — Ctrl-C, or an answer that means "no" — and both +// must produce the SAME thing: a visible "Cancelled — …" line, exit 0, and no +// side effect. This file is the drift guard for that (backend#1253, the test +// convention proposed for this finding class in backend#930). +// +// Why it exists: `client create` and `delete` used to map Ctrl-C straight to a +// nil error, so aborting at the prompt exited 0 having printed nothing about it — +// byte-for-byte indistinguishable from a completed run for anything reading the +// stream, and inconsistent with the declined-answer branch sitting right beside +// it. Asserting the exit code alone would not have caught that; every row here +// asserts the exit code AND the user-visible output. +// +// Adding a prompt? Add a row. Both prompt doubles are shared, and the "did it act +// anyway" probe keeps a row honest: a printed note over a completed side effect +// would be a worse lie than silence. + +package cli + +import ( + "context" + "net/http" + "path/filepath" + "strings" + "testing" + + "github.com/tracebloc/cli/internal/ui" +) + +// runner drives one command to its prompt, using the injected prompt double. +type runner func(*ui.Printer, prompter) error + +// sideEffectProbe reports whether the command took its irreversible action +// despite the cancellation (a provision POST, an offboard revoke/teardown). +type sideEffectProbe func() bool + +// setUpClientCreate wires `tracebloc client create` against a fake backend that +// records whether a client was ever POSTed. No --yes and a prompter present, so +// the run reaches the "Provision this client?" confirm. +func setUpClientCreate(t *testing.T) (runner, sideEffectProbe) { + t.Helper() + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + posted = true + } + _, _ = w.Write([]byte(`[]`)) // no existing clients on the account + }) + signInAs(t, "Lab", "lab@example.com") + return func(p *ui.Printer, pr prompter) error { + return runClientCreate(context.Background(), p, pr, clientCreateOpts{}) + }, func() bool { return posted } +} + +// setUpDelete wires `tracebloc delete` (offboard this machine) with a live +// client to remove and every teardown seam faked, so the run reaches the +// typed-client-name confirmation and any teardown step is recorded, not real. +func setUpDelete(t *testing.T) (runner, sideEffectProbe) { + t.Helper() + revoked := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke") { + revoked = true + } + _, _ = w.Write([]byte(`{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":0}`)) + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")} + fn.install(t) + return func(p *ui.Printer, pr prompter) error { + return runDelete(context.Background(), p, pr, deleteOpts{}) + }, func() bool { return revoked || len(fn.calls) > 0 } +} + +// TestPromptCancellation_IsVisibleAndCleanExit: for every prompt a user can back +// out of, the CLI prints why it stopped and exits 0 — and does not act. +// +// Exit 0 (not 130) is the convention: nothing was started, so there is no +// interrupted operation to report. exitInterrupted is reserved for a Ctrl-C that +// cuts short work already in flight — see exitcodes.go and cleanCancel. +func TestPromptCancellation_IsVisibleAndCleanExit(t *testing.T) { + // declineClientCreate answers "No" at the confirm (the Ctrl-C row's twin). + no := false + + cases := []struct { + name string + // how the user backed out, for failure messages. + how string + // setUp wires the command's world; pr is what the user "did". + setUp func(*testing.T) (runner, sideEffectProbe) + pr prompter + // wantOut is the line the user must see. The Ctrl-C and declined rows of + // one command share it wherever the reason is the same — `delete`'s + // mismatch row names the reason, which is more, never less. + wantOut string + }{ + { + name: "client create/ctrl-c at the confirm", + how: "Ctrl-C at \"Provision this client?\"", + setUp: setUpClientCreate, + pr: cancellingPrompter{}, + wantOut: "Cancelled — nothing was provisioned.", + }, + { + name: "client create/answered no at the confirm", + how: "answering \"No\" at \"Provision this client?\"", + setUp: setUpClientCreate, + pr: &fakePrompter{confirm: &no}, + wantOut: "Cancelled — nothing was provisioned.", + }, + { + name: "delete/ctrl-c while typing the name", + how: "Ctrl-C at the typed-name confirmation", + setUp: setUpDelete, + pr: cancellingPrompter{}, + wantOut: "Cancelled — nothing was removed.", + }, + { + name: "delete/typed a name that didn't match", + how: "typing the wrong client name", + setUp: setUpDelete, + pr: typedNamePrompter{reply: "wrong-name"}, + wantOut: "Cancelled — the name didn't match. Nothing was removed.", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + run, sideEffected := tc.setUp(t) + out := &strings.Builder{} + err := run(ui.New(out, ui.WithColor(false)), tc.pr) + + if got := ExitCodeFromError(err); got != exitOK { + t.Errorf("exit code after %s = %d, want %d (backing out is a choice, not a failure): %v", + tc.how, got, exitOK, err) + } + if !strings.Contains(out.String(), tc.wantOut) { + t.Errorf("%s printed no cancellation note — a silent exit 0 is indistinguishable from success.\nwant a line containing: %q\ngot:\n%s", + tc.how, tc.wantOut, out.String()) + } + if sideEffected() { + t.Errorf("%s must not act: the command went ahead anyway", tc.how) + } + }) + } +} diff --git a/internal/cli/client.go b/internal/cli/client.go index 7875cae7..cd662a41 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -333,12 +333,17 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien renderClientReview(p, name, namespace, location, clusterID) ok, cerr := pr.Confirm("Provision this client?", true) if cerr != nil { - return mapClientErr(cerr) + // Ctrl-C here is the same choice as answering "No" below, so it + // gets the same note and the same clean exit — never a silent + // exit 0 that a script reads as "provisioned" (backend#1253). + // Logged unconditionally: for a cancel the reason IS "cancelled + // by user", and a real terminal failure should say so too. + ilog.Logf("stopped at the confirm prompt: %v", cerr) + return mapPromptErr(p, cerr, "nothing was provisioned.") } if !ok { ilog.Logf("cancelled by user at the confirm prompt") - p.Hintf("Cancelled.") - return nil + return cleanCancel(p, "nothing was provisioned.") } } else if pr == nil && !opts.yes && opts.credentialFile == "" && !willAdopt { // Non-interactive with no way to confirm AND no --credential-file: a fresh @@ -855,14 +860,6 @@ func emailLocalPart(email string) string { return email } -// mapClientErr turns a cancelled interactive prompt into a clean exit. -func mapClientErr(err error) error { - if errors.Is(err, errInteractiveCancelled) { - return nil - } - return &exitError{code: exitFailure, err: err} -} - // randHex returns nbytes of crypto-random data hex-encoded. func randHex(nbytes int) string { b := make([]byte, nbytes) diff --git a/internal/cli/copy_catalog_test.go b/internal/cli/copy_catalog_test.go index 9701bb5c..37560f17 100644 --- a/internal/cli/copy_catalog_test.go +++ b/internal/cli/copy_catalog_test.go @@ -538,6 +538,14 @@ func (c *catalogPrompter) Confirm(label string, def bool) (bool, error) { // Both "…" and `…` raw strings; deduped + sorted. func harvestMessages(t *testing.T) []string { t.Helper() + // Package-local helpers that print through the Printer on the caller's + // behalf. Their note argument is user-facing copy no Printer-argument scan + // would see, and the helper supplies the sentence's opening — so harvest the + // ASSEMBLED line the user reads, not the bare clause (backend#1253). + copyHelperPrefix := map[string]string{ + "cleanCancel": "Cancelled — ", + "mapPromptErr": "Cancelled — ", + } methods := map[string]bool{ "Successf": true, "Warnf": true, "Errorf": true, "Infof": true, "Hintf": true, "Detailf": true, "Para": true, "Section": true, "PromptHint": true, "PromptHeader": true, @@ -580,7 +588,7 @@ func harvestMessages(t *testing.T) []string { } seen := map[string]struct{}{} - collect := func(exprs []ast.Expr) { + collect := func(prefix string, exprs []ast.Expr) { for _, arg := range exprs { lit, ok := arg.(*ast.BasicLit) if !ok || lit.Kind != token.STRING { @@ -595,7 +603,7 @@ func harvestMessages(t *testing.T) []string { if len([]rune(s)) < 4 || !strings.ContainsAny(s, "abcdefghijklmnopqrstuvwxyz") { continue } - seen[s] = struct{}{} + seen[prefix+s] = struct{}{} } } fset := token.NewFileSet() @@ -611,16 +619,21 @@ func harvestMessages(t *testing.T) []string { ast.Inspect(f, func(n ast.Node) bool { switch node := n.(type) { case *ast.CallExpr: + if id, ok := node.Fun.(*ast.Ident); ok { + if prefix, isHelper := copyHelperPrefix[id.Name]; isHelper { + collect(prefix, node.Args) + } + } if isCopyCall(node) { - collect(node.Args) + collect("", node.Args) } case *ast.CompositeLit: if isCopyStruct(node.Type) { for _, el := range node.Elts { if kv, ok := el.(*ast.KeyValueExpr); ok { - collect([]ast.Expr{kv.Value}) + collect("", []ast.Expr{kv.Value}) } else { - collect([]ast.Expr{el}) + collect("", []ast.Expr{el}) } } } diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index 6cf6049b..7b1e6d38 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -229,25 +229,27 @@ undone — re-ingesting the data is the only way back.`) "refusing to delete without confirmation: pass --yes or run on a terminal")} } p.PromptHint("This drops the table and removes the files listed above — there's no undo. Pass --yes next time to skip this prompt.") + // Both ways out of this prompt — Ctrl-C and an explicit "no" — report the + // same outcome: the shared cancellation note, one "declined" JSON object, + // exit 0. One closure so the pair can't drift apart. + declined := func() error { + if a.OutputJSON { + writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil) + jsonEmitted = true + } + return cleanCancel(p, "nothing was deleted.") + } ok, err := a.Prompter.Confirm(fmt.Sprintf("Delete %q and its files?", matched), false) if err != nil { + // Not mapPromptErr: a prompt that genuinely fails here is a + // local-environment problem (exit 3), not the generic exit 1. if errors.Is(err, errInteractiveCancelled) { - p.Infof("Cancelled — nothing was deleted.") - if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil) - jsonEmitted = true - } - return nil + return declined() } return &exitError{code: exitLocalEnv, err: err} } if !ok { - p.Infof("Cancelled — nothing was deleted.") - if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil) - jsonEmitted = true - } - return nil + return declined() } } diff --git a/internal/cli/data_ingest_cluster.go b/internal/cli/data_ingest_cluster.go index 48a96649..4a82987c 100644 --- a/internal/cli/data_ingest_cluster.go +++ b/internal/cli/data_ingest_cluster.go @@ -88,8 +88,11 @@ func connectIngestTarget(ctx context.Context, a *runDataIngestArgs) (target *clu return nil, "", false, aerr } if !proceed { - a.Printer.Infof("Cancelled — %q was left as-is; nothing was ingested.", existingTable) - return nil, "", true, nil + // Reached by both a declined replace and a Ctrl-C at that prompt + // (existingTableAction folds them into proceed=false), so one note + // covers both — via the shared cleanCancel. + return nil, "", true, cleanCancel(a.Printer, + "%q was left as-is; nothing was ingested.", existingTable) } a.Overwrite = true } diff --git a/internal/cli/data_ingest_local.go b/internal/cli/data_ingest_local.go index a4a394a5..a28436b4 100644 --- a/internal/cli/data_ingest_local.go +++ b/internal/cli/data_ingest_local.go @@ -102,8 +102,8 @@ func resolveLocalInput(out, errOut io.Writer, a *runDataIngestArgs) (layout *pus if a.Interactive && a.Prompter != nil { if err := runInteractive(a.Printer, a.Prompter, a, a.TaskSet); err != nil { if errors.Is(err, errInteractiveCancelled) { - a.Printer.Infof("Cancelled — nothing was ingested.") - return nil, nil, nil, true, nil + // cleanCancel prints the shared note and returns the clean exit. + return nil, nil, nil, true, cleanCancel(a.Printer, "nothing was ingested.") } // A typed exitError from a guided step (e.g. the path-existence // guard, which runInteractive runs before the family sniff) diff --git a/internal/cli/delete.go b/internal/cli/delete.go index e601f4a6..c2b3b163 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -195,11 +195,14 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er p.PromptHint("This is irreversible. Type the client name to confirm, or leave blank to cancel.") typed, perr := pr.Input(fmt.Sprintf("Type %q to offboard this machine", name), "", "", nil) if perr != nil { - return mapClientErr(perr) + // Ctrl-C mid-typing is the same "no" the mismatch branch below + // handles — same visible note, same clean exit. It used to return + // nil unprinted, so an aborted offboard looked exactly like a + // completed one to anything reading the stream (backend#1253). + return mapPromptErr(p, perr, "nothing was removed.") } if strings.TrimSpace(typed) != name { - p.Infof("Cancelled — the name didn't match. Nothing was removed.") - return nil + return cleanCancel(p, "the name didn't match. Nothing was removed.") } } diff --git a/internal/cli/exitcodes.go b/internal/cli/exitcodes.go index ba6db4fa..e5a2191c 100644 --- a/internal/cli/exitcodes.go +++ b/internal/cli/exitcodes.go @@ -17,8 +17,10 @@ package cli // constant per MEANING sharing the value, so each construction site stays // honest and the docs table maps number → per-command meaning. const ( - // exitOK: success. Includes --dry-run completing, a guided run the - // user cancelled cleanly, and doctor passing with warnings only. + // exitOK: success. Includes --dry-run completing, any prompt the user + // declined or cancelled with Ctrl-C (always with a visible "Cancelled — + // …" note — see cleanCancel in interactive.go), and doctor passing with + // warnings only. exitOK = 0 // exitFailure: generic failure with no more specific bucket (cobra @@ -83,8 +85,16 @@ const ( // couldn't be determined / followed within the watch window. exitIngestFailed = 9 - // exitInterrupted: the user hit Ctrl-C at an interactive prompt - // (128+SIGINT, the shell convention). Emitted silent (err == nil) so - // main() prints no "Error:" line on the way out. + // exitInterrupted: the user hit Ctrl-C while an operation was already in + // flight — the sign-in wait, `client status --wait`, the seal suite, or an + // installer re-run (upgrade / prepare-host) — 128+SIGINT, the shell + // convention. Emitted silent (err == nil) so main() prints no "Error:" + // line on the way out. + // + // NOT for a cancelled PROMPT. Backing out at a question starts nothing, so + // every prompt site reports that through cleanCancel instead: a visible + // "Cancelled — …" note and exitOK (interactive.go, backend#1253). This + // comment used to claim the prompt case, contradicting exitOK above and + // every call site. exitInterrupted = 130 ) diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index 1c56c729..c388483a 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -21,8 +21,9 @@ import ( // testable without a pseudo-terminal — the same trick kubernetes.Interface // uses to let cluster code run against a fake clientset. // errInteractiveCancelled is returned when the user declines the -// confirm prompt or hits Ctrl-C. It's control flow, not a failure: -// runDataIngest maps it to a clean exit (0) with a "Cancelled" note. +// confirm prompt or hits Ctrl-C. It's control flow, not a failure — +// every site reports it through cleanCancel / mapPromptErr below: a +// visible "Cancelled — …" note and a clean exit (0). var errInteractiveCancelled = errors.New("cancelled by user") type prompter interface { @@ -103,6 +104,48 @@ func mapErr(err error) error { return err } +// cleanCancel is the ONE place a cancelled prompt is reported to the user. It +// prints the CLI's cancellation line — "Cancelled — ." — and returns +// nil, which ExitCodeFromError maps to exitOK. +// +// Exit 0 is the convention every prompting command follows (data ingest, data +// delete, resources set, client create, delete): backing out at a question is a +// user choice, and nothing was started, so there is no failure to report. +// exitInterrupted (130) is for the OTHER Ctrl-C — the one that interrupts work +// already in flight (the sign-in wait, `client status --wait`, the seal suite, an +// installer re-run), where an operation really was cut short. See exitcodes.go. +// +// nothing says what did NOT happen ("nothing was changed."), and takes format +// args for the sites that name the thing they left alone. The prefix lives here +// so no site invents its own wording, and the argument is required so no site can +// report a cancellation without saying what it left untouched. +func cleanCancel(p *ui.Printer, nothing string, a ...any) error { + p.Infof("Cancelled — %s", fmt.Sprintf(nothing, a...)) + return nil +} + +// mapPromptErr maps a prompter error to the CLI's exit contract, so a prompt can +// neither fail nor be cancelled silently. Ctrl-C (errInteractiveCancelled, from +// mapErr above) goes through cleanCancel — the same visible note and the same +// exit 0 the site's declined-answer branch produces. Anything else is a real +// prompt failure: exit 1. +// +// The Printer and the note are in the signature deliberately. The bug this +// replaced mapped the cancellation straight to nil, so Ctrl-C exited 0 with no +// output at all — a script could not tell it apart from a completed run +// (backend#1253). Handling the sentinel now costs you a note; printing nothing +// is no longer reachable. +// +// Sites whose non-cancel error needs a code other than exitFailure keep their own +// errors.Is check and call cleanCancel directly — the printing still funnels +// through one place. +func mapPromptErr(p *ui.Printer, err error, nothing string, a ...any) error { + if errors.Is(err, errInteractiveCancelled) { + return cleanCancel(p, nothing, a...) + } + return &exitError{code: exitFailure, err: err} +} + // isInteractiveTTY reports whether we can run a guided prompt flow: // both stdin (we read answers) and stdout (we draw prompts) must be a // real terminal. Piped input, redirected output, or CI all fail this diff --git a/internal/cli/pure_helpers_coverage_test.go b/internal/cli/pure_helpers_coverage_test.go index c6e6a553..7e2b92b8 100644 --- a/internal/cli/pure_helpers_coverage_test.go +++ b/internal/cli/pure_helpers_coverage_test.go @@ -1,7 +1,9 @@ package cli import ( + "bytes" "errors" + "strings" "testing" "github.com/AlecAivazis/survey/v2/terminal" @@ -10,6 +12,7 @@ import ( "github.com/tracebloc/cli/internal/doctor" "github.com/tracebloc/cli/internal/resources" + "github.com/tracebloc/cli/internal/ui" ) // TestMapErr pins the interactive-cancel seam contract (interactive.go:82, @@ -29,17 +32,28 @@ func TestMapErr(t *testing.T) { } } -// TestMapClientErr pins client.go:1015 (0%): a cancelled prompt is a clean exit -// (nil); anything else becomes an exit-1 *exitError. -func TestMapClientErr(t *testing.T) { - if err := mapClientErr(errInteractiveCancelled); err != nil { +// TestMapPromptErr pins the seam's exit contract: a cancelled prompt is a clean +// exit (nil ⇒ exit 0) AND prints the shared note — the two are inseparable here, +// which is the whole point of the helper (backend#1253). Anything else becomes an +// exit-1 *exitError, with nothing printed (main() reports it). +func TestMapPromptErr(t *testing.T) { + var out bytes.Buffer + if err := mapPromptErr(ui.New(&out, ui.WithColor(false)), errInteractiveCancelled, "nothing was changed."); err != nil { t.Errorf("cancel must map to a clean nil, got %v", err) } - err := mapClientErr(errors.New("nope")) + if got := out.String(); !strings.Contains(got, "Cancelled — nothing was changed.") { + t.Errorf("cancel must print the shared note, got %q", got) + } + + out.Reset() + err := mapPromptErr(ui.New(&out, ui.WithColor(false)), errors.New("nope"), "nothing was changed.") var ee *exitError if !errors.As(err, &ee) || ee.Code() != 1 { t.Errorf("a real error must become exit 1, got %v", err) } + if out.String() != "" { + t.Errorf("a real prompt failure must not print a cancellation note, got %q", out.String()) + } } // TestWorseStatus pins the doctor verdict truth-table (doctor.go:229, was 40% — diff --git a/internal/cli/resources_set.go b/internal/cli/resources_set.go index 34010afa..d5146a46 100644 --- a/internal/cli/resources_set.go +++ b/internal/cli/resources_set.go @@ -216,9 +216,11 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * // failures pass through unchanged. desired, err := decideDesired(p, pr, req, node, current, machineGPUName, machineGPUCount, machineHasGPU) if err != nil { + // Not mapPromptErr: a validation error from the wizard carries its own + // code (exit 2) and must pass through unchanged, so only the cancel is + // funnelled through the shared note. if errors.Is(err, errInteractiveCancelled) { - p.Infof("Cancelled — nothing was changed.") - return nil + return cleanCancel(p, "nothing was changed.") } return err } @@ -273,18 +275,13 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * p.PromptHint("tracebloc keeps about 1 core and 3 GiB for itself on top of this — it fits on this machine.") proceed, cerr := pr.Confirm(fmt.Sprintf("Let each training run use up to %s?", perRunSize(desired)), true) if cerr != nil { - // Ctrl-C here is the same user choice as answering "No": print the - // same note the decline below (and a wizard interrupt above) prints, - // and exit 0 — never a silent success that hides the abort. - if errors.Is(cerr, errInteractiveCancelled) { - p.Infof("Cancelled — nothing was changed.") - return nil - } - return mapClientErr(cerr) + // Ctrl-C here is the same user choice as answering "No": the same + // note the decline below (and a wizard interrupt above) prints, and + // exit 0 — never a silent success that hides the abort. + return mapPromptErr(p, cerr, "nothing was changed.") } if !proceed { - p.Infof("Cancelled — nothing was changed.") - return nil + return cleanCancel(p, "nothing was changed.") } } diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 5781b16f..f2341e53 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -100,11 +100,13 @@ screen. %s/%d are runtime placeholders. "CSV %s has no columns" "Can't reach tracebloc from here." "Cancelled — %q was left as-is; nothing was ingested." +"Cancelled — %s" "Cancelled — nothing was changed." "Cancelled — nothing was deleted." "Cancelled — nothing was ingested." +"Cancelled — nothing was provisioned." +"Cancelled — nothing was removed." "Cancelled — the name didn't match. Nothing was removed." -"Cancelled." "Chart uninstall reported: %v" "Check on it later with: kubectl logs -f -n %s job/%s" "Check your data" From b766f395047584a9f723435cef220b7af4e1573f Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:09:56 +0200 Subject: [PATCH 2/9] docs(bugbot): add .cursor/BUGBOT.md project context (backend#930) (#409) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor Bugbot reviews this repo with zero project context today — no repo in the org has a BUGBOT.md. This repo draws the most findings of any (232), and the recurring classes here are dishonest outcome reporting and mishandled interrupts, not generic security nits. Writing the house rules down stops Bugbot re-deriving them and lets its one pass land on the hard findings. Encodes the invariants with the reason and a real reference each: honest outcome reporting via classifyPushOutcome (a Job exiting 0 with row failures is "completed_with_failures", not "succeeded"), the FROZEN exit code contract, visible feedback on every errInteractiveCancelled path — including that mapClientErr swallows it silently today — HTTP 426 as a hard stop never a warning, fail-closed cosign/SHA256 verification in install.sh, per-call timeouts, empty/nil guards at boundaries, the cross-repo pin + generated-artifact rules, and the STYLE.md output contract. Also records verified non-issues: .golangci.yml does NOT gate CI (pinned standalone binaries do), staticcheck's deliberate -ST1005 exclusion, the single documented nolint, and the deadcode allowlist. Deliberately omits a SLSA/provenance claim — signing here is cosign keyless, and the term appears nowhere in the repo. Item 4 of tracebloc/backend#930. Co-authored-by: Claude Fable 5 --- .cursor/BUGBOT.md | 127 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 .cursor/BUGBOT.md diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md new file mode 100644 index 00000000..e4e67b71 --- /dev/null +++ b/.cursor/BUGBOT.md @@ -0,0 +1,127 @@ +# Bugbot guide — tracebloc/cli + +## Context + +Public Go CLI (Apache-2.0), shipped as **signed 8-platform releases** (cosign keyless, +verified by `scripts/install.sh`). Customers run it on their own machines against their own +Kubernetes to operate a self-hosted secure environment. It talks to a public HTTPS backend +(`internal/api`) and to an in-cluster jobs-manager (`internal/submit`), and shells out to +`kubectl`/`helm`/`docker`. + +Two things make this repo unusual and should shape every finding: + +1. **Its exit codes are a scripting contract** — customers branch on them + (`internal/cli/exitcodes.go`: "the numeric values are FROZEN"). +2. **`make ci` mirrors CI exactly.** The `Makefile` header states it outright: "divergence + between local and CI is the bug this file exists to prevent." Tool versions are pinned in + lockstep with `.github/workflows/build.yml`. + +## Always flag + +- **A command that reports success it hasn't earned.** Exiting 0 is not the same as + succeeding. The reference pattern is `classifyPushOutcome` + (`internal/cli/data_ingest_output.go:25`): a Job that exits 0 but whose summary reports row + failures returns `"completed_with_failures"` + `exitIngestFailed`, *not* `"succeeded"` — its + comment cites this class explicitly. `internal/doctor` carries the same idea in + `StatusUnknown`: a check that ran but cannot back a green prints a neutral line rather than a + false ✔. Flag any new multi-step command where a partial failure collapses into success, and + any path where the `--output-json` status and the process exit code can disagree. + +- **An exit code that isn't a named constant from `internal/cli/exitcodes.go`**, a repurposed + numeric value, or a new failure path returning generic `1` when a specific code already + exists. Every non-test `&exitError{}` names its code. + +- **A prompt whose cancellation produces no visible feedback.** `errInteractiveCancelled` + (`internal/cli/interactive.go:26`) must print a `Cancelled — …` line via the Printer and then + return cleanly — see `resources_set.go:219`, `data_delete.go:233`, + `data_ingest_local.go:103`, `data_ingest_cluster.go:339`. Watch for + **`mapClientErr` (`internal/cli/client.go:858`), which maps it straight to `nil` with no + output at all** — a Ctrl-C routed through it exits 0 in total silence, right next to a + declined-answer branch that *does* print (`client.go:334`, `delete.go:196`). Check this at + every new or changed prompt site. Signals are wired centrally via `signal.NotifyContext` + (`cmd/tracebloc/main.go:58`) so deferred cleanup runs — a bare handler skips it and breaks + `push.Stage`'s cleanup contract. Interrupted-but-clean paths exit 130. + +- **HTTP 426 treated as anything other than a hard stop.** It is detected centrally + (`internal/api/client.go`, `parseUpgradeRequired` → `*UpgradeRequiredError`) so every caller + degrades to the same actionable "run `tracebloc upgrade`" message — see `auth.go:336`, + `doctor.go:119`, `client_status.go:129`, `delete.go:151,218`, `client.go:253`. Flag a new API + consumer that retries through it, frames it as a transient outage, or folds it into a generic + error. A too-old CLI never recovers by waiting, so `--wait` loops must fail fast on it. + +- **Verification that degrades to a warning.** In `scripts/install.sh` the SHA256 compare + aborts when no hashing tool is present, and `verify_cosign_signature()` bootstraps a pinned, + checksum-verified cosign (`COSIGN_VERSION=v2.4.1`) rather than skipping; the only bypass is an + explicit `TRACEBLOC_ALLOW_UNVERIFIED=1` with a loud warning. A previous "warn + continue + + still print ✓ matches" branch was caught as *both* a security regression and a dishonest log. + Also flag any `--version` / `RELEASE_VERSION` use that skips `validate_version_tag` before URL + interpolation. `tracebloc upgrade` and host prep must keep delegating to this verified script + instead of reimplementing verification in Go. + +- **An external call with no ceiling.** Backend HTTP: `defaultTimeout = 30 * time.Second` + (`internal/api/client.go:31`). In-cluster submit: `SubmitTimeout` + (`internal/submit/client.go:21`). Doctor probes: `httpProbeTimeout = 8s`. Every shell-out uses + `exec.CommandContext`. Flag a bare `exec.Command` in non-test code, an `http.Client{}` with no + `Timeout`, or a watch/poll loop with no deadline. + +- **A missing empty / nil / zero guard on anything crossing a boundary** (user input, API + response, cluster state). There is no shared validator — the convention is a colocated + `validate*` func: `internal/push/spec.go:100` (`ValidateTableName`), + `internal/cli/interactive.go:537-568`. Two specifics: a bare Enter yields `""` and must not be + treated as a real path (`validateDatasetPath` documents exactly this); and pagination must + fail loudly on an unparseable `next` link rather than silently truncating the list + (`internal/api/client.go`, `nextPath`). Where "empty" and "unknown" are different answers, + prefer a three-valued return (`internal/cluster/discover.go:302`). + +- **A cross-repo contract change that only lands on one side.** `scripts/.data-ingestors-ref`, + `scripts/.client-ref` and `scripts/.backend-ref` pin upstream refs deliberately so an + unrelated upstream commit can't red every open PR. Flag a hand-edit to a generated artifact + (`internal/schema/*.json`, `internal/api/testdata/*.json`, + `internal/push/testdata/parity/goldens.json`, `internal/cli/testdata/golden/*.golden`) that + doesn't also bump and re-sync its pin, and any change to a chart assumption (discovery labels, + jobs-manager port, PVC mount path) that doesn't update `scripts/chart-invariants` — a chart + rename otherwise ships green in both repos and breaks discovery in the field. + +- **Output that breaks the style contract** (`STYLE.md`): all colour goes through + `internal/ui`'s Printer — never inline an escape or brand hex outside `internal/ui` + (`scripts/check-style.sh` greps for it). Colour is never load-bearing: headings carry bold, + alerts carry a glyph, so the output still reads under `NO_COLOR`, in a pipe, and for a + colour-blind reader. User-facing copy follows the terminology table ("secure environment", + "ingest", "delete", "Online/Offline", "collaborators", "task"); only the workspace → secure + environment swap is grep-enforced, the rest is review judgement. A new user-facing string + almost always needs its golden regenerated: + `TB_UPDATE_GOLDEN=1 go test ./internal/cli/ -run TestCopyCatalog`. + +- **Errors that lose their type.** `%w` wrapping is the house convention (~325 sites), with + typed errors for the cases callers branch on: `APIError`, `UpgradeRequiredError`, + `SubmitError`, `WatchError`, `exitError`, `noParentReleaseError`. Flag string-matching on an + error message where `errors.Is`/`errors.As` applies. + +## Known non-issues — do not flag + +- **`.golangci.yml` does not gate CI.** `golangci-lint` is never invoked in a workflow (its + `staticcheck`/`unused` are disabled there for runner OOM reasons); the blocking Lint job runs + pinned standalone binaries — `errcheck`, `gofmt -s`, `goimports`, `ineffassign`, `misspell`, + `staticcheck`, plus `deadcode-check.sh`, `file-budget.sh`, `check-style.sh`. Don't infer + coverage from that file. +- **`staticcheck` runs `-checks all,-ST1005` deliberately** — do not flag error-string + capitalisation or punctuation. It is a tracked, intentional exclusion (cli#279). +- `internal/submit/client.go:78` — `InsecureSkipVerify` is intentional for cluster-internal + traffic with no recognisable CA, documented in place and marked `//nolint:gosec`. It is the + only `nolint` in the repo. +- `scripts/deadcode-allowlist.txt` entries are verified false positives (Stringers reached only + through `fmt` reflection; test-only parity harnesses that must live in production source). +- `test/integration/*` uses 30s–5min timeouts because it drives a real cluster — not the + production timeout convention. +- `mutation.yml` and `head-drift-canary.yml` are advisory and never gate a merge. +- No `vendor/` directory — the module cache is used on purpose. +- `// style-guard: allow` is a defined escape hatch but is currently used nowhere; if one + appears, it is a novel exception worth scrutiny rather than an accepted pattern. + +## Tone + +Direct. Name the file and line. Give a concrete fix, not "consider". State the customer-visible +consequence — what they see, and which exit code they get — not just the code smell. + +This repo is **public**: never put a customer name, internal hostname, or internal-only ticket +detail in a finding. A bare `tracebloc/backend#NNNN` reference is fine. From 292116ebaf753f9dfe4427b541eac23c3f3bfe92 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:12:30 +0200 Subject: [PATCH 3/9] docs(pr-template): owner-qualify cross-repo closing keywords (tracebloc/backend#930) (#411) GitHub auto-closes an issue in another repository only when the PR body names it owner-qualified. A bare `repo#N` merely cross-references and closes nothing -- and the template's own hint taught `Ref tracebloc/other-repo#456`, which is not a closing keyword at all. Eight code-complete issues stayed open for days-to-weeks this way (tracebloc/backend#1171-#1176, tracebloc/client#376, tracebloc/cli#393), dragging two epics to 0% and 14% when the true figures were 67% and 24%. Someone had to notice and close all eight by hand. Also corrects CONTRIBUTING.md, which asserted that a `Closes #N` body line auto-closes on merge. This repo's default branch is `main` while PRs land on `develop`, and GitHub fires closing keywords only on merges into the default branch -- so that claim was wrong in both directions and helped propagate the bug. cli#393 is one of the eight. Co-authored-by: Claude Fable 5 --- .github/pull_request_template.md | 3 ++- CONTRIBUTING.md | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 97c7f3c2..cf6b56bc 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -2,7 +2,7 @@ ## Related - + ## Type of change - [ ] Feature @@ -19,3 +19,4 @@ - [ ] `go build ./...`, `go vet`, and the Lint job's checks pass locally - [ ] Terminal output follows [STYLE.md](../STYLE.md) — Printer tones (no hardcoded colour/emoji), "secure environment" not "workspace"; `bash scripts/check-style.sh` passes - [ ] No secrets / credentials in the diff +- [ ] Cross-repo issues use `Fixes tracebloc/#N` — a bare `repo#N` closes nothing diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b1374a67..15123e47 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -55,7 +55,7 @@ This repo follows the same conventions as the rest of the tracebloc org: fix(#150): handle empty kubeconfig gracefully ``` -- **PR body** should include a `Closes #N` line (on its own line) for any ticket the PR fully resolves. GitHub auto-closes the issue on merge. The `feat(#N):` convention in the title is for kanban tracking; `Closes #N` in the body is what triggers auto-close. +- **PR body** should include a `Closes #N` line (on its own line) for any ticket the PR fully resolves. The `feat(#N):` convention in the title is for kanban tracking; the body line is what links the issue. Owner-qualify anything in another repo — `Fixes tracebloc/backend#123` — because a bare `backend#123` only cross-references and closes nothing. And since GitHub fires closing keywords only on merges into the default branch (`main`), a PR merged to `develop` won't auto-close on its own: confirm the ticket, and close it by hand if needed. - **One PR per ticket** when practical. Roll-up sync PRs (`Sync develop → main for vX.Y.Z release`) are an exception. From 25db81db83c4f3bd902f82d00e0dd6aa9c29e94a Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:14:58 +0200 Subject: [PATCH 4/9] docs(rfc): state qualified identifiers in the RFC headers (#412) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC numbers are assigned per repo, so a bare "RFC 0001" names four different documents across the org and "RFC 0002"/"RFC 0003" name two each. Add the qualified ID (`RFC-CLI-0001` … `RFC-CLI-0003`) to each header, in the blockquote style these documents already use. Two of the three also record which document the existing bare in-code references actually mean: nearly all `RFC-0001` comments across backend/cli/client point at RFC-CLI-0001, and the `RFC-0003` references in client's chart templates and docs/SEAL-CHECK.md point at RFC-CLI-0003 — not at backend's own 0001/0003. Nothing is renumbered and no file is moved, so existing links still resolve. The org-wide index lives in tracebloc/backend (private) at docs/rfcs/README.md. Co-authored-by: Claude Fable 5 --- docs/rfcs/0001-cli-auth-and-client-provisioning.md | 6 ++++++ docs/rfcs/0002-data-ingest-flow.md | 4 ++++ docs/rfcs/0003-storage-and-offboard-hygiene.md | 6 ++++++ 3 files changed, 16 insertions(+) diff --git a/docs/rfcs/0001-cli-auth-and-client-provisioning.md b/docs/rfcs/0001-cli-auth-and-client-provisioning.md index ff49b697..7f1ccab7 100644 --- a/docs/rfcs/0001-cli-auth-and-client-provisioning.md +++ b/docs/rfcs/0001-cli-auth-and-client-provisioning.md @@ -1,5 +1,11 @@ # RFC 0001 — Browser-based auth & one-command client provisioning +> **Qualified ID: `RFC-CLI-0001`.** Cite this document by its qualified ID, never as +> a bare "RFC 0001" — `backend` and `client` each have their own 0001. Note that +> most existing bare `RFC-0001` references in the `backend`, `cli` and `client` +> codebases mean **this** document. Org-wide index: `docs/rfcs/README.md` in +> `tracebloc/backend` (private). +> > **Status: ACCEPTED — implemented.** The design in this RFC shipped in > **CLI v0.4.0** ([cli#107](https://github.com/tracebloc/cli/pull/107)); the > tracking epic ([cli#54](https://github.com/tracebloc/cli/issues/54)) is closed. diff --git a/docs/rfcs/0002-data-ingest-flow.md b/docs/rfcs/0002-data-ingest-flow.md index 1a3f6814..df4207bd 100644 --- a/docs/rfcs/0002-data-ingest-flow.md +++ b/docs/rfcs/0002-data-ingest-flow.md @@ -1,5 +1,9 @@ # RFC 0002 — `tracebloc data ingest`: flow, terminology & task taxonomy +> **Qualified ID: `RFC-CLI-0002`.** Cite this document by its qualified ID, never as +> a bare "RFC 0002" — `backend` also has a 0002 (platform cost & autoscaling). +> Org-wide index: `docs/rfcs/README.md` in `tracebloc/backend` (private). +> > **Status: DRAFT — for discussion.** Owner: @LukasWodka. Last updated: 2026-07-07. > > This RFC captures the redesign of the `tracebloc data ingest` user diff --git a/docs/rfcs/0003-storage-and-offboard-hygiene.md b/docs/rfcs/0003-storage-and-offboard-hygiene.md index eeb381dd..2f21dfc4 100644 --- a/docs/rfcs/0003-storage-and-offboard-hygiene.md +++ b/docs/rfcs/0003-storage-and-offboard-hygiene.md @@ -1,5 +1,11 @@ # RFC 0003 — The secure environment: dataset storage, offboard hygiene & the boundary +> **Qualified ID: `RFC-CLI-0003`.** Cite this document by its qualified ID, never as +> a bare "RFC 0003" — `backend` also has a 0003 (configurable preprocessing & +> imputation). Note that the existing bare `RFC-0003` references in the `client` +> chart templates and `docs/SEAL-CHECK.md` mean **this** document. Org-wide index: +> `docs/rfcs/README.md` in `tracebloc/backend` (private). +> > **Status: DECIDED — v2.3; decisions D1–D20 locked (D1–D15 2026-07-22; > D16–D20 2026-07-23). Execution tickets filed and cross-linked in §10/§12; > D16–D20 (per-dataset isolation) are decided but not yet ticketed, with From 19ea73fc9917fd569425b2f9540169cdf9a1adb5 Mon Sep 17 00:00:00 2001 From: shujaat_tracebloc <153823837+shujaatTracebloc@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:48:29 +0200 Subject: [PATCH 5/9] fix: skip update check (not just cache write) when config dir is absent (Bugbot #397) (#414) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #404 made writeUpdateCache refuse to recreate a wiped ~/.tracebloc, but latestReleaseVersion still fetched from GitHub whenever the cache couldn't be read. On a fresh install / after offboard the dir is absent, so the throttle can never be persisted and every TTY command re-hit the releases API, burning updateCheckTimeout each time. Reconcile both: introduce configDirExists as the single "can the throttle be persisted?" gate, used by writeUpdateCache (skip write — #404) AND latestReleaseVersion (skip the network check entirely — #397) when the dir is absent. A dir-present-but-stale/unreadable cache still falls through to the normal throttled fetch, so the everyday path is unchanged. Co-authored-by: shujaat hasan Co-authored-by: Claude Opus 4.8 --- internal/cli/update_check.go | 31 +++++++++++++++++++--- internal/cli/update_check_test.go | 44 +++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/internal/cli/update_check.go b/internal/cli/update_check.go index e4d319e6..8f317c48 100644 --- a/internal/cli/update_check.go +++ b/internal/cli/update_check.go @@ -80,6 +80,16 @@ func latestReleaseVersion() string { if c, ok := readUpdateCache(path); ok && time.Since(c.CheckedAt) < updateCheckInterval { return c.Latest } + // No fresh cache. If the config dir is absent, the throttle can't be persisted + // (writeUpdateCache won't recreate it — Bugbot #404), so fetching here would + // repeat on EVERY command and burn updateCheckTimeout each time. A missing dir + // also means the CLI isn't set up (fresh install) or was offboarded — nothing + // to nudge about. Skip the network check entirely. The dir-present but + // stale/unreadable case still falls through to the throttled fetch below, so + // this doesn't defeat the normal path (Bugbot #397). + if !configDirExists(path) { + return "" + } latest, err := fetchLatestRelease(latestReleaseURL, updateCheckTimeout) if err != nil { if c, ok := readUpdateCache(path); ok { @@ -134,6 +144,22 @@ func updateCachePath() string { return filepath.Join(dir, updateCacheFile) } +// configDirExists reports whether the tracebloc config dir (the parent of the +// update cache) is present — the single gate for "can the update-check throttle +// be persisted?". When it's absent (a fresh install, or a wiped/offboarded +// ~/.tracebloc) the cache can neither be read nor written, so the caller must +// no-op: latestReleaseVersion skips the network check (so a fetch isn't repeated +// unthrottled on every command — Bugbot #397) and writeUpdateCache skips the +// write (so a throttle cache never resurrects a just-wiped dir — Bugbot #404). +// An empty path (config.Dir() failed) counts as absent. +func configDirExists(cachePath string) bool { + if cachePath == "" { + return false + } + _, err := os.Stat(filepath.Dir(cachePath)) + return err == nil +} + func readUpdateCache(path string) (updateCache, bool) { if path == "" { return updateCache{}, false @@ -158,10 +184,7 @@ func readUpdateCache(path string) (updateCache, bool) { // login/client-create and delete, never by a throttle cache. A missing dir is a // silent no-op (the throttle simply isn't persisted until the dir exists again). func writeUpdateCache(path string, c updateCache) error { - if path == "" { - return nil - } - if _, err := os.Stat(filepath.Dir(path)); err != nil { + if !configDirExists(path) { return nil // dir gone (fresh machine, or just-offboarded) — don't recreate it } raw, err := json.Marshal(c) diff --git a/internal/cli/update_check_test.go b/internal/cli/update_check_test.go index 3a5bb3f1..989d95ed 100644 --- a/internal/cli/update_check_test.go +++ b/internal/cli/update_check_test.go @@ -118,6 +118,50 @@ func TestLatestReleaseVersion_FreshCacheSkipsNetwork(t *testing.T) { } } +// An ABSENT config dir (fresh install / offboarded) must SKIP the network check +// entirely — not hit GitHub on every command. writeUpdateCache can't persist the +// throttle without the dir (Bugbot #404), so an unconditional fetch would repeat +// forever and burn updateCheckTimeout each time (Bugbot #397). Distinct from the +// dir-present-but-stale case, which still fetches (throttled) below. +func TestLatestReleaseVersion_MissingConfigDirSkipsNetwork(t *testing.T) { + absent := filepath.Join(t.TempDir(), "nope") // deliberately never created + t.Setenv("TRACEBLOC_CONFIG_DIR", absent) + // A server that fails the test if it's ever hit — proves no fetch is attempted. + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Error("network hit despite an absent config dir — the update check must be skipped") + })) + defer srv.Close() + swapURL(t, srv.URL) + + if got := latestReleaseVersion(); got != "" { + t.Errorf("latestReleaseVersion = %q, want \"\" (skipped: no config dir)", got) + } + // The skipped check must not have resurrected the dir either (reconciles #404). + if _, err := os.Stat(absent); !os.IsNotExist(err) { + t.Errorf("update check must not create the missing config dir %s (err=%v)", absent, err) + } +} + +// The dir-present-but-no-cache case (e.g. right after login created ~/.tracebloc) +// must still fetch and then persist the throttle — the missing-dir skip must NOT +// bleed into the normal path, or the once-a-day throttle would never arm. +func TestLatestReleaseVersion_DirPresentNoCacheFetchesAndPersists(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) // dir exists; no cache file yet + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"tag_name":"v2.0.0"}`)) + })) + defer srv.Close() + swapURL(t, srv.URL) + + if got := latestReleaseVersion(); got != "2.0.0" { + t.Errorf("latestReleaseVersion = %q, want 2.0.0 (dir present, no cache → fetch)", got) + } + // The fetch must have persisted the throttle so the next call is served from cache. + if c, ok := readUpdateCache(updateCachePath()); !ok || c.Latest != "2.0.0" { + t.Errorf("throttle not persisted after a dir-present fetch: %+v ok=%v", c, ok) + } +} + func TestLatestReleaseVersion_StaleCacheFetchesAndRewrites(t *testing.T) { t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { From cc715379b804a57176a5b3bfc7898540c3f60c76 Mon Sep 17 00:00:00 2001 From: "Asad Iqbal (Saadi)" Date: Mon, 27 Jul 2026 20:07:54 +0500 Subject: [PATCH 6/9] chore: back-merge main into develop after v0.10.0 promote (#416) Co-authored-by: Claude Opus 4.8 From 83606743c9385eedde1291e485b08a5a41568aae Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:09:19 +0200 Subject: [PATCH 7/9] fix(install): anchor cosign identity to version tags, not any ref (D29) (#415) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit install.sh verified downloaded binaries against certificate-identity-regexp '.../release.yml@.*' — @.* matches ANY ref, so a binary signed by release.yml running on a feature branch verified identically to a tagged release. Anchor to '@refs/tags/v.*' so only tag-built releases are trusted. Same anchor applied to the documented verify command in release.yml. Part of tracebloc/backend#1269 (row 4). Co-authored-by: Claude Opus 4.8 --- .github/workflows/release.yml | 2 +- scripts/install.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 627d1bde..b18b4965 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -19,7 +19,7 @@ # # cosign verify-blob \ # --certificate-identity-regexp \ -# 'https://github.com/tracebloc/cli/.github/workflows/release.yml@.*' \ +# 'https://github.com/tracebloc/cli/.github/workflows/release.yml@refs/tags/v.*' \ # --certificate-oidc-issuer 'https://token.actions.githubusercontent.com' \ # --certificate .cert \ # --signature .sig \ diff --git a/scripts/install.sh b/scripts/install.sh index dcac1f5d..312ee668 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -361,7 +361,7 @@ verify_cosign_signature() { if "$COSIGN_BIN" verify-blob \ --certificate-identity-regexp \ - "https://github.com/${GITHUB_REPO}/.github/workflows/release.yml@.*" \ + "https://github.com/${GITHUB_REPO}/.github/workflows/release.yml@refs/tags/v.*" \ --certificate-oidc-issuer 'https://token.actions.githubusercontent.com' \ --certificate "$TMP/$BINARY_FILE.cert" \ --signature "$TMP/$BINARY_FILE.sig" \ From b8841107b6bb4e25f92382a2cc425dcdbbd7c9db Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:25:42 +0200 Subject: [PATCH 8/9] chore: declare next release in VERSION (0.10.1) for the release train (#417) * chore: declare next release in VERSION (0.10.1) for the release train The train reads this file to cut tags: vX.Y.Z-rc.N on every staging promotion (pre-release binaries for FR; 'latest' never sees them) and vX.Y.Z on the prod promotion. The binary's own version still derives from the tag at build time (release.yml ldflags) -- this file only declares intent, uniform with tracebloc-py-package's pyproject version. 0.10.1 ships the cosign identity anchor fix (#415). Co-Authored-By: Claude Opus 4.8 * feat(release): enforce tag==VERSION + strict prerelease detection Two guards raised in release-train review: (1) any tag's base X.Y.Z must match the VERSION file (train-cut or manual), so the file can never go silently stale after an out-of-train release; pre-VERSION tags are grandfathered for rebuilds. (2) STRICT stability: only plain vX.Y.Z is a stable release -- rc tags AND malformed variants (v1.2.3rc1, no dash) are prereleases, so 'releases/latest' (the installer bootstrap) can only ever resolve a real production build. Co-Authored-By: Claude Opus 4.8 * style: quote GITHUB_OUTPUT redirects (shellcheck SC2086) Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .github/workflows/release.yml | 36 +++++++++++++++++++++++++++++------ VERSION | 1 + 2 files changed, 31 insertions(+), 6 deletions(-) create mode 100644 VERSION diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b18b4965..bb79a56b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -100,8 +100,23 @@ jobs: REF="${{ inputs.ref || github.ref_name }}" # Strip the leading v for use in -X main.version VERSION="${REF#v}" - echo "tag=$REF" >> $GITHUB_OUTPUT - echo "version=$VERSION" >> $GITHUB_OUTPUT + # The VERSION file declares the next release (read by the release + # train to cut rc/final tags). Any tag -- train-cut OR manual -- + # must agree with it, so the file can never go silently stale + # after an out-of-train release. Refs from before the file existed + # (rebuilds of old tags) are grandfathered with a warning. + BASE=$(printf '%s' "$VERSION" | grep -oE '^[0-9]+\.[0-9]+\.[0-9]+') + if [ -f VERSION ]; then + FILE_VERSION=$(tr -d '[:space:]' < VERSION) + if [ "$BASE" != "$FILE_VERSION" ]; then + echo "::error::tag $REF (base $BASE) does not match VERSION ($FILE_VERSION) - bump VERSION on develop first." + exit 1 + fi + else + echo "::warning::no VERSION file at this ref (pre-train tag) - skipping the consistency check." + fi + echo "tag=$REF" >> "$GITHUB_OUTPUT" + echo "version=$VERSION" >> "$GITHUB_OUTPUT" echo "Building $REF (version=$VERSION)" - name: Build binary @@ -202,7 +217,16 @@ jobs: id: tag run: | REF="${{ inputs.ref || github.ref_name }}" - echo "tag=$REF" >> $GITHUB_OUTPUT + # STRICT stability rule: only a plain vX.Y.Z tag is a stable release. + # Anything else (v1.2.3-rc.1, and typos like v1.2.3rc1) is marked + # prerelease, so it can never become 'latest' -- which is what the + # installer bootstrap (releases/latest/download/...) resolves. + if printf '%s' "$REF" | grep -qE '^v[0-9]+\.[0-9]+\.[0-9]+$'; then + echo "prerelease=false" >> "$GITHUB_OUTPUT" + else + echo "prerelease=true" >> "$GITHUB_OUTPUT" + fi + echo "tag=$REF" >> "$GITHUB_OUTPUT" - name: Create GitHub Release uses: softprops/action-gh-release@v3 @@ -210,9 +234,9 @@ jobs: tag_name: ${{ steps.tag.outputs.tag }} name: ${{ steps.tag.outputs.tag }} generate_release_notes: true - # Mark as prerelease for v*.*.* tags containing - (e.g. - # v0.1.0-rc1). Plain semver releases are stable. - prerelease: ${{ contains(steps.tag.outputs.tag, '-') }} + # Strict: only plain vX.Y.Z is stable (computed above); rc tags and + # malformed variants are prereleases and never become 'latest'. + prerelease: ${{ steps.tag.outputs.prerelease == 'true' }} files: | dist/tracebloc-* dist/SHA256SUMS diff --git a/VERSION b/VERSION new file mode 100644 index 00000000..57121573 --- /dev/null +++ b/VERSION @@ -0,0 +1 @@ +0.10.1 From cd995b501803881fd27037d6265e2617d4b9ac86 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:04:48 +0200 Subject: [PATCH 9/9] fix(install.ps1): anchor cosign identity to version tags (parity with #415) (#422) install.sh rejects signatures from non-tag workflow runs; install.ps1 still accepted @.* -- Windows trusted what Unix refused. Same anchor now: refs/tags/v.* (covers rc tags too). Co-authored-by: Claude Opus 4.8 --- scripts/install.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/install.ps1 b/scripts/install.ps1 index ab8f84b0..cc63714b 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -163,7 +163,7 @@ try { # exception. & invokes cosign as an external process, # which doesn't interact with $ErrorActionPreference. & cosign verify-blob ` - --certificate-identity-regexp "https://github.com/$GitHubRepo/.github/workflows/release.yml@.*" ` + --certificate-identity-regexp "https://github.com/$GitHubRepo/.github/workflows/release.yml@refs/tags/v.*" ` --certificate-oidc-issuer 'https://token.actions.githubusercontent.com' ` --certificate (Join-Path $tmpDir "$binaryFile.cert") ` --signature (Join-Path $tmpDir "$binaryFile.sig") `