fix(client/v2/autocli): prevent duplicate addition of customCommands #22576
Conversation
fastfadingviolets
requested review from
julienrbrt,
JulianToledano and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
client/v2/CHANGELOG.md (1)
61-61: Consider adding more context to the changelog entry.While the entry correctly references the issue, it would be more helpful for users to understand the specific problem that was fixed. Consider expanding it to:
-* [cosmos/gaia#3412](https://github.com/cosmos/gaia/issues/3412) Fix duplicate command addition in `autocli` +* [cosmos/gaia#3412](https://github.com/cosmos/gaia/issues/3412) Fix duplicate command addition in `autocli` where certain module commands appeared multiple times in the CLI help menuclient/v2/autocli/common.go (1)
143-151: LGTM! Consider enhancing error handling consistency.The additional check for custom commands with names different from their modules effectively prevents duplicate command creation, addressing the core issue. This is a solid fix for the duplicate entries problem reported in issue #3412.
Consider wrapping the error returns in a more descriptive error using
fmt.Errorffor consistency with other error handling in the codebase:if hasModuleOptions { // check if we need to enhance the existing command - if err := enhanceCustomCmd(b, subCmd, cmdType, modOpts); err != nil { - return err - } + if err := enhanceCustomCmd(b, subCmd, cmdType, modOpts); err != nil { + return fmt.Errorf("failed to enhance existing custom command %s: %w", custom.Name(), err) + } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
client/v2/CHANGELOG.md(1 hunks)client/v2/autocli/common.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
client/v2/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
client/v2/autocli/common.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
client/v2/autocli/common.go (1)
143-151: Verify impact on existing custom commands.
Let's ensure this change doesn't affect existing command structures.
✅ Verification successful
Let me gather more specific information about the command registration and custom command handling.
Changes to custom command handling are safe and well-implemented
The code changes implement a safe enhancement to custom command handling in the CLI. The implementation:
- Properly checks for existing commands using
findSubCommandbefore adding or enhancing them - Maintains backward compatibility by preserving existing command structures
- Has proper fallback mechanisms when commands don't exist
- Correctly handles both module-named and custom-named commands through separate checks
The changes ensure that:
- Custom commands are not duplicated
- Existing command hierarchies are preserved
- Command enhancement is optional and controlled via module options
- Command name conflicts are properly handled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing custom commands and their structure
# Look for custom command registrations
echo "Checking for custom command registrations..."
rg -A 5 "AddCommand\(.*custom.*\)" --type go
# Look for module command definitions
echo "Checking for module command definitions..."
rg "type.*Command.*struct" --type go
# Look for potential command name conflicts
echo "Checking for potential command name conflicts..."
rg "Use:.*\".*\"" --type go
Length of output: 25112
Script:
#!/bin/bash
# Check for custom command handling and registration patterns
echo "Checking custom command handling..."
rg -B 5 -A 5 "findSubCommand" --type go
echo "Checking command registration patterns..."
rg -B 5 -A 5 "Name\(\)" --type go | grep -A 5 -B 5 "custom"
echo "Checking for potential command conflicts..."
rg -B 3 -A 3 "enhanceCustomCmd" --type go
Length of output: 15246
fastfadingviolets
force-pushed
the
fix/duplicate-autocli-commands
branch
from
f2b0f2e to
f17e361
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
client/v2/CHANGELOG.md (1)
61-61: Enhance the changelog entry descriptionWhile the entry correctly references the issue, consider making it more descriptive to better explain the fix:
-* [cosmos/gaia#3412](https://github.com/cosmos/gaia/issues/3412) Fix duplicate command addition in `autocli` +* [cosmos/gaia#3412](https://github.com/cosmos/gaia/issues/3412) Fix duplicate command addition in `autocli` by improving command existence checks for non-prefixed module commandsThis provides more context about the nature of the fix and aligns with the level of detail in other changelog entries.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
client/v2/CHANGELOG.md(1 hunks)client/v2/autocli/common.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/v2/autocli/common.go
🧰 Additional context used
📓 Path-based instructions (1)
client/v2/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (1)
client/v2/CHANGELOG.md (1)
Line range hint 1-24: LGTM! Changelog structure follows best practices
The changelog follows the Keep a Changelog format and properly organizes entries into appropriate sections with clear version headers and dates.
julienrbrt
changed the title
fastfadingviolets
force-pushed
the
fix/duplicate-autocli-commands
branch
from
f17e361 to
78db252
Compare
julienrbrt
added
backport/v0.50.x
Co-authored-by: Julien Robert <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
client/v2/CHANGELOG.md (1)
61-61: Enhance the changelog entry with more details.The current entry could be more descriptive about the root cause, impact, and solution.
Consider this more detailed entry:
-* [#22576](https://github.com/cosmos/cosmos-sdk/pull/22576) Fix duplicate command addition in `autocli` when custom enhanced command has a different name than module name +* [#22576](https://github.com/cosmos/cosmos-sdk/pull/22576) Fix duplicate command entries in `autocli` help menu caused by custom enhanced commands having different names than their module names. The fix improves command name checking in `enhanceCommandCommon` to prevent unintended command duplication.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
client/v2/CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
client/v2/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
…22576) Co-authored-by: Julien Robert <[email protected]> (cherry picked from commit 685218e)
…22576) Co-authored-by: Julien Robert <[email protected]> (cherry picked from commit 685218e) # Conflicts: # client/v2/CHANGELOG.md
…(backport #22576) (#22614) Co-authored-by: violet <[email protected]>
…(backport #22576) (#22615) Co-authored-by: violet <[email protected]> Co-authored-by: Julien Robert <[email protected]>
* main: feat(serverv2): add benchmarks of (old) cacheKV vs branch (#22497) fix: match comets bls implmentation (#22613) feat(math): Upstream GDA based decimal type (#21982) fix(client/v2/autocli): prevent duplicate addition of customCommands (#22576) build(deps): Bump cosmossdk.io/math from 1.3.0 to 1.4.0 (#22580)
Description
Closes cosmos/gaia#3412
Not every module's custom commands are prefixed by the name of the module. Because of how Gaia bootstraps its tx command, autocli can end up adding duplicate commands if it's only checking whether there's already a command with the module's name added to the CLI. This PR fixes this by adding a second check to autocli's enhanceCommandCommon.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Breaking Changes