fix(cli): global flags visible, position-tolerant, and surface usage errors#10
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a global-flag registry and exported StripGlobals to extract globals from anywhere in argv; switches Dispatch and main to use it; narrows stderr suppression so leaf usage errors are not silently swallowed; appends a canonical "Global Flags:" section to root and leaf help output. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Main
participant Dispatch as CLI Dispatch
participant Strip as StripGlobals
participant Cmd as Command
participant FlagSet as Leaf FlagSet
rect rgba(100,200,150,0.5)
Note over Client,Cmd: Globals may be anywhere in argv
Client->>Dispatch: Dispatch(args)
Dispatch->>Strip: StripGlobals(args)
Strip->>Strip: Scan tokens, set known globals, preserve rest
Strip-->>Dispatch: (Global, restArgs, error)
Dispatch->>Cmd: Route using restArgs
Cmd->>FlagSet: Parse restArgs
FlagSet-->>Cmd: (flags, usage/error)
Cmd-->>Client: Execute or return error
end
rect rgba(200,150,100,0.5)
Note over Dispatch,Cmd: Help emission includes globals
Client->>Dispatch: Dispatch(args with -h/--help)
Dispatch->>Cmd: Route to command help
Cmd->>Cmd: cmd.Help() (+ inherited Flags block)
Cmd->>Dispatch: Request globalFlagsHelp()
Dispatch-->>Client: Combined help output + "Global Flags:"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
main() skipped stderr emission for any error wrapping ErrUsage on the theory the error had already been reported. Leaf flag errors bubble up through ParseFlags wrapped in ErrUsage, but every leaf's FlagSet uses io.Discard output — so nothing was ever printed. Result: misplaced or unknown leaf flags exited 1 with no output. Print every non-ErrHelp error to stderr. ErrHelp stays skipped since its text is already written to stdout by the help-rendering code path.
Stdlib flag.FlagSet.Parse stops at the first positional, so ParseGlobal only saw flags placed before the verb. `ana --json org show` worked; `ana org show --json` fell through to the leaf FlagSet, which rejected the unknown flag and (since 1a) now surfaces the error properly but is still the wrong outcome. Add StripGlobals: a single linear walk over argv that consumes every known global wherever it appears (before, after, or interleaved with the verb path and its positionals), with `--` as a hard terminator. Everything unknown passes through in original order so the leaf's FlagSet still reports precise unknown-flag errors. Dispatch uses StripGlobals instead of ParseGlobal; cmd/ana/main's config-resolution pre-pass uses it too so a trailing --profile/--endpoint /--token-file still influences config load. ParseGlobal stays because it's still the reference FlagSet shape for the registry-sync test and a few existing callers. TestGlobalFlagsRegistrySync prevents drift: any new global added to ParseGlobal must also land in globalFlagRegistry.
Globals (--json/--endpoint/--token-file/--profile) were invisible in help output — only discoverable by reading the source. Add a canonical Global Flags section rendered by globalFlagsHelp (one source of truth for naming/usage) and append it to: - RootHelp (ana --help / ana with no args) - Leaf --help (via dispatchChild's --help short-circuit) Group.Help is left alone: children are already listed, and adding a global block at every Group level would duplicate noise without discovery value. The Global Flags block renders once at the root and once on each leaf's help — the surfaces users actually land on.
gofmt complained about a stray blank line between globalFlagsHelp and the Global type doc. Same pass drops the two string-flag fs.Set error paths in StripGlobals — stdlib stringValue.Set cannot fail, so those branches were dead code that kept internal/cli at 97.9% and broke the 100% gate. Keep the bool-flag fs.Set error path (reachable via --json=notbool) and add tests for it plus the Dispatch StripGlobals-error surface path.
bradfair
force-pushed
the
fix/global-flags-help-visibility
branch
from
8a1c33f to
4ac73ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/ana/main.go (1)
57-105:⚠️ Potential issue | 🟠 MajorSkip config resolution for help paths.
run()now resolvesglobal.Profilebeforecli.Dispatchgets a chance to short-circuit help. That makesana help --profile ghostandana org show --help --profile ghostfail withunknown profileinstead of rendering help, which is the opposite of the current dispatch contract. Use the strippedrestto detect help requests and bypass the config-resolution pre-pass for them.Based on learnings: The
--help/-hscan and rendering must occur indispatchChildBEFORECommand.Runis called; Groups are skipped so help reaches the deepest leaf, and bare positionalhelpis left alone so leaves can receive it as an argument.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ana/main.go` around lines 57 - 105, The pre-dispatch config Resolve in run() is happening even for help requests, causing unknown-profile errors when users ask for help; update the logic that follows cli.StripGlobals(args) to inspect the returned rest slice for help indicators (e.g. "-h", "--help", or the literal "help" as the first positional) and, if a help request is detected, skip loading cfgPath/config.Load and calling config.Resolve(env, loaded, global.Profile) so cli.Dispatch/dispatchChild can short-circuit to render help; keep the existing behavior for non-help paths (still use global.TokenFile, config.DefaultPath, and Resolve) and reference the existing symbols global, rest, cfgPath, loaded, config.Load, and config.Resolve when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ana/main.go`:
- Around line 35-40: The main() post-dispatch error handler is double-reporting
usage/dispatch diagnostics; update the check around fmt.Fprintln(stdio.Stderr,
err) to also skip cli.ErrUsage in addition to cli.ErrHelp so errors already
written by cli.Dispatch or run() aren't printed again. Locate the error handling
in main() (the block that currently checks errors.Is(err, cli.ErrHelp) and calls
fmt.Fprintln) and add a guard that returns/does not print when errors.Is(err,
cli.ErrUsage) (and keep the existing cli.ErrHelp check) so only truly unreported
errors are emitted.
---
Outside diff comments:
In `@cmd/ana/main.go`:
- Around line 57-105: The pre-dispatch config Resolve in run() is happening even
for help requests, causing unknown-profile errors when users ask for help;
update the logic that follows cli.StripGlobals(args) to inspect the returned
rest slice for help indicators (e.g. "-h", "--help", or the literal "help" as
the first positional) and, if a help request is detected, skip loading
cfgPath/config.Load and calling config.Resolve(env, loaded, global.Profile) so
cli.Dispatch/dispatchChild can short-circuit to render help; keep the existing
behavior for non-help paths (still use global.TokenFile, config.DefaultPath, and
Resolve) and reference the existing symbols global, rest, cfgPath, loaded,
config.Load, and config.Resolve when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e312a924-43f7-4f2c-a2ac-8c3c5dab7faa
⛔ Files ignored due to path filters (1)
internal/cli/CLAUDE.mdis excluded by!**/*.md
📒 Files selected for processing (6)
cmd/ana/main.gocmd/ana/main_test.gointernal/cli/cli.gointernal/cli/cli_test.gointernal/cli/dispatch.gointernal/cli/root.go
…print CodeRabbit found two issues post-rebase: 1. Dispatch and run() both write diagnostics to stderr, and main now also prints on any non-ErrHelp error (introduced in 1a so leaf FlagSets whose output is io.Discard get their errors surfaced). Result: bad globals, unknown commands, and unknown profiles produce two stderr lines — the callee's clean message plus main's wrapped repeat. Fix: add cli.ErrReported sentinel. Dispatch + run wrap their errors with errors.Join(err, ErrReported) after emitting the diagnostic themselves. main's fallback stderr print skips any error carrying ErrReported. Leaf flag errors still flow through main's print path unchanged. 2. StripGlobals only recognized --name; stdlib flag.FlagSet.Parse treats -name and --name as equivalent (per its docs: "One or two dashes may be used; they are equivalent"). A user running `ana -json org show` would have -json silently passed through to the org-show FlagSet, which then fails with "flag provided but not defined". Fix: parseFlagToken now accepts both single- and double-dash forms. Bare -, --, ---, -=val noise tokens are left in rest untouched so the leaf receives them as-is.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ana/main.go`:
- Line 104: Wrap and join the existing errors so rerr's chain is preserved while
still wrapping cli.ErrUsage for exit-code mapping: replace the current return
with a join that includes a wrapped cli.ErrUsage plus rerr and cli.ErrReported
(e.g. use errors.Join(fmt.Errorf("%w", cli.ErrUsage), rerr, cli.ErrReported)),
referencing rerr, cli.ErrUsage, cli.ErrReported, errors.Join and fmt.Errorf to
locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c68d762d-566b-44e3-8c7b-c556869ba6f1
⛔ Files ignored due to path filters (1)
internal/cli/CLAUDE.mdis excluded by!**/*.md
📒 Files selected for processing (6)
cmd/ana/main.gocmd/ana/main_test.gointernal/cli/cli_test.gointernal/cli/dispatch.gointernal/cli/errors.gointernal/cli/root.go
CodeRabbit nit + golangci-lint errorlint: formatting the inner error with %s in fmt.Errorf loses its chain, so downstream errors.Is against the inner sentinel fails. ErrUnknownProfile is checked upstream so behavior was fine today, but the pattern is brittle. Go 1.20+ fmt.Errorf accepts multiple %w verbs; switching "%w: %s" → "%w: %w" preserves both sentinels (ErrUsage for exit-code mapping plus the inner error for future errors.Is callers) while keeping the rendered string identical.
This was referenced Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three related global-flag bugs:
ana org show --no-such-flagexited 1 silently —ErrUsageerrors were suppressed on stderr. Now printed.ana org show --jsonsilently succeeded with empty output becauseflag.FlagSet.Parsestopped at the first positional, so post-verb globals never reachedParseGlobal. NewStripGlobalswalks argv linearly and recognizes globals regardless of position, with--terminator support,=value/ space-form, and unknown-flag passthrough to the leaf.ana --help/ana <leaf> --helpdid not list--json/--endpoint/--token-file/--profile. A singleglobalFlagsHelp()block is now appended to both.Independent of the
feat/group-inheritable-flagsbranch; can merge in either order.Summary by CodeRabbit
New Features
Bug Fixes
Tests