Improve admin CLI per-method parameter help UX (MIR-746)#823
Conversation
Make per-method help discoverable and informative: - Treat --help/-h as method help (same as --func-help) instead of erroring with "unknown parameter(s): help". - Render method help when a method that declares params is invoked with none, instead of falling through to a raw -32602 JSON-RPC error. - Embed the full method definition (term, description, tree-formatted params with types and (required) markers) in validateAdminCall error output for both missing-required and unknown-param branches. - Distinguish "method takes no parameters" from "params not advertised by this method": fetchMethods now calls SetParams unconditionally when the wire payload includes a params key (even if empty), and renderMethodHelpString surfaces the distinction with a faint footer. validateAdminCall now rejects any supplied param on declared-empty- params methods (previously skipped because the guard required len(params) > 0). Refs MIR-746.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR enhances the admin command's help handling and parameter validation semantics. The CLI now detects help tokens ( Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/commands/admin.go`:
- Around line 374-388: The function hasHelpFlag currently treats a bare "help"
token as a help request which falsely intercepts flag values (e.g., --topic
help); update hasHelpFlag in cli/commands/admin.go to stop treating the bare
"help" string as a help flag by removing the "help" case from the switch and
only matching "--help", "-h", and the "--help=" / "-h=" prefixes on the arg
variable so that values passed to flags are not mistaken for a help request.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e636e65-a37b-4599-b20a-cc36c2a75298
📒 Files selected for processing (2)
cli/commands/admin.goservers/admin/admin.go
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
Yer boy and yer bot here. We like this overall — good improvements. The (no parameters) vs (parameters not advertised) distinction in particular is a small clarity that punches above its weight (which presumes we're gonna be boxing with this API I guess?).
CodeRabbit caught a real one on hasHelpFlag — agree with the diagnosis, but we'd lean toward just dropping case "help" entirely rather than the preceding-flag heuristic. --help/-h are the conventions people know; bare help was the form that broke things.
One thing inline.
Seeing all this code shift around without any tests is a little itchy for Paul, and for Claude it kinda tickles:
- hasHelpFlag no longer treats bare "help" as a help flag, so invocations
like `m admin <method> --topic help` (or `topic=help`) pass through to
parseUnknownFlags instead of being intercepted.
- The no-params-supplied branch now returns an error embedding the method
help, mirroring missing-required-params. This keeps the exit code
non-zero so shell chains like `m admin -a foo bar && echo ok` don't
treat a help-rendered call as success. Explicit --help/--func-help
still exits zero.
- Add tests for hasHelpFlag (including the --topic help edge case),
paramShapeNote ("no parameters" vs "parameters not advertised"),
methodDefinition, and renderMethodHelpString.
Refs MIR-746.
evanphx
deleted the
mir-746-improve-admin-cli-per-method-parameter-help-ux
branch
2 participants
Summary
Smooths out three rough edges in
miren admin <method>:--help/-hon a method now shows method help (same as--func-help) instead of erroring withunknown parameter(s): help.-32602 Invalid paramsJSON-RPC error.validateAdminCallmissing-required and unknown-param errors now embed the full method definition (description, tree-formatted params with types and(required)markers), matching what--list/--func-helpproduce.Also distinguishes two cases that previously looked identical on the wire:
(no parameters)— method explicitly declares an empty param list.(parameters not advertised by this method)— method's$methodsresponse omits theparamskey entirely.servers/admin/admin.gofetchMethodsnow callsSetParamsunconditionally when the source JSON includes aparamskey, even if empty. Combined with a tightened client guard (HasParams()instead ofHasParams() && len > 0), declared-zero-params methods now also reject supplied params.Test plan
go build ./...go test ./cli/commands/ ./servers/admin/— 208 passingmake lint— 0 issuesm admin -a <app> <method> --helpmatches--func-helpoutputm admin -a <app> <method>(no params) renders method help, not-32602m admin -a <app> <method> bogus=1shows rich definition list after "unknown parameter(s)"m admin -a <app> <method>(declared zero params) still calls server cleanlym admin -a <app> <method> --no-validatestill sends empty callm admin -a <app> --listandm admin -a <app> <method> --func-helpunchangedRefs MIR-746.