Fix group help display and refactor command resolution#33
Merged
Conversation
Two bugs: (1) bare group invocation (`myapp config`) showed "Unknown command" instead of group help, (2) help output showed raw `_command` positional arg instead of listing subcommands by name. Introduces ResolvedCommand/ResolvedGroup/ResolvedNothing union type so `_resolve_command_from_args` distinguishes commands, groups, and misses. Group and root help now render from CLI/Group registries instead of extracting from argparse internals. Suppresses noisy defaults (False, 0, empty string) in help template and tightens whitespace. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Coverage Report✅ 87.5% overall coverage
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes CLI help rendering for command groups and refactors command resolution to distinguish between resolved commands, resolved groups (bare group invocation), and no match.
Changes:
- Introduces
ResolvedCommand/ResolvedGroup/ResolvedNothingand updates dispatch logic to use these outcomes. - Refactors root and group help output to render from CLI/Group registries instead of argparse positional internals.
- Updates help template formatting and adds targeted tests for resolution and help output.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_groups.py | Adds tests covering resolve-result types, bare group invocation behavior, and help subcommand listing. |
| src/milo/templates/help.kida | Extends template to render registry-driven commands and options sections and suppress noisy defaults. |
| src/milo/help.py | Extends HelpState with commands and options to support registry-driven help rendering. |
| src/milo/groups.py | Adds Group.format_help() to render group help from registries. |
| src/milo/commands.py | Adds resolve-result dataclasses, refactors command/group resolution + dispatch, and adds _format_root_help(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+785
to
+787
| if isinstance(result, ResolvedGroup): | ||
| result.group.format_help(self.name) | ||
| return None |
There was a problem hiding this comment.
For nested groups, group.format_help(self.name) only prefixes the CLI name, so bengal site config will render as bengal config in the help header. To show the correct invocation path, carry the resolved group path/prefix through _resolve_group_command (e.g., accumulate prog_prefix during recursion or store a full prog string on ResolvedGroup) and pass that into format_help.
src/milo/commands.py
Outdated
Comment on lines
+974
to
+1005
| # Built-in options | ||
| builtin_opts = [ | ||
| {"flags": "-h, --help", "help": "Show this help message and exit"}, | ||
| {"flags": "-v, --verbose", "help": "Increase verbosity (-v verbose, -vv debug)"}, | ||
| {"flags": "-q, --quiet", "help": "Suppress non-error output"}, | ||
| {"flags": "--no-color", "help": "Disable color output"}, | ||
| {"flags": "-n, --dry-run", "help": "Show what would happen without making changes"}, | ||
| { | ||
| "flags": "-o, --output-file", | ||
| "metavar": "FILE", | ||
| "help": "Write output to FILE instead of stdout", | ||
| }, | ||
| ] | ||
|
|
||
| if self.version: | ||
| builtin_opts.insert(0, {"flags": "--version", "help": f"Show version ({self.version})"}) | ||
|
|
||
| # User-defined global options | ||
| for opt in self._global_options: | ||
| flags = f"--{opt.name.replace('_', '-')}" | ||
| if opt.short: | ||
| flags = f"{opt.short}, {flags}" | ||
| entry: dict[str, Any] = {"flags": flags, "help": opt.description} | ||
| if opt.default and not opt.is_flag: | ||
| entry["default"] = str(opt.default) | ||
| builtin_opts.append(entry) | ||
|
|
||
| state = HelpState( | ||
| prog=self.name, | ||
| description=self.description, | ||
| commands=commands, | ||
| options=tuple(builtin_opts), |
There was a problem hiding this comment.
_format_root_help() hardcodes a small set of built-in options, but build_parser() also registers --llms-txt, --mcp, --mcp-install, --mcp-uninstall, and --completions (and potentially others). With the new registry-based root help, those flags will no longer appear in myapp --help. Consider generating this list from the actual parser (or maintaining a single shared built-in option registry) so root help stays in sync with the real CLI flags.
Suggested change
| # Built-in options | |
| builtin_opts = [ | |
| {"flags": "-h, --help", "help": "Show this help message and exit"}, | |
| {"flags": "-v, --verbose", "help": "Increase verbosity (-v verbose, -vv debug)"}, | |
| {"flags": "-q, --quiet", "help": "Suppress non-error output"}, | |
| {"flags": "--no-color", "help": "Disable color output"}, | |
| {"flags": "-n, --dry-run", "help": "Show what would happen without making changes"}, | |
| { | |
| "flags": "-o, --output-file", | |
| "metavar": "FILE", | |
| "help": "Write output to FILE instead of stdout", | |
| }, | |
| ] | |
| if self.version: | |
| builtin_opts.insert(0, {"flags": "--version", "help": f"Show version ({self.version})"}) | |
| # User-defined global options | |
| for opt in self._global_options: | |
| flags = f"--{opt.name.replace('_', '-')}" | |
| if opt.short: | |
| flags = f"{opt.short}, {flags}" | |
| entry: dict[str, Any] = {"flags": flags, "help": opt.description} | |
| if opt.default and not opt.is_flag: | |
| entry["default"] = str(opt.default) | |
| builtin_opts.append(entry) | |
| state = HelpState( | |
| prog=self.name, | |
| description=self.description, | |
| commands=commands, | |
| options=tuple(builtin_opts), | |
| def _root_options_from_parser() -> tuple[dict[str, Any], ...]: | |
| parser = self.build_parser() | |
| options: list[dict[str, Any]] = [] | |
| for action in parser._actions: | |
| if not getattr(action, "option_strings", None): | |
| continue | |
| if action.help == argparse.SUPPRESS: | |
| continue | |
| entry: dict[str, Any] = { | |
| "flags": ", ".join(action.option_strings), | |
| "help": action.help or "", | |
| } | |
| metavar = getattr(action, "metavar", None) | |
| if metavar not in (None, argparse.SUPPRESS) and action.nargs != 0: | |
| if isinstance(metavar, tuple): | |
| entry["metavar"] = " ".join(str(part) for part in metavar) | |
| else: | |
| entry["metavar"] = str(metavar) | |
| default = getattr(action, "default", argparse.SUPPRESS) | |
| if ( | |
| default not in (None, argparse.SUPPRESS) | |
| and default is not False | |
| and action.nargs != 0 | |
| ): | |
| entry["default"] = str(default) | |
| options.append(entry) | |
| return tuple(options) | |
| state = HelpState( | |
| prog=self.name, | |
| description=self.description, | |
| commands=commands, | |
| options=_root_options_from_parser(), |
Comment on lines
+184
to
+214
| def format_help(self, prog_prefix: str = "") -> str: | ||
| """Render help from this group's command/group registries. | ||
|
|
||
| Returns the rendered help string. Does not print. | ||
| """ | ||
| import sys | ||
|
|
||
| from milo.help import HelpState | ||
| from milo.templates import get_env | ||
|
|
||
| prog = f"{prog_prefix} {self.name}".strip() if prog_prefix else self.name | ||
| commands = tuple( | ||
| [ | ||
| {"name": cmd.name, "help": getattr(cmd, "description", "")} | ||
| for cmd in self._commands.values() | ||
| if not getattr(cmd, "hidden", False) | ||
| ] | ||
| + [ | ||
| {"name": g.name, "help": g.description} | ||
| for g in self._groups.values() | ||
| if not g.hidden | ||
| ] | ||
| ) | ||
|
|
||
| state = HelpState(prog=prog, description=self.description, commands=commands) | ||
| env = get_env() | ||
| template = env.get_template("help.kida") | ||
| output = template.render(state=state) | ||
| sys.stdout.write(output + "\n") | ||
| sys.stdout.flush() | ||
| return output |
There was a problem hiding this comment.
The docstring says format_help() "Does not print", but the implementation writes to sys.stdout and flushes. Either update the docstring to reflect the side effect, or change the method to only return the rendered string and let the caller decide where/how to output it (which also improves testability).
…ing fix - Add prog field to ResolvedGroup and accumulate full path through group resolution so nested groups render correct prog prefix (e.g. "bengal site config") - Replace hardcoded built-in options in _format_root_help() with parser-derived options so new flags like --llms-txt, --mcp are never missed - Fix Group.format_help() docstring to accurately reflect stdout writing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
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
myapp config) showed "Unknown command: 'config'. Did you mean 'config'?" instead of group help — now displays the group's subcommands and description_commandpositional arg instead of listing subcommands by name — root and group help now render from CLI/Group registries instead of extracting from argparse internalsResolvedCommand/ResolvedGroup/ResolvedNothingdiscriminated union so command resolution distinguishes all three outcomes cleanlyGroup.format_help()andCLI._format_root_help()that build help state directly from command/group registries, eliminating dependence on argparse private APIs(default: False),(default: ),(default: 0)) in help template and tightens section whitespaceTest plan
TestResolveResult,TestGroupBareInvocation,TestHelpSubcommandListingmyapp,myapp config,myapp site configall show correct helpmyapp cofnigstill shows did-you-mean suggestion🤖 Generated with Claude Code