Skip to content

fix(cli): Interaction progress review - #7

Closed
thebrandonlucas wants to merge 3 commits into
masterfrom
fix/cli-interaction-progress-review
Closed

thebrandonlucas wants to merge 3 commits into
masterfrom
fix/cli-interaction-progress-review

Conversation

@thebrandonlucas

@thebrandonlucas thebrandonlucas commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

PR review: noninteractive controls and interrupt-safe progress

Branch: fix/cli-interaction-progress-review (533605c feature + f3afd02 review fixes) → master
Verdict: ✅ Ready to merge once the PR description explains the exceptions listed below.
Gate: fmt ✅ · clippy -D warnings ✅ · tests 72 unit + 48 integration ✅ · check-coverage.py ✅ · cargo machete not run (not installed here; no new dependencies)

The samples below are real runs of target/debug/voltage against a local server that delays every response by 30 s. Paths are shortened to ~/.config/voltage and repeated confirmation summaries are shortened to {…}.


Usage walkthrough

1. --no-input without --yes stops before any network call

$ voltage --no-input payments create --org $ORG --env $ENV --data @send.json
{
  "action": "payments create",
  "environments": ["22222222-…"],
  "organization": "11111111-…",
  "request": { "currency": "btc", "id": "44444444-…", "type": "bolt11", … },
  "resource_id": "44444444-…"
}
Network/provider fee limits exclude additional processing fees. The wallet determines the network.
{"error":{"detail":null,"exit_code":2,"message":"This operation requires --yes when input is disabled or noninteractive"}}
$ echo $?
2

✅ The CLI prints the summary and then fails. It makes no credential refresh, no wallet check and writes no journal entry. The integration test asserts the server received zero requests.

2. --yes never supplies a secret

$ voltage --no-input --yes auth import-key --account ci --org $ORG --env $ENV
{"error":{"detail":null,"exit_code":2,"message":"Use --stdin to import an API key when input is disabled or noninteractive (--yes does not supply a key)"}}
$ echo $?
2

# The supported noninteractive form:
$ printf '%s' "$KEY" | voltage auth import-key --stdin --account ci --org $ORG --env $ENV

3. Ctrl-C during a read makes no submission claim

$ voltage --json wallets list --org $ORG
^C
{"error":{"detail":null,"exit_code":130,"message":"Interrupted before any change was submitted"}}

4. Ctrl-C during a payment keeps the recovery ID

$ voltage --json --yes payments create --org $ORG --env $ENV --data @send.json
{…confirmation summary…}
Resource ID: 44444444-4444-4444-8444-444444444444. Recovery record: ~/.config/voltage/requests/44444444-….json
^C
{"error":{
  "exit_code":130,
  "message":"Interrupted after payment 44444444-… may have been submitted; query its original ID before resubmitting. The payment was not cancelled.",
  "detail":{"outcome":"unknown","resource_id":"44444444-…","organization_id":"11111111-…",
            "environment_ids":["22222222-…"],
            "action":"Query the original resource ID before deciding whether to resubmit"}}}

# Reconcile, don't resubmit:
$ voltage payments get 44444444-… --org $ORG --env $ENV

✅ The payment is not resent. The journal entry is written before the request goes out. The error detail has the same shape as the one for a dropped connection (exit 4), so scripts can handle both the same way.

5. Ctrl-C during a non-payment write (fixed in review)

$ voltage --json --yes wallets delete $WALLET --org $ORG
{…confirmation summary…}
^C
{"error":{
  "exit_code":130,
  "message":"Interrupted after the request may have been submitted; check its result before retrying. The request was not cancelled.",
  "detail":{"outcome":"unknown","resource_id":"33333333-…","organization_id":"11111111-…", …}}}

Before the fix: this printed "Interrupted before a payment submission; no payment was sent by this command". The DELETE was already in flight, so "before" was wrong. The error also had no detail, which dropped the uncertainty.

6. --quiet keeps what you need to recover

$ voltage -q --json --yes --no-input payments create --org $ORG --env $ENV --data @send.json
{…confirmation summary…}                     ← still shown (required)
Resource ID: 44444444-…. Recovery record: …  ← still shown (recovery)
^C
{"error":{"exit_code":130,"message":"Interrupted after payment 44444444-… may have been submitted; …", …}}

✅ Quiet mode hides only progress and routine notices, like Payment … status: pending.

7. Progress only appears on a terminal

# stderr is a terminal: a status line appears and is erased (\r\e[2K) when the response arrives
$ voltage wallets list --org $ORG
Waiting for API response...

# stderr is redirected: nothing, and no escape codes
$ voltage wallets list --org $ORG 2>err.log; wc -c < err.log
0

progress_is_terminal_only_and_cleared_on_completion runs this on a real PTY. It asserts the terminal output ends with the clear sequence.

8. Ctrl-C at the confirmation prompt (fixed in review)

$ voltage payments create --org $ORG --env $ENV --data @send.json
{…confirmation summary…}
Proceed? [y/N] ^C
{"error":{"detail":null,"exit_code":130,"message":"Interrupted before any change was submitted"}}
Before the fix (6 PTY runs) After the fix (8 PTY runs)
4 × hung until Enter was pressed 8 × exit 130 with the report above
2 × killed by the raw signal, no error report —

The cause: stdin().read_line() blocked the same thread that listens for Ctrl-C. Whether the listener was registered first was random, and when it was, the signal was captured but never handled. The new test interrupt_at_the_confirmation_prompt_exits_130_without_submitting fails against the pre-fix code.


Findings, fixed in f3afd02

# Severity Finding Fix
1 High Ctrl-C at Proceed? or the hidden API-key prompt hangs (sample 8) Terminal::confirm and read_hidden read on a detached thread and report back over a oneshot channel
2 Medium Interrupting a non-payment write says "before a payment submission" (sample 5) Record every non-GET write immediately before sending it
3 Medium SubmissionState was an Arc<Mutex<Option<…>>> in startup that api imported, so the dependency pointed backwards Write-once OnceLock, owned by api
4 Low quiet: bool was passed through lock, revoke, refresh, logout and profiles; Terminal::new(false) was built ad hoc One Terminal value built from the global flags
5 Low The checkout session wait's status line overlapped the request's status line The wait status covers only the pause between polls
6 Style A crate::terminal::… path inside a function, split use blocks, drop(_progress) Guide-conformant imports and naming; Progress is #[must_use]
7 Tests New process tests: skipped scrubbing VOLTAGE_*, waited in 20 ms sleep loops, used unsafe libc::kill(… as i32) Shared process_at helper, request-arrival notification, kill -INT as in the existing tests, SAFETY: comment on each openpty block

Exceptions the PR description must explain

The quality guide's Review exceptions section requires these three:

  • Detached std::thread for terminal reads. A blocked read can't be cancelled, and the runtime waits for spawn_blocking work when it shuts down. The channel reports completion, and the process exit ends the thread.
  • tokio sync feature. It provides oneshot for the reads above and Notify in the tests. It is not a new crate.
  • unsafe libc::openpty in tests. Rust's standard library has no way to open a PTY, and the terminal-only progress needs a real terminal to test.

Open questions and follow-ups (existed before this PR)

  • -q --yes still prints the full confirmation summary (sample 6). This follows the plan's rule that confirmation information is mandatory. In CI logs it adds about 15 lines per payment. Should --yes combined with --quiet shorten it to one line?
  • Ctrl-C during the hidden API-key prompt can leave terminal echo off, because rpassword only restores it when its read returns. The workaround is stty sane.
  • Access to the macOS keychain still blocks the thread that listens for Ctrl-C. It's the same kind of bug as feat: sign and attest release artifacts (#12) #1 and is worth a ticket.

Confirmation and hidden API-key prompts blocked the thread that listens
for Ctrl-C, so interrupting at a prompt hung or died by raw signal
without the exit-130 report. Terminal reads now run on a detached thread
that reports back over a channel.

Record any non-GET write, not only payments, immediately before it is
sent, so an interrupted delete reports an uncertain submission instead
of "before a payment submission". The write record is now a write-once
OnceLock owned by api rather than an Arc<Mutex> in startup.

Pass one Terminal built from the global flags instead of threading
quiet/no_input bools, keep the checkout projection status from nesting
with request status, and mark Progress #[must_use].

Tests share an env-scrubbed process helper, wait on request arrival
through a notification instead of sleep polling, signal with kill
instead of unsafe libc::kill, document each openpty unsafe block, and
cover Ctrl-C at the prompt and during a non-payment write.
announced, error_report, and the Notify import are used only by
cfg(unix) process tests, so Windows clippy rejected them as dead code.
@thebrandonlucas
thebrandonlucas force-pushed the fix/cli-interaction-progress-review branch from 7cad498 to f21eeb3 Compare September 25, 2026 18:32
@thebrandonlucas thebrandonlucas changed the title Fix/cli interaction progress review fix(cli): Interaction progress review Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant