fix: allow custom commands to merge into built-in namespaces at any level

[Erik Osterman (Cloud Posse) (osterman)]

what

• Custom commands in atmos.yaml can now be merged into built-in command namespaces at any nesting depth
• Built-in commands are preserved when custom commands collide (no silent replacement)
• When a custom command with steps conflicts with an existing command, a warning is emitted explaining which steps are ignored
• Added comprehensive tests verifying namespace merging, collision handling, and deep nesting

why

Custom commands silently failed when sharing a namespace with built-in commands. For example, users couldn't define mcp aws install because the built-in mcp command existed. The root causes were:

1. Top-level collision detection only: The old code only checked for collisions at the top level (topLevel=true branch) using a separate getTopLevelCommands() map
2. No non-top-level detection: When recursing into nested subcommands, there was no collision check at all, allowing Cobra's AddCommand to silently replace existing subcommands

This 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

• Removed the topLevel bool parameter from processCustomCommands()
• Replaced top-level-only collision detection with findSubcommand() that checks the parent command's subcommands at any level
• Extracted command creation logic into createCustomCommand() helper to reduce nesting complexity (fixes golangci-lint nestif warning)
• When a collision is detected, the existing command is reused and nested custom subcommands are merged into it
• All call sites and tests updated to use the new signature

Summary by CodeRabbit

• Bug Fixes

  • Custom commands that collide with built-in commands are now merged into existing namespaces; when collisions occur, custom execution steps are skipped and a user-facing warning is shown.
  • Flag and shorthand conflicts are validated earlier to prevent registration issues and clearer UI feedback.

• Tests

  • Added tests covering collision handling, namespace merging, and deep-nesting preservation of built-in handlers.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues found.

Scanned Files

None

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors custom command processing: removes the topLevel parameter, centralizes command creation and flag validation into createCustomCommand and helpers, reuses existing subcommands via findSubcommand, switches some logging to ui.Writeln, and adds collision-handling tests. Call sites updated to the new processCustomCommands arity.

Changes

Cohort / File(s)	Summary
Custom Command Processing cmd/cmd_utils.go	Removed top-level handling in processCustomCommands; added findSubcommand, createCustomCommand, centralized flag validation (validateCustomCommandFlags, validateFlag*) and registration helpers (registerCustomCommandFlags, registerFlag*); validations run before registration; some logs replaced with ui.Writeln; added pflag and ui imports.
Collision Handling Tests cmd/custom_command_collision_test.go	Added three tests validating merge/collision behavior between custom and built-in commands (namespace merge, non-top-level collision preserved, deep nesting merge).
Test Callsite & Main Update cmd/custom_command_flag_conflict_test.go, cmd/custom_command_identity_test.go, cmd/custom_command_integration_test.go, cmd/root.go	Removed final boolean argument from all processCustomCommands calls (signature arity changed from 4 to 3); updated test helpers and call sites accordingly.
Behavioral Note cmd/...	When a custom command collides with an existing built-in subcommand, custom steps are warned via UI and ignored; command creation now applies identity, descriptions, and pre-run/run wiring consistently across new commands.

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

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• feat: Add command registry pattern foundation with 100% test coverage #1643: Changes to built-in command registration and merging semantics that interact with the new collision/merge behavior.
• Fix custom command flag conflicts with global flags #1947: Centralizes custom-command flag validation and command construction; closely related code-level changes.
• add bool flag in custom command which was ignored before #807: Prior work on flag handling for custom commands; related to flag-type and shorthand handling.

Suggested reviewers

• aknysh
• gberenice

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: enabling custom commands to merge into built-in namespaces at any nesting level, which is the core refactoring removing the topLevel parameter.
Docstring Coverage	✅ Passed	Docstring coverage is 95.83% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch osterman/aws-cmd-registry

📝 Coding Plan

• Generate coding plan for human review comments

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 91.44737% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.40%. Comparing base (d68c499) to head (53026ef).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/cmd_utils.go	91.39%	10 Missing and 3 partials ⚠️

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     

Flag	Coverage Δ
unittests	77.40% <91.44%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cmd/root.go	67.58% <100.00%> (ø)
cmd/cmd_utils.go	56.98% <91.39%> (+1.22%)	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

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 err loses structured context and diverges from project error-handling policy.

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()
 			}
 		}

As per coding guidelines: "Error Handling (MANDATORY): All errors MUST be wrapped using static errors defined in `errors/errors.go`."

🤖 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

📥 Commits

Reviewing files that changed from the base of the PR and between 76a269b and c8cf4bb.

📒 Files selected for processing (6)

• cmd/cmd_utils.go
• cmd/custom_command_collision_test.go
• cmd/custom_command_flag_conflict_test.go
• cmd/custom_command_identity_test.go
• cmd/custom_command_integration_test.go
• cmd/root.go

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between c8cf4bb and e0e619e.

📒 Files selected for processing (1)

• cmd/cmd_utils.go

[coderabbitai]

🧹 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

📥 Commits

Reviewing files that changed from the base of the PR and between e0e619e and bb5c27b.

📒 Files selected for processing (1)

• cmd/cmd_utils.go

[github-actions]

These changes were released in v1.210.0-rc.0.

[github-actions]

These changes were released in v1.210.0-test.25.