fix(shortcuts): reject unsupported shortcut commands#896
Open
liangshuo-1 wants to merge 3 commits into
Open
Conversation
github-actions
Bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds preflight validation to detect unsupported ChangesCommand Invocation Validation
Sequence Diagram(s)sequenceDiagram
participant CLI
participant validateCommandInvocation
participant handleRootError
participant setupNotices
participant CobraExecute
CLI->>validateCommandInvocation: validateCommandInvocation(rootCmd, args)
validateCommandInvocation-->>handleRootError: return unknown_shortcut / unknown_command (ExitError)
validateCommandInvocation->>setupNotices: nil (validation OK)
setupNotices->>CobraExecute: rootCmd.Execute()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
liangshuo-1
force-pushed
the
fix/unknown-shortcut-error
branch
from
d301499 to
5c0e2b8
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@df71c4d8d91f7374459e633bf4a43fee384d875c🧩 Skill updatenpx skills add larksuite/cli#fix/unknown-shortcut-error -y -g |
![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
🧹 Nitpick comments (1)
cmd/root_integration_test.go (1)
524-535: ⚡ Quick winAssert the machine-readable
detailpayload too.These tests only lock down
type,message, and hint text. The new contract also includesshortcut,service, andavailable_shortcuts; if those fields disappear or change shape, this suite still passes even though agent-facing behavior regressed.Suggested assertion additions
env := parseEnvelope(t, stderr) if env.Error == nil { t.Fatal("expected error envelope detail") } if env.Error.Type != "unknown_shortcut" { t.Fatalf("error type = %q, want unknown_shortcut", env.Error.Type) } + if env.Error.Detail["shortcut"] != "+nonexistent-thing" { + t.Fatalf("detail.shortcut = %#v, want +nonexistent-thing", env.Error.Detail["shortcut"]) + } + if env.Error.Detail["service"] != "drive" { + t.Fatalf("detail.service = %#v, want drive", env.Error.Detail["service"]) + } + if got, ok := env.Error.Detail["available_shortcuts"].([]interface{}); !ok || len(got) == 0 { + t.Fatalf("detail.available_shortcuts = %#v, want non-empty list", env.Error.Detail["available_shortcuts"]) + } if !strings.Contains(env.Error.Message, `shortcut "+nonexistent-thing" is not supported for "lark-cli drive"`) { t.Fatalf("message missing unsupported shortcut context: %q", env.Error.Message) }🤖 Prompt for 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. In `@cmd/root_integration_test.go` around lines 524 - 535, The test currently verifies env.Error.Type, Message, and Hint but not the machine-readable detail fields; update the integration test that calls parseEnvelope/t and checks env.Error to also assert the presence and expected values/shapes of env.Error.Shortcut (e.g. equals "+nonexistent-thing" or non-empty), env.Error.Service (e.g. "drive"), and env.Error.AvailableShortcuts (contains "+upload" and is a non-empty list), and ensure available_shortcuts is the expected slice/map shape rather than nil so regressions in those fields fail the test; reference the parseEnvelope result via env.Error.Shortcut, env.Error.Service, and env.Error.AvailableShortcuts when adding these assertions.
🤖 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 `@cmd/shortcut_preflight.go`:
- Around line 26-27: availableShortcutCommands currently reads from the global
shortcut registry and so can return commands that have been pruned from the
mounted Cobra tree; change it to traverse the mounted Cobra tree starting from
the current command/root using cmd.Commands() (or cmd.Root().Commands()) and
collect shortcuts only from commands that are present and available (skip
commands where cmd.Hidden is true or !cmd.IsAvailableCommand() or
cmd.IsHelpCommand()), then return that set for available_shortcuts and hints;
update all call sites (the uses around available :=
availableShortcutCommands(cmd.Name()) and the block at lines 47-60) to pass the
active cmd/root or to call the new tree-based helper so suggestions respect
runtime pruning.
---
Nitpick comments:
In `@cmd/root_integration_test.go`:
- Around line 524-535: The test currently verifies env.Error.Type, Message, and
Hint but not the machine-readable detail fields; update the integration test
that calls parseEnvelope/t and checks env.Error to also assert the presence and
expected values/shapes of env.Error.Shortcut (e.g. equals "+nonexistent-thing"
or non-empty), env.Error.Service (e.g. "drive"), and
env.Error.AvailableShortcuts (contains "+upload" and is a non-empty list), and
ensure available_shortcuts is the expected slice/map shape rather than nil so
regressions in those fields fail the test; reference the parseEnvelope result
via env.Error.Shortcut, env.Error.Service, and env.Error.AvailableShortcuts when
adding these assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 428355ff-fbec-43d7-a3d0-af2618bb0733
📒 Files selected for processing (4)
CHANGELOG.mdcmd/root.gocmd/root_integration_test.gocmd/shortcut_preflight.go
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/root.go
Comment on lines
+26
to
+27
| available := availableShortcutCommands(cmd.Name()) | ||
| if len(available) == 0 { |
There was a problem hiding this comment.
Build available_shortcuts from the mounted Cobra tree, not the global shortcut registry.
availableShortcutCommands(cmd.Name()) ignores runtime pruning, so unknown_shortcut can suggest commands that the current tree has already hidden. This file’s strict-mode coverage already proves im --help can hide +messages-search; with the current helper, im +bogus would still advertise it in both the hint and available_shortcuts detail.
Suggested fix
- available := availableShortcutCommands(cmd.Name())
+ available := availableShortcutCommands(cmd)
if len(available) == 0 {
return nil
}
@@
-func availableShortcutCommands(service string) []string {
+func availableShortcutCommands(cmd *cobra.Command) []string {
seen := map[string]struct{}{}
out := make([]string, 0)
- for _, shortcut := range shortcuts.AllShortcuts() {
- if shortcut.Service != service || shortcut.Hidden {
+ for _, child := range cmd.Commands() {
+ name := child.Name()
+ if child.Hidden || !strings.HasPrefix(name, "+") {
continue
}
- if _, ok := seen[shortcut.Command]; ok {
+ if _, ok := seen[name]; ok {
continue
}
- seen[shortcut.Command] = struct{}{}
- out = append(out, shortcut.Command)
+ seen[name] = struct{}{}
+ out = append(out, name)
}
return out
}Also applies to: 47-60
🤖 Prompt for 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.
In `@cmd/shortcut_preflight.go` around lines 26 - 27, availableShortcutCommands
currently reads from the global shortcut registry and so can return commands
that have been pruned from the mounted Cobra tree; change it to traverse the
mounted Cobra tree starting from the current command/root using cmd.Commands()
(or cmd.Root().Commands()) and collect shortcuts only from commands that are
present and available (skip commands where cmd.Hidden is true or
!cmd.IsAvailableCommand() or cmd.IsHelpCommand()), then return that set for
available_shortcuts and hints; update all call sites (the uses around available
:= availableShortcutCommands(cmd.Name()) and the block at lines 47-60) to pass
the active cmd/root or to call the new tree-based helper so suggestions respect
runtime pruning.
Unsupported root commands and +shortcut tokens could fall through to parent Cobra commands, producing normal help output or generic Cobra text instead of a stable agent-readable error. That made command typos look successful to agents. Add root command preflight validation for two narrow cases: an unmatched top-level command token returns structured unknown_command, and an unmatched service +shortcut token returns structured unknown_shortcut. Normal help, known top-level commands, known shortcut flag errors, and dynamic API resource/method paths stay on the existing Cobra paths. Expose available_commands and available_services for root typos, and available_shortcuts for shortcut typos, with truncated human hints and machine-readable detail. Cover unknown command, unknown command with --help, unknown command followed by +shortcut, unsupported shortcut, unsupported shortcut with --help, empty shortcut token, root/service help, non-service top-level commands, and known-shortcut unknown flag handling in integration tests. Change-Id: I045a4e7fbcb56aa57d5314e812d602a543dc5575
liangshuo-1
force-pushed
the
fix/unknown-shortcut-error
branch
from
5c0e2b8 to
da65f77
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #896 +/- ##
========================================
Coverage 65.90% 65.90%
========================================
Files 520 521 +1
Lines 49274 49409 +135
========================================
+ Hits 32474 32563 +89
- Misses 14026 14059 +33
- Partials 2774 2787 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/command_preflight.go (1)
58-106: 💤 Low valueConsider adding inline comments for flag parsing branches.
The flag parsing logic correctly handles long flags (
--name=value), short flags (-h value), and edge cases (--,--help), but the control flow through multiple branches could benefit from brief inline comments explaining each case (e.g.,// Skip double-dash terminator and process next tokenat line 65).This would improve maintainability when revisiting the parser logic.
🤖 Prompt for 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. In `@cmd/command_preflight.go` around lines 58 - 106, In function firstRootCommandToken add brief inline comments to clarify each parsing branch: annotate the handling of the double-dash terminator ("--") and that it returns the next token, the explicit help flags check ("--help", "-h"), the long-flag branch (strings.HasPrefix(arg, "--")) including note about handling inline values ("--name=value") and advancing i when the flag consumes a value, the short-flag branch (strings.HasPrefix(arg, "-") && arg != "-") including the shorthand lookup and value-consumption advance, and the default case that returns the first non-flag token; reference the symbols root.PersistentFlags(), flag := flags.Lookup(...), flags.ShorthandLookup(...), and flagConsumesValue(flag) when placing comments.
🤖 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.
Nitpick comments:
In `@cmd/command_preflight.go`:
- Around line 58-106: In function firstRootCommandToken add brief inline
comments to clarify each parsing branch: annotate the handling of the
double-dash terminator ("--") and that it returns the next token, the explicit
help flags check ("--help", "-h"), the long-flag branch (strings.HasPrefix(arg,
"--")) including note about handling inline values ("--name=value") and
advancing i when the flag consumes a value, the short-flag branch
(strings.HasPrefix(arg, "-") && arg != "-") including the shorthand lookup and
value-consumption advance, and the default case that returns the first non-flag
token; reference the symbols root.PersistentFlags(), flag := flags.Lookup(...),
flags.ShorthandLookup(...), and flagConsumesValue(flag) when placing comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 288ecf2e-1e30-4a0d-bba7-7e79fde2d070
📒 Files selected for processing (4)
CHANGELOG.mdcmd/command_preflight.gocmd/root.gocmd/root_integration_test.go
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Split the err branch and document intentional deferral to Execute with //nolint:nilerr so golangci-lint no longer flags returning nil after Traverse errors. Change-Id: Id671fc7f4fda15e74a45f86d2723c958401860a9 Co-authored-by: Cursor <cursoragent@cursor.com>
The command preflight intentionally leaves ordinary Cobra parse errors on the normal Execute path. Discard the Traverse error directly instead of returning nil from a non-nil error branch, avoiding nilerr without a nolint escape hatch. Change-Id: I482033a5de635cc0a93621ef046ca94680142fed
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
1 participant
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
+shortcuttokens fail before Cobra falls back to parent help.unknown_commandfor root-level command typos.unknown_shortcutfor service shortcut typos.Root cause
Cobra can match a parent command when a token remains unmatched. For service shortcuts this meant
lark-cli drive +nonexistent-thingmatched thedriveparent and returned normal help with exit code 0. The same class of issue can hide root-level typos behind generic Cobra behavior instead of a stable agent-readable error contract.Change
The new
validateCommandInvocationpreflight handles only the two agent-hostile cases before normal Cobra execution:unknown_command.+, and that shortcut is not in the service shortcut catalog -> structuredunknown_shortcut.The preflight intentionally does not parse dynamic API resource/method paths, known shortcut flags, or other Cobra parse errors. Those stay on the existing command-specific paths.
Verification
go test ./cmd -run 'TestIntegration_(UnknownTopLevelCommand|RootHelpStillSucceeds|NonServiceTopLevelCommandStillWorks|UnknownShortcut|EmptyShortcut|ServiceHelpStillSucceeds|KnownShortcutUnknownFlagKeepsExistingBehavior)' -count=1go test ./cmd ./shortcuts ./shortcuts/common ./shortcuts/drive -count=1go test $(go list ./... | grep -v '/tests/cli_e2e')lark-cli nonexistent-service-> exit 2 structuredunknown_command, empty stdout, no Cobra "Did you mean" text.lark-cli nonexistent-service --help-> exit 2 structuredunknown_command, empty stdout.lark-cli drive +nonexistent-thing-> exit 2 structuredunknown_shortcut, empty stdout.lark-cli drive +nonexistent-thing --help-> exit 2 structuredunknown_shortcut, empty stdout.lark-cli drive --help-> existing parent help, exit 0.lark-cli drive +search --bogus-> existing unknown-flag behavior preserved.Notes
go test ./...was also attempted before this revision, but existing live e2e packages failed due environment/scopes and stale e2e expectations unrelated to this change. Non-e2e packages are green.Summary by CodeRabbit
Bug Fixes
Tests