Resolve a bare numeric cards <id> to a clear error instead of silent exit-0 - #678
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. |
There was a problem hiding this comment.
🟡 Changes recommended
Existing end-to-end expectations will fail, and typo suggestions do not currently use Cobra’s configured distance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents bare positional arguments on command groups from silently exiting successfully.
Changes:
- Converts unknown group arguments into usage errors with hints.
- Suppresses misleading help output.
- Adds unit and end-to-end coverage.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
internal/cli/root.go |
Creates structured unknown-subcommand errors and hints. |
internal/cli/help.go |
Detects unknown positional arguments on groups. |
internal/cli/help_test.go |
Tests detection, hints, and valid routing. |
e2e/errors.bats |
Verifies non-zero structured errors. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if helpFlag := cmd.Flags().Lookup("help"); helpFlag != nil && helpFlag.Changed { | ||
| return false | ||
| } | ||
| return len(cmd.Flags().Args()) > 0 |
There was a problem hiding this comment.
Right call — these nested groups carry the identical silent-exit-0 footgun, so I extended the contract rather than narrowing the predicate. chat foobar, cards column foobar, and cards step foobar now return the same usage error, and I updated their expectations in e2e/chat.bats and e2e/cards_columns_steps.bats. Ran the full e2e suite to be sure — those three were the only affected cases. Fixed in e1fd344.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/cli/help.go">
<violation number="1" location="internal/cli/help.go:23">
P3: `basecamp help cards 12345` now prints nothing and exits 0. Cobra's `help` subcommand calls cards.Help() directly (no --help flag set), so isBareGroupWithUnknownArg(cards) fires in rootHelpFunc and suppresses output, while Execute() won't convert it because executedCmd is the runnable `help` command. This reintroduces the exact silent exit-0 no-op the PR targets, but on the explicit help path. Gate the suppression so it doesn't apply when the group is reached through the `help` subcommand.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // help output so Execute() can emit a usage error with non-zero exit. | ||
| // Must run before machine-help branches to avoid mixed output. | ||
| if isBareGroupWithFlags(cmd) { | ||
| if isBareGroupWithFlags(cmd) || isBareGroupWithUnknownArg(cmd) { |
There was a problem hiding this comment.
P3: basecamp help cards 12345 now prints nothing and exits 0. Cobra's help subcommand calls cards.Help() directly (no --help flag set), so isBareGroupWithUnknownArg(cards) fires in rootHelpFunc and suppresses output, while Execute() won't convert it because executedCmd is the runnable help command. This reintroduces the exact silent exit-0 no-op the PR targets, but on the explicit help path. Gate the suppression so it doesn't apply when the group is reached through the help subcommand.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cli/help.go, line 23:
<comment>`basecamp help cards 12345` now prints nothing and exits 0. Cobra's `help` subcommand calls cards.Help() directly (no --help flag set), so isBareGroupWithUnknownArg(cards) fires in rootHelpFunc and suppresses output, while Execute() won't convert it because executedCmd is the runnable `help` command. This reintroduces the exact silent exit-0 no-op the PR targets, but on the explicit help path. Gate the suppression so it doesn't apply when the group is reached through the `help` subcommand.</comment>
<file context>
@@ -20,7 +20,7 @@ func rootHelpFunc() func(*cobra.Command, []string) {
// help output so Execute() can emit a usage error with non-zero exit.
// Must run before machine-help branches to avoid mixed output.
- if isBareGroupWithFlags(cmd) {
+ if isBareGroupWithFlags(cmd) || isBareGroupWithUnknownArg(cmd) {
return
}
</file context>
There was a problem hiding this comment.
Good catch. In the explicit basecamp help cards 12345 route the help subcommand renders cards' help itself without parsing cards' flags, so cmd.Flags().Args() is empty there and the guard does not fire — verified it still prints help and exits 0, and added an e2e regression test (help for a group with a stray id still renders help). 809e8b5.
There was a problem hiding this comment.
🟡 Changes recommended
Numeric hints can recommend show commands that do not accept or use an identifier.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| if isAllDigits(arg) && hasSubcommandNamed(cmd, "show") { | ||
| return output.ErrUsageHint(msg, fmt.Sprintf("Did you mean %q?", cmd.CommandPath()+" show "+arg)) |
There was a problem hiding this comment.
Fixed in 809e8b5 — the numeric show <id> hint is now gated on the group's show subcommand actually declaring an identifier positional (via ParseArgs). accounts, hillcharts, and notes fall back to --help; cards, todos, docs, etc. keep the hint. Added a singleton regression case in both the unit table and e2e.
![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: e1fd344fa2
ℹ️ 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".
| if isAllDigits(arg) && hasSubcommandNamed(cmd, "show") { | ||
| return output.ErrUsageHint(msg, fmt.Sprintf("Did you mean %q?", cmd.CommandPath()+" show "+arg)) |
There was a problem hiding this comment.
show <id> when the subcommand accepts an ID
For numeric input, checking only that a show child exists produces invalid remediation for singleton resources. For example, basecamp accounts 123 now suggests basecamp accounts show 123, but newAccountsShowCmd (internal/commands/accounts.go:171-183) shows the current account and does not consume an ID; because it also lacks an argument validator, following the hint can return plausible details for the wrong account. hillcharts has the same shape. Restrict this hint to groups whose show command actually accepts an ID, otherwise fall back to help.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 809e8b5 — restricted the show <id> hint to groups whose show accepts an identifier (checked via ParseArgs), so accounts 123 and hillcharts 3 now fall back to --help instead of steering to a show that ignores the id. Thanks for naming the wrong-record risk.
A group noun handed a positional that matches no subcommand — "cards 12345", "todos 999" — stopped at the group and rendered help with a zero exit. An agent that meant "cards show 12345" read that success while nothing ran: a silent no-op masked as OK. Cobra only rejects an unknown first arg for the root command; every non-root group falls through to help. Catch the leftover positional at the same seam that already converts a bare group with stray flags into a usage error, and return a non-zero usage error that names the unknown command and points at the canonical path — spelling out "<group> show <id>" for a numeric id, offering spelling suggestions otherwise. Bare "cards" and "cards --help" still show help; "cards show 123" still routes to the subcommand.
Borrow the root's SuggestionsMinimumDistance so a mistyped subcommand
("messages shwo") suggests "show" — group commands leave the threshold at
zero, which matches only an exact prefix.
The guard already covers every non-runnable group, so unknown positionals
on nested groups ("chat foobar", "cards column foobar", "cards step
foobar") now return the same usage error; update their expectations from
silent help to the error contract. Add a regression test proving explicit
--help still renders help past the guard.
Suggest "<group> show <id>" only when that group's show subcommand actually takes an identifier. Singleton reads (accounts, hillcharts, notes) have a show that ignores a positional, so steering a numeric arg there would silently return the wrong record; those now fall back to the group's help. Decide the --help exemption from the parsed flag value rather than whether it changed, so an explicit --help=false no longer slips a stray positional past the guard. Fold the shared non-runnable-group test into one isBareGroup helper so the flag and positional predicates can't drift.
809e8b5 to
6fbf3de
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fbf3dee4a
ℹ️ 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".
Check the stray-positional case before the bare-flags case so a scoped
invocation ("cards 12345 --in myproject") keeps the specific "did you
mean cards show 12345" hint instead of collapsing to the generic
"subcommand required".
Match the group's show action by alias as well as primary name, so a
command like chat's id-taking "line" that exposes "show" as an alias
still earns the numeric show hint.
The defect
basecamp cards 12345— a bare numeric positional on the group nouncards— returned exit 0 and the group help. To an agent that meantbasecamp cards show 12345, that reads as success while nothing ran: a silent no-op masked as OK.This is the residual from #416 ("Remove shortcut commands that shadow group nouns"). That PR fixed the top-level
basecamp card 12345case — it now errors non-zero withunknown command "card" … Did you mean cards?. But the same footgun survived one level down on the group nouns themselves.Investigation showed it is systemic, not cards-only:
cards,todos,messages, andcommentsall exited 0 + help on a bare positional. Root cause — Cobra's defaultlegacyArgsrejects an unknown first arg only for the root command; every non-root group parent (noRunE) falls through to help with a zero exit.Convention matched
The CLI already has a bare-group seam:
isBareGroupWithFlagsconvertsbasecamp cards --in X(a group invoked with stray flags, no subcommand) into a non-zero usage error, and the root help function suppresses help output soExecute()can emit it. This change extends that exact seam to cover a stray positional the same way it already covers stray flags — mirroring #416's top-level fix one level down, and honoring the Agent Accessibility principle: no silent exit-0 that hides a no-op. Barecards(no args) still shows help with exit 0; nothing about the command surface changes.The fix
isBareGroupWithUnknownArg— a non-runnable group carrying a leftover positional (parallel toisBareGroupWithFlags). The root help function suppresses help for it;Execute()converts it to a usage error.unknownSubcommandErrorreturnsoutput.ErrUsageHint(codeusage, exit 1) that names the unknown command and points at the canonical path: for a numeric id it spells outDid you mean "basecamp cards show 12345"?; otherwise it offers Cobra's spelling suggestions, falling back to--help.Because the fix lives at the shared seam, it resolves
cards(the residual) and every sibling group noun uniformly.The error is classified
usage(exit 1), notapi_error— an agent branching on the code sees a usage mistake it must not retry, per the same reasoning behindTestTransformCobraErrorClassifiesArityAsUsage.Test coverage
internal/cli/help_test.go:isBareGroupWithUnknownArgdetectscards 12345/todos 999/comments 55/ a typo, with help suppressed;unknownSubcommandErrorasserts code, message, and both hint variants (numeric→show, typo→help); a routing test confirmscards show 123resolves past the guard to the runnable leaf.e2e/errors.bats:basecamp cards 12345andbasecamp messages shwoexit non-zero with the usage envelope and hint.Tracking: Agent Accessibility card 10265932727 (residual from #416).
Summary by cubic
Group commands previously treated stray positional arguments as successful help; they now return a non-zero
usageerror. For example,basecamp cards 12345suggestscards show 12345instead of masking a no-op as success.cards,todos,messages,comments,chat, and nested groups such ascards columnandcards step.showcommands; singleton groups such asaccountsfall back to--help.cards 12345 --in myprojectretain the specific hint.--helpremain unchanged;--help=falsestill reports the stray argument.Written for commit 11011ee. Summary will update on new commits.