experiment(cli): evaluate clap dynamic completion - #163
Conversation
Captures the sandbox-ID-only design, its short-timeout failure behavior, shell-adapter boundary, and verification plan before implementation begins. Constraint: PR kvcache-ai#89 provides static completion but no stable dynamic hook\nRejected: Use clap_complete unstable-dynamic API | maintainer identified the surface as unstable\nConfidence: high\nScope-risk: narrow\nDirective: Keep dynamic lookup silent and separate from normal CLI timeouts\nTested: Design self-review and git diff --check\nNot-tested: CLI behavior; implementation has not started
Evaluate clap_complete's unstable dynamic completion engine for sandbox-ID lookup while preserving the existing generated completion command. Constraint: The experiment must reuse the existing sandbox API and keep completion failures silent. Rejected: Shell-specific API calls in generated scripts | duplicates authentication and response parsing across shells Confidence: medium Scope-risk: moderate Directive: Keep the unstable protocol isolated until maintainers choose the long-term completion architecture Tested: cargo check -p aenv --bin aenv; cargo fmt --all; generated COMPLETE=bash script; focused aenv tests (56 passed) Not-tested: One existing download test fails on macOS because /var is a symlink
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| candidates.sort_by(|left, right| left.sandbox_id.cmp(&right.sandbox_id)); | ||
| candidates | ||
| .into_iter() | ||
| .map(|sandbox| CompletionCandidate::new(sandbox.sandbox_id.clone())) |
There was a problem hiding this comment.
[performance · low]
sandboxes is owned and is not used afterward, but filter_sandboxes converts it into references, forcing every completed sandbox ID to be cloned. Filter and sort the owned ListedSandbox values instead, then move each sandbox_id into CompletionCandidate; the test-only filtering helper can likewise return an iterator or be adapted to owned values.
Suggestion:
| .map(|sandbox| CompletionCandidate::new(sandbox.sandbox_id.clone())) | |
| .map(|sandbox| CompletionCandidate::new(sandbox.sandbox_id)) |
LSX-s-Software
left a comment
LSX-s-Software
left a comment
There was a problem hiding this comment.
Thanks for contributing. I think this approach is more elegant and maintainable. However, there are some issues that should be fixed. In addition, please fix the clippy error and remove the docs generated by superpowers.
| pub fn active_sandbox_candidates() -> Vec<CompletionCandidate> { | ||
| sandbox_candidates(|state| matches!(state, Some("running") | Some("paused"))) | ||
| } |
There was a problem hiding this comment.
Since a sandbox can only be in one of two states—running or paused—this filter seems a bit redundant.
| const DYNAMIC_CONNECT_TIMEOUT: Duration = Duration::from_millis(250); | ||
| const DYNAMIC_REQUEST_TIMEOUT: Duration = Duration::from_millis(500); |
There was a problem hiding this comment.
I think the timeout is a bit too short for remote users. Perhaps 500ms for connect and 1s for request is more appropriate.
| #[arg(long = "sandbox-id")] | ||
| #[arg( | ||
| long = "sandbox-id", | ||
| add = crate::commands::completion::add_active_sandbox_candidates() |
There was a problem hiding this comment.
Please remove this completion candidate since the lifetime of the sandbox and its snapshots are independent.
2 participants
Summary
This is one of two parallel experiments for #37. It evaluates
clap_completeunstable-dynamicfor dynamic sandbox-ID completion while preserving the existing staticaenv completion bash|zsh|fishcommand.What it adds
CompleteEnvshell adapter for Bash, Zsh, and Fish.Trade-off
This keeps the shell protocol small and shell-portable, but depends on
clap_completeunstable-dynamic and its generated shell adapter contract. The companion PR proposes an AgentENV-ownedaenv __completeprotocol instead. These are alternatives; please review and choose one direction rather than merging both.Verification
cargo fmt --allcargo check -p aenv --bin aenvcargo test -p aenv --bin aenv(56 passed; one pre-existing macOS/varsymlink test failure in download tests)