feat: auto-install shell completions via install#504
Conversation
Coverage Report ✅
Coverage BadgeNo Go source files changed in this PR. |
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
Inline comments from cross-review (Claude Code + Codex + CodeRabbit)
Cross-Review Summary (Iterative Consensus)Reviewers: Claude Code, Codex, CodeRabbit Confirmed Issues (all three reviewers agree)
Contested Issues (2-1 split)
Dismissed Findings
Positive Observations
Iterative cross-review by Claude Code + Codex + CodeRabbit (2 rounds) |
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
Inline comments from iterative cross-review (Claude Code + Codex + CodeRabbit, 2 rounds)
| local target_dir tmp_file | ||
| target_dir=$(dirname "$target") | ||
| mkdir -p "$target_dir" 2>/dev/null || sudo mkdir -p "$target_dir" 2>/dev/null || return 1 | ||
| tmp_file=$(mktemp) || return 1 |
There was a problem hiding this comment.
[Major — confirmed by all 3 reviewers] mktemp creates files with 0600 permissions. After sudo mv to a system completion dir, non-root users can't read the file — completions silently fail. Add chmod 644 "$tmp_file" before the mv.
Iterative cross-review: Claude Code AGREE, Codex AGREE, CodeRabbit AGREE
| setup_fish_completions() { | ||
| local bin_path="$1" | ||
| local fish_dir="${XDG_CONFIG_HOME:-$HOME/.config}/fish" | ||
| [[ -d "$fish_dir" ]] || return 0 |
There was a problem hiding this comment.
[Minor — confirmed by all 3 reviewers] Fish completions only install to user dir. Under sudo, $HOME resolves to /root, so completions land in /root/.config/fish/ — useless for the actual user. Bash/zsh try system dirs first with a user-dir fallback, but fish has no system-dir attempt. Consider adding a system path fallback (e.g., /usr/share/fish/vendor_completions.d/).
Iterative cross-review: Claude Code AGREE (Low), Codex AGREE (Medium), CodeRabbit AGREE
| } | ||
|
|
||
| # ============================================================================== | ||
| # GitHub API Functions |
There was a problem hiding this comment.
[Minor — confirmed by all 3 reviewers] || true swallows all errors from setup_completions, including catastrophic failures like get_os failing. The individual sub-functions already warn, but a top-level catch would help edge cases. Consider: setup_completions || warn "Shell completion setup failed (non-fatal)"
Iterative cross-review: Claude Code AGREE, Codex AGREE, CodeRabbit AGREE
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Nice one! Love that completions are automatic now — been wanting this. Just one real blocker (the mktemp permissions thing) and a couple small suggestions. Should be quick to fix.
| if [[ -w "$target_dir" ]]; then | ||
| mv "$tmp_file" "$target" || { rm -f "$tmp_file"; return 1; } | ||
| else | ||
| sudo mv "$tmp_file" "$target" || { rm -f "$tmp_file"; return 1; } |
There was a problem hiding this comment.
This is the main thing — mktemp creates files 0600, so after sudo mv nobody else can read them and completions silently break. Slap a chmod 644 "$tmp_file" before the sudo mv branch and you're good.
| setup_fish_completions() { | ||
| local bin_path="$1" | ||
| local fish_dir="${XDG_CONFIG_HOME:-$HOME/.config}/fish" | ||
| [[ -d "$fish_dir" ]] || return 0 |
There was a problem hiding this comment.
When the script runs under sudo, $HOME is /root so fish completions end up in /root/.config/fish/ — not super useful. Bash/zsh handle this by trying system dirs first. Maybe try /usr/share/fish/vendor_completions.d/ before falling back to the user dir?
| } | ||
|
|
||
| # ============================================================================== | ||
| # GitHub API Functions |
There was a problem hiding this comment.
Nit: || true eats everything silently. Maybe || warn "Shell completion setup failed (non-fatal)" so there's at least a breadcrumb if something weird happens?
4 participants
Summary
prefix detection) and user-local XDG fallback
uninstall section
Test plan
Motivation / Context
Closes #338
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/api,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Risk Assessment
Rollout notes:
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info