Skip to content

feat(cli): ota completion bash|zsh|fish subcommand#6

Open
posquit0 wants to merge 2 commits into
mainfrom
worktree-wf_4bbd8026-cea-2
Open

feat(cli): ota completion bash|zsh|fish subcommand#6
posquit0 wants to merge 2 commits into
mainfrom
worktree-wf_4bbd8026-cea-2

Conversation

@posquit0

@posquit0 posquit0 commented Jul 5, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds ota completion <shell> as a positional subcommand handled ahead of flag.FlagSet.Parse in cmd/ota/main.go, so completion does not trip the flag parser.
  • Ships hand-written bash / zsh / fish completion scripts as raw string constants in cmd/ota/completion.go — no cobra, no new deps. Each script advertises every top-level flag (--config, --profile, --token-env, --debug, --poll-interval, --version, --check) plus the completion subcommand and its bash|zsh|fish argument.
  • README picks up a new Shell Completion section with install blocks per shell (eval for one-shot, $fpath / bash_completion.d / ~/.config/fish/completions for persistent).

Usage:

eval "$(ota completion bash)"
eval "$(ota completion zsh)"
ota completion fish | source

Test plan

  • go build ./...
  • go test ./... -race -count=1
  • completion_test.go — asserts each shell's script contains the expected markers (#compdef ota, _ota(), complete -c ota, __fish_use_subcommand) plus every flag name
  • completion_test.go — asserts missing / unknown shell exits 2 and prints a helpful message to stderr (echoes the bad shell name, enumerates supported shells)
  • Manual smoke: built ota, ran ota completion {bash,zsh,fish}, confirmed real completion scripts on stdout

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds shell completion support for Bash, Zsh, and Fish to the ota CLI tool, complete with documentation and unit tests. The review feedback suggests two important improvements: moving the subcommand check after flag parsing so that global flags are correctly handled, and declaring state variables as local in the Zsh completion script to avoid polluting the global shell namespace.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread cmd/ota/main.go Outdated
Comment on lines +50 to +58
// `ota completion <shell>` is a positional subcommand — handle it before
// the flag parser rejects the non-dash argument.
if len(args) > 0 && args[0] == "completion" {
shell := ""
if len(args) > 1 {
shell = args[1]
}
return runCompletion(shell, stdout, stderr)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current implementation checks for the completion subcommand before parsing flags. However, Go's standard flag package does not reject positional arguments; it simply stops parsing when it encounters the first non-flag argument (leaving it in fs.Args()).

By checking args[0] == "completion" before fs.Parse, any global flags passed before the subcommand (e.g., ota --profile dev completion bash or ota --config myconfig.yaml completion zsh) will bypass the subcommand check, causing fs.Parse to run and subsequently launch the TUI instead of printing the completion script.

Recommendation

Move the completion subcommand check to after fs.Parse(args) using fs.Arg(0) and fs.Arg(1):

	if err := fs.Parse(args); err != nil {
		// flag.ErrHelp is not an error — user asked for help.
		if errors.Is(err, flag.ErrHelp) {
			return 0
		}
		fmt.Fprintln(stderr, "ota:", err)
		return 2
	}

	if fs.NArg() > 0 && fs.Arg(0) == "completion" {
		shell := ""
		if fs.NArg() > 1 {
			shell = fs.Arg(1)
		}
		return runCompletion(shell, stdout, stderr)
	}

Comment thread cmd/ota/completion.go
# zsh completion for ota -*- shell-script -*-

_ota() {
local -a flags subcmds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In Zsh completion functions, when using _arguments -C with state transitions (->cmd, ->args), the _arguments function sets several variables like state, context, and line to communicate back to the caller.

If these variables are not declared as local within the completion function, they will leak into the global shell namespace. This can pollute the user's active shell session and potentially break or interfere with other Zsh completions.

Declaring local context state line at the beginning of the function is the standard best practice to prevent this.

Suggested change
local -a flags subcmds
local -a flags subcmds
local context state line

posquit0 added 2 commits July 5, 2026 23:50
Adds a `completion` subcommand to cmd/ota that prints a hand-written
completion script for the requested shell. Handled ahead of flag parsing
so the positional argument does not confuse flag.FlagSet. Scripts cover
every top-level flag (--config, --profile, --token-env, --debug,
--poll-interval, --version, --check) plus the `completion` subcommand
and its shell argument list.

README picks up a "Shell Completion" section with bash / zsh / fish
install blocks.
…sh state

- Move the `completion` subcommand detection to after fs.Parse so
  `ota --profile foo completion bash` still writes the script instead
  of booting the TUI.
- Declare `context state line` as local in the zsh _ota function so the
  values _arguments -C writes do not leak into the caller's shell.
- Add a regression test that runs `run()` with a global flag ahead of
  the completion subcommand and asserts the bash script is emitted.
@posquit0 posquit0 force-pushed the worktree-wf_4bbd8026-cea-2 branch from 6994c4b to 79a3a17 Compare July 5, 2026 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant