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.
Apply the maintainer feedback on the clap dynamic completion experiment and keep the PR focused on the CLI behavior. Constraint: Completion must remain non-blocking for remote users and preserve the existing static generator Rejected: Keep snapshot list sandbox completion | snapshot and sandbox lifetimes are independent Confidence: high Scope-risk: narrow Directive: Keep dynamic completion limited to resources whose lifecycle matches the command context Tested: cargo fmt --all; cargo clippy -p aenv --bin aenv -- -D warnings; focused completion tests (12 passed); git diff --check Not-tested: Full workspace test suite and live API-backed completion candidates Related: kvcache-ai#163
| pub fn running_sandbox_candidates() -> Vec<CompletionCandidate> { | ||
| sandbox_candidates(|state| state == Some("running")) | ||
| } |
There was a problem hiding this comment.
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:
| 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. |
There was a problem hiding this comment.
@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.
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)