fix(autocomplete): complete inherited flags after subcommands - #166
fix(autocomplete): complete inherited flags after subcommands#166markstuart-oai wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom code✅ No new custom-code files detected. 5 mixed files remain; 0 existing customizations changed. Compared 5 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34176372943 --repo openai/openai-cli \
--name castiron-custom-code-34176372943-1 --dir /tmp/castiron-custom-code-34176372943-1
git apply --stat /tmp/castiron-custom-code-34176372943-1/custom-code.patch
cat /tmp/castiron-custom-code-34176372943-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 094a4ab96e80c2952c830ae36a9a9f009617e5ec 3fa6361069c7a513f52718d4067d46e93dc289bc
python3 scripts/castiron/custom_code_report.py report \
--base 094a4ab96e80c2952c830ae36a9a9f009617e5ec \
--head 3fa6361069c7a513f52718d4067d46e93dc289bc --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-3fa6361069c7
cat /tmp/castiron-custom-code-3fa6361069c7/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fa6361069
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| completions = builder.createFromFlag(current, &flag, completions) | ||
| } | ||
| seen := map[string]bool{} | ||
| for flag := range inheritedFlags(cmd, ancestors) { |
There was a problem hiding this comment.
When a nested invocation already contains a standalone -- (for example, openai models list -- --fo), traversal does not record that option parsing has ended, so this loop newly suggests inherited flags such as --format even though every following token is positional. The same issue can misclassify a later inherited flag as value-taking and suppress normal positional/file completion; inherited lookup should be disabled after consuming the separator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
Hidden local flags are currently skipped for traversal but still suggested for completion, which can mis-handle value consumption and expose hidden flags in completion output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes CLI flag autocompletion so that nested subcommands (e.g., models list) can complete flags inherited from ancestor commands, while preserving urfave/cli inheritance precedence and value-consumption semantics.
Changes:
- Extend completion traversal to consider inherited (ancestor) flags and consume their values while walking subcommands.
- Add targeted regression tests covering raw vs initialized command trees, protocol behavior, and the production
mainentrypoint completion output. - Validate shadowing/precedence rules (nearest-ancestor wins, child aliases suppress ancestor flags,
Localflags don’t inherit, hidden inherited flags aren’t suggested).
File summaries
| File | Description |
|---|---|
| internal/autocomplete/autocomplete.go | Implements inherited-flag discovery during traversal and augments flag completion to include inherited candidates. |
| internal/autocomplete/protocol_test.go | Adds protocol-level test cases for inherited flag completion/value/file behavior under nested commands. |
| internal/autocomplete/inheritance_test.go | New unit tests that pin inheritance/precedence and parser contract behaviors for completion. |
| cmd/openai/completion_test.go | New integration-style test that runs the real main completion path in a subprocess with sanitized environment. |
Review details
Suppressed comments (1)
internal/autocomplete/autocomplete.go:307
- Flag-name completion currently includes hidden local flags because
cmd.Flagsare always passed tocreateFromFlagwithout checkingcli.VisibleFlag. This is inconsistent with the inherited-flag path (which filters hidden flags) and can leak Hidden=true flags into completion output.
if isFlag(current) {
for _, flag := range cmd.Flags {
completions = builder.createFromFlag(current, &flag, completions)
}
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -112,9 +113,44 @@ func findFlag(cmd *cli.Command, arg string) *cli.Flag { | |||
| return &flag | |||
| } | |||
| } | |||
Summary
Flag completion stopped at the current command's declarations, so
openai __complete -- models list --foreturned no suggestions even though--formatand--format-errorare valid there. Include inherited flag names and aliases after nested commands, and consume their values before continuing command traversal.Use the pinned urfave inheritance contract: preserve child name/alias overrides, nearest-ancestor precedence, hidden inherited flags, and flags marked local. Preserve completion statuses, file completion and explicit
@paths. Completion does not execute API requests. Regression coverage exercises raw and initialized command trees, the production entrypoint, and rendered Bash completion.Validation
Passed locally with Go 1.27.0 on Linux (module version and dependencies unchanged):
go test ./internal/autocomplete ./cmd/openai -count=1,go test ./internal/..., andgo test ./... -run '^$'.go mod verify,./scripts/lint(build only),go test -race ./internal/autocomplete ./cmd/openai -count=1, andgo vet ./internal/autocomplete ./cmd/openai.GOOS=windows GOARCH=amd64 go test -c -o /tmp/openai-cli-windows-tests/ ./...(output directory abbreviated here); no Windows runtime test.--fochanges from empty output to--formatand--format-error; root output stays the same. All four probes exit 0 with empty stderr. The initial inherited-completion regression failed on the original implementation.Full
./scripts/testmock suite NOT RUN locally: the pinned Steady bootstrap failed loading the JSR@std/climanifest. A direct connectivity probe and an existing local health response did not establish a working mock server. No network or security controls were bypassed. Full CI/mock-suite validation remains required.