feat(cli): add shell completion scaffolding - #89
Conversation
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 6 selected item(s). |
LSX-s-Software
left a comment
There was a problem hiding this comment.
Thanks for contributing. Could you please add some documentation to docs/src/getting-started/aenv-cli.md explaining how to use the completion command? In addition, could you help us implement the automatic shell completion installation mentioned in the issue?
Document the `aenv completion` command added in PR kvcache-ai#89: how to generate a bash/zsh/fish script and activate it for the current or future shell sessions. Static CLI surface only; dynamic resource-identifier completion remains a separate follow-up. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Added the documentation. Related to dynamic completion added a comment #37 (comment) related to clap . Could you please have a look and let know the best option, based on that I will do follow-up PR for dynamic shell completion. |
LSX-s-Software
left a comment
There was a problem hiding this comment.
Thanks for adding the completion documentation. However, the documented installation paths are not correct for Bash and Zsh. Could you please update these instructions based on the Bash and Zsh documentation?
It would also be preferable to implement the automatic installation and removal requested in issue #37: make install-aenv, scripts/install-cli.sh, and the full installer should install lightweight completion loaders into the standard shell completion directories, while make uninstall-aenv should remove them. The loaders should generate completion code from the currently installed aenv binary when the shell starts, so upgrading the CLI does not leave stale generated scripts behind.
Regarding the dynamic completion feature, I think it's OK to wait for clap to support it in a stable version.
Point the redirect example at the standard per-user completion directories instead of ad-hoc locations: bash: ~/.local/share/bash-completion/completions/aenv zsh: ~/.local/share/zsh/site-functions/_aenv fish: ~/.config/fish/completions/aenv.fish (unchanged) and document that zsh does not put the site-functions dir on fpath by default, so it must be added before compinit. Addresses review feedback on kvcache-ai#89 (paths did not match bash/zsh docs).
Address the CHANGES_REQUESTED item on PR kvcache-ai#89 / issue kvcache-ai#37: wire regenerating shell-completion loaders into make install-aenv, scripts/install-cli.sh, and scripts/install.sh, and remove them with make uninstall-aenv. The loaders source `aenv completion <shell>` at shell start (fish/zsh-user) or on first aenv <TAB> (bash lazy-load), so completion always matches the installed binary and never goes stale across upgrades. - scripts/shell-completion.sh: canonical install/uninstall helper (the single source of truth), with --prefix/--user flags and user-vs-system destination selection. - install-cli.sh / install.sh: inline the helper verbatim (these are curl|bash'd standalone) and call it after the binary install. - scripts/check-completion-sync.sh: drift guard enforcing the three copies stay byte-identical; wired into CI and `make check-shell-completion`. - Makefile: install-aenv/uninstall-aenv run the loader setup/teardown (AENV_INSTALL_COMPLETION=0 to skip). - scripts/tests/verify-shell-completion.sh: 7-case functional test including the empty-HOME mode-detection regression. - ci.yml: new shell-scripts job (shellcheck + drift + functional). - docs: note that loaders are now installed/removed automatically.
Shell-completion installers — status and dispositionsHeads-up for reviewers on the Implemented (per the issue #37 ask + hardening)
Intentionally declined (with rationale)These recur on every bot run; I'm recording the rationale once rather than re-litigating each round:
Happy to revisit any of these if a maintainer disagrees — flagging them here so the recurring bot comments can be read as known/decided rather than new. |
LSX-s-Software
left a comment
There was a problem hiding this comment.
Thanks for implementing the automatic completion installation as requested. However, I did not expect the installation support to require this much new infrastructure. After reviewing the resulting implementation, I have two blocking design concerns.
1. The installer must not modify ~/.zshrc
The user-mode installation currently installs Zsh completion by inserting a managed block into ~/.zshrc.
This directly conflicts with issue #37, which explicitly states:
Installers should use standard completion directories and must not modify
.bashrc,.zshrc, or Fish configuration files automatically. If additional shell configuration is required, the installer should print an actionable message.
Shell startup files are user-managed configuration. Automatically rewriting them introduces behavior users may not expect and requires the installer to handle marker parsing, upgrades, locking, atomic replacement, symlinks, file permissions, and uninstall restoration. Most of the complexity added by this PR exists solely because it chose to edit .zshrc.
Please install the Zsh completion file into the standard user completion directory instead, for example:
~/.local/share/zsh/site-functions/_aenv
If that directory is not present in the user’s fpath, the installer should print an actionable message such as:
fpath=(~/.local/share/zsh/site-functions $fpath)
autoload -Uz compinit
compinitThe user can then decide whether and where to add that configuration. Installation and removal should only manage the completion file itself, not the user’s shell startup files.
2. The installer implementation is disproportionately large and duplicated
This PR is described as a static completion scaffolding change, but it currently adds approximately 2,300 lines. A major part of that comes from copying the same roughly 500-line aenv_completion_install implementation into three files:
scripts/shell-completion.shscripts/install-cli.shscripts/install.sh
That is approximately 1,500 lines of duplicated shell code. The duplication then requires an additional synchronization checker, dedicated filesystem-management tests, and a separate CI job.
This is disproportionate to the feature being implemented. The required behavior is relatively small:
- install three lightweight completion loaders into standard directories;
- remove the files during uninstall;
- avoid overwriting unrelated files;
- cover basic installation, upgrade, and removal behavior;
- print configuration guidance when necessary.
Without editing .zshrc or implementing a general-purpose shell configuration file manager, I would expect the automatic installation portion to be roughly 200–300 lines including its tests. This is an estimate rather than a strict line-count requirement, but if a robust implementation cannot reasonably stay within approximately 300–400 lines, that is a strong indication that it should be reviewed separately.
Therefore, either of the following approaches would be acceptable:
Option A: Keep automatic installation in this PR
This is fine if the implementation can be significantly simplified:
- do not modify shell startup files;
- install only small files into the standard completion directories;
- remove the large duplicated helper and its synchronization machinery;
- keep the installation implementation and tests within a reasonably small scope.
Option B: Split the work into two PRs
If automatic installation still requires substantial installer-specific work, please split it:
-
This PR contains:
aenv completion bash|zsh|fish;- visible aliases;
- Rust tests;
- manual activation documentation.
-
A follow-up PR contains:
- automatic installation and removal;
- standard completion-directory integration;
- installer-specific tests and documentation.
Again, I appreciate that the combined scope came from my earlier request. The concern is not that automatic installation was included, but that the current solution has grown into a large and duplicated shell file-management subsystem. Please either simplify that portion substantially or move it into a focused follow-up PR.
Add `aenv completion <shell>` to emit static completion scripts for bash, zsh, and fish through clap_complete. The full command tree is rebuilt from the Cli derive spec, so generated completion cannot drift from the real CLI. Static completion covers top-level and nested subcommands, command aliases, flags, the --output table|json enum, and local path arguments such as the Dockerfile passed to `aenv build`. The command aliases (cn, ls, rm, snap, templates, plus the snapshot and template subcommand aliases) are promoted from hidden `alias` to `visible_alias`. clap_complete only emits visible aliases, and kvcache-ai#37 explicitly requires alias completion; the side effect is that the aliases now also appear in `--help`. Dynamic resource completion (sandbox IDs filtered by state, template and snapshot names, the start --cold OCI-ref exception) and the shell-loader/installer wiring are deliberately out of scope here and will follow up. Refs kvcache-ai#37
Address review feedback on the completion command: - Generate the script into an in-memory buffer before writing to stdout. clap_complete's generators panic on write errors (Generator::generate calls .expect), so emitting straight to stdout turned a closed downstream pipe into a panic. A Vec cannot fail, so generation is infallible; only the explicit stdout write can, and it propagates through the Result. - Replace loose substring assertions with bash-specific fragments that encode the relevant context (`aenv,cn)` for the connect alias, `aenv__subcmd__snapshot__subcmd__create` for the nested subcommand, and `compgen -W "table json"` for the --output enum), so a regression can no longer hide behind an incidental substring match.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Address further review feedback: the alias, nested-subcommand, and --output enum assertions matched clap_complete's generated bash internals (aenv,cn), (aenv__subcmd__snapshot__subcmd__create), and the exact compgen -W "table json" string), which a compatible clap_complete upgrade could reformat without changing completion behavior. Assert on the Command tree instead — connect's visible aliases, snapshot's create subcommand, and the --output argument's possible values — and keep only the per-shell smoke tests (bash/zsh/fish registration) on the generated output.
Address review feedback:
- Extract the generation/write path into write_completion(shell, &mut W),
so run() is a one-liner over locked stdout and the write branches are
unit-testable without depending on process-global stdout. Add a
FailingWriter stub and cover all three branches: success (Vec),
BrokenPipe treated as success (the `aenv completion bash | head` case),
and other I/O errors propagating.
- Wrap the propagated non-BrokenPipe error with
.context("writing completion script to stdout") so it surfaces with
actionable context rather than a bare OS error.
Document the `aenv completion` command added in PR kvcache-ai#89: how to generate a bash/zsh/fish script and activate it for the current or future shell sessions. Static CLI surface only; dynamic resource-identifier completion remains a separate follow-up. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Point the redirect example at the standard per-user completion directories instead of ad-hoc locations: bash: ~/.local/share/bash-completion/completions/aenv zsh: ~/.local/share/zsh/site-functions/_aenv fish: ~/.config/fish/completions/aenv.fish (unchanged) and document that zsh does not put the site-functions dir on fpath by default, so it must be added before compinit. Addresses review feedback on kvcache-ai#89 (paths did not match bash/zsh docs).
Address the CHANGES_REQUESTED item on PR kvcache-ai#89 / issue kvcache-ai#37: wire regenerating shell-completion loaders into make install-aenv, scripts/install-cli.sh, and scripts/install.sh, and remove them with make uninstall-aenv. The loaders source `aenv completion <shell>` at shell start (fish/zsh-user) or on first aenv <TAB> (bash lazy-load), so completion always matches the installed binary and never goes stale across upgrades. - scripts/shell-completion.sh: canonical install/uninstall helper (the single source of truth), with --prefix/--user flags and user-vs-system destination selection. - install-cli.sh / install.sh: inline the helper verbatim (these are curl|bash'd standalone) and call it after the binary install. - scripts/check-completion-sync.sh: drift guard enforcing the three copies stay byte-identical; wired into CI and `make check-shell-completion`. - Makefile: install-aenv/uninstall-aenv run the loader setup/teardown (AENV_INSTALL_COMPLETION=0 to skip). - scripts/tests/verify-shell-completion.sh: 7-case functional test including the empty-HOME mode-detection regression. - ci.yml: new shell-scripts job (shellcheck + drift + functional). - docs: note that loaders are now installed/removed automatically.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Address OpenCodeReview findings (bug/security/test) on the completion
loader wiring, plus fix the broken CI job introduced by an applied
suggestion (duplicate runs-on + empty permissions).
Canonical helper (scripts/shell-completion.sh, inlined verbatim into
install-cli.sh and install.sh):
- Marker removal now does single-pass structural validation: reject an
end-without-start, nested start, or unterminated start at EOF, and
leave the rc untouched on any malformation (previously equal marker
counts could let a stray start marker delete rc content through EOF).
- Install idempotency requires a complete well-formed block; a partial
block warns instead of leaving the user stuck or duplicating.
- Static zsh generation is atomic: generate into a temp file in the
dest dir and rename on success, so a failure never truncates an
existing valid _aenv.
- Static zsh generation prefers the just-installed ${prefix}/bin/aenv
over whatever aenv is first on PATH (avoids stale/skipped generation).
- Capture HOME once (${HOME:-}) and guard user-mode destination paths so
an unset HOME with --user/a bare invocation warns and skips instead of
aborting under set -u.
- Rewrite the rc in place (cat onto it) on removal to preserve its
inode, mode, and symlink target rather than replacing the link.
CI: fix the shell-scripts job (single runs-on, timeout-minutes: 15,
permissions: contents: read).
Drift checker: assert exactly one well-ordered BEGIN..END pair per file
and compare byte-preserving temp files with cmp.
Tests: hermetic no-aenv PATH; failing `aenv completion zsh` leaves the
existing _aenv intact with no temp leftovers; unrelated rc content
survives install+uninstall; --user with unset HOME warns and does not
abort.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Canonical helper (scripts/shell-completion.sh, inlined verbatim into install-cli.sh and install.sh): - Treat an orphan END marker as malformed (validation now triggers when either marker is present, not only on a start marker), so an rc with a lone END no longer gets a fresh block appended onto corrupt state. - Validate the awk rewrite before touching the live rc: chain `awk > tmp && cat tmp > rc` so a failed/partial awk never truncates ~/.zshrc (previously `cat` ran on an unvalidated tmp). - Guard every mktemp fallback and the awk/cat chain so a failure warns and returns 0 instead of aborting under set -e (closes the non-fatal contract for both _aenv_cc_put_zsh_static and _aenv_cc_rm_zsh_rc). - Guard the zsh rc-snippet with `command -v aenv` so a missing/uninstalled aenv no longer emits errors on every shell start. Drift checker: also require each installer to invoke the helper exactly once, so a deleted/broken call site cannot hide while the inlined blocks remain "in sync". Tests: - Malformed-marker safety now covers orphan-start, orphan-end, reversed, and nested layouts for both install and uninstall, asserting the rc is byte-for-byte unchanged. - Test 6 also asserts the fish completion is removed on uninstall. - Test 8 writes an invocation sentinel so it proves the prefix-local aenv was actually invoked (not just that _aenv was preserved).
…itive Self-review found that the previous rounds fixed the atomicity / symlink / metadata properties at one write site at a time while sibling sites kept the old behavior — the review bot then re-flagged the same class of bug in the adjacent function across consecutive rounds. Funnel ALL filesystem writes through a single `_aenv_cc_commit <tmp> <dest>` primitive so the pattern physically cannot drift between functions. All four write sites now: stage to a temp created IN the destination directory (same filesystem -> atomic rename), resolve a symlinked destination so the link is preserved (not replaced), and copy the destination's current mode (0644 for a new file): - `_aenv_cc_put` (bash/fish stubs): was `printf > path` (truncated, followed symlinks); now atomic + symlink-safe. - `_aenv_cc_put_zsh_rc` (rc append): was `>>` (6 separate writes, not atomic, interruption left a malformed block); now stages the full new rc and renames. - `_aenv_cc_put_zsh_static`: already atomic; routed through the primitive for consistency. - `_aenv_cc_rm_zsh_rc` (rc removal): was temp in /tmp + cross-FS mv (not atomic); now uses a same-directory temp via the primitive. Net: every write shares the same atomic / symlink-preserving / mode-preserving behavior, closing the consistency gap that produced the repeated findings.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
… empty-output Propagate the comprehensive block rewrite (already in install.sh from the maintainer's edits) across shell-completion.sh and install-cli.sh so all three inlined copies stay byte-identical, and fix two regressions introduced by that rewrite: - Static zsh completion: the ownership marker was PREPENDED, which pushes the generated `#compdef aenv` off line 1 — zsh only loads a function from site-functions when #compdef is the first line, so this would silently break zsh completion. Append the marker as a trailing comment instead. - Empty-output guard: with the marker appended, the generated temp was always non-empty, so the `[[ -s $tmp ]]` check no longer rejected a broken aenv that exits 0 with no bytes — it would replace a valid _aenv with just the marker. Validate the generated BODY for non-emptiness before adding the marker. - _aenv_cc_owns now grep-matches the marker anywhere (line 1 for bash/fish stubs, trailing comment for the static zsh file). Tests: add empty-output preservation, #compdef-first-line + marker presence, and uninstall-leaves-non-aenv-files-untouched cases (the ownership-marker contract). Block also brings (from the maintainer's rewrite): portable symlink resolution (no readlink -f dependency), resolve-before-mktemp (atomic across symlinks), best-effort flock around the rc read-modify-write, stale-block in-place upgrade, compdef-already-defined guard, and honest ACL/xattr non-preservation note.
… owns guard Address the fifth review pass. Real bugs in the ownership-marker/locking rewrite plus a compile break. crates/aenv: fix a compile break from the FailingWriter suggestion — the struct has named fields (kind, fail_on_flush) but the two tests still constructed it tuple-style; the crate could not compile. Use named-field construction. Canonical block (inlined into shell-completion.sh / install-cli.sh / install.sh): - _aenv_cc_resolve: replace the one-hop BSD fallback (which could return an intermediate symlink and let mv replace it) with a bounded, cycle-detecting readlink loop. Multi-hop chains resolve portably without readlink -f; broken symlinks are rejected; a plain non-existent path is returned as-is so first-install is not blocked. - _aenv_cc_commit: replace GNU-only chown --reference with portable uid:gid extraction (stat -c / stat -f) + chown; refuse to commit when ownership preservation was required but failed. - _aenv_cc_with_zsh_lock: lock the rc's own read-only fd instead of a sidecar lock file — removes the symlink-truncation vector for privileged runs, the set -e abort on an unwritable lock path, and the leftover-artifact file. - _aenv_cc_put / _aenv_cc_put_zsh_static: install-side ownership guard (symmetric with uninstall) — do not overwrite an existing unmanaged file; ensure a separating newline before the appended marker when generated output lacks a trailing newline. - _aenv_cc_rm_owned: resolve the path before the ownership check and removal so a cycle through a symlinked completion removes the managed target, not the link. Tests: seed a managed _aenv before swapping to failing/empty stubs (so the new install-side guard permits the re-install attempt); capture baselines before and clear the invocation sentinel between attempts; add install-over-unowned.
3c50218 to
5a03bb8
Compare
Removed installation related changes for separate PR. |
|
@yingdi-shan adding a comment/reason for closing would be helpful, so it is clear that the pr no longer relevant. |
Splits #37 into a static-first scaffolding change. This adds the
aenv completion <shell>command and the static completion surface; dynamic resource completion and automatic installer integration are deferred to follow-up work.What it does
aenv completion bash|zsh|fishemits a static completion script viaclap_complete. The command tree is rebuilt from theCliderive spec (crate::Cli::command()), so the generated script always reflects the real CLI. There is no separate completion definition to keep in sync.--output table|jsonenum, and local path arguments such as the Dockerfile passed toaenv build.fpathguidance and one-session activation commands.Behavior change worth calling out
The command aliases
cn,ls,rm,snap,templatesand thesnapshot/templatesubcommand aliases usevisible_alias.clap_completeemits only visible aliases, so this is required for alias completion to work. The visible side effect is that these aliases now also appear in--help. Runtime parsing is unchanged; the aliases were already accepted as input.Dependency
Adds
clap_complete = "4", matching the existingclap = "4"major.Testing
cargo test -p aenv --bin aenv commands::completion— 11 completion tests covering Bash, Zsh, Fish, aliases, the--outputenum, nested subcommands, and output/flush error handling.cargo fmt --all -- --checkpasses.cargo check -p aenv --bin aenvpasses.The workspace-wide
make fmt/make clippyalso build the server and storage crates, which require Linux and/dev/kvm, so only theaenv-scoped checks were run here. No integration tests apply to this CLI-only change.Out of scope
Dynamic resource completion (sandbox IDs filtered by command-applicable state, template/snapshot names, and the
aenv start --coldOCI-ref exception) and automatic completion installation/removal. The installer work will be handled separately using standard completion directories without modifying shell startup files.Refs #37 - static scaffolding portion; dynamic completion and installer integration remain separate follow-up work.