feat(cli): non-interactive foundation + ssh-keys add#476
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an interactivity gate and prompt helpers that return structured non-interactive errors; introduces two CliError variants; adds JSON/Human error rendering and re-exports; refactors SSH add/delete to use gate prompts and explicit non-interactive errors; tweaks progress/main error plumbing; adds key-compare helper and non-interactive tests. ChangesNon-interactive Flow Gating and Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/basilica-cli/src/interactive/gate.rs (1)
118-134: 💤 Low valueDocument why
defaultis ignored in non-interactive mode.
ask_confirmaccepts adefaultparameter but doesn't use it whencurrent()returnsNonInteractive—it always returnsMissingInputinstead. This differs fromask_text(lines 63-64), which returns the default value in non-interactive mode when one is provided. The asymmetry is likely intentional (confirmations often guard destructive operations), but the behavior should be documented in a doc comment to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/basilica-cli/src/interactive/gate.rs` around lines 118 - 134, Document that ask_confirm intentionally ignores the default in NonInteractive mode: add a doc comment on the ask_confirm function explaining that when current() returns Interactivity::NonInteractive the function returns Err(CliError::MissingInput) rather than using the default parameter (unlike ask_text), and state the rationale (safety for destructive/confirmation prompts) so callers understand the asymmetry with ask_text and why default is not applied in non-interactive runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/basilica-cli/src/output/error_render.rs`:
- Around line 50-64: The JSON output currently inserts "id" and "label" into the
object then merges c.meta which can overwrite them; in the choices_to_json
function change the insertion order so you first insert all entries from c.meta
into the serde_json::Map and then insert "id" and "label" (using c.id and
c.label) last, ensuring the authoritative Choice fields (c.id, c.label) cannot
be overwritten by keys in c.meta.
---
Nitpick comments:
In `@crates/basilica-cli/src/interactive/gate.rs`:
- Around line 118-134: Document that ask_confirm intentionally ignores the
default in NonInteractive mode: add a doc comment on the ask_confirm function
explaining that when current() returns Interactivity::NonInteractive the
function returns Err(CliError::MissingInput) rather than using the default
parameter (unlike ask_text), and state the rationale (safety for
destructive/confirmation prompts) so callers understand the asymmetry with
ask_text and why default is not applied in non-interactive runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1202dc38-4f2e-4e19-87b5-e74ed0150e0f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/basilica-cli/Cargo.tomlcrates/basilica-cli/src/cli/args.rscrates/basilica-cli/src/cli/commands.rscrates/basilica-cli/src/cli/handlers/ssh_keys.rscrates/basilica-cli/src/error.rscrates/basilica-cli/src/interactive/gate.rscrates/basilica-cli/src/interactive/mod.rscrates/basilica-cli/src/main.rscrates/basilica-cli/src/output/error_render.rscrates/basilica-cli/src/output/mod.rscrates/basilica-cli/src/progress/mod.rscrates/basilica-cli/tests/no_hang.rscrates/basilica-cli/tests/non_interactive.rs
Drops --force from `ssh-keys add`. Only one SSH key is allowed per user, so replacement was collapsing two destructive cloud-mutating actions (delete + register) into one silent flag, leaving no forensic trail in CI output. The caller now has to run `basilica ssh-keys delete -y` explicitly before re-registering. - Same public key already registered: idempotent no-op success (compared via parsed key data, ignoring trailing comment/whitespace). - Different key registered, non-interactive: emit MissingPrerequisite with the cleanup command in the hint, no auto-delete. - Different key registered, interactive: keep the original confirm + delete + register flow. Adds `ssh::same_public_key` (reusing the existing key-matcher parser) so equality holds across `user@host` comment differences.
- Use basilica_common::rental::generate_random_rental_name() as the default ssh key label instead of the literal "default" string, so non-interactive `ssh-keys add` without --name matches the rental flow's naming pattern. - Drop `schema_version` from the JSON error payload. Versioning with no documented acceptance contract and no second consumer was premature; can be reintroduced when a real breaking shape change arrives. - Remove timing-based `no_hang.rs` test. A 5s wall-clock threshold conflates slow network with stdin hangs; `non_interactive.rs` already covers gate behavior with positive JSON assertions.
The Choices/Choice/SelectItem types let caller-supplied `meta` keys
collide with the renderer's authoritative `id` and `label` fields,
silently shadowing the agent's stable handle. Rather than defend
reserved keys, drop the surface: `MissingInput { field, hint }` is
enough for agents to retry. `ask_select` now takes `&[&str]` of
labels.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/basilica-cli/src/cli/handlers/ssh_keys.rs (1)
324-336:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply the same max-length validation to prompted key names.
--nameinput enforces<= 100chars, but the interactiveask_textpath does not. This creates inconsistent behavior and pushes validation failures downstream to the API.Suggested patch
None => { let default_name = basilica_common::rental::generate_random_rental_name(); let input = ask_text( "name", Some(&default_name), "Pass --name <label> to label the registered key", )?; if input.trim().is_empty() { return Err(CliError::Internal(color_eyre::eyre::eyre!( "SSH key name cannot be empty" ))); } + if input.len() > 100 { + return Err(CliError::Internal(color_eyre::eyre::eyre!( + "SSH key name must be 100 characters or less" + ))); + } input }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/basilica-cli/src/cli/handlers/ssh_keys.rs` around lines 324 - 336, The interactive prompt path that uses ask_text to read the SSH key "name" should enforce the same max-length check as the --name flag; after obtaining and trimming the input produced by ask_text (the variable named input) validate input.len() <= 100 and return a CliError::Internal (using color_eyre::eyre::eyre! like the existing branch) when it exceeds 100 chars, preserving the current "SSH key name cannot be empty" check and error flow; update the block around generate_random_rental_name / ask_text / input to perform the non-empty and max-length validation before returning input so behavior matches the flag path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/basilica-cli/src/cli/handlers/ssh_keys.rs`:
- Around line 455-459: The confirmation prompt passed to ask_confirm currently
reads like usage guidance instead of a direct yes/no question; update the
message argument in the ask_confirm call (where confirmed is set) to a concise
confirmation question such as "Are you sure you want to delete the registered
SSH key?" (you can append "(y/N)" or mention "-y/--yes to skip" if desired) so
interactive users see a clear yes/no prompt; preserve the key name
"confirm_delete_ssh_key" and the call to ask_confirm(...) and keep handling the
returned confirmed value as before.
---
Outside diff comments:
In `@crates/basilica-cli/src/cli/handlers/ssh_keys.rs`:
- Around line 324-336: The interactive prompt path that uses ask_text to read
the SSH key "name" should enforce the same max-length check as the --name flag;
after obtaining and trimming the input produced by ask_text (the variable named
input) validate input.len() <= 100 and return a CliError::Internal (using
color_eyre::eyre::eyre! like the existing branch) when it exceeds 100 chars,
preserving the current "SSH key name cannot be empty" check and error flow;
update the block around generate_random_rental_name / ask_text / input to
perform the non-empty and max-length validation before returning input so
behavior matches the flag path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c47131bc-8b25-4b83-81cd-fb1eeb74ffc2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/basilica-cli/src/cli/handlers/ssh_keys.rscrates/basilica-cli/src/error.rscrates/basilica-cli/src/interactive/gate.rscrates/basilica-cli/src/output/error_render.rscrates/basilica-cli/src/ssh/key_matcher.rscrates/basilica-cli/src/ssh/mod.rscrates/basilica-cli/tests/non_interactive.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/basilica-cli/tests/non_interactive.rs
… guards `indicatif`'s default draw target already hides spinners and progress bars on non-TTY stderr, so the manual `match current()` guards in `start_spinner` and `create_spinner` were duplicating built-in behavior. Remove them so spinner construction matches the existing progress-bar paths. Also collapse `gate.rs`'s two-tier `OVERRIDE` + `DETECTED` `OnceLock`s into a single `CURRENT`. The override tier was only consumed by `set_for_test`, which a single `OnceLock::set` handles equivalently. No behavior change. Gate tests still pass.
The TTY + env check is cheap enough to run on every prompt, so the static cache just added complexity. Tests now toggle the env var via an RAII guard instead of seeding the OnceLock.
Separate the human-readable question shown via dialoguer from the stable machine field identifier used in non-interactive MissingInput errors.
`execute_with_auth_retry` caught `CliError::Auth` and unconditionally
started the OAuth callback server, which blocks 300s waiting for the
user. In CI / scripted use with `BASILICA_NON_INTERACTIVE=1` this hung
every auth-requiring command for the full timeout before failing.
In non-interactive mode, return `MissingPrerequisite { field: "authentication" }`
immediately instead. Drop the brittle ssh-keys integration test that
assumed the runner was logged in.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/basilica-cli/src/cli/handlers/ssh_keys.rs (2)
232-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--forceis documented in hints but not actually applied in the replacement path.Line 372 tells users to pass
--force, but this handler has noforceinput and always prompts (interactive) or errors (non-interactive) when a key already exists.Suggested patch
pub async fn handle_add_ssh_key( client: &BasilicaClient, name: Option<String>, file: Option<PathBuf>, + force: bool, ) -> Result<(), CliError> { @@ - if matches!(gate::current(), Interactivity::NonInteractive) { + if matches!(gate::current(), Interactivity::NonInteractive) && !force { return Err(CliError::MissingPrerequisite { field: "ssh_key_already_registered".into(), hint: format!( @@ - let confirmed = ask_confirm( - "replace_existing", - "Do you want to replace it with the new key?", - false, - "Pass --force to replace the existing SSH key without prompting.", - )?; - if !confirmed { - println!("Operation cancelled."); - return Ok(()); + if !force { + let confirmed = ask_confirm( + "replace_existing", + "Do you want to replace it with the new key?", + false, + "Pass --force to replace the existing SSH key without prompting.", + )?; + if !confirmed { + println!("Operation cancelled."); + return Ok(()); + } }Also applies to: 351-373
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/basilica-cli/src/cli/handlers/ssh_keys.rs` around lines 232 - 236, The handler handle_add_ssh_key currently lacks a force flag so the documented --force hint is not honored; update the function signature to accept a force: bool (and propagate from callers), then in the duplicate-key branch (the code that currently prompts or returns an error when a key already exists) use that force boolean to skip prompting and perform replacement immediately; also ensure non-interactive flows check force to avoid returning an error and that any prompt code path is only used when force is false.
325-338:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply the same max-length check to prompted key names.
The flag path enforces
<= 100chars, but the prompted path only checks empty input.Suggested patch
let input = ask_text( "name", "Enter a name for this SSH key", Some(&default_name), "Pass --name <label> to label the registered key", )?; if input.trim().is_empty() { return Err(CliError::Internal(color_eyre::eyre::eyre!( "SSH key name cannot be empty" ))); } + if input.len() > 100 { + return Err(CliError::Internal(color_eyre::eyre::eyre!( + "SSH key name must be 100 characters or less" + ))); + } input🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/basilica-cli/src/cli/handlers/ssh_keys.rs` around lines 325 - 338, The prompted SSH key name path (inside the block using basilica_common::rental::generate_random_rental_name() and ask_text) only checks for empty input but must enforce the same max-length <= 100 as the flag path; after obtaining and trimming input (the variable input), add a length check and return the same CliError::Internal (color_eyre::eyre::eyre!("SSH key name cannot be empty" style) but with a clear message for length, e.g. "SSH key name must be at most 100 characters") when input.len() > 100 so both code paths share the same validation for the SSH key name.
🧹 Nitpick comments (1)
crates/basilica-cli/tests/non_interactive.rs (1)
39-43: ⚡ Quick winStrengthen this assertion to validate the JSON error contract, not just “some string error.”
Right now this passes for many unrelated failures. Assert
schema_versionand presence/shape oferrorso regressions in JSON rendering are caught.Suggested patch
- assert!(v["error"].is_string()); + assert_eq!(v["schema_version"], serde_json::Value::from(1)); + assert!( + v.get("error").is_some(), + "missing `error` field in stderr json: {v}" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/basilica-cli/tests/non_interactive.rs` around lines 39 - 43, The current test only asserts v["error"].is_string(), which is too weak; update the assertions in crates/basilica-cli/tests/non_interactive.rs to validate the JSON error contract more strictly: assert that v contains a "schema_version" (e.g. assert!(v.get("schema_version").and_then(|s| s.as_str()).is_some() or equals the expected version), and assert that v["error"] is either a string or an object with a "message" field (e.g. check v["error"].is_string() || (v["error"].is_object() && v["error"].get("message").and_then(|m| m.as_str()).is_some())). Keep the existing assert that error is present but strengthen it by verifying "schema_version" and the shape of "error" using the variable v.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/basilica-cli/src/interactive/gate.rs`:
- Around line 104-116: The RAII guard currently always removes
BASILICA_NON_INTERACTIVE in NonInteractiveEnv::drop which clobbers any
pre-existing value; change NonInteractiveEnv to store the previous value (use
std::env::var_os to capture Option<OsString>) in the struct, have
NonInteractiveEnv::set() save that previous value before calling
std::env::set_var, and update impl Drop for NonInteractiveEnv to restore the
previous value: if the saved Option is Some(v) call std::env::set_var with that
value, otherwise call std::env::remove_var, ensuring the original environment is
preserved.
---
Outside diff comments:
In `@crates/basilica-cli/src/cli/handlers/ssh_keys.rs`:
- Around line 232-236: The handler handle_add_ssh_key currently lacks a force
flag so the documented --force hint is not honored; update the function
signature to accept a force: bool (and propagate from callers), then in the
duplicate-key branch (the code that currently prompts or returns an error when a
key already exists) use that force boolean to skip prompting and perform
replacement immediately; also ensure non-interactive flows check force to avoid
returning an error and that any prompt code path is only used when force is
false.
- Around line 325-338: The prompted SSH key name path (inside the block using
basilica_common::rental::generate_random_rental_name() and ask_text) only checks
for empty input but must enforce the same max-length <= 100 as the flag path;
after obtaining and trimming input (the variable input), add a length check and
return the same CliError::Internal (color_eyre::eyre::eyre!("SSH key name cannot
be empty" style) but with a clear message for length, e.g. "SSH key name must be
at most 100 characters") when input.len() > 100 so both code paths share the
same validation for the SSH key name.
---
Nitpick comments:
In `@crates/basilica-cli/tests/non_interactive.rs`:
- Around line 39-43: The current test only asserts v["error"].is_string(), which
is too weak; update the assertions in
crates/basilica-cli/tests/non_interactive.rs to validate the JSON error contract
more strictly: assert that v contains a "schema_version" (e.g.
assert!(v.get("schema_version").and_then(|s| s.as_str()).is_some() or equals the
expected version), and assert that v["error"] is either a string or an object
with a "message" field (e.g. check v["error"].is_string() ||
(v["error"].is_object() && v["error"].get("message").and_then(|m|
m.as_str()).is_some())). Keep the existing assert that error is present but
strengthen it by verifying "schema_version" and the shape of "error" using the
variable v.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68104801-da34-4dad-aad4-3b5583492aba
📒 Files selected for processing (5)
crates/basilica-cli/src/cli/args.rscrates/basilica-cli/src/cli/handlers/ssh_keys.rscrates/basilica-cli/src/interactive/gate.rscrates/basilica-cli/src/output/error_render.rscrates/basilica-cli/tests/non_interactive.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/basilica-cli/src/output/error_render.rs
Capture the previous env var on set and restore it on drop so nested or neighboring tests that rely on the variable aren't clobbered.
…sh-keys add hint - `gate::current()` no longer flips to NonInteractive when `BASILICA_NON_INTERACTIVE=""`; only non-empty values force the mode. - The replace-existing prompt in `ssh-keys add` referenced a `--force` flag that does not exist on the subcommand. Replace it with the same `basilica ssh-keys delete -y` guidance the non-interactive prerequisite branch already uses.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lands the foundation for non-interactive CLI execution — interactivity gate, error variants, JSON error renderer, spinner suppression — and migrates
ssh-keys addend-to-end so the plumbing has a real consumer.Non-interactive mode is detected via
!stdin.is_terminal() || $BASILICA_NON_INTERACTIVE. In that mode, every routed prompt returns a structuredMissingInput { field, hint, choices }(would have prompted) orMissingPrerequisite { field, hint }(would have set something up) instead of hanging on stdin. Errors render to stderr as a single JSON object per error when--jsonis set (schema_version: 1).Changes
CliError::MissingInputandCliError::MissingPrerequisitevariants.Interactivity::{Interactive, NonInteractive},current(),ask_text/ask_select/ask_confirmhelpers,SelectItembuilder,Choices/Choicetypes.render_error(err, mode, w)withRenderMode::{Human, Json}. Wired intomain.rsso the top-level error path honors the global--jsonflag.create_spinnerandProgressManager::start_spinnerreturnProgressBar::hidden()in non-interactive mode (decorative spinners corrupt stderr capture for agents)..interact*()site routed through the gate; added--forceflag to skip the replace-existing confirmation;deleteconfirm also gated so-yis the only way through non-interactively; refuses to silently generate a new SSH key in non-interactive mode.SshKeyAction::Addgains--force.Tests
BASILICA_NON_INTERACTIVE=1 basilica --json ssh-keys addemits the right JSON shapessh-keys add,ssh-keys list,balanceexit in ≤5s with stdin closed (will grow with follow-up PRs)Dev-dep additions:
serial_test = "3",assert_cmd = "2".Out of scope (deferred to follow-up PRs)
basilica upnon-interactive flags (--offering-id,--ssh-key-id,--max-hourly-rateguardrail, JSON response), rental selector sweep (down/ssh/exec/cp/logs), volumes/tokens/deploy/fund sweeps, docs updates.Summary by CodeRabbit
New Features
Improvements
Tests