Skip to content

fix: reject unsupported shells in completion command - #326

Open
fzipi wants to merge 2 commits into
mainfrom
fix/completion-arg-validation
Open

fix: reject unsupported shells in completion command#326
fzipi wants to merge 2 commits into
mainfrom
fix/completion-arg-validation

Conversation

@fzipi

@fzipi fzipi commented Aug 17, 2026

Copy link
Copy Markdown
Member

What

completion declared ValidArgs but validated with cobra.MatchAll(cobra.ExactArgs(1)). Without cobra.OnlyValidArgs, ValidArgs only drives tab-completion and is never enforced. An unrecognized shell passed validation, fell through the switch without matching a case, and left err nil:

$ crs-toolchain completion badshell
$ echo $?
0

A typo like zshell produced an empty completion script and a success exit code, with nothing to indicate anything went wrong. Now:

$ crs-toolchain completion badshell
Error: invalid argument "badshell" for "crs-toolchain completion"
$ echo $?
1

Testing

New table-driven test in cmd/completion/completion_test.go covers all four supported shells plus the rejected forms. Removing cobra.OnlyValidArgs makes it fail.

go test ./cmd/..., go vet clean.

Note

Found while auditing argument handling across all commands after #325. The two are the same pair of knobs used wrongly in opposite directions: #325 had OnlyValidArgs with a placeholder in ValidArgs, rejecting every real argument, while this one had real values in ValidArgs but no OnlyValidArgs, accepting everything. No other command uses OnlyValidArgs.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved completion command validation to require exactly one supported shell argument.
    • Invalid, misspelled, missing, or extra arguments are now rejected consistently.
  • Tests

    • Added coverage for valid shell options and invalid argument combinations.

The completion command declared ValidArgs but validated with
`cobra.MatchAll(cobra.ExactArgs(1))`, so ValidArgs only drove shell
completion and never validation. Any unrecognized shell passed validation,
fell through the switch without matching a case, and left err nil:

    $ crs-toolchain completion badshell
    $ echo $?
    0

A typo produced an empty completion script and a success exit code. Add
`cobra.OnlyValidArgs` so the declared shells are enforced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 888d2555-d7bb-4bdb-9294-b0e52367b982

📥 Commits

Reviewing files that changed from the base of the PR and between aa29e39 and 4b63ce4.

📒 Files selected for processing (2)
  • cmd/completion/completion.go
  • cmd/completion/completion_test.go

Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The completion command now requires exactly one supported shell argument. Table-driven tests cover valid shells, unsupported shell names, missing arguments, and multiple arguments.

Changes

Completion argument validation

Layer / File(s) Summary
Validate completion shell arguments
cmd/completion/completion.go, cmd/completion/completion_test.go
The command restricts input to one declared shell option. Tests cover valid and invalid argument cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 4b63c

The command now rejects unsupported shells instead of succeeding with an empty completion script, with coverage for supported and rejected inputs; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description explains what changed, why it changed, and how it was tested, but it omits the required references section.
Title check ✅ Passed The title clearly and concisely describes the main change: rejecting unsupported shells in the completion command.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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