fix: allow custom commands to merge into built-in namespaces at any level#2191
Merged
fix: allow custom commands to merge into built-in namespaces at any level#2191
Conversation
…at any level What changed: - Generalized collision detection in processCustomCommands to work at any nesting depth - Custom commands can now extend built-in command trees (e.g., mcp aws install alongside mcp start) - Built-in commands are preserved when custom commands collide; warning emitted if custom has conflicting steps - Removed topLevel bool parameter in favor of universal findSubcommand() check on parent - Extracted command creation logic into createCustomCommand() helper to reduce complexity Why: Custom commands in atmos.yaml silently failed when sharing a namespace with built-in commands. - Top-level collisions were detected but only at the top level (topLevel=true branch) - Non-top-level collisions had no detection; Cobra's AddCommand silently replaced built-in subcommands This prevented users from extending built-in namespaces with custom subcommands (e.g., mcp aws install). Tests added: - TestCustomCommand_NamespaceMerge_SubcommandsAdded: custom subcommands merge into built-in namespace - TestCustomCommand_NonTopLevelCollision_BuiltinPreserved: built-in subcommands preserved, not replaced - TestCustomCommand_DeepNesting_MergeWorks: merging works at arbitrary nesting depth All existing custom command tests pass. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
📝 WalkthroughWalkthroughRefactors custom command processing: removes the topLevel parameter, centralizes command creation and flag validation into Changes
Sequence Diagram(s)sequenceDiagram
participant Config as AtmosConfig
participant Processor as processCustomCommands
participant Cobra as Cobra RootCmd
participant UI as ui.Writeln
participant Flags as pflag Registry
Config->>Processor: provide custom command definitions
Processor->>Cobra: findSubcommand(name) ?
alt subcommand exists
Processor->>UI: warn about step conflict (if any)
Processor->>Cobra: reuse existing subcommand (merge children)
else no existing
Processor->>Processor: createCustomCommand(config)
Processor->>Flags: validateCustomCommandFlags(config.flags)
Flags-->>Processor: validation result
Processor->>Cobra: registerCustomCommandFlags + add command
end
Processor->>Processor: recurse into nested command configs with parent cmd
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2191 +/- ##
==========================================
+ Coverage 77.38% 77.40% +0.02%
==========================================
Files 962 962
Lines 91239 91284 +45
==========================================
+ Hits 70601 70655 +54
+ Misses 16558 16548 -10
- Partials 4080 4081 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/custom_command_collision_test.go (1)
130-132: Consider asserting the conflict warning path too.This test triggers the “custom steps ignored on collision” branch, but it currently validates only command behavior. Adding a stderr/UI warning assertion would lock in the full contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/custom_command_collision_test.go` around lines 130 - 132, The test currently calls processCustomCommands(atmosConfig, atmosConfig.Commands, RootCmd) and only asserts no error; add an assertion that the collision warning UI/stderr was emitted by capturing the command output (or the test UI/stderr buffer used by RootCmd) and checking it contains the expected "custom steps ignored on collision" (or similar) warning text so the collision branch is verified; place this capture/assert immediately after the processCustomCommands call and keep the existing require.NoError assertion.cmd/cmd_utils.go (1)
519-522: Wrap required-flag registration failures with sentinel/context.At Line 521, returning raw
errloses structured context and diverges from project error-handling policy.As per coding guidelines: "Error Handling (MANDATORY): All errors MUST be wrapped using static errors defined in `errors/errors.go`."Proposed patch
if flag.Required { err := customCommand.MarkPersistentFlagRequired(flag.Name) if err != nil { - return nil, err + return nil, errUtils.Build(errUtils.ErrInvalidFlag). + WithCause(err). + WithExplanationf("Failed to mark flag '--%s' as required for custom command '%s'", flag.Name, commandConfig.Name). + WithContext("command", commandConfig.Name). + WithContext("flag", flag.Name). + Err() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cmd_utils.go` around lines 519 - 522, Replace the raw return of err from customCommand.MarkPersistentFlagRequired(flag.Name) with a wrapped error using the static sentinel from errors/errors.go (e.g. errors.ErrRegisterFlag or the appropriate sentinel defined there), include the flag.Name and the original err as context, and use Go error wrapping (fmt.Errorf("%w: ...", sentinel, err) or equivalent) so callers receive the sentinel plus original detail; also add any required imports (fmt and the errors package) and update the return to return nil, wrappedErr instead of nil, err.
🤖 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/cmd_utils.go`:
- Around line 95-99: The format string passed to ui.Warningf (in the block
referencing commandConfig.Name and existing.CommandPath()) includes a trailing
"\n" which produces an extra blank line; remove the explicit newline from the
format string so ui.Warningf's built-in newline is used instead and the warning
prints with a single line.
---
Nitpick comments:
In `@cmd/cmd_utils.go`:
- Around line 519-522: Replace the raw return of err from
customCommand.MarkPersistentFlagRequired(flag.Name) with a wrapped error using
the static sentinel from errors/errors.go (e.g. errors.ErrRegisterFlag or the
appropriate sentinel defined there), include the flag.Name and the original err
as context, and use Go error wrapping (fmt.Errorf("%w: ...", sentinel, err) or
equivalent) so callers receive the sentinel plus original detail; also add any
required imports (fmt and the errors package) and update the return to return
nil, wrappedErr instead of nil, err.
In `@cmd/custom_command_collision_test.go`:
- Around line 130-132: The test currently calls
processCustomCommands(atmosConfig, atmosConfig.Commands, RootCmd) and only
asserts no error; add an assertion that the collision warning UI/stderr was
emitted by capturing the command output (or the test UI/stderr buffer used by
RootCmd) and checking it contains the expected "custom steps ignored on
collision" (or similar) warning text so the collision branch is verified; place
this capture/assert immediately after the processCustomCommands call and keep
the existing require.NoError assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a91a132-1168-4ec3-9091-a486f37c22a2
📒 Files selected for processing (6)
cmd/cmd_utils.gocmd/custom_command_collision_test.gocmd/custom_command_flag_conflict_test.gocmd/custom_command_identity_test.gocmd/custom_command_integration_test.gocmd/root.go
…and code - Remove trailing \n from Warningf format string (ui.Warningf already appends newline) - Pass atmosConfig by pointer to avoid copying 7KB struct (gocritic hugeParam) - Pass schema.CommandFlag by pointer in all helpers (gocritic hugeParam) - Extract flag validation into validateCustomCommandFlags/validateFlag helpers (gocognit) - Extract flag registration into registerCustomCommandFlags/registerFlag helpers (nestif) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Contributor
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/cmd_utils.go`:
- Around line 462-566: The shorthand validation must enforce that
flag.Shorthand, when non-empty, is exactly one rune to avoid pflag panics;
update validateFlagShorthand to check len([]rune(flag.Shorthand)) == 1 and if
not return a structured error (similar style to existing errors) indicating
invalid shorthand for the command (include cmdName, shorthand, and config path
"commands.<cmdName>.flags"), so invalid shorthands are caught at parse time
before registerBoolFlag/registerStringFlag call sites that call BoolP/StringP.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c54d35d5-ed78-492f-bebb-119435dce5e7
📒 Files selected for processing (1)
cmd/cmd_utils.go
pflag panics when BoolP/StringP receives a shorthand longer than one character. Add validation in validateFlagShorthand to catch this at config parsing time and return a structured error instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/cmd_utils.go (1)
91-116: Clean refactor for namespace merging.The collision detection now works at any nesting depth. The warning for conflicting steps is helpful.
One consideration: when a custom command collides with an existing command, flags from the custom config are silently ignored while only steps trigger a warning. Consider warning about ignored flags too if any are defined.
Optional: Warn about ignored flags on collision
existing := findSubcommand(parentCommand, commandConfig.Name) if existing != nil { if len(commandConfig.Steps) > 0 { ui.Warningf( "Custom command %q defines steps that conflict with built-in command %q; "+ "built-in behavior preserved, custom steps ignored", commandConfig.Name, existing.CommandPath(), ) } + if len(commandConfig.Flags) > 0 { + ui.Warningf( + "Custom command %q defines flags that conflict with built-in command %q; "+ + "built-in behavior preserved, custom flags ignored", + commandConfig.Name, existing.CommandPath(), + ) + } command = existing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cmd_utils.go` around lines 91 - 116, When a custom command collides with an existing command (detected by findSubcommand returning non-nil), we currently warn only if commandConfig.Steps is non-empty; extend this to also warn when commandConfig.Flags (or any flag-like field) is non-empty so custom flags aren’t silently ignored. Update the collision branch that uses existing := findSubcommand(parentCommand, commandConfig.Name) to inspect commandConfig.Flags (and commandConfig.Args/options if applicable) and call ui.Warningf with a clear message referencing commandConfig.Name and existing.CommandPath() (similar style to the steps warning) whenever flags are present; leave reuse of existing and subsequent calls to processCustomCommands/createCustomCommand unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/cmd_utils.go`:
- Around line 91-116: When a custom command collides with an existing command
(detected by findSubcommand returning non-nil), we currently warn only if
commandConfig.Steps is non-empty; extend this to also warn when
commandConfig.Flags (or any flag-like field) is non-empty so custom flags aren’t
silently ignored. Update the collision branch that uses existing :=
findSubcommand(parentCommand, commandConfig.Name) to inspect commandConfig.Flags
(and commandConfig.Args/options if applicable) and call ui.Warningf with a clear
message referencing commandConfig.Name and existing.CommandPath() (similar style
to the steps warning) whenever flags are present; leave reuse of existing and
subsequent calls to processCustomCommands/createCustomCommand unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3c077ba-9b8d-4378-9f96-1ed70d507dfd
📒 Files selected for processing (1)
cmd/cmd_utils.go
aknysh
approved these changes
Mar 15, 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
2 participants
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.
what
atmos.yamlcan now be merged into built-in command namespaces at any nesting depthwhy
Custom commands silently failed when sharing a namespace with built-in commands. For example, users couldn't define
mcp aws installbecause the built-inmcpcommand existed. The root causes were:topLevel=truebranch) using a separategetTopLevelCommands()mapAddCommandto silently replace existing subcommandsThis fix generalizes collision detection to all nesting depths by using a universal
findSubcommand(parent, name)check that works on any cobra command's children.how
topLevelbool parameter fromprocessCustomCommands()findSubcommand()that checks the parent command's subcommands at any levelcreateCustomCommand()helper to reduce nesting complexity (fixes golangci-lint nestif warning)Summary by CodeRabbit
Bug Fixes
Tests