cli: make output gooder #397

Merged

ryansquared merged 0 commits from refs/pull/397/head into main

2026-07-27 22:34:26 +00:00

ryansquared commented

2026-07-27 19:52:58 +00:00

(Migrated from codeberg.org)

No description provided.

caution-bot commented

2026-07-27 20:38:34 +00:00

(Migrated from codeberg.org)

Review finding

Thanks for the cleanup here. I found one significant regression worth fixing before merge:

Baremetal credential defaults are no longer reachable, and EOF can loop forever

prompt::text() now guarantees a non-empty line by re-prompting on empty input:

src/cli/src/prompt.rs:20-30

That is fine for required fields, but this PR also uses it for prompts that advertise defaults and whose following code still expects an empty answer to be meaningful:

src/cli/src/lib.rs:7775 — SSH Port [22]: followed by parse().unwrap_or(22)
src/cli/src/lib.rs:7780 — Use SSH key (k) or password (p)? [k]:
src/cli/src/lib.rs:7786-7794 — Path to SSH private key [~/.ssh/id_ed25519]: followed by an if key_path.is_empty() default branch

With the new helper, pressing Enter just re-prompts forever, so the displayed defaults are not actually selectable and the key_path.is_empty() branch is dead. This is a user-visible regression for caution credentials add ... baremetal.

There is also an EOF/non-interactive edge case: read_line() returns Ok(0) on EOF, input stays empty, and prompt::text() loops forever printing the prompt instead of returning an error. Existing code in lib.rs already has testable optional/non-empty prompt helpers (prompt_optional_line_from / prompt_line_from) that distinguish these cases, so I’d either reuse that pattern here or add an allow_empty/default prompt helper in prompt.rs and make text() return an error on EOF.

Checks I ran:

git diff --check origin/main...HEAD
cargo test -p cli

Both passed; this appears to be a behavior gap not covered by the current tests.

## Review finding Thanks for the cleanup here. I found one significant regression worth fixing before merge: ### Baremetal credential defaults are no longer reachable, and EOF can loop forever `prompt::text()` now guarantees a non-empty line by re-prompting on empty input: - `src/cli/src/prompt.rs:20-30` That is fine for required fields, but this PR also uses it for prompts that advertise defaults and whose following code still expects an empty answer to be meaningful: - `src/cli/src/lib.rs:7775` — `SSH Port [22]: ` followed by `parse().unwrap_or(22)` - `src/cli/src/lib.rs:7780` — `Use SSH key (k) or password (p)? [k]: ` - `src/cli/src/lib.rs:7786-7794` — `Path to SSH private key [~/.ssh/id_ed25519]: ` followed by an `if key_path.is_empty()` default branch With the new helper, pressing Enter just re-prompts forever, so the displayed defaults are not actually selectable and the `key_path.is_empty()` branch is dead. This is a user-visible regression for `caution credentials add ... baremetal`. There is also an EOF/non-interactive edge case: `read_line()` returns `Ok(0)` on EOF, `input` stays empty, and `prompt::text()` loops forever printing the prompt instead of returning an error. Existing code in `lib.rs` already has testable optional/non-empty prompt helpers (`prompt_optional_line_from` / `prompt_line_from`) that distinguish these cases, so I’d either reuse that pattern here or add an `allow_empty/default` prompt helper in `prompt.rs` and make `text()` return an error on EOF. Checks I ran: ```sh git diff --check origin/main...HEAD cargo test -p cli ``` Both passed; this appears to be a behavior gap not covered by the current tests.

caution-bot commented

2026-07-27 22:05:58 +00:00

(Migrated from codeberg.org)

Follow-up review

Thanks, the fix commit addresses the previous baremetal default/EOF regression I flagged: prompt::text() now returns UnexpectedEof on EOF, and the baremetal prompts that advertise defaults now use prompt::text_or_default().

One remaining merge blocker I found is formatting: the PR branch is not rustfmt clean for the changed CLI files.

cargo fmt --check -p cli

This currently fails with diffs in:

src/cli/src/apps/download_eif.rs
src/cli/src/apps/migrate_procfile.rs
src/cli/src/lib.rs

git diff --check origin/main...HEAD and cargo test -p cli both pass for me. I think this just needs cargo fmt -p cli (or equivalent repo formatter) before merge.

## Follow-up review Thanks, the fix commit addresses the previous baremetal default/EOF regression I flagged: `prompt::text()` now returns `UnexpectedEof` on EOF, and the baremetal prompts that advertise defaults now use `prompt::text_or_default()`. One remaining merge blocker I found is formatting: the PR branch is not `rustfmt` clean for the changed CLI files. ```sh cargo fmt --check -p cli ``` This currently fails with diffs in: - `src/cli/src/apps/download_eif.rs` - `src/cli/src/apps/migrate_procfile.rs` - `src/cli/src/lib.rs` `git diff --check origin/main...HEAD` and `cargo test -p cli` both pass for me. I think this just needs `cargo fmt -p cli` (or equivalent repo formatter) before merge.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

public/platform!397

No description provided.

Rows
Columns