feat(extension): add business command extension v1 - #2308
Open
sang-neo03 wants to merge 37 commits into
Open
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds typed command contracts, compilation, runtime execution, validation, schemas, pagination, external registration, authentication discovery, embedded content providers, wrapper integration, and generated-file checks. ChangesTyped command extension
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8d264ee3c4dcfefa9ff2c407cf521bbab4f0ca4a🧩 Skill updatenpx skills add larksuite/cli#feat/command-extension-v1 -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2308 +/- ##
==========================================
- Coverage 76.43% 76.04% -0.40%
==========================================
Files 1025 1063 +38
Lines 113661 118318 +4657
==========================================
+ Hits 86876 89974 +3098
- Misses 20111 21291 +1180
- Partials 6674 7053 +379 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (13)
shortcuts/common/runner.go (1)
954-960: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
RelationCoOccurin the schema-introspection regression test.
TestTypedFlagSchemaListsAndPrintsCompositeInputsBeforeExecutionalready coverscobra_annotation_one_requiredandcobra_annotation_mutually_exclusivethroughRelationExactlyOne. Add aRelationCoOccurcase and pass only one grouped flag with--print-schemato covercobra_annotation_required_if_others_set.🤖 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/runner.go` around lines 954 - 960, Extend TestTypedFlagSchemaListsAndPrintsCompositeInputsBeforeExecution with a RelationCoOccur case, invoking --print-schema while supplying only one flag from the group. Assert schema introspection succeeds without requiring the co-occurring flag, covering cobra_annotation_required_if_others_set alongside the existing RelationExactlyOne cases.shortcuts/common/typed_binder_benchmark_test.go (1)
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe direct-assignment baseline can be optimized away.
argsnever escapes and the result is discarded, so the compiler can remove the loop body. The benchmark then reports near-zero time and the comparison against the reflection path overstates the reflection cost. Assign to a package-level sink to keep the work observable.♻️ Proposed refactor
+var binderBenchmarkSink binderBenchmarkArgs + func BenchmarkTypedBinderDirectAssignment(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { args := binderBenchmarkArgs{} args.Value = Provided[int]{Value: 42, Set: true} - _ = args + binderBenchmarkSink = args } }🤖 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/typed_binder_benchmark_test.go` around lines 26 - 33, Update BenchmarkTypedBinderDirectAssignment to assign the constructed binderBenchmarkArgs value to a package-level sink so the direct assignment cannot be optimized away. Preserve the existing benchmark setup and assignment behavior while making the result observable across iterations.shortcuts/common/typed_compile_args.go (1)
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompiler diagnostics iterate Go maps, so the reported item is nondeterministic. Both sites select the item to report by ranging over a map. When more than one item is invalid, the panic or error text names an arbitrary one, and repeated builds report different items.
validateOutputHooksinshortcuts/common/typed_compile_output.goalready sorts its keys before reporting; apply the same convention here.
shortcuts/common/typed_compile_args.go#L74-L78: collect the remaining supplement names, sort them, and report them in one message.shortcuts/common/typed_compile_contract.go#L153-L153: replace the map literal with an ordered slice of label and path pairs so the artifact field labels are checked in a fixed order.🤖 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/typed_compile_args.go` around lines 74 - 78, The compiler diagnostics are nondeterministic because invalid items are selected from unordered maps. In shortcuts/common/typed_compile_args.go lines 74-78, collect and sort all remaining supplement names, then report them together in one deterministic error message. In shortcuts/common/typed_compile_contract.go line 153, replace the map literal with an ordered slice of label/path pairs so artifact field validation follows a fixed order; validateOutputHooks provides the existing sorting convention.shortcuts/common/typed_external.go (1)
110-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the external reserved flags synchronized with host registration.
The list currently covers
--profile, shortcut flags, pagination flags, schema flags, and Cobra’s--help. Add a shared flag-name source or a test that compares this map with all registration paths, including conditional flags such as--jsonand--yes.🤖 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/typed_external.go` around lines 110 - 125, Update validateExternalFlagNamespace so its reserved names stay synchronized with every host flag registration path, including conditional --json and --yes flags. Prefer reusing a shared flag-name source; otherwise add a test that compares the reserved map against all registrations and fails when names diverge.shortcuts/common/typed_schema.go (1)
141-146: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueThe authorization copy is shallow for conditional scopes.
Line 144 copies the
ConditionalScopesslice. Each copied element still shares the backing arrays of itsScopesandParamsslices with the caller'sDefinition. A caller that retains the definition and mutates those slices changes the compiled contract afterDefine. The help path inshortcuts/common/typed_help_render.go(cloneTypedHelpFacts) already copies these inner slices. Align the two paths.♻️ Proposed deep copy
for identity, authorization := range command.metadata.Authorization.Identities { authorization.RequiredScopes = append([]string{}, authorization.RequiredScopes...) authorization.ConditionalScopes = append([]ConditionalScope{}, authorization.ConditionalScopes...) + for i := range authorization.ConditionalScopes { + conditional := &authorization.ConditionalScopes[i] + conditional.Scopes = append([]string{}, conditional.Scopes...) + conditional.Params = append([]string{}, conditional.Params...) + } authorizationIdentities[identity] = authorization }🤖 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/typed_schema.go` around lines 141 - 146, Deep-copy each ConditionalScope in the authorization copy loop before storing it in authorizationIdentities, including independent copies of every element’s Scopes and Params slices. Update the logic alongside the existing ConditionalScopes copy in the command metadata handling, matching cloneTypedHelpFacts behavior so later caller mutations cannot alter the compiled contract.shortcuts/common/typed_map_binder_test.go (1)
72-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the typed error metadata, not the message alone.
Line 78 confirms only that the error is typed. Line 79 then asserts the message text. Add assertions for
Category,Subtype, andValidationError.Paramso a change in classification fails the test. The same gap exists in the "error if both" subtest at lines 97-103, which asserts onlyerrors.As.Error tests must assert typed metadata and cause preservation rather than message text alone, as per coding guidelines (
**/*_test.go).♻️ Proposed stronger assertions
_, err = bindTypedMap(command, map[string]any{}) + var validation *errs.ValidationError problem, ok := errs.ProblemOf(err) - if !ok || problem.Message != "--token is required" { + if !ok || !errors.As(err, &validation) || problem.Category != errs.CategoryValidation || + problem.Subtype != errs.SubtypeInvalidArgument || validation.Param != "--token" || + problem.Message != "--token is required" { t.Fatalf("error = %v, problem = %#v", err, problem) }Apply the same pattern to the "error if both" subtest.
🤖 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/typed_map_binder_test.go` around lines 72 - 82, Strengthen TestBindTypedMapRequiredMessageUsesLegacyCLIForm and the “error if both” subtest by asserting the typed problem’s Category, Subtype, and ValidationError.Param through the existing errs.ProblemOf/typed error structures. Retain the message assertion, and also verify the underlying cause is preserved as required by the test guidelines.Source: Coding guidelines
shortcuts/common/typed_help_render_test.go (1)
75-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the remaining constraint kinds to the table.
typedHelpConstraintTexthandlesat_most_one,same_value, the incompleterequirescase with one parameter, theTextpassthrough, and thedefaultbranch. The table covers five kinds only. Add the missing cases so a change to the wording of any branch fails a test.♻️ Proposed additional cases
tests := map[string]string{ + string(RelationAtMostOne): "at most one of: --a, --b", + string(RelationSameValue): "must have the same value: --a, --b", string(RelationExactlyOne): "exactly one of: --a, --b", string(RelationAtLeastOne): "at least one of: --a, --b", string(RelationRequires): "--a requires --b", string(RelationConflicts): "conflicting parameters: --a, --b", string(RelationCoOccur): "all or none of: --a, --b", }Add a separate assertion for the
Textpassthrough and forrequireswith a single parameter.🤖 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/typed_help_render_test.go` around lines 75 - 91, Add table entries in TestConstraintTextKinds for the at_most_one and same_value relation kinds, plus the default branch, using the exact expected wording from typedHelpConstraintText. Add separate assertions covering the single-parameter requires case and Text passthrough so every remaining branch is validated.shortcuts/common/typed_mount_guard.go (1)
38-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider capturing the remaining Cobra execution hooks.
The snapshot records
Args,PreRunE,RunE,UsageFunc, andHelpFunc. It does not recordPersistentPreRunE,PreRun,Run,PostRunE, orValidArgsFunction. APostMounthook can install one of these and change execution or completion behavior without failing the guard. The guard is fail-closed for the other fields, so extend it for consistency.Note also that
reflect.Value.Pointer()returns the code pointer of a function literal. Two closures created from the same literal compare equal. The guard therefore detects replacement by a different literal, not by a new instance of the same literal.♻️ Proposed extension of the snapshot
type typedMountSnapshot struct { use, short, long, example string aliases []string argsFunc, preRunFunc, runFunc, usageFunc, helpFunc uintptr + persistentPreRunFunc, postRunFunc, validArgsFunc uintptr annotations map[string]string flags map[string]typedMountedFlag }if command.RunE != nil { snapshot.runFunc = reflect.ValueOf(command.RunE).Pointer() } + if command.PersistentPreRunE != nil { + snapshot.persistentPreRunFunc = reflect.ValueOf(command.PersistentPreRunE).Pointer() + } + if command.PostRunE != nil { + snapshot.postRunFunc = reflect.ValueOf(command.PostRunE).Pointer() + } + if command.ValidArgsFunction != nil { + snapshot.validArgsFunc = reflect.ValueOf(command.ValidArgsFunction).Pointer() + }Add the new fields to the comparison at line 72.
🤖 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/typed_mount_guard.go` around lines 38 - 52, Extend the snapshot and its comparison logic to capture PersistentPreRunE, PreRun, Run, PostRunE, and ValidArgsFunction alongside the existing hooks, using the same nil checks and function-pointer approach in the current snapshot code. Ensure PostMount changes to any of these hooks cause the guard to fail closed, and add the corresponding snapshot fields and comparison checks without changing the existing closure identity semantics.extension/command/command_test.go (1)
173-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWiden the import guard to subpackages and to all
shortcutspaths.The guard protects the public extension contract. Two gaps remain:
filepath.Glob("*.go")scans only this directory.extension/command/commandtestis also a published helper package and is not checked.- The check rejects
shortcuts/commonexactly. An import ofshortcuts/baseor any othershortcuts/...package would pass.♻️ Proposed stricter guard
- files, err := filepath.Glob("*.go") - if err != nil { - t.Fatal(err) - } - for _, file := range files { - if strings.HasSuffix(file, "_test.go") { - continue - } + var files []string + if err := filepath.WalkDir(".", func(path string, entry fs.DirEntry, err error) error { + if err != nil { + return err + } + if !entry.IsDir() && strings.HasSuffix(path, ".go") && !strings.HasSuffix(path, "_test.go") { + files = append(files, path) + } + return nil + }); err != nil { + t.Fatal(err) + } + for _, file := range files { parsed, err := parser.ParseFile(token.NewFileSet(), file, nil, parser.ImportsOnly) if err != nil { t.Fatal(err) } for _, spec := range parsed.Imports { importPath, err := strconv.Unquote(spec.Path.Value) if err != nil { t.Fatal(err) } - if importPath == "github.com/larksuite/cli/cmd" || importPath == "github.com/larksuite/cli/shortcuts/common" || strings.Contains(importPath, "/internal/") { + forbidden := importPath == "github.com/larksuite/cli/cmd" || + strings.HasPrefix(importPath, "github.com/larksuite/cli/cmd/") || + strings.HasPrefix(importPath, "github.com/larksuite/cli/shortcuts") || + strings.Contains(importPath, "/internal/") + if forbidden { t.Errorf("%s imports forbidden package %q", file, importPath) } } }Add
"io/fs"to the imports.🤖 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 `@extension/command/command_test.go` around lines 173 - 196, Widen TestPublicPackageHasNoForbiddenImports to recursively scan Go files under the extension/command package tree, including published subpackages such as commandtest, while continuing to skip test files. Update the forbidden-import predicate to reject every github.com/larksuite/cli/shortcuts/... path, not only shortcuts/common, while preserving the existing cmd and /internal/ checks.extension/command/commandtest/commandtest_test.go (1)
25-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
datadeclaration.The
var data struct{...}zero value is never read. The followingdata, err := ...assigns it becauseerris new. Deleting the declaration also removes the duplicated anonymous struct literal.♻️ Proposed cleanup
- var data struct { - ChatID string `json:"chat_id"` - } data, err := command.CallJSON[struct { ChatID string `json:"chat_id"` }](ctx, commandContext, request)🤖 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 `@extension/command/commandtest/commandtest_test.go` around lines 25 - 30, Remove the unused var data declaration before the command.CallJSON invocation; rely on the existing short declaration to create data and err, eliminating the duplicated anonymous struct type.extension/command/commandtest/business_commands_test.go (1)
216-222: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert domain routing, not only the compiled count.
len(compiled) != 4passes even ifCompileSetsmounts every command into the wrong domain. Assert the resulting service and command names so the set-to-domain mapping is covered.💚 Proposed stronger assertion
compiled, err := commandhost.CompileSets(sets) if err != nil { t.Fatal(err) } - if len(compiled) != 4 { - t.Fatalf("compiled commands = %d", len(compiled)) - } + got := make(map[string]string, len(compiled)) + for _, shortcut := range compiled { + got[shortcut.Command] = shortcut.Service + } + want := map[string]string{ + "+business-document-get": "docs", + "+business-chat-list": "im", + "+business-chat-inspect": "im", + "+business-task-audit": "task", + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("compiled commands = %#v, want %#v", got, want) + } }🤖 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 `@extension/command/commandtest/business_commands_test.go` around lines 216 - 222, Strengthen the assertions after commandhost.CompileSets by verifying each compiled service and command name, not just len(compiled). Cover the expected set-to-domain routing and retain the existing count check as needed.extension/command/request.go (1)
54-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the copy behavior of
cloneQueryValueand document thatBodyis not copied.Two gaps weaken the documented immutability of
Request:
cloneQueryValuecopies[]string,[]int, and[]any, but not[]int64,[]float64, or[]bool.cloneJSONValueinextension/command/host.goat lines 280-287 handles more of these types. A caller that passes[]int64keeps a live reference into the request.Bodystores the caller value by reference, andInspectRequestpasses it through unchanged. The doc comment says the method "returns a copied request", which is true for the struct but not for the body value.♻️ Proposed change
-// Body sets the JSON request body and returns a copied request. +// Body sets the JSON request body and returns a copied request. +// The body value itself is not deep-copied; do not mutate it after this call. func (r Request) Body(body any) Request { r.body = body return r }func cloneQueryValue(value any) any { switch typed := value.(type) { case []string: return append([]string(nil), typed...) case []int: return append([]int(nil), typed...) + case []int64: + return append([]int64(nil), typed...) + case []float64: + return append([]float64(nil), typed...) + case []bool: + return append([]bool(nil), typed...) case []any: return append([]any(nil), typed...) default: return value } }Also applies to: 131-142
🤖 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 `@extension/command/request.go` around lines 54 - 58, Update cloneQueryValue to deep-copy []int64, []float64, and []bool alongside its existing slice cases, matching cloneJSONValue’s supported value types. Revise the Body method comment to state that the request struct is copied but the body value is retained by reference, and preserve Body’s current assignment behavior.shortcuts/register_external_test.go (1)
13-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the external-registration copy test to cover all mutable slices.
Add assertions for
UserScopes,BotScopes, conditional scope fields,AuthTypes,Tips,Flags, andFlags[].Aliases/Input, in addition to the existing fields. This prevents regressions incommon.CloneShortcutfrom leaving registered commands aliased to contributor-owned data.🤖 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_external_test.go` around lines 13 - 30, Extend TestPrepareExternalRegistrationCopiesInput to initialize and mutate every mutable slice handled by common.CloneShortcut, including UserScopes, BotScopes, conditional scope fields, AuthTypes, Tips, Flags, and each flag’s Aliases and Input; add assertions that registered retains the original values after each source mutation, alongside the existing Scopes and Enum checks.
🤖 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/auth/login_test.go`:
- Around line 320-321: Set the LARKSUITE_CLI_CONFIG_DIR environment variable to
t.TempDir() at the start of TestAuthLoginHelpMatchesInteractiveDomains, before
calling cmdutil.TestFactory, so the test uses an isolated temporary CLI
configuration directory.
In `@cmd/build.go`:
- Around line 215-218: Update the build flow around CompileSets and
buildInternalWithConfig so external commands are retained in build-scoped state
instead of being committed through the package-global shortcuts.RegisterExternal
registry before the build succeeds. Ensure RegisterShortcutsWithContext mounts
only the current build’s commands, clears or avoids failed-build state, and add
coverage for a failed build followed by a clean Build without WithCommandSets.
In `@extension/command/commandtest/business_commands_test.go`:
- Around line 137-139: Update the Execute hook for taskAuditDefinition so the
contact:user.base:readonly ScopeBestEffort failure degrades instead of returning
the preflight error: skip owner enrichment, record the scope failure in
Failures, and return command.Partial(data) while preserving already-collected
tasks. Add a regression test covering this failed-preflight behavior, keeping
the existing ScopeBestEffort declaration unchanged.
In `@extension/command/commandtest/commandtest.go`:
- Around line 310-320: Update cloneRequestView to decode JSON with UseNumber and
return an explicit error alongside the cloned command.RequestView instead of
falling back to the original value on marshal or unmarshal failure. At both
cloneRequestView call sites, handle the returned error with r.testing.Errorf
while preserving successful cloning and number types consistently with
responseDataObject.
In `@extension/command/errors.go`:
- Around line 29-34: Update PaginationLimitError to construct the internal error
with errs.SubtypeQuotaExceeded instead of errs.SubtypeInvalidResponse,
preserving the existing pagination-limit message and resume hint.
In `@internal/commandhost/compile_test.go`:
- Around line 194-199: Update the dry-run network error assertions in the
relevant test to type-assert callErr as *errs.ValidationError and verify its
subtype is errs.SubtypeInvalidArgument, while preserving the existing message
check. Add the required errs import and leave the plain compiler error
assertions unchanged.
In `@internal/commandhost/compile.go`:
- Around line 289-308: Update queryParams and ValidateRequestView consistently
so nil query values are not silently discarded: either reject nil values during
ValidateRequestView validation or encode them as an empty query value in
queryParams. Preserve the selected behavior across all request query parameters,
including values set via GET(path).Set.
In `@shortcuts/common/runner.go`:
- Around line 1085-1091: Move the pipeline documentation comment from above
runShortcutFlagSchema to immediately above runShortcut, preserving the comment
text and leaving runShortcutFlagSchema without that stale description.
- Around line 1226-1230: Update BotInfo’s offline branch to return an error
typed as errs.SubtypeFailedPrecondition instead of a raw fmt.Errorf, while
preserving the existing nil-function guard and offline behavior. Use the
existing error-construction pattern and symbols in BotInfo for consistent
command-facing handling.
In `@shortcuts/common/typed_binder.go`:
- Around line 341-351: Guard the ArrayShape validation in the typed binder
before calling v.Len(): after unwrapping pointers, verify the reflect.Value is
valid and has an array/slice-compatible kind, matching the existing StringShape
handling. Preserve nullable typed-nil arrays without panicking and only apply
MinItems/MaxItems checks when the unwrapped value can be safely sized.
- Around line 229-238: Update the conversion logic in readPFlagValue to reject
negative signed raw values when the target type is unsigned before calling
rawValue.Convert(target). Return the existing overflow-style validation error
for this case, while preserving the current signed and unsigned overflow checks
for non-negative conversions.
In `@shortcuts/common/typed_compile_args.go`:
- Around line 150-156: Update the supplement handling in collectArgFields so
schema constraints remain available when InputField.Shape is provided. Prefer
deriving the shape with shapeForType or otherwise preserve the parsed schemaTag
constraints, and ensure mergeInputSupplement replaces the derived shape only
when no schema constraint is declared, allowing its existing conflict guard to
reject conflicting Shape supplements.
In `@shortcuts/common/typed_compile_data.go`:
- Around line 129-141: Update shapeForType’s reflect.Slice/reflect.Array branch
to reject only []byte when input is false, before deriving the element shape,
because JSON marshals it as a base64 string. Preserve [N]byte handling as an
integer array and leave other slice/array types unchanged.
In `@shortcuts/common/typed_compiler.go`:
- Around line 384-396: Align EncodingRepeated handling across validation,
legacyFlagType, flag registration, and typed binding: either reject non-string
slice/array element types in validateInputCLI, or implement matching int and
other scalar compiler types with corresponding flag registration and binder
retrieval. Preserve string-array behavior and ensure every accepted declaration
uses the same flag type throughout.
In `@shortcuts/common/typed_map_binder.go`:
- Around line 79-83: Update the unknown-parameter validation loop in typed map
binding to pass "--" + name to WithParam, matching typedFieldValidation,
typedRequiredFieldValidation, and the alias validation errors while leaving the
error message unchanged.
In `@shortcuts/common/typed_runner_test.go`:
- Around line 334-339: Update the --payload argument in the runTypedFixture test
to pass valid JSON without literal backslashes, allowing decoding to reach the
unknown-field validation for extra. Strengthen the assertion around validation
to also verify the error identifies the extra field, while preserving the
existing category, type, and parameter checks.
---
Nitpick comments:
In `@extension/command/command_test.go`:
- Around line 173-196: Widen TestPublicPackageHasNoForbiddenImports to
recursively scan Go files under the extension/command package tree, including
published subpackages such as commandtest, while continuing to skip test files.
Update the forbidden-import predicate to reject every
github.com/larksuite/cli/shortcuts/... path, not only shortcuts/common, while
preserving the existing cmd and /internal/ checks.
In `@extension/command/commandtest/business_commands_test.go`:
- Around line 216-222: Strengthen the assertions after commandhost.CompileSets
by verifying each compiled service and command name, not just len(compiled).
Cover the expected set-to-domain routing and retain the existing count check as
needed.
In `@extension/command/commandtest/commandtest_test.go`:
- Around line 25-30: Remove the unused var data declaration before the
command.CallJSON invocation; rely on the existing short declaration to create
data and err, eliminating the duplicated anonymous struct type.
In `@extension/command/request.go`:
- Around line 54-58: Update cloneQueryValue to deep-copy []int64, []float64, and
[]bool alongside its existing slice cases, matching cloneJSONValue’s supported
value types. Revise the Body method comment to state that the request struct is
copied but the body value is retained by reference, and preserve Body’s current
assignment behavior.
In `@shortcuts/common/runner.go`:
- Around line 954-960: Extend
TestTypedFlagSchemaListsAndPrintsCompositeInputsBeforeExecution with a
RelationCoOccur case, invoking --print-schema while supplying only one flag from
the group. Assert schema introspection succeeds without requiring the
co-occurring flag, covering cobra_annotation_required_if_others_set alongside
the existing RelationExactlyOne cases.
In `@shortcuts/common/typed_binder_benchmark_test.go`:
- Around line 26-33: Update BenchmarkTypedBinderDirectAssignment to assign the
constructed binderBenchmarkArgs value to a package-level sink so the direct
assignment cannot be optimized away. Preserve the existing benchmark setup and
assignment behavior while making the result observable across iterations.
In `@shortcuts/common/typed_compile_args.go`:
- Around line 74-78: The compiler diagnostics are nondeterministic because
invalid items are selected from unordered maps. In
shortcuts/common/typed_compile_args.go lines 74-78, collect and sort all
remaining supplement names, then report them together in one deterministic error
message. In shortcuts/common/typed_compile_contract.go line 153, replace the map
literal with an ordered slice of label/path pairs so artifact field validation
follows a fixed order; validateOutputHooks provides the existing sorting
convention.
In `@shortcuts/common/typed_external.go`:
- Around line 110-125: Update validateExternalFlagNamespace so its reserved
names stay synchronized with every host flag registration path, including
conditional --json and --yes flags. Prefer reusing a shared flag-name source;
otherwise add a test that compares the reserved map against all registrations
and fails when names diverge.
In `@shortcuts/common/typed_help_render_test.go`:
- Around line 75-91: Add table entries in TestConstraintTextKinds for the
at_most_one and same_value relation kinds, plus the default branch, using the
exact expected wording from typedHelpConstraintText. Add separate assertions
covering the single-parameter requires case and Text passthrough so every
remaining branch is validated.
In `@shortcuts/common/typed_map_binder_test.go`:
- Around line 72-82: Strengthen TestBindTypedMapRequiredMessageUsesLegacyCLIForm
and the “error if both” subtest by asserting the typed problem’s Category,
Subtype, and ValidationError.Param through the existing errs.ProblemOf/typed
error structures. Retain the message assertion, and also verify the underlying
cause is preserved as required by the test guidelines.
In `@shortcuts/common/typed_mount_guard.go`:
- Around line 38-52: Extend the snapshot and its comparison logic to capture
PersistentPreRunE, PreRun, Run, PostRunE, and ValidArgsFunction alongside the
existing hooks, using the same nil checks and function-pointer approach in the
current snapshot code. Ensure PostMount changes to any of these hooks cause the
guard to fail closed, and add the corresponding snapshot fields and comparison
checks without changing the existing closure identity semantics.
In `@shortcuts/common/typed_schema.go`:
- Around line 141-146: Deep-copy each ConditionalScope in the authorization copy
loop before storing it in authorizationIdentities, including independent copies
of every element’s Scopes and Params slices. Update the logic alongside the
existing ConditionalScopes copy in the command metadata handling, matching
cloneTypedHelpFacts behavior so later caller mutations cannot alter the compiled
contract.
In `@shortcuts/register_external_test.go`:
- Around line 13-30: Extend TestPrepareExternalRegistrationCopiesInput to
initialize and mutate every mutable slice handled by common.CloneShortcut,
including UserScopes, BotScopes, conditional scope fields, AuthTypes, Tips,
Flags, and each flag’s Aliases and Input; add assertions that registered retains
the original values after each source mutation, alongside the existing Scopes
and Enum checks.
🪄 Autofix
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 Plus
Run ID: e4135269-e359-4841-8c11-6d597c4359ab
⛔ Files ignored due to path filters (1)
extension/command/internal/gen/main.gois excluded by!**/gen/**
📒 Files selected for processing (64)
.github/workflows/ci.ymlcmd/auth/login.gocmd/auth/login_interactive.gocmd/auth/login_messages.gocmd/auth/login_test.gocmd/build.gocmd/command_sets_test.gocmd/platform_guards.goextension/command/command_test.goextension/command/commandtest/business_commands_test.goextension/command/commandtest/commandtest.goextension/command/commandtest/commandtest_test.goextension/command/context.goextension/command/definition.goextension/command/domain.goextension/command/domains_gen.goextension/command/dryrun.goextension/command/errors.goextension/command/generate.goextension/command/host.goextension/command/output.goextension/command/pagination.goextension/command/request.goextension/command/shape.gointernal/commandhost/compile.gointernal/commandhost/compile_test.goshortcuts/common/clone.goshortcuts/common/clone_test.goshortcuts/common/runner.goshortcuts/common/typed_api.goshortcuts/common/typed_api_test.goshortcuts/common/typed_authorization_test.goshortcuts/common/typed_binder.goshortcuts/common/typed_binder_benchmark_test.goshortcuts/common/typed_compile_args.goshortcuts/common/typed_compile_contract.goshortcuts/common/typed_compile_data.goshortcuts/common/typed_compile_output.goshortcuts/common/typed_compiler.goshortcuts/common/typed_compiler_invalid_test.goshortcuts/common/typed_compiler_test.goshortcuts/common/typed_contract.goshortcuts/common/typed_definition.goshortcuts/common/typed_external.goshortcuts/common/typed_flag_collisions.goshortcuts/common/typed_flag_collisions_test.goshortcuts/common/typed_flag_schema.goshortcuts/common/typed_flag_schema_test.goshortcuts/common/typed_help.goshortcuts/common/typed_help_render.goshortcuts/common/typed_help_render_test.goshortcuts/common/typed_map_binder.goshortcuts/common/typed_map_binder_test.goshortcuts/common/typed_mount_guard.goshortcuts/common/typed_output.goshortcuts/common/typed_result_protocol.goshortcuts/common/typed_result_protocol_test.goshortcuts/common/typed_runner.goshortcuts/common/typed_runner_test.goshortcuts/common/typed_schema.goshortcuts/common/typed_shape.goshortcuts/common/types.goshortcuts/register.goshortcuts/register_external_test.go
💤 Files with no reviewable changes (1)
- cmd/auth/login_messages.go
Comment on lines
+150
to
+156
| var shape ValueShape | ||
| if supplement, ok := supplements[flagName]; !ok || supplement.Shape == nil { | ||
| shape, err = shapeForType(valueType, schema, true) | ||
| if err != nil { | ||
| return fmt.Errorf("Args field %s (--%s): %w", field.Name, flagName, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Schema constraints are silently dropped when InputField.Shape supplements the field.
When a supplement declares Shape, collectArgFields skips shapeForType, so field.shape stays nil. mergeInputSupplement then guards with shapeHasConstraints(field.shape) at Line 207, which returns false for nil. A declaration such as schema:"optional;minLength=3" combined with InputField.Shape therefore compiles without the conflict error, and minLength never reaches the published contract.
Detect the conflict from the parsed schemaTag instead of from the derived shape, or always derive the shape and let the existing guard fire.
🐛 Proposed fix: record the parsed constraints so the merge guard can fire
var shape ValueShape
- if supplement, ok := supplements[flagName]; !ok || supplement.Shape == nil {
- shape, err = shapeForType(valueType, schema, true)
- if err != nil {
- return fmt.Errorf("Args field %s (--%s): %w", field.Name, flagName, err)
- }
- }
+ shape, err = shapeForType(valueType, schema, true)
+ if err != nil {
+ return fmt.Errorf("Args field %s (--%s): %w", field.Name, flagName, err)
+ }mergeInputSupplement then replaces the derived shape only when no schema constraint is declared, which is the documented contract.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var shape ValueShape | |
| if supplement, ok := supplements[flagName]; !ok || supplement.Shape == nil { | |
| shape, err = shapeForType(valueType, schema, true) | |
| if err != nil { | |
| return fmt.Errorf("Args field %s (--%s): %w", field.Name, flagName, err) | |
| } | |
| } | |
| var shape ValueShape | |
| shape, err = shapeForType(valueType, schema, true) | |
| if err != nil { | |
| return fmt.Errorf("Args field %s (--%s): %w", field.Name, flagName, err) | |
| } |
🤖 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/typed_compile_args.go` around lines 150 - 156, Update the
supplement handling in collectArgFields so schema constraints remain available
when InputField.Shape is provided. Prefer deriving the shape with shapeForType
or otherwise preserve the parsed schemaTag constraints, and ensure
mergeInputSupplement replaces the derived shape only when no schema constraint
is declared, allowing its existing conflict guard to reject conflicting Shape
supplements.
Comment on lines
+129
to
+141
| case reflect.Slice, reflect.Array: | ||
| if baseType == jsonRawMessageType { | ||
| return nil, fmt.Errorf("json.RawMessage requires an explicit Shape") | ||
| } | ||
| if len(schema.enum) > 0 || hasStringConstraints(schema) || hasNumberConstraints(schema) || schema.format != "" { | ||
| return nil, fmt.Errorf("array field has incompatible schema constraint") | ||
| } | ||
| elementSchema := schemaTag{required: true} | ||
| elementShape, err := shapeForType(baseType.Elem(), elementSchema, input) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("array item: %w", err) | ||
| } | ||
| shape = ArrayShape{Items: elementShape, MinItems: schema.minItems, MaxItems: schema.maxItems} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find Data structs that declare byte-slice fields reaching shapeForType.
rg -nP --type=go -C2 '\[\]byte\s+`json:' -g '!**/*_test.go'
rg -nP --type=go -C2 'json\.RawMessage\s+`json:' -g '!**/*_test.go'Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target implementation ---'
sed -n '1,190p' shortcuts/common/typed_compile_data.go
printf '%s\n' '--- shapeForType callers and Data declarations ---'
rg -n -C2 'shapeForType|type Data|Data struct|Compile.*Data|compile.*Data' --glob '*.go' .
printf '%s\n' '--- byte-slice declarations and JSON handling ---'
rg -n -C3 '\[\]byte|json\.RawMessage|MarshalJSON|UnmarshalJSON' --glob '*.go' .Repository: larksuite/cli
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
log=/tmp/coderabbit-shell-logs/shell-output-okoksn
printf '%s\n' '--- target implementation ---'
awk '/--- target implementation ---/{show=1; next}/--- shapeForType callers/{show=0}show' "$log" | sed -n '1,210p'
printf '%s\n' '--- shapeForType references ---'
rg -n -C2 'shapeForType' --glob '*.go' shortcuts/common
printf '%s\n' '--- likely Data declarations ---'
rg -n -C2 'type .*Data|Data struct|json.RawMessage|\\[\\]byte' --glob '*.go' --glob '!**/*_test.go' shortcuts/common shortcutsRepository: larksuite/cli
Length of output: 25463
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- non-test byte slices ---'
rg -n -C3 '\[\]byte' --glob '*.go' --glob '!**/*_test.go' .
printf '%s\n' '--- typed Definition instantiations ---'
rg -n -C3 'Definition\[' --glob '*.go' --glob '!**/*_test.go' shortcuts
printf '%s\n' '--- Output.Data definitions and registrations ---'
rg -n -C4 'Output\s*:|DataDefinition|Output\.Data|Data:' --glob '*.go' --glob '!**/*_test.go' shortcutsRepository: larksuite/cli
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- []byte struct-field declarations outside tests ---'
python3 - <<'PY'
import pathlib, re
field = re.compile(r'^\s*[A-Za-z_][A-Za-z0-9_]*\s+\[\]byte(?:\s+|$)')
for p in pathlib.Path('.').rglob('*.go'):
if p.name.endswith('_test.go'):
continue
for n, line in enumerate(p.read_text(errors='replace').splitlines(), 1):
if field.search(line):
print(f'{p}:{n}:{line.strip()}')
PY
printf '%s\n' '--- Definition instantiations outside tests ---'
rg -n 'Definition\[' shortcuts --glob '*.go' --glob '!**/*_test.go' | head -300
printf '%s\n' '--- Output/Data configuration outside tests ---'
rg -n 'DataDefinition|Output[[:space:]]*:|Data[[:space:]]*:' shortcuts --glob '*.go' --glob '!**/*_test.go' | head -300Repository: larksuite/cli
Length of output: 12130
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Define call sites ---'
rg -n -C3 '\b(common\.)?Define\s*\(|\bDefinition\s*\{' --glob '*.go' --glob '!**/*_test.go' .
printf '%s\n' '--- encoding/json byte slice versus byte array ---'
cat >/tmp/json-byte-shape-probe.go <<'EOF'
package main
import (
"encoding/json"
"fmt"
)
func main() {
s, _ := json.Marshal(struct{ V []byte `json:"v"` }{V: []byte{1, 2, 255}})
a, _ := json.Marshal(struct{ V [3]byte `json:"v"` }{V: [3]byte{1, 2, 255}})
fmt.Printf("slice=%s\narray=%s\n", s, a)
}
EOF
if command -v go >/dev/null 2>&1; then
go run /tmp/json-byte-shape-probe.go
else
printf '%s\n' 'go unavailable'
fiRepository: larksuite/cli
Length of output: 2021
Reject []byte fields in shapeForType
encoding/json marshals []byte as a base64 string, but this branch derives an integer-array shape. compileData passes input == false, so the existing unsigned-integer guard does not apply.
No current typed Data registration uses []byte. Add a guard for byte slices only. Do not reject [N]byte, which marshals as an integer array.
🤖 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/typed_compile_data.go` around lines 129 - 141, Update
shapeForType’s reflect.Slice/reflect.Array branch to reject only []byte when
input is false, before deriving the element shape, because JSON marshals it as a
base64 string. Preserve [N]byte handling as an integer array and leave other
slice/array types unchanged.
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
extension/command/command_test.go-101-109 (1)
101-109: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the typed error contract.
These tests only assert that an error exists. Assert the expected typed error metadata for rejected requests and dry-run calls. If an error wraps a cause, also assert cause preservation with
errors.Isorerrors.As.As per coding guidelines, “Error tests must assert typed metadata and cause preservation rather than message text alone.”
Also applies to: 162-169
🤖 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 `@extension/command/command_test.go` around lines 101 - 109, Update the rejection tests around ValidateRequestView and the dry-run calls to assert the expected typed error metadata, not merely that an error is non-nil. For wrapped errors, verify the underlying cause is preserved with errors.Is or errors.As, while keeping the existing invalid-request coverage intact.Source: Coding guidelines
shortcuts/common/typed_map_binder.go-79-83 (1)
79-83: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the
WithParamvalue consistent with the other typed errors.Line 81 passes the bare
name. Lines 43, 49 and the field helpers use the---prefixed spelling. Consumers that match onparamthen see two formats for the same concept. Use one format.♻️ Proposed change
- return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "unknown parameter %q", name).WithParam(name) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "unknown parameter %q", name).WithParam("--" + name)🤖 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/typed_map_binder.go` around lines 79 - 83, Update the unknown-parameter validation in the typed map binder to pass the same ---prefixed parameter spelling used by the other typed errors and field helpers to WithParam, while preserving the existing error message and validation behavior.shortcuts/common/typed_help_render.go-297-298 (1)
297-298: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the identity slice against an empty string.
identity.Identity[:1]panics whenIdentityis empty. The value comes from a map key incommand.metadata.Authorization.Identities, so an empty key reaches help rendering as a panic instead of a compile-time rejection. Add a guard, or confirm that the compiler rejects an emptyIdentitykey.🛡️ Proposed guard
for _, identity := range identities { - fmt.Fprintf(b, " %s:\n", strings.ToUpper(identity.Identity[:1])+identity.Identity[1:]) + label := identity.Identity + if label == "" { + continue + } + fmt.Fprintf(b, " %s:\n", strings.ToUpper(label[:1])+label[1:])🤖 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/typed_help_render.go` around lines 297 - 298, Guard the identity formatting in the loop over identities before slicing identity.Identity[:1]. Handle empty Identity values without panicking, while preserving the existing capitalization and rendering behavior for non-empty values; do not rely on compile-time validation of map keys.extension/command/host.go-266-291 (1)
266-291: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winIncomplete hand-written deep-copy switches alias values across the host boundary. The
commandpackage isolates the host from business definitions throughInspectCommand,InspectRequest, andCloneSets. That isolation depends on three separate hand-maintained type switches, each enumerating a different and incomplete set of types. Every unhandled type reaches adefaultbranch and returns the original reference, so the copy silently shares mutable state. Replace the three switches with one shared deep-copy helper so the handled set cannot drift again.
extension/command/host.go#L266-L291:cloneJSONValuecopiesmap[string]any,[]any,[]string,[]int,[]int64, and[]float64. Route thedefaultbranch through the shared helper so types such asmap[string]stringand[]boolare copied.extension/command/request.go#L131-L142:cloneQueryValuecopies only[]string,[]int, and[]any. Call the same shared helper so its coverage matchescloneJSONValue.extension/command/shape.go#L65-L73: thevalueShape()markers use value receivers, so a pointer such as*StringShapealso satisfiesValueShape.cloneValueShapeinextension/command/host.goswitches on value types only, so a pointer variant falls todefaultat Line 262 and itsEnumslice stays shared. Either dereference pointer variants incloneValueShape, or reject them during compilation.🤖 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 `@extension/command/host.go` around lines 266 - 291, Replace the separate deep-copy switches with one shared helper covering all supported mutable value types. In extension/command/host.go lines 266-291, update cloneJSONValue to use that helper for its fallback; in extension/command/request.go lines 131-142, update cloneQueryValue to use the same helper. Also update cloneValueShape in extension/command/host.go to dereference pointer variants such as *StringShape before cloning, or reject those pointers during compilation, so Enum slices cannot remain shared; the valueShape markers in extension/command/shape.go lines 65-73 require this handling and need no direct change.extension/command/commandtest/commandtest.go-310-320 (1)
310-320: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument or fix the lossy JSON round-trip in
cloneRequestView.Two behaviors need attention:
- The round-trip changes value types. A
Bodysupplied as a struct returns asmap[string]any. Numbers return asfloat64. Tests that readRequests()[i].Bodyreceive a different type than they sent.AssertDryRunMatchesis unaffected because it re-marshals both sides, but direct assertions onRequests()will fail in a confusing way.- On marshal or unmarshal failure the function returns the original value. The original shares its
Querymap with the caller, which defeats the isolation the clone provides.Record the decode error through
r.testinginstead of returning the aliased original, and state the type-normalization behavior in theRequestsdoc comment.♻️ Proposed fix
-// Requests returns copied requests in execution order. +// Requests returns copied requests in execution order. +// Values are normalized by a JSON round-trip: struct bodies become map[string]any +// and numbers become json.Number-free float64 values. func (r *Recorder) Requests() []command.RequestView {🤖 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 `@extension/command/commandtest/commandtest.go` around lines 310 - 320, Update cloneRequestView to record marshal/unmarshal failures via request.testing instead of returning the original aliased RequestView, while preserving the clone’s isolation contract. Update the Requests documentation to explicitly describe JSON round-trip type normalization, including structs becoming map[string]any and numbers becoming float64.extension/command/errors.go-30-34 (1)
30-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a dedicated internal subtype for the local page limit.
SubtypeInvalidResponseis documented for malformed JSON.SubtypeFailedPreconditionbelongs toCategoryValidationand is not appropriate withNewInternalError. Add an internal pagination-limit subtype and use it inPaginationLimitError.🤖 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 `@extension/command/errors.go` around lines 30 - 34, Define a dedicated internal error subtype for pagination limits alongside the existing error subtype declarations, then update PaginationLimitError to pass that subtype to errs.NewInternalError instead of errs.SubtypeInvalidResponse. Keep the existing message and resume hint unchanged.shortcuts/common/typed_compile_data.go-355-370 (1)
355-370: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
mutateObjectFieldgives up after the first object variant of aOneOfShape.Lines 361-368 return on the first variant that is an
ObjectShape, including when the recursive call fails. If aOneOfShapeholds two object variants and the target segment exists only in the second, the override fails withfield %q does not exist.When the segment does exist in several variants, only the first is mutated, so the variants diverge after the override.
The common
OneOf{Object, Null}shape produced by anullablestruct field has exactly one object variant and is unaffected.🐛 Proposed fix to apply the override to every object variant
case OneOfShape: + applied := false for variantIndex, variant := range nested.Variants { - if nestedObject, ok := variant.(ObjectShape); ok { - err := mutateObjectField(&nestedObject, parts[1:], mutate) - nested.Variants[variantIndex] = nestedObject - field.Shape = nested - return err + nestedObject, ok := variant.(ObjectShape) + if !ok { + continue } + if err := mutateObjectField(&nestedObject, parts[1:], mutate); err != nil { + continue + } + nested.Variants[variantIndex] = nestedObject + applied = true + } + if applied { + field.Shape = nested + 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/common/typed_compile_data.go` around lines 355 - 370, Update mutateObjectField’s OneOfShape handling to attempt the recursive mutation on every ObjectShape variant rather than returning after the first one. Preserve successful mutations across all matching variants, and only return the existing missing-field error when no object variant contains the target segment.shortcuts/common/typed_binder.go-291-299 (1)
291-299: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe
OneOfShapefast path can reject a value that the authoritative check accepts.Lines 292-299 select the first non-null variant and then apply that variant's constraints at lines 300-352. The authoritative check at line 361 uses the full
field.shapeand accepts any matching variant.For a
OneOfShapewhose variants share a JSON type but differ in constraints, for example twoStringShapevariants with differentEnumsets, the fast path applies only the first variant. A value valid under the second variant is rejected at line 317 before line 361 runs.The kind guards at lines 306 and 322 prevent this for variants of different JSON types, so only the same-type case is affected.
Consider skipping the fast path when the shape is a
OneOfShapeand relying on the JSON validation at line 361.🐛 Proposed fix to skip the fast path for multi-variant oneOf
shape := field.shape if one, ok := shape.(OneOfShape); ok { + // Narrowing is only safe when exactly one non-null variant exists. + // Otherwise the fast path would apply one variant's constraints to a + // value that legitimately matches another variant. + nonNull := 0 for _, variant := range one.Variants { if _, null := variant.(NullShape); !null { - shape = variant - break + nonNull++ + shape = variant } } + if nonNull != 1 { + shape = nil + } }
shape = nilfalls through the type switch at line 300 to the JSON validation at line 353.🤖 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/typed_binder.go` around lines 291 - 299, Skip the specialized fast path in the binder when field.shape is a OneOfShape, leaving shape unset so validation reaches the authoritative full-shape check near the existing JSON validation. Update the shape-selection logic around OneOfShape and preserve the current fast paths for non-OneOfShape fields.shortcuts/common/typed_binder.go-229-237 (1)
229-237: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSigned-to-unsigned conversion has no negativity guard.
Line 231 checks overflow only when both the source and the target are signed. Line 234 checks only when both are unsigned. A signed source converted to an unsigned target passes both checks and reaches
rawValue.Convert(target)at line 237.
readPFlagValuereads every integer kind throughGetIntat line 136, so the source is always a signedint. For an unsigned target,-1converts silently to the maximum unsigned value instead of failing.
shapeForTyperejects unsigned CLI input, butcollectArgFieldsskipsshapeForTypewhen anInputField.Shapesupplement is present, so an unsigned field with an explicit shape reaches this path.🐛 Proposed fix to reject a negative source for an unsigned target
if rawValue.Type().ConvertibleTo(target) { converted := reflect.New(target).Elem() if isSignedIntegerKind(rawValue.Kind()) && isSignedIntegerKind(target.Kind()) && converted.OverflowInt(rawValue.Int()) { return nil, fmt.Errorf("%v overflows %s", raw, target) } + if isSignedIntegerKind(rawValue.Kind()) && isUnsignedIntegerKind(target.Kind()) { + if rawValue.Int() < 0 || converted.OverflowUint(uint64(rawValue.Int())) { + return nil, fmt.Errorf("%v is out of range for %s", raw, target) + } + } if isUnsignedIntegerKind(rawValue.Kind()) && isUnsignedIntegerKind(target.Kind()) && converted.OverflowUint(rawValue.Uint()) { return nil, fmt.Errorf("%v overflows %s", raw, target) } return rawValue.Convert(target).Interface(), 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/common/typed_binder.go` around lines 229 - 237, Update the conversion checks in the reflected-value conversion path to reject negative signed sources when target.Kind() is unsigned, before rawValue.Convert(target) runs. Preserve the existing signed-to-signed and unsigned-to-unsigned overflow validation and return the established overflow-style error for invalid negative values.shortcuts/common/runner.go-1085-1088 (1)
1085-1088: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe doc comment describes
runShortcut, notrunShortcutFlagSchema.The comment at lines 1085-1087 documents the identity → config → scopes → runtime → validation → execute pipeline. That describes
runShortcut, which now starts at line 1112 and has no doc comment.runShortcutFlagSchemaonly handles--print-schema.📝 Proposed fix to move the comment
-// runShortcut is the execution pipeline for a declarative shortcut. -// Each step is a clear phase: identity → config → scopes → runtime → -// canonical validation → execute. +// runShortcutFlagSchema handles the local --print-schema introspection path. +// It reports whether the invocation was handled, so callers skip execution. func runShortcutFlagSchema(cmd *cobra.Command, f *cmdutil.Factory, s *Shortcut) (bool, error) {Then restore the original comment above
runShortcutat line 1112:// runShortcut is the execution pipeline for a declarative shortcut. // Each step is a clear phase: identity → config → scopes → runtime → // canonical validation → execute. func runShortcut(cmd *cobra.Command, f *cmdutil.Factory, s *Shortcut, botOnly bool) error {🤖 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/runner.go` around lines 1085 - 1088, Move the execution-pipeline doc comment from runShortcutFlagSchema to runShortcut, which is the function it describes. Leave runShortcutFlagSchema documented only as appropriate for its --print-schema behavior, and restore the full comment immediately above runShortcut.shortcuts/common/typed_compile_args.go-291-311 (1)
291-311: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDead duplicate check, and
encoding=repeatedaccepts pointer elements.Two issues in this block:
- Lines 297-301 are unreachable. Lines 291-296 already reject every complex kind that has an empty encoding. The
case ""arm repeats the identical condition and returns a second, different message. Remove it.- Line 306 tests
indirectType(field.valueType).Elem().Kind()forStructandMap. For[]*Foothe element kind isPointer, so the check passes.readPFlagValuethen reads the flag as[]stringat line 142, andconvertReflectValuefalls through tojson.Unmarshalof a bare string into*Foo. The author sees a decode error at command execution time instead of a registration error.🐛 Proposed fix
switch field.cli.Encoding { - case "": - if kind == reflect.Slice || kind == reflect.Array || kind == reflect.Struct || kind == reflect.Map || kind == reflect.Interface { - return fmt.Errorf("complex input requires encoding") - } + case "": case EncodingRepeated: if kind != reflect.Slice && kind != reflect.Array { return fmt.Errorf("encoding repeated requires an array or slice") } - if indirectType(field.valueType).Elem().Kind() == reflect.Struct || indirectType(field.valueType).Elem().Kind() == reflect.Map { + switch indirectType(indirectType(field.valueType).Elem()).Kind() { + case reflect.Struct, reflect.Map, reflect.Slice, reflect.Array, reflect.Interface: return fmt.Errorf("encoding repeated only supports scalar arrays") }Apply the same
indirectTypeunwrap to the element kind at line 316 so[]*intis classified as an integer array rather than falling through tostring_slice.🤖 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/typed_compile_args.go` around lines 291 - 311, Remove the unreachable duplicate complex-kind validation in the empty-encoding case, keeping the earlier explicit-encoding error. In the EncodingRepeated validation, unwrap pointer elements before checking their kind so pointer-to-struct and pointer-to-map elements are rejected during registration, while scalar pointer elements retain the correct array classification.shortcuts/register.go-104-108 (1)
104-108: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winAvoid rebuilding the full shortcut clone on each runtime lookup. Non-test paths call
AllShortcuts, including authentication scope resolution and command-set compilation.CloneShortcutsclones every shortcut and rebuilds each typed schema contract. Preserve the copy-on-read behavior while caching derived immutable data and invalidating it afterRegisterExternalchanges the registry.🤖 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 104 - 108, Update AllShortcuts to preserve copy-on-read semantics without rebuilding every shortcut and typed schema contract on each lookup: cache the derived immutable shortcut data, return a fresh copy from that cache, and invalidate or rebuild the cache whenever RegisterExternal mutates the registry. Keep registry access synchronized with shortcutRegistryMu and ensure subsequent lookups reflect newly registered shortcuts.
🧹 Nitpick comments (11)
shortcuts/common/typed_result_protocol.go (1)
251-253: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign the subtype used for result-protocol violations.
resultProtocolErroruseserrs.SubtypeUnknown, but lines 30 and 34 report the same class of failure witherrs.SubtypeInvalidResponse. Both describe a typed result that does not match its declared output contract. Use one subtype so consumers can classify these failures.🤖 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/typed_result_protocol.go` around lines 251 - 253, Update resultProtocolError to use errs.SubtypeInvalidResponse instead of errs.SubtypeUnknown, matching the result-protocol violations reported at lines 30 and 34 so consumers classify all typed output contract mismatches consistently.shortcuts/common/typed_help_render.go (1)
415-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
Relation*constants for every case.The switch mixes string literals (
"at_most_one","exactly_one") withstring(RelationCoOccur). A rename of a relation constant then silently falls through to thedefaultbranch. Usestring(RelationAtMostOne)and the other constants for all cases.🤖 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/typed_help_render.go` around lines 415 - 436, Update the switch in the fact-rendering logic to use string conversions of the corresponding Relation* constants for every relation case, including at_most_one, exactly_one, at_least_one, requires, conflicts, and same_value. Keep the existing case behavior unchanged while replacing the string literals so renamed constants cannot fall through to default.extension/command/shape.go (1)
48-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument how
AdditionalPropertiesandAdditionalPropertiesShapeinteract.
ObjectShapeexposes both a boolean and a shape for additional properties. The declaration does not state the resolution rules. A business author cannot tell from this file:
- Does a non-nil
AdditionalPropertiesShapeimply that additional properties are allowed, even whenAdditionalPropertiesis false?- Is
AdditionalProperties: falsewith a non-nilAdditionalPropertiesShapea contradiction, and does the compiler reject it?This is an exported contract in
extension/. Add doc comments that state the rules the compiler enforces.As per coding guidelines: "Treat exported plugin or host-integration symbols as compatibility commitments and keep orchestration internal."
🤖 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 `@extension/command/shape.go` around lines 48 - 52, The exported ObjectShape fields AdditionalProperties and AdditionalPropertiesShape lack documentation describing their compiler-enforced interaction. Add doc comments to both fields stating whether a non-nil AdditionalPropertiesShape enables additional properties, how AdditionalProperties takes precedence, and whether the false-plus-shape combination is rejected or otherwise resolved; document the actual existing compiler behavior without changing implementation logic.Source: Coding guidelines
extension/command/commandtest/commandtest.go (1)
104-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
"partial"string literal with an exported outcome constant.
result.Outcomeis produced from the unexportedoutcomeKindconstants inextension/command/output.go.HostResult.Outcomeis an exported plainstring, and the package exports no constants for its values. This helper therefore hardcodes a copy of a private value across a package boundary. If the outcome vocabulary changes, this comparison compiles and silently returnsPartial: false.Export the outcome values from the
commandpackage and use them here and in host adapters.♻️ Suggested contract addition in `extension/command/output.go`
// OutcomeSuccess and OutcomePartial are the HostResult.Outcome values. const ( OutcomeSuccess = string(outcomeSuccess) OutcomePartial = string(outcomePartial) )Then in
extension/command/commandtest/commandtest.go:- return Execution[Data]{Data: data, Partial: result.Outcome == "partial"}, nil + return Execution[Data]{Data: data, Partial: result.Outcome == command.OutcomePartial}, nilAs per coding guidelines: "Treat exported plugin or host-integration symbols as compatibility commitments and keep orchestration internal."
🤖 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 `@extension/command/commandtest/commandtest.go` around lines 104 - 108, Export stable outcome constants from the command package in output.go, including OutcomeSuccess and OutcomePartial derived from the existing unexported outcome kinds. Update commandtest and all host adapters to compare HostResult.Outcome against OutcomePartial instead of the "partial" literal, preserving the existing partial-result behavior.Source: Coding guidelines
shortcuts/common/typed_external.go (1)
111-125: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe reserved flag list duplicates
registerShortcutFlagsWithContextand can drift.This map hardcodes the host flag names.
registerShortcutFlagsWithContextinshortcuts/common/runner.goregisters them at lines 1565-1588, plusensureJSONShorthandandcmdutil.AddShortcutIdentityFlag. The two lists currently agree.If a future host flag is added to the runner and not added here, an external command can declare the same flag name. pflag panics on a duplicate registration, so the failure appears at mount time as a process abort rather than as a compile error from
CompileSets.Add a regression test that mounts a minimal shortcut, enumerates the resulting flag set, and asserts every host-registered flag name appears in this map.
#!/bin/bash # Description: List every flag the host registers so the reserved map can be compared. set -euo pipefail ast-grep run --pattern 'func registerShortcutFlagsWithContext($$$) { $$$ }' --lang go shortcuts/common/runner.go ast-grep run --pattern 'func ensureJSONShorthand($$$) { $$$ }' --lang go shortcuts/common/ rg -n --type=go -C 3 'func AddShortcutIdentityFlag' internal/cmdutil/🤖 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/typed_external.go` around lines 111 - 125, Add a regression test around the reserved map and shortcut mounting that builds a minimal shortcut, enumerates the resulting host flag set, and asserts every host-registered flag name from registerShortcutFlagsWithContext, ensureJSONShorthand, and cmdutil.AddShortcutIdentityFlag exists in reserved. Keep the test focused on detecting omissions so newly added host flags cannot be exposed for external commands.shortcuts/common/typed_compile_args.go (2)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe hardcoded extension package path can drift silently.
Line 24 hardcodes
"github.com/larksuite/cli/extension/command"and line 185 comparest.PkgPath()against it. Line 22 derives the internal path withreflect.TypeFor, which survives a move or rename. The external constant does not.If the package moves or the module path changes,
unwrapProvidedstops recognizing the publicProvided[T]type. It then returns the wrapper struct as the value type, andvalidateInputCLIfails at lines 292-296 with "struct input must explicitly declare CLI encoding". That message points at the wrong cause.Importing
extension/commandfromshortcuts/commonwould invert the dependency direction, so the constant is reasonable here. Add an assertion in a package that already imports both, for exampleinternal/commandhost, so a path change fails a test instead of degrading a diagnostic.♻️ Proposed guard in a package that imports both
// internal/commandhost/compile_test.go func TestProvidedPackagePathMatchesCompilerConstant(t *testing.T) { got := reflect.TypeFor[command.Provided[string]]().PkgPath() if got != "github.com/larksuite/cli/extension/command" { t.Fatalf("extension/command package path changed to %q; update extensionCommandPkgPath in shortcuts/common/typed_compile_args.go", got) } }Also applies to: 185-185
🤖 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/typed_compile_args.go` at line 24, The hardcoded extension package path used by unwrapProvided must be guarded against drift. Add a test in internal/commandhost, such as TestProvidedPackagePathMatchesCompilerConstant, that obtains the package path from command.Provided via reflect.TypeFor and fails with an actionable message when it differs from extensionCommandPkgPath; keep the existing constant and dependency direction unchanged.
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe unknown-flag diagnostic is nondeterministic.
The loop returns on its first iteration. Go randomizes map iteration order, so when
Input.Fieldsreferences several unknown flags, the reported name changes between runs. A test that asserts this message is flaky, and a developer sees a different error after each build.♻️ Proposed fix to report all unknown flags in a stable order
if len(supplements) > 0 { - for name := range supplements { - return nil, nil, fmt.Errorf("Input.Fields references unknown flag --%s", name) - } + unknown := make([]string, 0, len(supplements)) + for name := range supplements { + unknown = append(unknown, "--"+name) + } + sort.Strings(unknown) + return nil, nil, fmt.Errorf("Input.Fields references unknown flags %s", strings.Join(unknown, ", ")) }Add
"sort"to the import block.🤖 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/typed_compile_args.go` around lines 74 - 78, Update the unknown-flag handling around supplements to collect all unknown flag names, sort them deterministically with sort.Strings, and return one stable diagnostic containing the sorted names instead of returning from the first map iteration.shortcuts/common/typed_compile_data.go (2)
236-256: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEnum entries are not checked against the other constraints.
StringShapevalidatesMinLengthagainstMaxLengthbut never checks that eachEnumentry satisfies them.IntegerShapeandNumberShapehave the same gap forMinimumandMaximum.A declaration such as
StringShape{Enum: []string{"ab"}, MinLength: ptr(5)}compiles. Every value is then rejected at command execution time, becausevalidateJSONValueAgainstShapeinshortcuts/common/typed_binder.goapplies both rules. The author error surfaces as a user-facing failure instead of a registration failure.♻️ Proposed additions
case StringShape: if value.MinLength != nil && *value.MinLength < 0 || value.MaxLength != nil && *value.MaxLength < 0 { return fmt.Errorf("%s string lengths must be nonnegative", path) } if value.MinLength != nil && value.MaxLength != nil && *value.MinLength > *value.MaxLength { return fmt.Errorf("%s minLength exceeds maxLength", path) } + for _, entry := range value.Enum { + length := len([]rune(entry)) + if value.MinLength != nil && length < *value.MinLength || + value.MaxLength != nil && length > *value.MaxLength { + return fmt.Errorf("%s enum value %q violates its own length constraints", path, entry) + } + } case BooleanShape: case IntegerShape: if value.Minimum != nil && value.Maximum != nil && *value.Minimum > *value.Maximum { return fmt.Errorf("%s minimum exceeds maximum", path) } + for _, entry := range value.Enum { + if value.Minimum != nil && entry < *value.Minimum || + value.Maximum != nil && entry > *value.Maximum { + return fmt.Errorf("%s enum value %d violates its own range constraints", path, entry) + } + }🤖 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/typed_compile_data.go` around lines 236 - 256, Update the shape validation switch in the typed compile-data validation function to verify every StringShape.Enum entry satisfies MinLength and MaxLength, and every IntegerShape and NumberShape.Enum entry satisfies Minimum and Maximum. Reject declarations when any enum value violates a configured bound, while preserving the existing finite-number and min/max relationship checks.
217-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
inputguard makes the description error misleading.Line 218 requires a description only when
inputis true. ButvalidateShapeat lines 284-286 requires a non-blankDescriptionfor everyObjectShapefield, andcompileDataruns it at line 54 for Data shapes.So a Data struct field without a
doctag is still rejected, just later and with a path-based message such asOutput.Data field "x" Description is requiredinstead of the field-level message at line 219 that names the Go field.Drop the guard so both cases report the clearer message.
♻️ Proposed fix
description := strings.TrimSpace(field.Tag.Get("doc")) - if input && description == "" { + if description == "" { return ObjectShape{}, fmt.Errorf("%s field %s (%s): description is required via doc", path, field.Name, name) }🤖 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/typed_compile_data.go` around lines 217 - 220, Update the description validation in the shape-compilation function containing the `field.Tag.Get("doc")` call to reject blank descriptions unconditionally, removing the `input` guard. Preserve the existing field-level error message so both input and data fields identify the Go field and name.shortcuts/common/runner.go (1)
954-960: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the schema regression test to cover required-together groups.
TestTypedFlagSchemaListsAndPrintsCompositeInputsBeforeExecutionalready coversMarkFlagsOneRequiredandMarkFlagsMutuallyExclusivethroughRelationExactlyOne. Add aRelationCoOccurcase to protectMarkFlagsRequiredTogetherfrom Cobra annotation changes.🤖 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/runner.go` around lines 954 - 960, Extend TestTypedFlagSchemaListsAndPrintsCompositeInputsBeforeExecution with a RelationCoOccur case that exercises MarkFlagsRequiredTogether, verifying schema inspection still lists and prints composite inputs before execution despite Cobra required-together annotations.internal/commandhost/compile.go (1)
44-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate intermediate
CompileSetserrors.
cmd/build.gopasses these errors toinstallCommandSetErrorGuard, which converts them to typed errors and preserves the cause. Add a reasoned//nolint:forbidigodirective to each intermediatefmt.Errorfreturn ininternal/commandhost/compile.go, matchingshortcuts/register.go.🤖 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 `@internal/commandhost/compile.go` around lines 44 - 63, The intermediate error-wrapping returns in CompileSets need lint annotations before installCommandSetErrorGuard converts them to typed errors. Add reasoned //nolint:forbidigo directives to each relevant fmt.Errorf return in internal/commandhost/compile.go, matching the annotation style used by shortcuts/register.go.Sources: Coding guidelines, Pipeline 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.
Inline comments:
In `@extension/command/host.go`:
- Around line 76-96: Update the erased hook wrappers in the host adapter to use
comma-ok assertions for args.(*Args) instead of panicking assertions. For
Normalize, Validate, and Execute, return the appropriate typed errs.* error on
mismatch; for DryRun, return nil. Also update the renderer wrapper’s data.(Data)
assertion to return a typed errs.* error on mismatch, preserving normal behavior
for valid inputs.
In `@internal/commandhost/compile.go`:
- Around line 289-308: Update queryParams to detect typed nil values, including
nil pointers, with reflection and skip them instead of converting them to
"<nil>". Apply the same nil filtering and dereferencing to elements in []any
before formatting them, while preserving existing handling for nil interfaces,
[]string, and ordinary scalar values.
In `@shortcuts/common/runner.go`:
- Around line 1031-1049: Update resolveDryRunIdentity to reuse the production
identity-resolution logic, including credential hints, strict mode, and
IdentityAutoDetected handling, instead of duplicating fallback selection and
unconditionally clearing the flag. Preserve dry-run’s offline behavior while
ensuring it produces the same resolved identity and warnings as execution.
In `@shortcuts/common/typed_binder.go`:
- Around line 77-121: Update the independent-alias handling loop in the typed
binder to compare each alias against the running accepted state (`value`/`set`),
not only `canonicalRaw`/`canonicalSet`. Ensure `AliasErrorIfBoth`,
`AliasCanonicalWins`, and `AliasTrimmedEqualOrError` consistently detect or
reconcile conflicts between previously accepted aliases and the current alias,
while preserving canonical-value precedence and existing error behavior.
In `@shortcuts/common/typed_compile_args.go`:
- Line 4: Determine the suppression syntax recognized by the deterministic
no_bare_helper_error gate, then add it beside the existing justified
//nolint:forbidigo directive in shortcuts/common/typed_compile_args.go:4,
shortcuts/common/typed_compile_data.go:4, shortcuts/common/typed_compiler.go:4,
and shortcuts/common/typed_external.go:4. Preserve the current justification
text and ensure the suppression covers the reported helpers used by Define and
CompileErasedDefinition; alternatively, replace those helpers with typed errs.*
errors while preserving causes.
- Around line 206-218: Preserve parsed schema-tag constraints when compiling
fields and use them for the conflict check. In collectArgFields, store the
parsed schema value on the compiledInputField via schemaConstraints, then update
the InputField.Shape handling block to call a schemaTagHasConstraints helper
that checks enum, format, string, number, and item constraints instead of
shapeHasConstraints(field.shape); retain nullable validation and existing shape
validation.
In `@shortcuts/common/typed_compile_contract.go`:
- Around line 208-255: Update shapeAsObject, unwrapArray, and shapeHasType in
shortcuts/common/typed_compile_contract.go (lines 208-255) so every applicable
OneOfShape variant satisfies the requested object traversal, array traversal, or
field-type contract; do not accept only the first compatible variant, and
preserve order-independent behavior. Add a nearby regression case in
shortcuts/common/typed_compiler_invalid_test.go (lines 191-206) containing one
valid and one incompatible variant, verifying compilation fails regardless of
variant order.
In `@shortcuts/common/typed_compiler.go`:
- Around line 384-397: Update the repeated-encoding validation and type mapping
so EncodingRepeated cannot compile for non-string slice or array elements. In
validateInputCLI, reject non-string element kinds with the established
validation error pattern, preserving string-array support; ensure the related
compiler path does not fall through to the plain "string" flag type.
In `@shortcuts/common/typed_external.go`:
- Around line 50-53: Wrap the external definition.Hooks.NewArgs invocation in a
panic-recovering helper such as probeNewArgs, converting any recovered panic
into an error returned by CompileErasedDefinition. Update the surrounding
compilation flow to handle that error before type validation, preserving the
existing invalid-type error behavior and avoiding panic propagation from the
erased path.
---
Minor comments:
In `@extension/command/command_test.go`:
- Around line 101-109: Update the rejection tests around ValidateRequestView and
the dry-run calls to assert the expected typed error metadata, not merely that
an error is non-nil. For wrapped errors, verify the underlying cause is
preserved with errors.Is or errors.As, while keeping the existing
invalid-request coverage intact.
In `@extension/command/commandtest/commandtest.go`:
- Around line 310-320: Update cloneRequestView to record marshal/unmarshal
failures via request.testing instead of returning the original aliased
RequestView, while preserving the clone’s isolation contract. Update the
Requests documentation to explicitly describe JSON round-trip type
normalization, including structs becoming map[string]any and numbers becoming
float64.
In `@extension/command/errors.go`:
- Around line 30-34: Define a dedicated internal error subtype for pagination
limits alongside the existing error subtype declarations, then update
PaginationLimitError to pass that subtype to errs.NewInternalError instead of
errs.SubtypeInvalidResponse. Keep the existing message and resume hint
unchanged.
In `@extension/command/host.go`:
- Around line 266-291: Replace the separate deep-copy switches with one shared
helper covering all supported mutable value types. In extension/command/host.go
lines 266-291, update cloneJSONValue to use that helper for its fallback; in
extension/command/request.go lines 131-142, update cloneQueryValue to use the
same helper. Also update cloneValueShape in extension/command/host.go to
dereference pointer variants such as *StringShape before cloning, or reject
those pointers during compilation, so Enum slices cannot remain shared; the
valueShape markers in extension/command/shape.go lines 65-73 require this
handling and need no direct change.
In `@shortcuts/common/runner.go`:
- Around line 1085-1088: Move the execution-pipeline doc comment from
runShortcutFlagSchema to runShortcut, which is the function it describes. Leave
runShortcutFlagSchema documented only as appropriate for its --print-schema
behavior, and restore the full comment immediately above runShortcut.
In `@shortcuts/common/typed_binder.go`:
- Around line 291-299: Skip the specialized fast path in the binder when
field.shape is a OneOfShape, leaving shape unset so validation reaches the
authoritative full-shape check near the existing JSON validation. Update the
shape-selection logic around OneOfShape and preserve the current fast paths for
non-OneOfShape fields.
- Around line 229-237: Update the conversion checks in the reflected-value
conversion path to reject negative signed sources when target.Kind() is
unsigned, before rawValue.Convert(target) runs. Preserve the existing
signed-to-signed and unsigned-to-unsigned overflow validation and return the
established overflow-style error for invalid negative values.
In `@shortcuts/common/typed_compile_args.go`:
- Around line 291-311: Remove the unreachable duplicate complex-kind validation
in the empty-encoding case, keeping the earlier explicit-encoding error. In the
EncodingRepeated validation, unwrap pointer elements before checking their kind
so pointer-to-struct and pointer-to-map elements are rejected during
registration, while scalar pointer elements retain the correct array
classification.
In `@shortcuts/common/typed_compile_data.go`:
- Around line 355-370: Update mutateObjectField’s OneOfShape handling to attempt
the recursive mutation on every ObjectShape variant rather than returning after
the first one. Preserve successful mutations across all matching variants, and
only return the existing missing-field error when no object variant contains the
target segment.
In `@shortcuts/common/typed_help_render.go`:
- Around line 297-298: Guard the identity formatting in the loop over identities
before slicing identity.Identity[:1]. Handle empty Identity values without
panicking, while preserving the existing capitalization and rendering behavior
for non-empty values; do not rely on compile-time validation of map keys.
In `@shortcuts/common/typed_map_binder.go`:
- Around line 79-83: Update the unknown-parameter validation in the typed map
binder to pass the same ---prefixed parameter spelling used by the other typed
errors and field helpers to WithParam, while preserving the existing error
message and validation behavior.
In `@shortcuts/register.go`:
- Around line 104-108: Update AllShortcuts to preserve copy-on-read semantics
without rebuilding every shortcut and typed schema contract on each lookup:
cache the derived immutable shortcut data, return a fresh copy from that cache,
and invalidate or rebuild the cache whenever RegisterExternal mutates the
registry. Keep registry access synchronized with shortcutRegistryMu and ensure
subsequent lookups reflect newly registered shortcuts.
---
Nitpick comments:
In `@extension/command/commandtest/commandtest.go`:
- Around line 104-108: Export stable outcome constants from the command package
in output.go, including OutcomeSuccess and OutcomePartial derived from the
existing unexported outcome kinds. Update commandtest and all host adapters to
compare HostResult.Outcome against OutcomePartial instead of the "partial"
literal, preserving the existing partial-result behavior.
In `@extension/command/shape.go`:
- Around line 48-52: The exported ObjectShape fields AdditionalProperties and
AdditionalPropertiesShape lack documentation describing their compiler-enforced
interaction. Add doc comments to both fields stating whether a non-nil
AdditionalPropertiesShape enables additional properties, how
AdditionalProperties takes precedence, and whether the false-plus-shape
combination is rejected or otherwise resolved; document the actual existing
compiler behavior without changing implementation logic.
In `@internal/commandhost/compile.go`:
- Around line 44-63: The intermediate error-wrapping returns in CompileSets need
lint annotations before installCommandSetErrorGuard converts them to typed
errors. Add reasoned //nolint:forbidigo directives to each relevant fmt.Errorf
return in internal/commandhost/compile.go, matching the annotation style used by
shortcuts/register.go.
In `@shortcuts/common/runner.go`:
- Around line 954-960: Extend
TestTypedFlagSchemaListsAndPrintsCompositeInputsBeforeExecution with a
RelationCoOccur case that exercises MarkFlagsRequiredTogether, verifying schema
inspection still lists and prints composite inputs before execution despite
Cobra required-together annotations.
In `@shortcuts/common/typed_compile_args.go`:
- Line 24: The hardcoded extension package path used by unwrapProvided must be
guarded against drift. Add a test in internal/commandhost, such as
TestProvidedPackagePathMatchesCompilerConstant, that obtains the package path
from command.Provided via reflect.TypeFor and fails with an actionable message
when it differs from extensionCommandPkgPath; keep the existing constant and
dependency direction unchanged.
- Around line 74-78: Update the unknown-flag handling around supplements to
collect all unknown flag names, sort them deterministically with sort.Strings,
and return one stable diagnostic containing the sorted names instead of
returning from the first map iteration.
In `@shortcuts/common/typed_compile_data.go`:
- Around line 236-256: Update the shape validation switch in the typed
compile-data validation function to verify every StringShape.Enum entry
satisfies MinLength and MaxLength, and every IntegerShape and NumberShape.Enum
entry satisfies Minimum and Maximum. Reject declarations when any enum value
violates a configured bound, while preserving the existing finite-number and
min/max relationship checks.
- Around line 217-220: Update the description validation in the
shape-compilation function containing the `field.Tag.Get("doc")` call to reject
blank descriptions unconditionally, removing the `input` guard. Preserve the
existing field-level error message so both input and data fields identify the Go
field and name.
In `@shortcuts/common/typed_external.go`:
- Around line 111-125: Add a regression test around the reserved map and
shortcut mounting that builds a minimal shortcut, enumerates the resulting host
flag set, and asserts every host-registered flag name from
registerShortcutFlagsWithContext, ensureJSONShorthand, and
cmdutil.AddShortcutIdentityFlag exists in reserved. Keep the test focused on
detecting omissions so newly added host flags cannot be exposed for external
commands.
In `@shortcuts/common/typed_help_render.go`:
- Around line 415-436: Update the switch in the fact-rendering logic to use
string conversions of the corresponding Relation* constants for every relation
case, including at_most_one, exactly_one, at_least_one, requires, conflicts, and
same_value. Keep the existing case behavior unchanged while replacing the string
literals so renamed constants cannot fall through to default.
In `@shortcuts/common/typed_result_protocol.go`:
- Around line 251-253: Update resultProtocolError to use
errs.SubtypeInvalidResponse instead of errs.SubtypeUnknown, matching the
result-protocol violations reported at lines 30 and 34 so consumers classify all
typed output contract mismatches consistently.
🪄 Autofix
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 Plus
Run ID: e4135269-e359-4841-8c11-6d597c4359ab
⛔ Files ignored due to path filters (1)
extension/command/internal/gen/main.gois excluded by!**/gen/**
📒 Files selected for processing (64)
.github/workflows/ci.ymlcmd/auth/login.gocmd/auth/login_interactive.gocmd/auth/login_messages.gocmd/auth/login_test.gocmd/build.gocmd/command_sets_test.gocmd/platform_guards.goextension/command/command_test.goextension/command/commandtest/business_commands_test.goextension/command/commandtest/commandtest.goextension/command/commandtest/commandtest_test.goextension/command/context.goextension/command/definition.goextension/command/domain.goextension/command/domains_gen.goextension/command/dryrun.goextension/command/errors.goextension/command/generate.goextension/command/host.goextension/command/output.goextension/command/pagination.goextension/command/request.goextension/command/shape.gointernal/commandhost/compile.gointernal/commandhost/compile_test.goshortcuts/common/clone.goshortcuts/common/clone_test.goshortcuts/common/runner.goshortcuts/common/typed_api.goshortcuts/common/typed_api_test.goshortcuts/common/typed_authorization_test.goshortcuts/common/typed_binder.goshortcuts/common/typed_binder_benchmark_test.goshortcuts/common/typed_compile_args.goshortcuts/common/typed_compile_contract.goshortcuts/common/typed_compile_data.goshortcuts/common/typed_compile_output.goshortcuts/common/typed_compiler.goshortcuts/common/typed_compiler_invalid_test.goshortcuts/common/typed_compiler_test.goshortcuts/common/typed_contract.goshortcuts/common/typed_definition.goshortcuts/common/typed_external.goshortcuts/common/typed_flag_collisions.goshortcuts/common/typed_flag_collisions_test.goshortcuts/common/typed_flag_schema.goshortcuts/common/typed_flag_schema_test.goshortcuts/common/typed_help.goshortcuts/common/typed_help_render.goshortcuts/common/typed_help_render_test.goshortcuts/common/typed_map_binder.goshortcuts/common/typed_map_binder_test.goshortcuts/common/typed_mount_guard.goshortcuts/common/typed_output.goshortcuts/common/typed_result_protocol.goshortcuts/common/typed_result_protocol_test.goshortcuts/common/typed_runner.goshortcuts/common/typed_runner_test.goshortcuts/common/typed_schema.goshortcuts/common/typed_shape.goshortcuts/common/types.goshortcuts/register.goshortcuts/register_external_test.go
💤 Files with no reviewable changes (1)
- cmd/auth/login_messages.go
🚧 Files skipped from review as they are similar to previous changes (29)
- extension/command/generate.go
- cmd/auth/login_interactive.go
- shortcuts/common/typed_compile_output.go
- shortcuts/common/typed_flag_collisions.go
- shortcuts/common/typed_api_test.go
- shortcuts/common/typed_help_render_test.go
- extension/command/domains_gen.go
- shortcuts/common/typed_flag_schema.go
- .github/workflows/ci.yml
- shortcuts/common/typed_runner_test.go
- internal/commandhost/compile_test.go
- shortcuts/common/typed_authorization_test.go
- shortcuts/common/typed_flag_collisions_test.go
- shortcuts/register_external_test.go
- shortcuts/common/typed_contract.go
- shortcuts/common/typed_result_protocol_test.go
- shortcuts/common/typed_compiler_test.go
- cmd/command_sets_test.go
- shortcuts/common/typed_binder_benchmark_test.go
- extension/command/commandtest/commandtest_test.go
- cmd/auth/login_test.go
- shortcuts/common/types.go
- extension/command/definition.go
- shortcuts/common/typed_map_binder_test.go
- shortcuts/common/typed_flag_schema_test.go
- shortcuts/common/typed_output.go
- cmd/auth/login.go
- shortcuts/common/clone_test.go
- extension/command/commandtest/business_commands_test.go
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| //nolint:forbidigo // Compiler diagnostics are registration-time programmer errors consumed by Define's panic boundary, not command-facing failures. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The //nolint:forbidigo directives do not suppress the no_bare_helper_error gate. All four files carry a file-level //nolint:forbidigo with a justification, yet the CI job CI / 2308 / 9_deterministic-gate.txt still reports no_bare_helper_error against each of them. The shared root cause is that the deterministic gate is a separate check from the golangci-lint forbidigo linter, so a //nolint directive has no effect on it. Determine the gate's own suppression mechanism, then apply it consistently, or convert the reported helpers to typed errs.* errors that preserve causes.
shortcuts/common/typed_compile_args.go#L4-L4: reported over lines 148-629. Add the gate's suppression form next to the existing//nolint:forbidigo, keeping the current justification text.shortcuts/common/typed_compile_data.go#L4-L4: reported over lines 25-428. Apply the same suppression form.shortcuts/common/typed_compiler.go#L4-L4: reported over lines 41-226. Apply the same suppression form.shortcuts/common/typed_external.go#L4-L4: reported over lines 45-132. Apply the same suppression form.
The justifications themselves are accurate. Each bare error is wrapped before it reaches a command boundary: Define converts them to a panic at registration, and CompileErasedDefinition returns them to internal/commandhost.CompileSets for the startup guard. The gate result, not the design, is what needs resolving.
#!/bin/bash
# Description: Locate the deterministic gate implementation and its suppression mechanism.
set -euo pipefail
rg -n --iglob '!**/vendor/**' 'no_bare_helper_error' -C 6
fd -t f -e sh -e go -e py . scripts tools .github 2>/dev/null | xargs rg -ln 'deterministic' 2>/dev/null || true
rg -n 'deterministic-gate' .github/ -C 6📍 Affects 4 files
shortcuts/common/typed_compile_args.go#L4-L4(this comment)shortcuts/common/typed_compile_data.go#L4-L4shortcuts/common/typed_compiler.go#L4-L4shortcuts/common/typed_external.go#L4-L4
🤖 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/typed_compile_args.go` at line 4, Determine the suppression
syntax recognized by the deterministic no_bare_helper_error gate, then add it
beside the existing justified //nolint:forbidigo directive in
shortcuts/common/typed_compile_args.go:4,
shortcuts/common/typed_compile_data.go:4, shortcuts/common/typed_compiler.go:4,
and shortcuts/common/typed_external.go:4. Preserve the current justification
text and ensure the suppression covers the reported helpers used by Define and
CompileErasedDefinition; alternatively, replace those helpers with typed errs.*
errors while preserving causes.
Sources: Coding guidelines, Pipeline failures
Comment on lines
+206
to
+218
| if supplement.Shape != nil { | ||
| if shapeHasConstraints(field.shape) || field.nullable != nil { | ||
| return fmt.Errorf("Shape conflicts with schema constraints or nullable declaration") | ||
| } | ||
| if err := validateShape(supplement.Shape, "InputField.Shape"); err != nil { | ||
| return err | ||
| } | ||
| if !shapeCompatibleWithType(supplement.Shape, field.valueType) { | ||
| return fmt.Errorf("InputField.Shape %T is incompatible with Go type %s", supplement.Shape, field.valueType) | ||
| } | ||
| field.shape = supplement.Shape | ||
| field.shapeExplicit = true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A schema-tag constraint is silently dropped when InputField.Shape is supplied.
Line 207 detects a conflict with shapeHasConstraints(field.shape). But collectArgFields skips shapeForType whenever the supplement carries a Shape, at lines 151-156, so field.shape is nil here. shapeHasConstraints(nil) returns false through the default arm.
The result: a field declared as schema:"optional;minLength=3" together with an InputField.Shape passes this check. field.shape is replaced at line 216 and the minLength=3 constraint parsed into schema.minLength is never applied. The command then accepts input the author declared as invalid.
Only field.nullable is still detected, because it is stored separately on the field.
Check the parsed schemaTag constraints rather than the derived shape.
🐛 Proposed fix to carry and check the parsed constraints
Store the parsed tag on the compiled field in collectArgFields:
// in the compiledInputField literal at line 157
schemaConstraints: schema,Then check it here:
if supplement.Shape != nil {
- if shapeHasConstraints(field.shape) || field.nullable != nil {
+ if schemaTagHasConstraints(field.schemaConstraints) || shapeHasConstraints(field.shape) || field.nullable != nil {
return fmt.Errorf("Shape conflicts with schema constraints or nullable declaration")
}with:
func schemaTagHasConstraints(s schemaTag) bool {
return len(s.enum) > 0 || s.format != "" ||
hasStringConstraints(s) || hasNumberConstraints(s) || hasItemConstraints(s)
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 208-208: shortcuts/common/typed_compile_args.go#L208
Added line #L208 was not covered by tests
[warning] 211-211: shortcuts/common/typed_compile_args.go#L211
Added line #L211 was not covered by tests
🤖 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/typed_compile_args.go` around lines 206 - 218, Preserve
parsed schema-tag constraints when compiling fields and use them for the
conflict check. In collectArgFields, store the parsed schema value on the
compiledInputField via schemaConstraints, then update the InputField.Shape
handling block to call a schemaTagHasConstraints helper that checks enum,
format, string, number, and item constraints instead of
shapeHasConstraints(field.shape); retain nullable validation and existing shape
validation.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/auth/login.go (1)
557-586: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate
--domaincompletion to useallKnownDomains.Make
completeDomainbrand-aware and include scope-bearing shortcut services. Add a regression test for an injected shortcut service.🤖 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/auth/login.go` around lines 557 - 586, Update completeDomain to accept the current Lark brand and derive its suggestions through allKnownDomainsWithShortcuts, so completion includes eligible registered shortcut services with declared scopes while preserving existing known domains. Add a regression test covering an injected shortcut service and verify it appears in --domain completion.
🧹 Nitpick comments (4)
shortcuts/common/typed_external_pagination.go (1)
23-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct regression coverage for
CollectCommandPages.Existing tests cover
internalpagination.Walkand the publicCollectPagescallback, but they do not execute this adapter. Cover cursor replacement, policy limits, partial state, and translated walker errors.🤖 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/typed_external_pagination.go` around lines 23 - 75, Add direct regression tests for CollectCommandPages, using a stub CommandContext and typed API response to verify page-token replacement across requests, commandPagePolicy limits and delays, partial collection state on fetch failure, and translation of walker errors through paginationWalkError. Keep existing internalpagination.Walk and CollectPages tests unchanged.Source: Coding guidelines
extension/command/commandtest/commandtest.go (1)
143-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDefault
page-delayto 0 in the harness.
parsePaginationFlagsdefaultspage-delayto 200 ms.collectPagesapplies that delay between pages wheneverallis false, which is theRunWithFlagspath. A test that passes--page-allwithout--page-delay=0therefore sleeps 200 ms per page boundary.TestListCommandUsesHostPaginationavoids this only because it passes--page-delay=0explicitly.If the harness must mirror the production default, document the requirement on
RunWithFlags. Otherwise default to 0.♻️ Proposed change
- pageDelay := flags.Int("page-delay", 200, "") + // The harness defaults to no delay so scripted tests do not sleep. + pageDelay := flags.Int("page-delay", 0, "")🤖 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 `@extension/command/commandtest/commandtest.go` around lines 143 - 160, Update parsePaginationFlags to default page-delay to 0 so RunWithFlags does not impose an unintended 200 ms delay between pages when --page-all is omitted. Preserve explicit --page-delay values and the existing PaginationOptions conversion.extension/command/commandtest/business_commands_test.go (1)
382-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew error tests assert message text or bare non-nil instead of typed metadata. Each of these paths returns a typed
errs.*error, so the assertions can checkerrs.ProblemOffields and cause preservation. As per coding guidelines for**/*_test.go: "Error tests must assert typed metadata and cause preservation rather than message text alone."
extension/command/commandtest/business_commands_test.go#L382-L384: replace the "hard limit" substring match with an assertion onerrs.SubtypeInvalidResponsefromcommand.PaginationLimitError.extension/command/commandtest/business_commands_test.go#L346-L348: replace the non-nil check with an assertion on theerrs.SubtypeInvalidResponsecursor error.extension/command/commandtest/commandtest_test.go#L200-L203: replace the non-nil check with an assertion on the*errs.ValidationErrorreturned byRunWithFlags.🤖 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 `@extension/command/commandtest/business_commands_test.go` around lines 382 - 384, Update the error assertions in extension/command/commandtest/business_commands_test.go:346-348 and 382-384 to inspect errs.ProblemOf metadata, asserting errs.SubtypeInvalidResponse for the cursor error and command.PaginationLimitError instead of checking only nil or message text. In extension/command/commandtest/commandtest_test.go:200-203, assert that RunWithFlags returns a *errs.ValidationError and verify its typed metadata and preserved cause.Source: Coding guidelines
cmd/auth/login_test.go (1)
323-334: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert domain-set equality, not only cardinality.
Equal lengths plus rejection of unknown names still allows one known domain to be duplicated while another is missing. Track seen names and verify that every key in
allKnownDomainsappears exactly once.🤖 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/auth/login_test.go` around lines 323 - 334, Update TestGetDomainMetadataMatchesAllKnownDomains to track each metadata domain name in a seen set, reject duplicate names, and verify every key in allKnownDomains appears exactly once. Retain the existing length and unknown-domain checks while asserting true set equality.
🤖 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/command_sets_test.go`:
- Around line 166-172: Update the schema command assertions in the test around
root.ExecuteC to decode stdout as JSON, then directly assert the command name
equals "im +business-surface" and inputSchema and outputSchema are non-null.
Replace the current strings.Contains checks while preserving the existing
command execution and failure reporting.
In `@cmd/schema/schema.go`:
- Around line 183-184: Update shortcut schema filtering in shortcutSchemaVisible
and resolveShortcutSchema to reject shortcuts where shortcut.Hidden is true,
regardless of whether visibility is nil or permits the shortcut. Ensure both
direct schema lookup and completion use shortcutSchemaVisible, and add
regression coverage for hidden shortcuts in each path while preserving existing
visibility filtering.
In `@internal/commandhost/compile_test.go`:
- Around line 259-262: Extend both DryRunE error tests at
internal/commandhost/compile_test.go:259-262 and
shortcuts/common/runner_jq_test.go:358-360 to use errors.As, asserting the
propagated error is a *errs.ValidationError with errs.SubtypeInvalidArgument;
retain the existing errors.Is cause-preservation assertions in both sites.
---
Outside diff comments:
In `@cmd/auth/login.go`:
- Around line 557-586: Update completeDomain to accept the current Lark brand
and derive its suggestions through allKnownDomainsWithShortcuts, so completion
includes eligible registered shortcut services with declared scopes while
preserving existing known domains. Add a regression test covering an injected
shortcut service and verify it appears in --domain completion.
---
Nitpick comments:
In `@cmd/auth/login_test.go`:
- Around line 323-334: Update TestGetDomainMetadataMatchesAllKnownDomains to
track each metadata domain name in a seen set, reject duplicate names, and
verify every key in allKnownDomains appears exactly once. Retain the existing
length and unknown-domain checks while asserting true set equality.
In `@extension/command/commandtest/business_commands_test.go`:
- Around line 382-384: Update the error assertions in
extension/command/commandtest/business_commands_test.go:346-348 and 382-384 to
inspect errs.ProblemOf metadata, asserting errs.SubtypeInvalidResponse for the
cursor error and command.PaginationLimitError instead of checking only nil or
message text. In extension/command/commandtest/commandtest_test.go:200-203,
assert that RunWithFlags returns a *errs.ValidationError and verify its typed
metadata and preserved cause.
In `@extension/command/commandtest/commandtest.go`:
- Around line 143-160: Update parsePaginationFlags to default page-delay to 0 so
RunWithFlags does not impose an unintended 200 ms delay between pages when
--page-all is omitted. Preserve explicit --page-delay values and the existing
PaginationOptions conversion.
In `@shortcuts/common/typed_external_pagination.go`:
- Around line 23-75: Add direct regression tests for CollectCommandPages, using
a stub CommandContext and typed API response to verify page-token replacement
across requests, commandPagePolicy limits and delays, partial collection state
on fetch failure, and translation of walker errors through paginationWalkError.
Keep existing internalpagination.Walk and CollectPages tests unchanged.
🪄 Autofix
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 Plus
Run ID: 78c6c97d-c600-4e34-b3f8-a8a361041fab
📒 Files selected for processing (33)
affordance/content.goaffordance/content_test.gocmd/auth/login.gocmd/auth/login_test.gocmd/command_sets_test.gocmd/schema/schema.gocontent_embed.goextension/command/command_test.goextension/command/commandtest/business_commands_test.goextension/command/commandtest/commandtest.goextension/command/commandtest/commandtest_test.goextension/command/context.goextension/command/definition.goextension/command/host.goextension/command/pagination.goextension/command/testdata/wrapper/main.goextension/command/wrapper_e2e_test.goextension/platform/README.mdextension/platform/skillsoverlay.gointernal/commandhost/compile.gointernal/commandhost/compile_test.gointernal/pagination/walk.gointernal/pagination/walk_test.goshortcuts/common/paginate_into.goshortcuts/common/runner.goshortcuts/common/runner_jq_test.goshortcuts/common/typed_compiler.goshortcuts/common/typed_definition.goshortcuts/common/typed_external_pagination.goshortcuts/common/typed_schema_export.goshortcuts/common/types.goskills/content.goskills/content_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/common/typed_definition.go
- shortcuts/common/typed_compiler.go
- extension/command/definition.go
- extension/command/host.go
- internal/commandhost/compile.go
- shortcuts/common/runner.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extension/command/command_test.go (1)
125-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact modifier results.
The test only checks that the POST body is non-nil. A wrong body can still pass.
Paramsalso overwrites the samepage_sizekey set bySet, so a brokenSetimplementation can pass.Use distinct query keys and compare the complete body.
Suggested assertions
- Set("page_size", 20). + Set("set_page_size", 20). Params(map[string]any{"page_size": 50}). ... - if view.Requests[0].Query["page_size"] != 50 { + if view.Requests[0].Query["set_page_size"] != 20 || + view.Requests[0].Query["page_size"] != 50 { t.Fatalf("GET query = %#v", view.Requests[0].Query) } ... - if view.Requests[1].Body == nil { - t.Fatal("POST body is nil") + body, ok := view.Requests[1].Body.(map[string]any) + if !ok || len(body) != 1 || body["name"] != "example" { + t.Fatalf("POST body = %#v", view.Requests[1].Body) }As per coding guidelines, “Every behavior change requires a nearby regression test that fails when the implementation is reverted; tests should assert fields, requests, typed errors, or side effects directly.”
🤖 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 `@extension/command/command_test.go` around lines 125 - 151, Strengthen TestDryRunBuilderSupportsEveryRequestMethodAndModifier by using distinct keys for Set and Params, then assert each modifier’s exact query result so Set cannot be masked by Params. Replace the non-nil POST body check with a deep comparison against the complete expected body, preserving the existing request-method assertions.Source: Coding guidelines
🤖 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 `@extension/command/command_test.go`:
- Around line 125-151: Strengthen
TestDryRunBuilderSupportsEveryRequestMethodAndModifier by using distinct keys
for Set and Params, then assert each modifier’s exact query result so Set cannot
be masked by Params. Replace the non-nil POST body check with a deep comparison
against the complete expected body, preserving the existing request-method
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea7ebd89-3709-4ed5-a413-1700a9ea1a5f
📒 Files selected for processing (5)
extension/command/command_test.gointernal/commandhost/compile.gointernal/commandhost/compile_test.goshortcuts/common/typed_api.goshortcuts/common/typed_compiler_test.go
💤 Files with no reviewable changes (2)
- shortcuts/common/typed_api.go
- internal/commandhost/compile.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/commandhost/compile_test.go
- shortcuts/common/typed_compiler_test.go
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/auth/login_interactive.go`:
- Around line 40-41: Update getDomainMetadataWithShortcuts to accept the active
brand and pass it to allKnownDomainsWithShortcuts instead of an empty string;
update runInteractiveLoginWithShortcuts to supply its brand argument. Add
coverage verifying brand-restricted shortcut services are excluded for other
brands, while preserving existing CLI behavior.
In `@cmd/auth/login.go`:
- Around line 103-105: Update completeDomain to pass shortcuts.AllShortcuts()
into completeDomainWithShortcuts instead of nil, matching sortedKnownDomains and
restoring global shortcut-service completions. Add a regression test that
verifies a registered shortcut service is completed through completeDomain.
In `@extension/command/command_test.go`:
- Around line 79-84: Update the assertInternal helper to validate the complete
error contract: retain the existing errs.InternalError type and SubtypeUnknown
checks, then assert errs.ProblemOf(err).Category against the expected category
and verify the original wrapped cause is preserved. Use the helper’s existing
error input and failure reporting without relying on message text.
In `@extension/command/commandtest/commandtest.go`:
- Around line 466-467: Add a nearby regression test for the request-recording
flow around the JSON decoder and Recorder.Requests(), using a numeric query or
body value and asserting the decoded value is json.Number. Ensure the assertion
depends on decoder.UseNumber() so the test fails if that call is removed.
In `@shortcuts/common/typed_map_binder_test.go`:
- Around line 321-326: Update
TestConvertReflectValueRejectsNegativeUnsignedInput to exercise bindTypedMap or
the first boundary that wraps convertReflectValue, then assert the resulting
typed binder error’s validation type, subtype, and parameter along with
preservation of the underlying conversion cause; replace the current
message-only strings.Contains check.
🪄 Autofix
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 Plus
Run ID: feed08bb-3af1-4de3-8ead-c9d904509277
📒 Files selected for processing (34)
cmd/auth/auth.gocmd/auth/login.gocmd/auth/login_interactive.gocmd/auth/login_test.gocmd/build.gocmd/command_sets_test.gocmd/error_auth_hint.gocmd/error_presenter_test.gocmd/root_test.gocmd/schema/schema.gocmd/schema/schema_test.goextension/command/command_test.goextension/command/commandtest/business_commands_test.goextension/command/commandtest/commandtest.goextension/command/commandtest/commandtest_test.goextension/command/errors.goextension/command/host.gointernal/cmdmeta/meta.gointernal/commandhost/compile.gointernal/commandhost/compile_test.goshortcuts/common/runner.goshortcuts/common/runner_botinfo_test.goshortcuts/common/runner_jq_test.goshortcuts/common/typed_binder.goshortcuts/common/typed_compile_args.goshortcuts/common/typed_compile_contract.goshortcuts/common/typed_compile_data.goshortcuts/common/typed_compiler_invalid_test.goshortcuts/common/typed_external.goshortcuts/common/typed_map_binder.goshortcuts/common/typed_map_binder_test.goshortcuts/common/typed_runner_test.goshortcuts/register.goshortcuts/register_external_test.go
🚧 Files skipped from review as they are similar to previous changes (18)
- shortcuts/common/runner_jq_test.go
- shortcuts/common/typed_map_binder.go
- shortcuts/common/typed_compile_contract.go
- cmd/command_sets_test.go
- shortcuts/common/typed_external.go
- shortcuts/common/typed_binder.go
- extension/command/errors.go
- shortcuts/common/typed_compiler_invalid_test.go
- extension/command/commandtest/commandtest_test.go
- shortcuts/register_external_test.go
- shortcuts/common/typed_compile_data.go
- extension/command/host.go
- shortcuts/common/typed_compile_args.go
- internal/commandhost/compile.go
- cmd/auth/login_test.go
- cmd/build.go
- extension/command/commandtest/business_commands_test.go
- shortcuts/common/runner.go
Comment on lines
+79
to
+84
| assertInternal := func(name string, err error) { | ||
| t.Helper() | ||
| var internal *errs.InternalError | ||
| if !errors.As(err, &internal) || internal.Subtype != errs.SubtypeUnknown { | ||
| t.Fatalf("%s error = %#v", name, err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert complete error metadata and the wrapped cause.
assertInternal checks only the concrete type and Subtype. Also assert the errs.ProblemOf(err).Category value and the preserved wrapped cause. This prevents the test from accepting adapters that change the error contract.
As per coding guidelines, “Error tests must assert typed metadata and cause preservation rather than message text alone.”
🤖 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 `@extension/command/command_test.go` around lines 79 - 84, Update the
assertInternal helper to validate the complete error contract: retain the
existing errs.InternalError type and SubtypeUnknown checks, then assert
errs.ProblemOf(err).Category against the expected category and verify the
original wrapped cause is preserved. Use the helper’s existing error input and
failure reporting without relying on message text.
Source: Coding guidelines
Comment on lines
+466
to
+467
| decoder := json.NewDecoder(bytes.NewReader(encoded)) | ||
| decoder.UseNumber() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a regression test for numeric request values.
UseNumber changes the observable type in Recorder.Requests(). Add a test that records a numeric query or body value and asserts json.Number. The test must fail if decoder.UseNumber() is removed.
As per coding guidelines, “Every behavior change requires a nearby regression test that fails when the implementation is reverted.”
🤖 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 `@extension/command/commandtest/commandtest.go` around lines 466 - 467, Add a
nearby regression test for the request-recording flow around the JSON decoder
and Recorder.Requests(), using a numeric query or body value and asserting the
decoded value is json.Number. Ensure the assertion depends on
decoder.UseNumber() so the test fails if that call is removed.
Source: Coding guidelines
Comment on lines
+321
to
+326
| func TestConvertReflectValueRejectsNegativeUnsignedInput(t *testing.T) { | ||
| _, err := convertReflectValue(int64(-1), reflect.TypeFor[uint64]()) | ||
| if err == nil || !strings.Contains(err.Error(), "cannot be represented") { | ||
| t.Fatalf("error = %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the typed binder error for negative unsigned input.
Line 323 checks error text only. Route this case through bindTypedMap, or assert the typed error at the first boundary that wraps this conversion error. Verify the validation type, subtype, and parameter.
As per coding guidelines, “Error tests must assert typed metadata and cause preservation rather than message text alone.”
🤖 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/typed_map_binder_test.go` around lines 321 - 326, Update
TestConvertReflectValueRejectsNegativeUnsignedInput to exercise bindTypedMap or
the first boundary that wraps convertReflectValue, then assert the resulting
typed binder error’s validation type, subtype, and parameter along with
preservation of the underlying conversion cause; replace the current
message-only strings.Contains check.
Source: Coding guidelines
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/common/typed_api_test.go (1)
34-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding an error-path assertion for log ID propagation.
This test pins the success contract. It confirms the header log ID does not overwrite the business
log_idindata.The paired guarantee is not covered here: on failure,
ClassifyAPIResponseWithmust still attach the transport log ID to the typed error. A future change that stops readingX-Tt-Logidentirely would keep this test green and silently remove the diagnostic from error envelopes.Add a sibling test that registers a stub with a non-zero
codeand aX-Tt-Logidheader, then asserts the returned error is typed and carries the log ID.As per coding guidelines, "Error tests must assert typed metadata and cause preservation rather than message text alone."
🤖 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/typed_api_test.go` around lines 34 - 56, Add a sibling failure-path test beside TestDoTypedAPIJSONPreservesSuccessData that registers a non-zero response code and X-Tt-Logid header, invokes DoTypedAPIJSON, and asserts the returned error has the expected typed error metadata, including the transport log ID, while also verifying the underlying cause is preserved rather than checking only its message.Source: Coding guidelines
shortcuts/common/clone.go (1)
157-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe reflective clone helpers are duplicated across two packages. Both files define the same
cloneScalarPointer,cloneJSONValue,cloneVisit,cloneJSONReflect,cloneJSONInterface,cloneJSONPointer,cloneJSONMap,cloneJSONSlice,cloneJSONArray, andcloneJSONStructwith identical logic. Both copies are correct today. The risk is divergence: a later fix to cycle handling, unexported-struct-field handling, or slice identity must land in both places, and nothing enforces that.The split appears deliberate, because
extension/commandis a public package and must not import internal code. So do not merge them intoshortcuts/common. Extract the algorithm into a small dependency-free package that both may import, or add a generated-file check that fails when the two blocks drift.
shortcuts/common/clone.go#L157-L274: move this helper set into the shared package and import it, or mark it as the generated source of truth.extension/command/host.go#L304-L421: replace this copy with the shared import, or mark it as generated from the same source.Note one shared property worth documenting wherever the algorithm lands:
cloneJSONStructcopies unexported fields by value throughresult.Set(value), so unexported reference fields stay shared between the original and the clone.🤖 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/clone.go` around lines 157 - 274, Eliminate the duplicated reflective clone helper set by establishing one shared dependency-free implementation for cloneScalarPointer, cloneJSONValue, cloneVisit, cloneJSONReflect, and the related cloneJSON* helpers; update shortcuts/common/clone.go lines 157-274 and extension/command/host.go lines 304-421 to use that source or be generated from it. Preserve the existing behavior, including cloneJSONStruct copying unexported fields by value so unexported reference fields remain shared, and document this property where the canonical implementation resides.
🤖 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/clone.go`:
- Around line 157-274: Eliminate the duplicated reflective clone helper set by
establishing one shared dependency-free implementation for cloneScalarPointer,
cloneJSONValue, cloneVisit, cloneJSONReflect, and the related cloneJSON*
helpers; update shortcuts/common/clone.go lines 157-274 and
extension/command/host.go lines 304-421 to use that source or be generated from
it. Preserve the existing behavior, including cloneJSONStruct copying unexported
fields by value so unexported reference fields remain shared, and document this
property where the canonical implementation resides.
In `@shortcuts/common/typed_api_test.go`:
- Around line 34-56: Add a sibling failure-path test beside
TestDoTypedAPIJSONPreservesSuccessData that registers a non-zero response code
and X-Tt-Logid header, invokes DoTypedAPIJSON, and asserts the returned error
has the expected typed error metadata, including the transport log ID, while
also verifying the underlying cause is preserved rather than checking only its
message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c07b5ae6-d011-49fa-8087-d66b77bf9293
📒 Files selected for processing (19)
cmd/auth/login.gocmd/auth/login_brand_filter_test.gocmd/auth/login_interactive.gocmd/auth/login_test.gocmd/schema/schema.gocmd/schema/schema_test.goextension/command/command_test.goextension/command/definition.goextension/command/host.goextension/command/output.goextension/command/pagination.goextension/command/request.gointernal/commandhost/compile.gointernal/commandhost/compile_test.goshortcuts/common/clone.goshortcuts/common/clone_test.goshortcuts/common/typed_api.goshortcuts/common/typed_api_test.goshortcuts/common/typed_runner.go
💤 Files with no reviewable changes (1)
- extension/command/definition.go
🚧 Files skipped from review as they are similar to previous changes (9)
- shortcuts/common/clone_test.go
- shortcuts/common/typed_runner.go
- cmd/auth/login_interactive.go
- cmd/auth/login.go
- internal/commandhost/compile.go
- extension/command/pagination.go
- cmd/auth/login_test.go
- extension/command/command_test.go
- cmd/schema/schema.go
… main Move the declared-scope filter from allKnownDomains to the interactive selector only. On main, a scope-less shortcut domain (event) passes --domain validation and fails later with "no matching scopes found"; the previous unified filter changed that to "unknown domain" and dropped it from the --help list. The interactive picker still hides scope-less domains — selecting one can only fail.
Business code concatenates IDs into request paths but had no public escape helper (internal/validate.EncodePathSegment is unreachable from extension/command). Mirror its url.PathEscape semantics, use it in the business command examples, and pin the traversal defense: the validator decodes percent-encoding before the canonical check, so both raw and escaped dot sequences fail same-origin validation.
Mirror the audit-observer precedent: a buildable wrapper main under examples/ showing WithCommandSets against the real distribution shape (plugins, strict mode, and service commands stay enabled). Covers the single-read command (Validate, shared DryRun request, CallJSON, PathSegment, Tips) and a Page[T] list command whose pagination flags come from the compiler. The testdata/wrapper fixture stays test-only. Verified offline: --help renders tag-driven parameters, +chat-brief-list exposes --page-all/--page-limit/--page-delay, and --dry-run previews the request with a fake env token and no network access.
Two pagination contract fixes from the extension design (owner plan §8.3): Page decoding accepted only a literal "items" array, so endpoints that spell their list field differently (drive uses files, some responses use records) walked every page while decoding nothing — CollectAllPages then returned an empty set marked complete, and downstream writes ran against it. Each page now normalizes its single top-level array field into Page.Items; zero or multiple array fields fail closed with a typed invalid-response error. CollectAllPages previously reused the user-facing --page-limit maximum (1000) as its walk bound. A complete-set collection holds every page in memory before the workflow's writes run, so it now uses the design's dedicated workflow bound of 100 pages.
A Page[T] command's dry-run can only show the first request; fabricating response-dependent page tokens is forbidden. Append the bounded-repeat explanation to the previewed request (preserving any business description), matching the design's dry-run contract. Also give the example's list command a --page-token resume flag seeded into the request, documenting the resume convention: the framework owns --page-all/--page-limit/--page-delay while the starting cursor is a business-declared input, independent of --page-all.
The generator emitted Chinese titles while the rest of the public extension packages document in English. Switch the generator to the "en" service title and regenerate. The generator already rejects a domain missing either locale, so the switch keeps its own guard.
The Host* types, InspectCommand, InspectDomain and CloneSets exist for lark-cli's host adapter, not for business commands, but nothing said so at the symbols themselves. They cannot move to a subpackage: a Command holds its declaration unexported, so a sibling package has no way to reach it, and moving the wire types to internal/ would cycle back through CommandMetadata and CommandContext. Also correct HostPagination, which is not adapter-only -- ContextOptions and commandtest both carry it.
A set already declares its domain through ExtendDomain(DomainIm), yet every command repeated the same domain as a bare string that only the host compiler checked. Typing the field points authors at the generated enum and forces an explicit conversion when the value comes from a string variable. This does not make a mistyped literal a compile error -- an untyped constant still converts to DomainName -- so the mismatch check in CompileSets stays the actual net. The example and the wrapper fixture now declare command.DomainIm. The chat-brief example was also not gofmt-clean, which the CI format gate would have caught.
buildInternalWithConfig is an orchestrator, and the business command sets were compiled inline inside it. Move that step to resolveShortcutSnapshot so the entry point reads as one call and the built-in/external merge has a name. newCommand carried six near-identical blocks that each nil-checked a hook and wrapped it in the same type assertion. Split them into one binder per hook shape; Normalize and Validate now share bindArgsHook since their signatures match. The behaviour is unchanged: an undeclared hook still erases to nil, and an empty renderer map still yields nil.
AllShortcuts deep-copies because a Shortcut carries slice fields whose backing arrays a shallow copy would share: an external distribution mutating registered[0].Flags[0] would corrupt the process-global list. That copy is worth its ~165us over 500+ shortcuts. Paying it four times per startup is not. auth, schema and the mount path each cloned the snapshot again, but they receive it from AllShortcutsWithExternal with no third-party code in between, and nothing in this repository mutates a shortcut element -- mountDeclarative takes a value receiver and only replaces slice headers. Drop those three copies and document the boundary on AllShortcuts so the next reader does not reintroduce them. Startup drops from four full clones to one. Benchmarks pin the remaining cost so a regression points at a new clone rather than at growth in the shortcut set.
Two review findings, both of which let a business command pass its tests
and then misbehave in production.
The commandtest recorder walked 1000 pages for a complete-set collection
while the host adapter stops at 100, so a command tested against 300
pages of fixtures would fail its first real --page-all run with
PaginationLimitError. The bound now lives in internal/pagination, which
both sides already import, and the hard-limit test scripts itself from
that constant instead of restating 1000 -- the literal was what let the
two drift apart.
The testdata wrapper concatenated args.ID straight into the request path,
contradicting PathSegment's own documented rule and the chat-brief
example. ValidateRequestView does not cover this: "abc/other-users-file"
cleans to itself, so an unescaped separator silently retargets the
request. Since testdata is what an integrator copies first, route both
call sites through one readRequest helper, mirroring chat-brief.
The e2e assertion could not have caught it either -- PathSegment("chat_1")
is "chat_1", so the check passed with or without the call. It now sends
"chat/1" and asserts %2F reaches the wire; removing PathSegment fails it.
…iew findings
Normalize and Validate run before the high-risk confirmation gate, and
both received the full CommandContext, so a high-risk business command
could POST or DELETE from Validate and leave remote side effects behind
before the user was ever asked to confirm. Moving the gate earlier would
contradict the documented hook order and would also make --dry-run
require --yes. The design already forbids this from the other side --
Validate is specified as parameter checking that issues no request -- so
enforce that instead: Normalize and Validate get a context whose CallJSON
and CollectPages refuse, while PreflightScopes stays available. The guard
sits in CommandContext rather than in the wiring, so a future adapter
that wires the callbacks anyway still cannot reach the API. commandtest
mirrors it, otherwise a command would pass its tests and fail only in
production.
Page.Items now starts non-nil. It is declared required;nonnullable, but a
zero-item collection encoded as {"items":null}, which a caller generating
types from the published schema would reject.
NewCmdAuthWithRecovery and NewCmdSchemaWithVisibility are restored as
wrappers. Both were dropped for shortcut-aware variants, and both are
reachable from outside this module: CommandVisibility is an ordinary
exported func type, and *recovery.Projector cannot be named by an outside
caller but can be passed as nil. A signature test now pins them.
The path-traversal fixture said "../../secret", which the deterministic
gate rejects as a generic credential assignment -- the reason CI is
currently red. The filename carries no meaning; it is now "../../outside".
The compatibility wrappers restored for outside callers are unreachable from inside this repository by construction, so the incremental dead-code gate rejected them. A signature-only assertion did not help: taking a function value and discarding it leaves the body unreachable, and it proved nothing about whether the wrapper still builds a working command. Call each one and assert the command it returns. NewCmdAuthWithRecovery is called with a nil projector, which is the exact call an outside module can make and the reason the wrapper has to keep compiling. Verified with the same deadcode version CI runs: neither function is reported, and no other function in this branch's files is either.
…tency Hooks is the first type a business author reads and carried no field documentation, so the rules lived only in the design doc: which of DryRun and DryRunE to set, that setting both fails to compile, that Execute owns the API call and must not write stdout, and that Normalize and Validate run before the confirmation gate and therefore get no network. The choice between DryRun and DryRunE is not old-versus-new -- neither is legacy. It follows from whether building the preview can fail, which is now what the field docs say. Also pin the dry-run note as idempotent. convertDryRun writes the bounded-repeat note into the projection it builds, never back into the hook's *DryRun, and DryRunAPI.Desc assigns rather than appends, so a hook that caches and returns the same preview cannot accumulate the note. Both properties were true and neither was tested.
Three narrowings of the V1 business-command contract, none of which has a published compatibility surface: extension/command does not exist on main. Preview and NewDryRun were the same constructor twice -- one empty, one seeded with requests. Fold them into a variadic NewDryRun. Every existing NewDryRun() call keeps compiling, and the domain word in the contract is now spelled one way. The type DryRun already owns that identifier in this package, so naming the constructor DryRun outright cannot compile. Drop Metadata.Tips. It was pure passthrough into common.Shortcut.Tips and nothing in the execution path read it, so business commands lose only the ability to declare help tips; the repository's own typed shortcuts keep theirs. The mount test asserted a tip reached the rendered help as proof that metadata survives the extension -> commandhost -> common.Shortcut -> help conversion; it now asserts the risk line, which travels the same path. Hand-write the domain enumeration and delete the generator. Generating from shortcuts.AllShortcuts silently omitted approval, attendance and mindnotes: all three are published under `lark-cli --help` and served by typed and raw API commands, they just own no shortcut. The enum is now the 23 domains the CLI actually exposes. Those three would otherwise have been constants that compile and always fail, because CompileSets derived its mountable domains from the same shortcut list. It now reads the service registry, and shortcuts/register.go already creates a domain command group on demand when no built-in occupies it, so a business command can mount under a shortcut-less domain.
The domain enumeration is hand-written now and extension/command holds no go:generate directive, so the path was a no-op that still read as if the package carried generated files.
…lt-in DryRunE has no counterpart in the shipped CLI: `git show main:shortcuts/common/types.go` has no such field, it arrived with the typed-shortcut framework this branch imported, and no shortcut in the repository sets one. Business commands get the single DryRun hook that built-in shortcuts have. Validate already runs before it and owns the error channel, so a preview that cannot be built still fails there with a typed error -- which is what the repointed tests now assert, end to end through --dry-run and through commandtest.Preview. Also stop appending the bounded-repeat note. convertDryRun added "with --page-all, repeats with the returned page_token until exhaustion or --page-limit" to every Page[T] preview, so an external command's dry-run carried a sentence its author never wrote. Built-in paginated shortcuts say this themselves when they want it (im_chat_members_list.go calls dry.Desc), and external commands now do the same: the framework renders the description it was given and nothing else. The dry-run context keeps refusing requests -- runner.go does the same for built-ins, so removing that would be the divergence, not the alignment. Only the word changes: "offline" was our own vocabulary for what the rest of the CLI calls dry-run.
Business commands now return Success only. Partial, OutcomeDefinition, PartialFailureDefinition and FailedItemDefinition leave the public surface along with Execution.Partial and the host adapter's receipt conversion. Result keeps its outcome field. It is no longer a choice -- Success is the only value -- but it is also how the host tells a returned Result apart from the zero value that accompanies an error, which is the check commandtest.Execute makes before reporting "returned both Result and error". Collapsing it to nothing would delete that signal. The exemplar commands that returned Partial keep their scenarios: a best-effort scope failure still marks every item failed and appends the snapshot, and the multi-call audit still records the owner it could not resolve. That information lives in the command's own Data (Items[].State plus Failures), not in the outcome, so the tests assert the same facts and only the outcome assertion is gone. The deep-copy test moved its nested JSON exemplar from FailedValues to InputDefault.Value, keeping cloneJSONValue covered. shortcuts/common still defines PartialFailure for built-in typed shortcuts. That is the imported framework, untouched here.
Commit 4d0c6ea added internal/pagination, moved PaginateInto onto it, and then wrote a second caller for externally declared commands. Both assembled the same Walk options, cloned the same params, read the same cursor and mapped the same walk error; only the policy source, the call path and the accumulator ever differed. Those three now parameterize one pageWalk. PaginateInto keeps calling through the RuntimeContext and keeps its per-page progress line; external commands keep CallTypedAPI, the walker's context and their undecoded pages, which the public contract needs because it decodes them into its own Page[T]. Behavior is unchanged on both sides -- the external walk still leaves Wait nil, which internal/pagination fills with WaitContext, so --page-delay works exactly as before. pageWalk is deliberately generic-free so one struct serves both callers; the typed half of the built-in path moved to addDecodedPage.
CollectCommandPages now differs from PaginateInto only where the context type forces it. It takes the same PageAccumulator, decodes each page into T through the same addDecodedPage, returns the same *output.PaginationMeta and reads the same state out of the same walk, in the same order. What is left is what the interface cannot supply. An externally declared command compiles in the business module, so it holds a CommandContext rather than a *RuntimeContext: the context arrives as a parameter because the interface carries none, the call goes through CallTypedAPI, and there is no progress line because deciding to print one needs StderrIsTerminal, JqExpr and Format, none of which the interface exposes. The all parameter stays. It is the complete-set policy CollectAllPages depends on -- collect to exhaustion under the hard page bound instead of obeying --page-all and --page-limit -- and PaginateInto has no way to express it, since resolvePaginationPolicy only ever reads flags. Dropping it would quietly turn a command that must see the whole set into one a user can truncate with --page-limit 1. CommandPageCollection is gone with it: pages accumulate in commandhost's own accumulator, the way every built-in shortcut already accumulates its own. One consequence of sharing the decode: a page whose response carries no data object is now an error on this path too, as it always was for built-ins.
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
Add the V1 public contract and host integration for build-time business command extensions. Preserve the official CLI command surface while allowing wrapper binaries to assemble validated command sets explicitly.
Changes
Test Plan
Related Issues
Summary by CodeRabbit