Skip to content

Fixes cg connecting before parsing the --context flag - #86

Merged
piotrpio merged 1 commit into
synadia-io:mainfrom
jnmoyne:jnm/fix-cg-args-parsing
Oct 2, 2026
Merged

piotrpio merged 1 commit into
synadia-io:mainfrom
jnmoyne:jnm/fix-cg-args-parsing

Conversation

@jnmoyne

@jnmoyne jnmoyne commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Fixes the cg tool ignoring it's --context flag

Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Claude finished @jnmoyne's task in 1m 6s —— View job


Review: cg --context flag fix

  • Fetch PR diff and read pcgroups/cg/main.go
  • Verify fisk/kingpin action-ordering semantics (parent command actions run before child/leaf actions)
  • Check error handling and variable scoping changes
  • Post review

Correctness: The root cause was real — natscontext.Connect(cg.natsContext) was called in main() before app.MustParseWithUsage(os.Args[1:]), so cg.natsContext was always still the zero value, silently ignoring --context. Moving the connect logic into connectAction and attaching it via .Action(cg.connectAction) on the static and elastic parent commands is the right fix: in fisk (kingpin-derived), action callbacks for a matched command path run in order from the outermost matched command to the leaf, so connectAction executes after all flags (including --context) are parsed but before the leaf command's own action (e.g. consumeStaticAction, promptStaticAction) runs. This also incidentally avoids attempting a NATS connection for --help/--version/usage-error invocations, which is a nice side benefit.

Error handling: Converting from log.Fatalf to returning wrapped errors (fmt.Errorf("...: %w", err)) from the action is an improvement — it lets fisk's usual error/usage reporting path handle failures instead of an abrupt os.Exit.

Minor scoping note (not a bug): err := <-cg.cgContext.Done() correctly switched from = to := since err is no longer declared earlier in main() — verified this compiles cleanly with the removed block.

No correctness, security, or performance issues found. The change is a straightforward, well-scoped fix.

@jnmoyne
jnmoyne requested review from Jarema and scottf September 28, 2026 14:37

@scottf scottf 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.

LGTM

@piotrpio piotrpio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@piotrpio
piotrpio merged commit 8898afe into synadia-io:main Oct 2, 2026
5 checks passed
@jnmoyne
jnmoyne deleted the jnm/fix-cg-args-parsing branch October 2, 2026 22:35
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.

3 participants