feat: completions for tool

[vbvictor]

No description provided.

[vbvictor]

@claude review this

[claude]

Claude finished @vbvictor's task in 2m 15s —— View job

---

Code Review: feat: WIP completions

Tasks

• Gather context and understand the request
• Read changed files
• Analyze code changes
• Post review feedback

---

Overall this is a solid addition. The implementation is clean, tests are comprehensive (both unit and functional), and the Docker-based integration tests are a nice touch. A few issues to address before merging:

---

Bugs

1. Bash completion: acp pr <TAB> shows nothing

In acp.py lines 26-47, the bash completion's final fallthrough only triggers when cur starts with -:

if [[ ${cur} == -* ]]; then
    COMPREPLY=( $(compgen -W "${opts}" -- "${cur}") )
fi

When the user types acp pr and hits TAB, cur is empty (""), so this condition is false and no completions are returned. The user has to explicitly type acp pr --<TAB> to see options. For the pr subcommand, showing options when cur is empty would be more useful.

One way to fix this: check which subcommand is active before the final if:

local subcmd="${COMP_WORDS[1]}"
if [[ ${cur} == -* ]] || [[ "${subcmd}" == "pr" && -z "${cur}" ]]; then
    COMPREPLY=( $(compgen -W "${opts}" -- "${cur}") )
fi

Fix this →

---

2. CI workflow: incorrect actions/checkout version comment

In .github/workflows/completions.yaml line 19:

- uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1

actions/checkout is only at v4.x — there is no v6.0.1. The SHA may point to a valid commit, but the comment is misleading. If the SHA is correct, the comment should say # v4.x.x (or whatever version it actually pins to). Pinning by SHA is good practice; the incorrect version comment could cause confusion during dependency audits.

Fix this →

---

Design Observations

3. args.message used as the shell argument

In main() (acp.py:936–943):

if args.command == "completions":
    shell = args.message

Reusing args.message (the commit message positional) for the shell name works but is semantically confusing. Since you're not refactoring the parser right now, this is an acceptable tradeoff — but consider a comment explaining why message is used here.

4. Hardcoded completion scripts will drift

The completion scripts in get_bash_completion(), get_zsh_completion(), and get_fish_completion() hardcode all flags. Every time a new flag is added to acp.py, all three completion scripts need to be updated manually. This is a common tradeoff and is fine for a tool this size — just something to be aware of (and the CI test will catch drift in flag names).

5. Zsh completion uses _describe instead of _arguments

The zsh completion works and is syntactically valid, but using _arguments would give richer behavior (e.g., automatically marking --body and --reviewers as taking arguments, showing inline descriptions per flag). _describe is simpler but less idiomatic for zsh. Not a blocker, but worth noting for a future improvement.

---

What's Good

• The three-function structure (get_bash_completion, get_zsh_completion, get_fish_completion) is clean and easy to maintain.
• TestBashCompletionFunctional actually runs bash in a subprocess to validate completion output — this is much stronger than purely unit testing the string content.
• The Docker + shell integration test (test_completions.sh) covers syntax validation across all three shells, which is hard to do in a pure Python test.
• The make test-completions target is a nice ergonomic addition.
• Help text (show_help()) correctly documents the new subcommand and includes installation instructions for each shell.

---