Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/aenv/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ anyhow = "1"
bytes = "1"
chrono = { version = "0.4", features = ["serde"] }
clap = { version = "4", features = ["derive"] }
clap_complete = "4"
clap_complete = { version = "4", features = ["unstable-dynamic"] }
crossterm = "0.28"
directories = "5"
envd = { path = "../../thirdparty/envd" }
Expand Down
18 changes: 16 additions & 2 deletions crates/aenv/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,24 @@ impl Client {
}

pub fn new(url: &str, api_key: &str) -> Result<Self> {
Self::new_with_timeouts(
url,
api_key,
Duration::from_secs(5),
Duration::from_secs(120),
)
}

pub fn new_with_timeouts(
url: &str,
api_key: &str,
connect_timeout: Duration,
request_timeout: Duration,
) -> Result<Self> {
let base = url.trim_end_matches('/').to_string();
let agent = ureq::AgentBuilder::new()
.timeout_connect(Duration::from_secs(5))
.timeout(Duration::from_secs(120))
.timeout_connect(connect_timeout)
.timeout(request_timeout)
.build();
Ok(Self {
agent,
Expand Down
165 changes: 138 additions & 27 deletions crates/aenv/src/commands/completion.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
use anyhow::Context;
use anyhow::Result;
use clap::Args as ClapArgs;
use clap::CommandFactory;
use clap::ValueEnum;
use clap_complete::Shell as ClapShell;
use clap_complete::engine::{ArgValueCandidates, CompletionCandidate};
use clap_complete::env::{Bash, EnvCompleter, Fish, Zsh};
use std::io::Write;
use std::time::Duration;

use crate::client::sandboxes::ListedSandbox;

const DYNAMIC_CONNECT_TIMEOUT: Duration = Duration::from_millis(500);
const DYNAMIC_REQUEST_TIMEOUT: Duration = Duration::from_secs(1);

/// Shell to generate completion for.
///
Expand All @@ -15,12 +21,13 @@ pub enum Shell {
Fish,
}

impl From<Shell> for ClapShell {
fn from(shell: Shell) -> Self {
match shell {
Shell::Bash => ClapShell::Bash,
Shell::Zsh => ClapShell::Zsh,
Shell::Fish => ClapShell::Fish,
impl Shell {
/// The `EnvCompleter` used to emit this shell's registration script.
fn completer(self) -> &'static dyn EnvCompleter {
match self {
Shell::Bash => &Bash,
Shell::Zsh => &Zsh,
Shell::Fish => &Fish,
}
}
}
Expand All @@ -37,19 +44,89 @@ pub fn run(args: Args) -> Result<()> {
write_completion(args.shell, &mut std::io::stdout().lock())
}

/// Generate the completion script for `shell` and write it to `out`.
pub fn running_sandbox_candidates() -> Vec<CompletionCandidate> {
sandbox_candidates(|state| state == Some("running"))
}
Comment on lines +47 to +49

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
These callbacks require clap_complete's dynamic completion protocol, but run still emits a static script via clap_complete::generate. A script installed with the advertised aenv completion <shell> command therefore does not invoke the binary at completion time, so these API-backed candidates will not appear. Generate the CompleteEnv registration script here (or otherwise make run use the dynamic engine) so the normal installation path activates these providers.

Suggestion:

Suggested change
pub fn running_sandbox_candidates() -> Vec<CompletionCandidate> {
sandbox_candidates(|state| state == Some("running"))
}
// Generate the shell registration through `CompleteEnv` so completion
// requests are routed back to `Cli::command` and evaluate these providers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickNYU Please check this (but ignore the suggestion). This is a real bug. Perhaps you should make write_completion write the dynamic engine's registration script.


pub fn paused_sandbox_candidates() -> Vec<CompletionCandidate> {
sandbox_candidates(|state| state == Some("paused"))
}

pub fn active_sandbox_candidates() -> Vec<CompletionCandidate> {
sandbox_candidates(|_| true)
}
Comment thread
LSX-s-Software marked this conversation as resolved.

fn sandbox_candidates<F>(state_matches: F) -> Vec<CompletionCandidate>
where
F: Fn(Option<&str>) -> bool,
{
let Ok(credentials) = crate::auth::load() else {
return Vec::new();
};
let Ok(client) = crate::client::Client::new_with_timeouts(
&credentials.url,
&credentials.api_key,
DYNAMIC_CONNECT_TIMEOUT,
DYNAMIC_REQUEST_TIMEOUT,
) else {
return Vec::new();
};
let Ok(sandboxes) = client.list_sandboxes() else {
return Vec::new();
};
Comment on lines +63 to +76

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance · medium
Every dynamic completion invocation synchronously reads the credentials file, constructs a client, and performs a blocking list_sandboxes() HTTP request. Shells commonly invoke completion on each Tab/word change, so an unavailable or slow API can stall the interactive shell for up to DYNAMIC_REQUEST_TIMEOUT on every invocation and repeatedly load the service. Consider caching/debouncing the sandbox list (with a short TTL) or otherwise avoiding a network round trip for each completion request.


let mut candidates = filter_sandboxes(sandboxes, state_matches);
candidates.sort_by(|left, right| left.sandbox_id.cmp(&right.sandbox_id));
candidates
.into_iter()
.map(|sandbox| CompletionCandidate::new(sandbox.sandbox_id))
.collect()
}

fn filter_sandboxes<I, F>(sandboxes: I, state_matches: F) -> Vec<ListedSandbox>
where
I: IntoIterator<Item = ListedSandbox>,
F: Fn(Option<&str>) -> bool,
{
sandboxes
.into_iter()
.filter(|sandbox| state_matches(sandbox.state.as_deref()))
.collect()
}

pub fn add_running_sandbox_candidates() -> ArgValueCandidates {
ArgValueCandidates::new(running_sandbox_candidates)
}

pub fn add_paused_sandbox_candidates() -> ArgValueCandidates {
ArgValueCandidates::new(paused_sandbox_candidates)
}

pub fn add_active_sandbox_candidates() -> ArgValueCandidates {
ArgValueCandidates::new(active_sandbox_candidates)
}

/// Generate the completion registration script for `shell` and write it to
/// `out`.
///
/// The emitted script is the dynamic engine's registration: it hooks the
/// shell so that each completion request calls back into the current `aenv`
/// binary (`COMPLETE=<shell> aenv -- ...`), which is what evaluates the
/// dynamic `ArgValueCandidates` providers (e.g. live sandbox IDs). Emitting
/// the static `clap_complete::generate` script instead would silently disable
/// those providers, so the two must not be mixed up here.
///
/// Generation goes through an in-memory buffer first: clap_complete's
/// generators panic on write errors (`Generator::generate` calls `.expect`),
/// so writing straight to `out` would turn a closed downstream pipe into a
/// panic. The buffer cannot fail, so generation is infallible; only the
/// explicit write below can. A `BrokenPipe` there (e.g. `aenv completion bash
/// | head`) is normal and treated as success; any other error propagates with
/// context. Split out from `run` so the write branches are unit-testable.
/// Generation goes through an in-memory buffer first: the buffer cannot fail,
/// so registration is infallible; only the explicit write below can. A
/// `BrokenPipe` there (e.g. `aenv completion bash | head`) is normal and
/// treated as success; any other error propagates with context. Split out
/// from `run` so the write branches are unit-testable.
fn write_completion<W: Write>(shell: Shell, out: &mut W) -> Result<()> {
let mut cmd = crate::Cli::command();
let mut script = Vec::new();
clap_complete::generate(ClapShell::from(shell), &mut cmd, "aenv", &mut script);
shell
.completer()
.write_registration("COMPLETE", "aenv", "aenv", "aenv", &mut script)
.expect("writing to an in-memory buffer cannot fail");
match out.write_all(&script).and_then(|_| out.flush()) {
Ok(()) => Ok(()),
Err(err) if err.kind() == std::io::ErrorKind::BrokenPipe => Ok(()),
Expand All @@ -60,6 +137,29 @@ fn write_completion<W: Write>(shell: Shell, out: &mut W) -> Result<()> {
#[cfg(test)]
mod tests {
use super::*;
use clap::CommandFactory as _;

fn sandbox(id: &str, state: &str) -> ListedSandbox {
ListedSandbox {
sandbox_id: id.to_string(),
template_id: "template".to_string(),
alias: None,
state: Some(state.to_string()),
cpu_count: None,
memory_mib: None,
disk_size_mib: None,
started_at: None,
end_at: None,
}
}

#[test]
fn state_filter_keeps_only_matching_sandboxes() {
let sandboxes = [sandbox("paused", "paused"), sandbox("running", "running")];
let running = filter_sandboxes(sandboxes, |state| state == Some("running"));
assert_eq!(running.len(), 1);
assert_eq!(running[0].sandbox_id, "running");
}

fn generate_for(shell: Shell) -> String {
let mut buf = Vec::new();
Expand Down Expand Up @@ -88,30 +188,41 @@ mod tests {
}
}

// The registration scripts below must route completion requests back into
// the `aenv` binary via the `COMPLETE=<shell>` environment variable: that
// callback is what makes the dynamic `ArgValueCandidates` providers (live
// sandbox IDs) reachable. A static script would contain the same command
// tree but never invoke the binary at completion time.

#[test]
fn bash_has_compdef_or_complete_f() {
fn bash_registers_dynamic_callback() {
let s = generate_for(Shell::Bash);
assert!(
s.contains("complete -F") || s.contains("compdef"),
"bash output should register the binary; got:\n{s}"
s.contains("_clap_complete_aenv")
&& s.contains(r#"COMPLETE="bash""#)
&& s.contains(r#""aenv" --"#),
"bash output should register a callback into the aenv binary; got:\n{s}"
);
}

#[test]
fn zsh_has_compdef_header() {
fn zsh_registers_dynamic_callback() {
let s = generate_for(Shell::Zsh);
assert!(
s.starts_with("#compdef"),
"zsh output should start with a #compdef header; got:\n{s}"
s.starts_with("#compdef aenv")
&& s.contains("_clap_dynamic_completer_aenv")
&& s.contains(r#"COMPLETE="zsh""#),
"zsh output should register a callback into the aenv binary; got:\n{s}"
);
}

#[test]
fn fish_has_complete_calls() {
fn fish_registers_dynamic_callback() {
let s = generate_for(Shell::Fish);
assert!(
s.contains("complete "),
"fish output should contain `complete` invocations; got:\n{s}"
s.contains("complete --keep-order --exclusive --command aenv")
&& s.contains("COMPLETE=fish aenv"),
"fish output should register a callback into the aenv binary; got:\n{s}"
);
}

Expand Down
1 change: 1 addition & 0 deletions crates/aenv/src/commands/connect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const SANDBOX_LOST_TIMEOUT: Duration = Duration::from_secs(10);

#[derive(ClapArgs)]
pub struct Args {
#[arg(add = crate::commands::completion::add_active_sandbox_candidates())]
sandbox_id: String,
}

Expand Down
1 change: 1 addition & 0 deletions crates/aenv/src/commands/delete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use clap::Args as ClapArgs;

#[derive(ClapArgs)]
pub struct Args {
#[arg(add = crate::commands::completion::add_active_sandbox_candidates())]
sandbox_id: String,
}

Expand Down
1 change: 1 addition & 0 deletions crates/aenv/src/commands/download.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use tokio::time::timeout;
aenv download --user app --force <sandbox-id> result.txt ./result.txt")]
pub struct Args {
/// Sandbox ID
#[arg(add = crate::commands::completion::add_running_sandbox_candidates())]
sandbox_id: String,
/// Source file or directory path inside the sandbox
remote_path: String,
Expand Down
1 change: 1 addition & 0 deletions crates/aenv/src/commands/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use envd::process::StartResponse;

#[derive(ClapArgs)]
pub struct Args {
#[arg(add = crate::commands::completion::add_running_sandbox_candidates())]
sandbox_id: String,
/// Command and arguments to run. Flags intended for the remote command
/// that collide with aenv's own flags can be escaped with a leading `--`.
Expand Down
1 change: 1 addition & 0 deletions crates/aenv/src/commands/pause.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use clap::Args as ClapArgs;

#[derive(ClapArgs)]
pub struct Args {
#[arg(add = crate::commands::completion::add_running_sandbox_candidates())]
sandbox_id: String,
}

Expand Down
1 change: 1 addition & 0 deletions crates/aenv/src/commands/resume.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use clap::Args as ClapArgs;

#[derive(ClapArgs)]
pub struct Args {
#[arg(add = crate::commands::completion::add_paused_sandbox_candidates())]
sandbox_id: String,
/// TTL in seconds from now. Must be longer than the sandbox's current TTL.
#[arg(long, default_value_t = super::DEFAULT_TIMEOUT_SECS)]
Expand Down
1 change: 1 addition & 0 deletions crates/aenv/src/commands/snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub struct Args {
enum Sub {
/// Create a persistent snapshot from a running sandbox
Create {
#[arg(add = crate::commands::completion::add_running_sandbox_candidates())]
sandbox_id: String,
/// Snapshot name or alias. If omitted, the server returns the generated snapshot ID.
#[arg(long)]
Expand Down
1 change: 1 addition & 0 deletions crates/aenv/src/commands/timeout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use clap::Args as ClapArgs;

#[derive(ClapArgs)]
pub struct Args {
#[arg(add = crate::commands::completion::add_running_sandbox_candidates())]
sandbox_id: String,
/// Seconds from now until the sandbox should expire
seconds: u32,
Expand Down
1 change: 1 addition & 0 deletions crates/aenv/src/commands/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use walkdir::WalkDir;
aenv upload --user app <sandbox-id> ./config.json config.json")]
pub struct Args {
/// Sandbox ID
#[arg(add = crate::commands::completion::add_running_sandbox_candidates())]
sandbox_id: String,
/// Local file or directory to upload
local_path: PathBuf,
Expand Down
Loading
Loading