feat(shortcuts): introduce TypedShortcut framework#1110
Open
sang-neo03 wants to merge 3 commits into
Open
Conversation
github-actions
Bot
added
domain/im
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a typed-shortcut protocol and reflection binder, many arg-type validators, RuntimeContext typed-args storage, typed help, migrates im +messages-send to TypedShortcut, refactors registration to descriptor/mountable, and updates auth/scope call sites to use descriptor accessors. ChangesTyped Shortcut Framework & ImMessagesSend Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ccf654d3f079ce4d307302ab55615c40584deb1d🧩 Skill updatenpx skills add larksuite/cli#feat/shortcut-protocol -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: 12
🧹 Nitpick comments (3)
shortcuts/register.go (1)
126-135: ⚡ Quick winAdd a duplicate shortcut key guard before mounting.
Right now, duplicate
service/commandentries across legacy + typed lists are appended silently. A small guard here prevents accidental double-mount regressions.Suggested patch
byService := make(map[string][]common.Mountable) + seen := make(map[string]struct{}) for _, d := range allShortcuts { m, ok := d.(common.Mountable) if !ok { panic(fmt.Sprintf("shortcut %s/%s missing Mountable", d.GetService(), d.GetCommand())) } + key := d.GetService() + "/" + d.GetCommand() + if _, exists := seen[key]; exists { + panic(fmt.Sprintf("duplicate shortcut registration: %s", key)) + } + seen[key] = struct{}{} byService[d.GetService()] = append(byService[d.GetService()], m) }🤖 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 `@shortcuts/register.go` around lines 126 - 135, There is no duplicate-key guard when building byService from allShortcuts, so identical service/command pairs can be silently appended; add a seen set (map[string]struct{}) keyed by fmt.Sprintf("%s/%s", d.GetService(), d.GetCommand()) inside the loop that iterates allShortcuts and before appending to byService check the seen map and panic or return an error/log a fatal if the key already exists to prevent double-mounts; update code around the byService creation, the loop over allShortcuts, and use the common.Mountable assertion (m, ok := d.(common.Mountable)) together with the seen check to locate the duplicate and stop execution.shortcuts/im/im_messages_send_test.go (1)
85-86: ⚡ Quick winUse
cmdutil.TestFactory(t, config)in unit tests instead of&cmdutil.Factory{}.Line 85 and Line 115 instantiate a bare factory directly. Please switch both mount tests to
cmdutil.TestFactory(...)to align with the test harness contract.As per coding guidelines, "
**/*_test.go: Usecmdutil.TestFactory(t, config)for test factories in unit tests".Also applies to: 115-116
🤖 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 `@shortcuts/im/im_messages_send_test.go` around lines 85 - 86, Replace the direct instantiation of cmdutil.Factory (used when mounting the command with ImMessagesSend.MountWithContext(context.Background(), imParent, &cmdutil.Factory{})) with the test harness helper cmdutil.TestFactory(t, config); update both occurrences (the one at the ImMessagesSend.MountWithContext call and the second mount test near lines 115–116) to call cmdutil.TestFactory(t, config) so the tests use the proper test factory contract, passing the current *testing.T and the appropriate test config.shortcuts/im/builders_test.go (1)
363-418: ⚡ Quick winExercise typed-args behavior directly in these ImMessagesSend tests.
Most new calls pass
&ImMessagesSendArgs{}and rely on runtime flags, so these cases can still pass even if typed field usage regresses. Prefer constructing meaningfulImMessagesSendArgsper case (or binding once through the framework path) so Validate/DryRun coverage tracks the typed contract.Also applies to: 705-718
🤖 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 `@shortcuts/im/builders_test.go` around lines 363 - 418, Tests for ImMessagesSend call Validate with an empty &ImMessagesSendArgs{} and only runtime flags, so they don't exercise the typed-args contract; update each failing subtest to construct and pass a meaningful ImMessagesSendArgs that matches the scenario (e.g., set Text for the "valid text" case, set Video and VideoCover for the "video with video-cover" case, set only Video for the "video without video-cover" case, set only VideoCover for the "video-cover without video" case, and set MsgType plus Image for the "conflicting explicit msg-type" case) before calling ImMessagesSend.Validate(context.Background(), args, runtime), and apply the same change to the similar tests at the other location (lines ~705-718) so Validate/DryRun exercises the typed fields instead of relying solely on runtime flags; alternatively bind args via the framework path once so the typed struct is populated for each test.
🤖 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 `@shortcuts/common/argstype/safe_path.go`:
- Around line 35-36: The current check uses filepath.Clean(s) which can hide
parent-directory segments (e.g. "a/../b") — first scan the raw input string s
for any literal ".." path segments and reject if found before calling
filepath.Clean; implement this by splitting s on both '/' and '\' (or use
strings.FieldsFunc) and returning an error if any segment == "..", then continue
with the existing clean := filepath.Clean(s) and the remaining checks; reference
the existing variable clean and the use of filepath.Clean in safe_path.go when
making the change.
In `@shortcuts/common/argstype/spreadsheet_ref.go`:
- Around line 28-31: The URL token extraction currently checks segments from
strings.Split(trimmed, "/") and accepts segments that start with "shtcn" even
when they include query or fragment suffixes (e.g., "shtcn...?..."). Update the
extraction logic to strip any query ('?') or fragment ('#') portion from each
segment before checking HasPrefix and before returning SpreadsheetRef; do the
same fix in the other loop/block referenced around the SheetRef validation (the
code that returns SpreadsheetRef(seg) and the similar check at lines ~55-61).
Ensure you sanitize each seg by cutting at the first '?' or '#' (or parse the
URL and use the path segments) so only the raw token is validated and returned.
In `@shortcuts/common/argstype/user_open_id_list.go`:
- Around line 32-46: In UserOpenIDList.ValidateValue, add an explicit guard to
reject an explicitly empty parsed list (check len(l) == 0) and return an
errs.ValidationError referencing the flagName and a clear message like "empty
user open_id list" so inputs like ",," fail fast; keep the existing per-item
"ou_" prefix checks (function: ValidateValue on type UserOpenIDList, variables:
l and flagName, helper itoa) and ensure the new empty-list error uses the same
errs.Problem Category/Subtype pattern as the other errors.
In `@shortcuts/common/binder.go`:
- Around line 308-320: The code always calls cmd.Flags().GetString for both
pointer and non-pointer fields (s.FlagName) and then reflect.Converts that
string into the target type, which breaks for non-string leaves (bool/int) and
can panic; change the binding in binder.go (the s.IsPtr branch and the
non-pointer branch that use fv and elemType) to detect the target kind (use
elemType := fv.Type().Elem() for pointers and target := fv.Type() for
non-pointers) and switch on kind to call the appropriate flag getter (GetBool,
GetInt/GetInt64/GetUint, GetFloat64, GetStringSlice, etc.), handle errors from
those getters, and then set the reflect.Value using
reflect.ValueOf(parsedValue).Convert(target) (or set a new pointer when s.IsPtr)
so typed leaves are bound with their native types instead of always using
strings.
In `@shortcuts/common/typed_help_test.go`:
- Around line 29-33: In TestTypedHelp_RendersSections, the test must isolate
config state by setting the LARKSUITE_CLI_CONFIG_DIR environment variable; at
the start of the test (inside TestTypedHelp_RendersSections, before creating the
cobra.Command or calling walkArgs/reflect.TypeOf(&helpDemoArgs{})), call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) so the test uses a temporary
config directory and doesn't depend on ambient state.
- Around line 33-34: The test currently ignores the error returned by
walkArgs(reflect.TypeOf(&helpDemoArgs{})); change this to capture the error
(e.g. specs, err := walkArgs(...)) and fail fast if err != nil by calling the
test helper (e.g. t.Fatalf or t.Fatal) with the error message before calling
buildTypedHelp; keep using the same symbols walkArgs, helpDemoArgs, specs, and
buildTypedHelp so the rest of the test remains unchanged.
In `@shortcuts/common/typed_help.go`:
- Around line 147-156: renderGlobalFlags currently uses fs.VisitAll and appends
every flag (via the loop containing fs.VisitAll and globals = append(...))
without checking for hidden flags, causing flags with f.Hidden==true to appear
in "GLOBAL FLAGS"; update the VisitAll callback to skip flags where f.Hidden is
true (i.e., add an early return when f.Hidden), preserving the existing dedup
logic (checks against rendered and seen) and still appending only non-hidden
flags to the globals slice (refer to fs.VisitAll, the closure's f variable, the
rendered/seen maps, and globalFlag{Name:..., Shorthand:..., Usage:...}).
- Around line 28-29: Change the typed help output to write to stderr and omit
hidden flags when rendering global flags: replace uses of cmd.OutOrStdout() with
cmd.ErrOrStderr() (so human-readable help goes to stderr), and update
renderGlobalFlags (the VisitAll iteration over flags) to skip flags where
f.Hidden is true (e.g., continue if f.Hidden) so hidden flags are not listed
under GLOBAL FLAGS. Ensure both changes reference the existing cmd object and
the renderGlobalFlags/VisitAll flag iteration so behavior and signatures remain
unchanged.
In `@shortcuts/common/typed_shortcut_test.go`:
- Around line 65-66: Replace direct construction of cmdutil.Factory (the
&cmdutil.Factory{} passed into ts.MountWithContext and other test calls) with
the test helper cmdutil.TestFactory(t, config); before creating the factory,
call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate config state;
create or pass an appropriate minimal config value into cmdutil.TestFactory(t,
config) for these tests (references: ts.MountWithContext, cmdutil.Factory,
cmdutil.TestFactory, t.Setenv, "LARKSUITE_CLI_CONFIG_DIR").
In `@shortcuts/common/typed_shortcut.go`:
- Around line 189-192: The anonymous Execute wrapper currently calls s.Execute
unconditionally which can panic if s.Execute is nil; update the wrapper (the
Execute func that receives context.Context and *RuntimeContext and calls
rt.TypedArgs()) to first check if s.Execute == nil and return a clear error
(e.g., using fmt.Errorf or errors.New) when nil, otherwise invoke s.Execute(c,
args, rt) as before so missing handlers fail gracefully instead of panicking.
In `@shortcuts/im/helpers.go`:
- Around line 88-94: validateMediaFlagPath currently ignores os.IsNotExist
errors so missing local files pass validation; update the fio.Stat error
handling in validateMediaFlagPath to treat any non-nil err (including
os.IsNotExist) as a validation failure by returning output.ErrValidation("%s:
%v", flagName, err) when err != nil, leaving the early-returns for
empty/http/media-key values unchanged.
In `@shortcuts/register_test.go`:
- Around line 82-90: The test for AllShortcuts currently only compares lengths
after appending and can pass if slices alias; change it to capture the original
slice (orig := AllShortcuts()), then call got := AllShortcuts(), append to got,
and assert that orig's length and element values are unchanged by the append
(e.g., len(orig) == len(AllShortcuts()) and elements of orig equal elements of
AllShortcuts()); use the AllShortcuts symbol and the test function in
shortcuts/register_test.go to locate and replace the existing
length-after-append check so the test proves the returned slice is a true
defensive copy rather than an alias.
---
Nitpick comments:
In `@shortcuts/im/builders_test.go`:
- Around line 363-418: Tests for ImMessagesSend call Validate with an empty
&ImMessagesSendArgs{} and only runtime flags, so they don't exercise the
typed-args contract; update each failing subtest to construct and pass a
meaningful ImMessagesSendArgs that matches the scenario (e.g., set Text for the
"valid text" case, set Video and VideoCover for the "video with video-cover"
case, set only Video for the "video without video-cover" case, set only
VideoCover for the "video-cover without video" case, and set MsgType plus Image
for the "conflicting explicit msg-type" case) before calling
ImMessagesSend.Validate(context.Background(), args, runtime), and apply the same
change to the similar tests at the other location (lines ~705-718) so
Validate/DryRun exercises the typed fields instead of relying solely on runtime
flags; alternatively bind args via the framework path once so the typed struct
is populated for each test.
In `@shortcuts/im/im_messages_send_test.go`:
- Around line 85-86: Replace the direct instantiation of cmdutil.Factory (used
when mounting the command with
ImMessagesSend.MountWithContext(context.Background(), imParent,
&cmdutil.Factory{})) with the test harness helper cmdutil.TestFactory(t,
config); update both occurrences (the one at the ImMessagesSend.MountWithContext
call and the second mount test near lines 115–116) to call
cmdutil.TestFactory(t, config) so the tests use the proper test factory
contract, passing the current *testing.T and the appropriate test config.
In `@shortcuts/register.go`:
- Around line 126-135: There is no duplicate-key guard when building byService
from allShortcuts, so identical service/command pairs can be silently appended;
add a seen set (map[string]struct{}) keyed by fmt.Sprintf("%s/%s",
d.GetService(), d.GetCommand()) inside the loop that iterates allShortcuts and
before appending to byService check the seen map and panic or return an
error/log a fatal if the key already exists to prevent double-mounts; update
code around the byService creation, the loop over allShortcuts, and use the
common.Mountable assertion (m, ok := d.(common.Mountable)) together with the
seen check to locate the duplicate and stop execution.
🪄 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: 209aa335-569d-4ed7-8c1a-08a780d09bdd
📒 Files selected for processing (42)
cmd/auth/login.gocmd/auth/login_interactive.gocmd/auth/login_test.gocmd/diagnose_scope_test.gocmd/error_auth_hint.goerrs/subtypes_shortcut.goerrs/subtypes_shortcut_test.goshortcuts/common/argstype/chat_id.goshortcuts/common/argstype/chat_id_test.goshortcuts/common/argstype/media_input.goshortcuts/common/argstype/media_input_test.goshortcuts/common/argstype/safe_path.goshortcuts/common/argstype/safe_path_test.goshortcuts/common/argstype/spreadsheet_ref.goshortcuts/common/argstype/spreadsheet_ref_test.goshortcuts/common/argstype/user_open_id.goshortcuts/common/argstype/user_open_id_list.goshortcuts/common/argstype/user_open_id_list_test.goshortcuts/common/argstype/user_open_id_test.goshortcuts/common/binder.goshortcuts/common/binder_test.goshortcuts/common/protocol.goshortcuts/common/protocol_test.goshortcuts/common/runner.goshortcuts/common/runner_args_test.goshortcuts/common/typed_help.goshortcuts/common/typed_help_test.goshortcuts/common/typed_shortcut.goshortcuts/common/typed_shortcut_test.goshortcuts/common/types.goshortcuts/common/types_test.goshortcuts/im/builders_test.goshortcuts/im/helpers.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_send.goshortcuts/im/im_messages_send_test.goshortcuts/im/protocol.goshortcuts/im/protocol_test.goshortcuts/im/shortcuts.goshortcuts/im/validate_media_test.goshortcuts/register.goshortcuts/register_test.go
sang-neo03
force-pushed
the
feat/shortcut-protocol
branch
from
6d67e5b to
bd95794
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/im/helpers.go (1)
92-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast on missing local media paths.
Line 92 currently treats missing files as valid input. This defers a user-input error to a later stage and weakens preflight validation.
Suggested fix
import ( "bytes" "context" "encoding/binary" "encoding/json" "fmt" "io" "math" "net/http" "net/url" - "os" "path" "path/filepath" "regexp" "strconv" "strings" @@ func validateMediaFlagPath(fio fileio.FileIO, flagName, value string) error { if value == "" || strings.HasPrefix(value, "http://") || strings.HasPrefix(value, "https://") || isMediaKey(value) { return nil } - if _, err := fio.Stat(value); err != nil && !os.IsNotExist(err) { + if _, err := fio.Stat(value); err != nil { return output.ErrValidation("%s: %v", flagName, err) } return nil }🤖 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 `@shortcuts/im/helpers.go` around lines 92 - 93, The preflight validation in shortcuts/im/helpers.go currently ignores os.IsNotExist errors when statting the path; change the check in the block that calls fio.Stat(value) (the validation that returns output.ErrValidation("%s: %v", flagName, err)) to treat any non-nil err — including os.IsNotExist — as a validation failure so missing local media paths fail fast; locate the fio.Stat(value) call and remove the special-case os.IsNotExist(err) condition so the function that returns output.ErrValidation using flagName and value runs whenever err != nil.
🤖 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.
Duplicate comments:
In `@shortcuts/im/helpers.go`:
- Around line 92-93: The preflight validation in shortcuts/im/helpers.go
currently ignores os.IsNotExist errors when statting the path; change the check
in the block that calls fio.Stat(value) (the validation that returns
output.ErrValidation("%s: %v", flagName, err)) to treat any non-nil err —
including os.IsNotExist — as a validation failure so missing local media paths
fail fast; locate the fio.Stat(value) call and remove the special-case
os.IsNotExist(err) condition so the function that returns output.ErrValidation
using flagName and value runs whenever err != nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a01b8809-103e-4b21-a758-f64431a04621
📒 Files selected for processing (42)
cmd/auth/login.gocmd/auth/login_interactive.gocmd/auth/login_test.gocmd/diagnose_scope_test.gocmd/error_auth_hint.goerrs/subtypes_shortcut.goerrs/subtypes_shortcut_test.goshortcuts/common/argstype/chat_id.goshortcuts/common/argstype/chat_id_test.goshortcuts/common/argstype/media_input.goshortcuts/common/argstype/media_input_test.goshortcuts/common/argstype/safe_path.goshortcuts/common/argstype/safe_path_test.goshortcuts/common/argstype/spreadsheet_ref.goshortcuts/common/argstype/spreadsheet_ref_test.goshortcuts/common/argstype/user_open_id.goshortcuts/common/argstype/user_open_id_list.goshortcuts/common/argstype/user_open_id_list_test.goshortcuts/common/argstype/user_open_id_test.goshortcuts/common/binder.goshortcuts/common/binder_test.goshortcuts/common/protocol.goshortcuts/common/protocol_test.goshortcuts/common/runner.goshortcuts/common/runner_args_test.goshortcuts/common/typed_help.goshortcuts/common/typed_help_test.goshortcuts/common/typed_shortcut.goshortcuts/common/typed_shortcut_test.goshortcuts/common/types.goshortcuts/common/types_test.goshortcuts/im/builders_test.goshortcuts/im/helpers.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_send.goshortcuts/im/im_messages_send_test.goshortcuts/im/protocol.goshortcuts/im/protocol_test.goshortcuts/im/shortcuts.goshortcuts/im/validate_media_test.goshortcuts/register.goshortcuts/register_test.go
✅ Files skipped from review due to trivial changes (2)
- shortcuts/common/argstype/user_open_id.go
- shortcuts/im/validate_media_test.go
🚧 Files skipped from review as they are similar to previous changes (38)
- shortcuts/common/types.go
- errs/subtypes_shortcut.go
- errs/subtypes_shortcut_test.go
- shortcuts/common/argstype/safe_path_test.go
- shortcuts/common/runner_args_test.go
- shortcuts/common/argstype/spreadsheet_ref_test.go
- shortcuts/common/argstype/user_open_id_list_test.go
- shortcuts/common/typed_help_test.go
- cmd/auth/login_interactive.go
- cmd/error_auth_hint.go
- shortcuts/common/argstype/chat_id_test.go
- shortcuts/common/argstype/user_open_id_test.go
- shortcuts/common/argstype/spreadsheet_ref.go
- shortcuts/common/argstype/media_input.go
- shortcuts/common/argstype/chat_id.go
- shortcuts/im/shortcuts.go
- shortcuts/common/types_test.go
- cmd/auth/login.go
- cmd/diagnose_scope_test.go
- shortcuts/common/argstype/user_open_id_list.go
- shortcuts/im/protocol_test.go
- shortcuts/common/argstype/safe_path.go
- shortcuts/im/protocol.go
- shortcuts/register.go
- shortcuts/common/protocol.go
- cmd/auth/login_test.go
- shortcuts/common/runner.go
- shortcuts/common/argstype/media_input_test.go
- shortcuts/im/im_messages_send_test.go
- shortcuts/common/binder_test.go
- shortcuts/im/helpers_test.go
- shortcuts/im/builders_test.go
- shortcuts/common/typed_shortcut.go
- shortcuts/common/typed_shortcut_test.go
- shortcuts/common/binder.go
- shortcuts/im/im_messages_send.go
- shortcuts/register_test.go
- shortcuts/common/typed_help.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1110 +/- ##
==========================================
+ Coverage 67.92% 68.22% +0.30%
==========================================
Files 601 628 +27
Lines 55766 58195 +2429
==========================================
+ Hits 37880 39706 +1826
- Misses 14751 15188 +437
- Partials 3135 3301 +166 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sang-neo03
force-pushed
the
feat/shortcut-protocol
branch
from
bd95794 to
c82ad7b
Compare
Add strongly-typed shortcut protocol that coexists with the legacy common.Shortcut. The new common.TypedShortcut[T] is a generic outer wrapper backed by a reflect-driven binder; both legacy and typed shortcuts satisfy a new common.Mountable / common.ShortcutDescriptor interface pair so register.go can dispatch either through the same pipeline. Framework (shortcuts/common): - protocol.go — Mountable / ShortcutDescriptor / OneOfMarker / Validatable / Normalizable[T] / ArgsValidator / Maybe[T] / HelpExample - binder.go — reflect Args walk, intra-Args flag-tag uniqueness panic, cobra flag registration, bindFlags + bindMaybe, runNormalize (via MethodByName dispatch — Normalizable[T] can't be type-asserted through a non-generic interface), runValidateValue, runFrameworkRules for required / enum / OneOf / group - typed_shortcut.go — TypedShortcut[T] struct, 8 descriptor methods, mountTyped adapter that synthesizes a legacy Shortcut shell and reuses runShortcut verbatim (identity / scopes / @file / stdin / jq / dry-run / high-risk gate) - typed_help.go — sectioned --help (CHOOSE ONE / OPTIONAL / EXAMPLES) with cmdutil.GetRisk/GetTips passthrough so typed shortcuts keep the Risk: and Tips: blocks - runner.go — typedArgs lifecycle slot on RuntimeContext - types.go — 5 GetX accessors on *Shortcut so legacy shortcuts satisfy ShortcutDescriptor alongside the existing pointer-receiver scope methods Typed primitives (shortcuts/common/argstype): - ChatID / UserOpenID / UserOpenIDList — prefix-validated identifiers - SafePath — cwd-relative, rejects absolute paths and ".." segments - MediaInput — tri-state (URL bypass / img_xxx-file_xxx key bypass / SafePath delegation) - SpreadsheetRef — Normalize extracts shtcn token from feishu URLs Error contract (errs): - 3 new Subtype constants: shortcut_oneof_missing / shortcut_oneof_multiple / shortcut_group_incomplete. Per-field failures (required / enum / typed primitive format) reuse the existing SubtypeInvalidArgument so no new error type is introduced. Registry refactor (shortcuts + cmd): - AllShortcuts() now returns []common.ShortcutDescriptor; legacy shortcuts are boxed as *Shortcut (pointer required for the pointer-receiver scope methods), typed shortcuts boxed as Mountable - Register dispatches via the Mountable interface - cmd/auth/login, cmd/auth/login_interactive, cmd/error_auth_hint, cmd/diagnose_scope_test, shortcuts/register_test (shortcuts.json generator) updated to read through GetService / GetCommand / GetAuthTypes / GetDescription / DeclaredScopesForIdentity - shortcutSupportsIdentity helpers accept ShortcutDescriptor Pilot (shortcuts/im): - protocol.go — MessageTarget / MessageContent (with seven content variants) / VideoContent (paired video + cover) / RawContent (--content with explicit msg-type, validates JSON in ValidateValue) - im_messages_send.go — migrated to TypedShortcut[*ImMessagesSendArgs]. Inline Validate closure is replaced by framework-derived checks (OneOf target, OneOf content, VideoContent group, typed-primitive formats, RawContent JSON). Helpers (resolveMediaContent, wrapMarkdownAsPostForDryRun, normalizeAtMentions, etc.) reused verbatim; only the field-access pattern changes from runtime.Str("x") to args.X. - shortcuts.go — new TypedShortcuts() exporter; ImMessagesSend removed from the legacy Shortcuts() slice so it is not double-mounted - register.go wires addTyped(im.TypedShortcuts()) into init Known follow-ups: - runFrameworkRules and bindFlags do not recurse into OneOf bucket / group sub-structs; im messages-send compensates with a local bindMessagesSendArgs + validateVideoGroup. Generalizing the binder to recurse will let future migrations drop the local shim. - common.ValidateChatID / common.ValidateUserID become redundant once all legacy shortcuts that call them migrate; can be retired with the last legacy caller. Refs: docs/superpowers/specs/2026-05-26-shortcut-protocol-design.md
sang-neo03
force-pushed
the
feat/shortcut-protocol
branch
from
c82ad7b to
a07239b
Compare
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 `@shortcuts/im/im_messages_send_test.go`:
- Around line 40-49: The test currently only compares lengths and iterates
values which allows duplicates like ["bot","bot"] to pass; change the assertion
to compare unique auth types by converting ImMessagesSend.GetAuthTypes() into a
map/set (e.g., seen := map[string]bool{}) then ensure every expected key in
wantAuth exists in seen and that seen has no unexpected keys and equal
cardinality to wantAuth; use ImMessagesSend.GetAuthTypes to locate the call and
update the loop/checks to detect duplicates and missing members.
🪄 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: 71545eb9-59c3-4571-8da0-3176d56f82db
📒 Files selected for processing (42)
cmd/auth/login.gocmd/auth/login_interactive.gocmd/auth/login_test.gocmd/diagnose_scope_test.gocmd/error_auth_hint.goerrs/subtypes_shortcut.goerrs/subtypes_shortcut_test.goshortcuts/common/argstype/chat_id.goshortcuts/common/argstype/chat_id_test.goshortcuts/common/argstype/media_input.goshortcuts/common/argstype/media_input_test.goshortcuts/common/argstype/safe_path.goshortcuts/common/argstype/safe_path_test.goshortcuts/common/argstype/spreadsheet_ref.goshortcuts/common/argstype/spreadsheet_ref_test.goshortcuts/common/argstype/user_open_id.goshortcuts/common/argstype/user_open_id_list.goshortcuts/common/argstype/user_open_id_list_test.goshortcuts/common/argstype/user_open_id_test.goshortcuts/common/binder.goshortcuts/common/binder_test.goshortcuts/common/protocol.goshortcuts/common/protocol_test.goshortcuts/common/runner.goshortcuts/common/runner_args_test.goshortcuts/common/typed_help.goshortcuts/common/typed_help_test.goshortcuts/common/typed_shortcut.goshortcuts/common/typed_shortcut_test.goshortcuts/common/types.goshortcuts/common/types_test.goshortcuts/im/builders_test.goshortcuts/im/helpers.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_send.goshortcuts/im/im_messages_send_test.goshortcuts/im/protocol.goshortcuts/im/protocol_test.goshortcuts/im/shortcuts.goshortcuts/im/validate_media_test.goshortcuts/register.goshortcuts/register_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/im/validate_media_test.go
🚧 Files skipped from review as they are similar to previous changes (37)
- shortcuts/common/typed_help_test.go
- shortcuts/common/argstype/chat_id.go
- shortcuts/common/runner_args_test.go
- errs/subtypes_shortcut_test.go
- cmd/auth/login.go
- cmd/auth/login_interactive.go
- shortcuts/common/types_test.go
- shortcuts/common/argstype/spreadsheet_ref.go
- shortcuts/im/helpers.go
- shortcuts/common/runner.go
- shortcuts/common/argstype/spreadsheet_ref_test.go
- shortcuts/common/protocol.go
- shortcuts/common/argstype/media_input_test.go
- shortcuts/common/argstype/user_open_id_list.go
- shortcuts/im/builders_test.go
- shortcuts/common/argstype/user_open_id_test.go
- cmd/diagnose_scope_test.go
- errs/subtypes_shortcut.go
- shortcuts/common/typed_help.go
- shortcuts/im/protocol.go
- shortcuts/common/typed_shortcut.go
- shortcuts/common/argstype/user_open_id.go
- shortcuts/common/argstype/safe_path.go
- shortcuts/common/argstype/chat_id_test.go
- shortcuts/common/protocol_test.go
- shortcuts/im/protocol_test.go
- shortcuts/common/typed_shortcut_test.go
- cmd/error_auth_hint.go
- shortcuts/common/types.go
- shortcuts/register.go
- cmd/auth/login_test.go
- shortcuts/register_test.go
- shortcuts/im/helpers_test.go
- shortcuts/common/binder_test.go
- shortcuts/common/binder.go
- shortcuts/common/argstype/media_input.go
- shortcuts/im/im_messages_send.go
- im_messages_send_test: assert auth-type set membership (reject duplicates / missing members) per CodeRabbit review - binder_coverage_test: cover bindMaybe, bindBuckets / bindBucketInner, bucketLeafValue, runNormalize / asString, checkGroup, checkEnumAndRequired, and group recursion in runValidateValue — lifts patch coverage over the 60% gate
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/common/binder_coverage_test.go (2)
159-171: ⚡ Quick winAssert pointer normalization explicitly.
TokenPtris set up to exercise pointer normalization but never asserted. Add a direct check so this branch is validated, not just executed.Proposed assertion
if args.Token != "c:raw" { t.Errorf("Token = %q, want c:raw", args.Token) } +if args.TokenPtr == nil || *args.TokenPtr != "c:xy" { + t.Errorf("TokenPtr = %v, want c:xy", args.TokenPtr) +} if got := buf.String(); got == "" || !bytes.Contains([]byte(got), []byte("hint: raw")) { t.Errorf("stderr = %q, want it to contain the normalize hint", got) }🤖 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 `@shortcuts/common/binder_coverage_test.go` around lines 159 - 171, After calling runNormalize with bcNormArgs (ptr created by bcNormField("xy")), add an explicit assertion that the pointer-normalized value was updated: check the dereferenced TokenPtr on args (e.g., *args.TokenPtr) equals the expected normalized string ("c:xy") and fail the test with t.Fatalf or t.Errorf if it does not match; reference bcNormField, bcNormArgs, TokenPtr, runNormalize and args when locating where to add this assertion.
28-29: ⚡ Quick winStop discarding setup errors in tests.
Several tests ignore errors from
walkArgs,registerFlags, andParseFlags. This can mask regressions and make failures harder to diagnose.Suggested pattern to apply across this file
- specs, _ := walkArgs(reflect.TypeOf(&bcMaybeArgs{})) - _ = registerFlags(cmd, specs) - _ = cmd.ParseFlags([]string{"--m-on"}) + specs, err := walkArgs(reflect.TypeOf(&bcMaybeArgs{})) + if err != nil { + t.Fatalf("walkArgs: %v", err) + } + if err := registerFlags(cmd, specs); err != nil { + t.Fatalf("registerFlags: %v", err) + } + if err := cmd.ParseFlags([]string{"--m-on"}); err != nil { + t.Fatalf("ParseFlags: %v", err) + }Also applies to: 53-54, 119-120, 161-162, 180-180, 201-204, 217-217, 221-223, 243-243, 247-248, 259-260, 267-268, 286-286
🤖 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 `@shortcuts/common/binder_coverage_test.go` around lines 28 - 29, The tests are discarding errors from setup calls (e.g., walkArgs, registerFlags, ParseFlags) which can hide failures; update each call site (such as the walkArgs(reflect.TypeOf(&bcMaybeArgs{})) call, registerFlags(cmd, specs), and any ParseFlags uses) to check the returned error and fail the test immediately (use t.Fatalf or t.Fatalf-like helper or testify/require.NoError) with a clear message including the error; ensure you propagate and assert errors for all occurrences listed so test setup failures cause visible test failures.
🤖 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 `@shortcuts/common/binder_coverage_test.go`:
- Around line 159-171: After calling runNormalize with bcNormArgs (ptr created
by bcNormField("xy")), add an explicit assertion that the pointer-normalized
value was updated: check the dereferenced TokenPtr on args (e.g.,
*args.TokenPtr) equals the expected normalized string ("c:xy") and fail the test
with t.Fatalf or t.Errorf if it does not match; reference bcNormField,
bcNormArgs, TokenPtr, runNormalize and args when locating where to add this
assertion.
- Around line 28-29: The tests are discarding errors from setup calls (e.g.,
walkArgs, registerFlags, ParseFlags) which can hide failures; update each call
site (such as the walkArgs(reflect.TypeOf(&bcMaybeArgs{})) call,
registerFlags(cmd, specs), and any ParseFlags uses) to check the returned error
and fail the test immediately (use t.Fatalf or t.Fatalf-like helper or
testify/require.NoError) with a clear message including the error; ensure you
propagate and assert errors for all occurrences listed so test setup failures
cause visible test failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77a27179-29fe-4765-9b55-186259dd15c4
📒 Files selected for processing (2)
shortcuts/common/binder_coverage_test.goshortcuts/im/im_messages_send_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/im/im_messages_send_test.go
The im +messages-send TypedShortcut pilot was exploratory only; revert it while retaining the TypedShortcut framework for future migrations. - shortcuts/im: restored to pre-pilot state (im_messages_send.go back to legacy common.Shortcut; deleted protocol.go, protocol_test.go, im_messages_send_test.go; shortcuts.go drops TypedShortcuts()). - shortcuts/register.go: removed addTyped(im.TypedShortcuts()) wiring and the now-unused addTyped helper; legacy addLegacy + Mountable dispatch retained. - shortcuts/common, argstype, errs subtypes, cmd/auth adapters: kept. - framework doc comments: replaced examples referencing the removed pilot types (MessageTarget/MessageContent/VideoContent/RawContent) with neutral descriptions; noted no typed shortcut is registered today. Framework now has no production caller. Verified: go build ./... and go test ./shortcuts/... ./errs/... ./cmd/auth/... ./cmd/ all pass.
sang-neo03
changed the title
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
Introduces a strongly-typed shortcut protocol that coexists with the legacy
common.Shortcut. A genericTypedShortcut[T]outer wrapper, a reflect-driven binder, six typed primitives, and a sectioned--helprenderer give shortcut authors compile-time argument types and structured error envelopes (shortcut_oneof_missing/shortcut_oneof_multiple/shortcut_group_incomplete) without forking the existingrunShortcutpipeline.Note
Update (ccf654d): the
im +messages-sendpilot was reverted. The pilotwas an exploratory end-to-end validation of the framework only. It has been
rolled back so this PR lands the framework alone —
im +messages-sendstays on the legacy
common.Shortcutpath, identical tomain. Theframework retains no production caller yet; the first real migration will
come in a follow-up PR. See the "Net result after revert" section below for
the current diff. The Changes / Test Plan sections below describe the
original pilot commits (a07239b / ad4368e) and are kept for history.
Net result after revert (current PR diff)
shortcuts/common/framework (protocol / binder / typed_shortcut / typed_help / runner / types),shortcuts/common/argstype/primitives,errsshortcut subtypes,cmd/auth+register.goShortcutDescriptor/Mountabledual-track dispatch.shortcuts/im/restored to its pre-pilot (main) state —im_messages_send.goback to legacycommon.Shortcut;protocol.go/protocol_test.go/im_messages_send_test.godeleted;shortcuts.gono longer exportsTypedShortcuts().addTyped(im.TypedShortcuts())and the now-unusedaddTypedhelper dropped fromregister.go; framework doc comments that referenced the pilot types (MessageTarget/MessageContent/VideoContent/RawContent) rewritten as neutral descriptions.go build ./...andgo test ./shortcuts/... ./errs/... ./cmd/auth/... ./cmd/all pass;gofmtclean; no dangling references to the removed pilot types remain in the framework.Changes (original pilot — see Update note above)
shortcuts/common/protocol.go—Mountable/ShortcutDescriptor/OneOfMarker/Validatable/Normalizable[T]/ArgsValidator/Maybe[T]/HelpExampleshortcuts/common/binder.go— reflect Args walker with intra-Args tag-uniqueness panic, cobra flag registration, value binding,bindBucketsfor nested OneOf / group sub-structs,runNormalize(reflect-dispatched, generic-safe),runValidateValue(recurses into buckets / groups),runFrameworkRules(required / enum / OneOf / group, recursive)shortcuts/common/typed_shortcut.go—TypedShortcut[T]with 8 metadata methods +mountTypedadapter that synthesizes a legacy shell and reuses the existing identity / scopes /@file/ stdin / jq / dry-run / high-risk gateshortcuts/common/typed_help.go— sectioned--help(CHOOSE ONE <FIELD>/OPTIONAL/EXAMPLES/GLOBAL FLAGS) withRisk:/Tips:passthroughshortcuts/common/argstype/— 6 typed primitives:ChatID(oc_ prefix),UserOpenID(ou_ prefix),UserOpenIDList(CSV),SafePath(cwd-relative, rejects abs /..),MediaInput(URL / key / path tri-state),SpreadsheetRef(URL → token Normalize)errs/subtypes_shortcut.go—SubtypeShortcutOneOfMissing/SubtypeShortcutOneOfMultiple/SubtypeShortcutGroupIncompleteshortcuts/common/types.go— 5Get*accessors on*Shortcut(GetService/GetCommand/GetDescription/GetAuthTypes/GetRisk)shortcuts/common/runner.go—typedArgs anylifecycle slot +TypedArgs/SetTypedArgsaccessors onRuntimeContextshortcuts/register.go—AllShortcuts()returns[]ShortcutDescriptor;RegisterShortcutsWithContextdispatchesMountableso legacy and typed shortcuts share one path;addLegacyhelper boxes each domain's contributions (theaddTypedhelper andimpilot wiring were later reverted — see Update note)Add(reverted)shortcuts/im/protocol.go—MessageTarget/MessageContent/VideoContent/RawContentModify(reverted; back to legacy)shortcuts/im/im_messages_send.go— migrated toTypedShortcut[*ImMessagesSendArgs]Modify(reverted)shortcuts/im/shortcuts.go— newTypedShortcuts()exportercmd/auth/login.go,cmd/auth/login_interactive.go,cmd/auth/login_test.go,cmd/error_auth_hint.go,cmd/diagnose_scope_test.go,shortcuts/register_test.go— read shortcuts throughShortcutDescriptorinterface methods (GetService/GetCommand/GetAuthTypes/DeclaredScopesForIdentity);shortcutSupportsIdentityaccepts the interfaceTest Plan
go build ./...and unit tests pass (go test ./shortcuts/... ./errs/... ./cmd/auth/... ./cmd/)gofmtclean; no dangling references to reverted pilot typesunused-lint hazard from retaining the frameworkim +messages-sendreverts to its existing (already-tested) legacy behavior.Related Issues
N/A
Summary by CodeRabbit
New Features
Bug Fixes
Tests