fix(prompts): a cancelled prompt is never silent — one helper, exit 0, a visible note - #410
Conversation
…, a visible note 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 — <nothing>." 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 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 396793e. Configure here.
Independent verification of the exit-code callI re-derived this from // exitOK: success. Includes --dry-run completing, a guided run the
// user cancelled cleanly, and doctor passing with warnings only.
exitOK = 0
...
// exitInterrupted: the user hit Ctrl-C at an interactive prompt
// (128+SIGINT, the shell convention). ...
exitInterrupted = 130Those two comments sit a few lines apart in So fixing the two doc lines rather than the numbers is the right call — the numeric values are a scripting contract and changing one would break callers for a comment's sake. Two corrections to the brief I gave, both worth recording:
On the design
The mutation check is what I'd want to see on any test of this class — reinstating |
Summary
Ctrl-C at an interactive prompt exited 0 with nothing printed at two sites.
mapClientErrmapped the cancellation sentinel straight tonilwith no Printer call, so:client create— Ctrl-C at "Provision this client?" printed the Review block, then nothing. A script could not tell it apart from a completed provision. The very next branch (an explicit "No") did printCancelled.delete— Ctrl-C while typing the client name printed the whole irreversible "This will remove …" preview, then nothing, while the mismatched-name branch just below printedCancelled — the name didn't match. Nothing was removed.Rather than duplicate the existing pattern a fifth and sixth time, the convention is now centralised.
Exit-code decision: 0, not 130 — and the docs were self-contradictory
Decision: exit 0 with a visible
Cancelled — …line. Evidence from the code, not assumption:resources_set.go×3,data_delete.go×2,data_ingest_local.go,data_ingest_cluster.go)nil→ exit 0 — unanimous, no inconsistency between themexitInterrupted(130) sites (seal.go×2,upgrade.go,auth.go,client_status.go,prepare_host.go)exitcodes.goexitOKSo the two categories were already cleanly separated in behaviour. What was wrong was the documentation:
exitInterrupted's comment claimed "the user hit Ctrl-C at an interactive prompt", anddocs/troubleshooting.mdhad the same claim in the130row — directly contradicting its own0row two lines above, and contradicting all twelve call sites. Both are corrected here (comment + user-facing table row) to say what 130 actually means. Exit codes are frozen as a scripting contract, so no numeric value changed.Approach
mapClientErrhad 3 non-test callers (client.go,delete.go,resources_set.go), so changing its signature was safe:cleanCancel(p, nothing, …)— the one place a cancellation is reported. PrintsCancelled — <nothing>.and returns the clean exit. TheCancelled —prefix lives in the helper, so no site can invent its own wording.mapPromptErr(p, err, nothing, …)replacesmapClientErr, and moves next to the sentinel ininteractive.go. The Printer and the note are in the signature on purpose: the silent-return nilshape is no longer expressible — to handle the sentinel at all you have to say what didn't happen. Non-cancel errors still map to exit 1.resources set→ exit 2 validation passthrough,data delete→ exit 3) keep their ownerrors.Ischeck and callcleanCanceldirectly — the printing still funnels through one place.client create's terseCancelled.→Cancelled — nothing was provisioned., matching the other five.cleanCancelis a printf wrapper,go vetnow format-checks every call site.Tone follows
classifyPushOutcome— name the outcome honestly and say what state the user is in, rather than letting a clean exit code imply success.Test
internal/cli/cancel_test.go— table-driven over every prompt a user can back out of, asserting exit code + user-visible output + no side effect (per the convention proposed intracebloc/backend#930). Rows pair each Ctrl-C path with its declined-answer twin, which is what pins the consistency:client createCancelled — nothing was provisioned.client createCancelled — nothing was provisioned.deleteCancelled — nothing was removed.deleteCancelled — the name didn't match. Nothing was removed.Mutation-checked: reinstating
return nilat the two sites fails exactly the two Ctrl-C rows ("printed no cancellation note — a silent exit 0 is indistinguishable from success"), while the declined-answer twins keep passing — i.e. the test reproduces the reported inconsistency.TestMapClientErrbecameTestMapPromptErrand now asserts the note is printed, not just that the error is nil.Copy catalog
Regenerated (
TB_UPDATE_GOLDEN=1 go test ./internal/cli/ -run TestCopyCatalog). The harvester learned about the new copy helper — it now assemblesCancelled —+ the note argument, so every cancellation line stays reviewable in the catalog instead of degrading to a bare%s. Diff is 4 lines: the pre-existing four lines unchanged,"Cancelled."→"Cancelled — nothing was provisioned.", plus"Cancelled — nothing was removed.".make ciAll green (
vet,test -race×17 packages,lint,fmt-check,schema-check,vulncheck,file-budget,deadcode,check-style). One pre-existing advisory, unrelated and unchanged:deadcodereports a stale allowlist entry forinternal/doctor/doctor.go: Status.String— verified identical on a clean tree.Notes / one contradiction with the report
The ticket lists
data_ingest_cluster.goamong the sites that print. It doesn't —existingTableActionreturns(false, nil)and its caller prints the note for both the decline and the Ctrl-C. Behaviour was already correct, so this is a nit, but the fold is worth knowing about when reading that site. Also worth noting: in productionclient create's Ctrl-C wasn't quite zero output — the deferred log line still printed a barefull log: <path>, which reads more like something happened than like an abort.Found in
tracebloc/cli#409. Second-most-common Bugbot finding class in the org ("silent failure / dishonest outcome reporting", 60 of 103 findings in this repo) and a learned Bugbot rule flagged 5 times.Fixes tracebloc/backend#1253
Note
Low Risk
UX and scripting-contract changes only—no auth, cluster mutations, or frozen exit code values changed; behavior is covered by new cancellation tests.
Overview
Interactive prompt cancellations are no longer silent. Ctrl-C or declining at a question now always prints a
Cancelled — …line and exits 0 (nothing had started).cleanCancelandmapPromptErrininteractive.goreplacemapClientErr, which could return nil without printing—so scripts could mistake an abort for success (notablyclient createanddelete).All prompting flows (
data ingest,data delete,resources set,client create, offboarddelete) route declines and Ctrl-C through the same helpers so wording and exit behavior stay paired.Docs and comments now separate prompt cancel (exit 0 + visible note) from
exitInterrupted(130) for Ctrl-C during work already running (login wait, seal check, upgrade, etc.).docs/troubleshooting.mdmatches that split.cancel_test.gotable-tests exit code, output, and no side effects. Copy-catalog harvesting includes assembledCancelled —lines from the new helpers; golden strings updated.Reviewed by Cursor Bugbot for commit 396793e. Bugbot is set up for automated code reviews on this repo. Configure here.