feat: add framework flag aliases and unified IM pagination - #2146
Open
liangshuo-1 wants to merge 1 commit into
Open
feat: add framework flag aliases and unified IM pagination#2146liangshuo-1 wants to merge 1 commit into
liangshuo-1 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe change adds declarative flag aliases, shared shortcut normalization, automatic IM pagination, pagination-aware output metadata, manifest alias support, and flag-contract linting. It also updates shortcut implementations, tests, and documentation. ChangesFlag aliases and contracts
Pagination and output
Shortcut migrations
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
github-actions
Bot
added
domain/base
Introduce declarative exact-name flag aliases at the shortcut framework boundary while keeping semantic compatibility domain-owned. Add a shared, format-aware IM pagination pipeline with consistent flags, metadata, safety bounds, resumable cursors, and request throttling.
liangshuo-1
force-pushed
the
feat/framework-flag-aliases
branch
from
August 2, 2026 19:50
a435d6f to
e32d7cb
Compare
liangshuo-1
changed the title
liangshuo-1
marked this pull request as ready for review
August 2, 2026 19:51
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@e32d7cb42e8076b27081653f578b327b0bb5e911🧩 Skill updatenpx skills add larksuite/cli#feat/framework-flag-aliases -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2146 +/- ##
==========================================
+ Coverage 75.57% 75.65% +0.08%
==========================================
Files 931 938 +7
Lines 99162 99580 +418
==========================================
+ Hits 74937 75340 +403
+ Misses 18501 18476 -25
- Partials 5724 5764 +40 ☔ 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: 9
🧹 Nitpick comments (3)
lint/flagcontract/scan.go (1)
65-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe alias-flag rule is a fail-open heuristic. Consider documenting the limits.
Three conditions must all hold for a violation to fire, and each one under-matches:
aliasDescriptionmatches three fixed phrases. ADescsuch as"deprecated name for --order"or"accepts the old spelling of --order"is not detected.- Line 120 recognizes only the bare identifier
true.Hidden: isHiddenorHidden: someConstis not detected.hiddenFlagLiteralinspects any composite literal that hasName,Desc, andHiddenkeys. It does not confirm the literal is acommon.Flag. The fixture atlint/flagcontract/scan_test.golines 16-19 uses an anonymous struct and still triggers the rule.Fail-open is a defensible choice for a new lint domain. The concern is the description in
lint/README.mdlines 46-48, which states the guard "rejects ... independent hidden flags described as aliases" without qualification. A maintainer may treat the lint as an authoritative gate when it is a best-effort signal.Either narrow the literal check to
common.Flagusing type information, as the siblingdomaincontractpackage does, or state the heuristic nature in the README.Also applies to: 103-140
🤖 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 `@lint/flagcontract/scan.go` around lines 65 - 71, Update the flag-contract lint documentation in lint/README.md to describe the alias-flag check as a best-effort heuristic rather than an authoritative rejection, noting that it may miss alternate alias descriptions or hidden-value expressions and may match structurally similar non-common.Flag literals. Keep the existing detection behavior unchanged.shortcuts/im/im_chat_search.go (1)
322-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the named constant instead of the literal default.
params["page_size"] = 20duplicateschatSearchDefaultPageSize(already used for the flag default and validation bound). Using the literal risks drift if the default ever changes.♻️ Proposed fix
if n := runtime.Int("page-size"); n > 0 { params["page_size"] = n } else { - params["page_size"] = 20 + params["page_size"] = chatSearchDefaultPageSize }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/im/im_chat_search.go` around lines 322 - 333, Replace the literal fallback value in buildSearchChatParams with the existing chatSearchDefaultPageSize constant, while preserving the current handling of positive page-size values and page tokens.shortcuts/im/im_chat_messages_list.go (1)
121-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePage size is validated twice on the execute path.
Executevalidates page size at Line 122.buildChatMessageListRequestat Line 129 runs the sameValidatePageSizeTypedcall at Line 242 and returns the same typed--page-sizeerror. Remove the Line 122 call to keep one validation site.♻️ Proposed cleanup
Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { - if _, err := common.ValidatePageSizeTyped(runtime, "page-size", chatMessagesListDefaultPageSize, 1, chatMessagesListMaxPageSize); err != nil { - return err - } chatId, err := resolveChatIDForMessagesList(runtime, false)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/im/im_chat_messages_list.go` around lines 121 - 124, Remove the redundant ValidatePageSizeTyped call from the Execute function and let buildChatMessageListRequest remain the single page-size validation site, preserving its existing typed --page-size error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/common/paginate_into.go`:
- Around line 105-115: In the pagination flow around the maxPages check, return
successfully when pageNumber reaches policy.maxPages before validating
nextPageToken against seen. Keep requiring a non-empty token when another
request is allowed, and update the affected paginate_into tests to expect echoed
cursors to succeed for single-page reads.
In `@shortcuts/common/runner_normalize_test.go`:
- Line 121: Remove the ineffective normalizeCalled flag and its assertion from
the test, since the current flow never invokes Normalize through runShortcut.
Keep the direct contract assertions around ParseFlags and ValidateRequiredFlags
unchanged, or update the test to exercise runShortcut if ordering coverage is
required.
- Around line 38-40: Update the shortcut test setup around newTestFactory and
newTestShortcutCmd to use cmdutil.TestFactory(t, config) instead of constructing
an empty factory directly. Set LARKSUITE_CLI_CONFIG_DIR to t.TempDir() with
t.Setenv before creating the factory, while preserving the existing input stream
and command setup.
- Around line 79-81: Update the error assertion around runShortcut in the
normalization failure test to inspect errs.ProblemOf rather than only checking
for a non-nil error. Assert the expected category, subtype, and param values,
and verify the original cause is preserved, following the existing
typed-metadata assertions in this file’s test around lines 108-114.
In `@shortcuts/im/sort_flags.go`:
- Around line 58-67: Update the legacy-flag handling around flags.Str and
SetCanonicalFrom so an explicitly provided empty legacy value is not propagated
to the canonical flag. Validate it like other unrecognized values and attribute
the error to the legacy flag, or leave the canonical default unchanged; then
update the corresponding test case in sort_flags_test.go to reflect the
corrected behavior.
In `@skills/lark-im/SKILL.md`:
- Around line 109-117: Escape the literal pipe in the “asc|desc” text within the
+chat-messages-list and +threads-messages-list table entries so Markdown treats
it as cell content and preserves the intended table structure.
In `@tests/cli_e2e/base/base_limit_dryrun_test.go`:
- Around line 90-101: Strengthen
TestBaseListDryRunValidatesPageSizeAliasAsCanonicalLimit by asserting the
complete validation error contract: verify error.type is "validation",
error.subtype is "invalid_argument", and stdout is empty, while preserving the
existing error.param and message assertions.
In `@tests/cli_e2e/mail/mail_triage_dryrun_test.go`:
- Around line 45-70: Add a self-contained live E2E test alongside
TestMail_TriageDryRunUsesPageSizeAsExactMaxAlias that executes mail +triage with
the relevant --page-size/--max alias combinations against the configured test
mailbox, rather than inspecting dry-run request parameters. Assert the command
succeeds and verify the resulting triage behavior reflects the alias precedence
and exact page-size/max semantics.
- Around line 53-55: Update the test cases around the mail triage dry-run flag
parsing to preserve canonical --max precedence: when both --max and its
--page-size alias are supplied, expect the --max value regardless of argument
order. Change the “alias last” case to expect 7 while keeping the single-flag
and “canonical last” cases aligned with this behavior.
---
Nitpick comments:
In `@lint/flagcontract/scan.go`:
- Around line 65-71: Update the flag-contract lint documentation in
lint/README.md to describe the alias-flag check as a best-effort heuristic
rather than an authoritative rejection, noting that it may miss alternate alias
descriptions or hidden-value expressions and may match structurally similar
non-common.Flag literals. Keep the existing detection behavior unchanged.
In `@shortcuts/im/im_chat_messages_list.go`:
- Around line 121-124: Remove the redundant ValidatePageSizeTyped call from the
Execute function and let buildChatMessageListRequest remain the single page-size
validation site, preserving its existing typed --page-size error handling.
In `@shortcuts/im/im_chat_search.go`:
- Around line 322-333: Replace the literal fallback value in
buildSearchChatParams with the existing chatSearchDefaultPageSize constant,
while preserving the current handling of positive page-size values and page
tokens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a60d2b20-14e8-4749-b43d-ebb464649ed7
📒 Files selected for processing (102)
internal/flagalias/flagalias.gointernal/flagalias/flagalias_test.gointernal/output/emit.gointernal/output/emitter.gointernal/output/emitter_contract_test.gointernal/output/emitter_legacy_compat_test.gointernal/output/envelope.gointernal/output/testdata/runtime_context_legacy.golden.jsoninternal/qualitygate/cmd/manifest-export/collect.gointernal/qualitygate/cmd/manifest-export/collect_alias_test.gointernal/qualitygate/cmd/manifest-export/main_test.gointernal/qualitygate/manifest/io_test.gointernal/qualitygate/manifest/schema.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/refs.gointernal/qualitygate/rules/refs_test.golint/README.mdlint/flagcontract/scan.golint/flagcontract/scan_test.golint/main.goshortcuts/base/base_dryrun_ops_test.goshortcuts/base/base_execute_test.goshortcuts/base/base_resolve.goshortcuts/base/base_resolve_test.goshortcuts/base/base_shortcut_helpers.goshortcuts/base/base_shortcuts_test.goshortcuts/base/field_list.goshortcuts/base/field_ops.goshortcuts/base/field_search_options.goshortcuts/base/record_list.goshortcuts/base/record_ops.goshortcuts/base/record_query.goshortcuts/base/record_search.goshortcuts/base/table_list.goshortcuts/base/table_ops.goshortcuts/base/view_list.goshortcuts/base/view_ops.goshortcuts/common/flag_aliases.goshortcuts/common/flag_context.goshortcuts/common/page_all_flags.goshortcuts/common/paginate_into.goshortcuts/common/paginate_into_test.goshortcuts/common/runner.goshortcuts/common/runner_flag_alias_test.goshortcuts/common/runner_normalize_test.goshortcuts/common/types.goshortcuts/im/builders_test.goshortcuts/im/coverage_additional_test.goshortcuts/im/helpers.goshortcuts/im/im_chat_list.goshortcuts/im/im_chat_list_test.goshortcuts/im/im_chat_members_list.goshortcuts/im/im_chat_messages_list.goshortcuts/im/im_chat_messages_list_test.goshortcuts/im/im_chat_search.goshortcuts/im/im_chat_search_test.goshortcuts/im/im_feed_group_item_test.goshortcuts/im/im_feed_group_list.goshortcuts/im/im_feed_group_list_item.goshortcuts/im/im_flag_aliases_test.goshortcuts/im/im_flag_list.goshortcuts/im/im_list_page_all_test.goshortcuts/im/im_list_pagination.goshortcuts/im/im_messages_mget.goshortcuts/im/im_messages_search.goshortcuts/im/im_page_size_validation_test.goshortcuts/im/im_search_notice_test.goshortcuts/im/im_threads_messages_list.goshortcuts/im/im_threads_messages_list_test.goshortcuts/im/mute_filter.goshortcuts/im/mute_filter_test.goshortcuts/im/sort_flags.goshortcuts/im/sort_flags_test.goshortcuts/im/with_sender_name_test.goshortcuts/mail/mail_triage.goshortcuts/mail/mail_triage_test.goshortcuts/sheets/flag_ergonomics.goshortcuts/sheets/lark_sheet_history_list.goshortcuts/sheets/shortcuts.goshortcuts/sheets/shortcuts_alias_test.goshortcuts/slides/presentation_flag.goshortcuts/slides/shortcuts.goshortcuts/slides/shortcuts_alias_test.goshortcuts/slides/slides_history.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_replace_pages.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_screenshot.goshortcuts/slides/slides_xml_get.goskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-chat-list.mdskills/lark-im/references/lark-im-chat-members-list.mdskills/lark-im/references/lark-im-chat-messages-list.mdskills/lark-im/references/lark-im-chat-search.mdskills/lark-im/references/lark-im-feed-shortcut-list.mdskills/lark-im/references/lark-im-threads-messages-list.mdtests/cli_e2e/base/base_limit_dryrun_test.gotests/cli_e2e/im/im_flag_aliases_dryrun_test.gotests/cli_e2e/im/im_list_page_all_dryrun_test.gotests/cli_e2e/im/im_page_all_live_test.gotests/cli_e2e/mail/mail_triage_dryrun_test.gotests/cli_e2e/sheets/sheets_token_alias_dryrun_test.go
💤 Files with no reviewable changes (3)
- shortcuts/base/base_shortcut_helpers.go
- shortcuts/base/base_dryrun_ops_test.go
- internal/output/testdata/runtime_context_legacy.golden.json
Comment on lines
+105
to
+115
| if nextPageToken == "" { | ||
| return meta, invalidPageCursor("response reports more pages but returned no page token") | ||
| } | ||
| if _, repeated := seen[nextPageToken]; repeated { | ||
| return meta, invalidPageCursor("response repeated page token %q, which would paginate forever", nextPageToken) | ||
| } | ||
|
|
||
| meta.NextToken = nextPageToken | ||
| if pageNumber == policy.maxPages { | ||
| return meta, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Repeated-cursor detection also fires for single-page reads.
The default policy is maxPages: 1. In that mode no second request is ever made, so a repeated cursor cannot cause an unbounded walk. The check at Line 108 still runs, and seen already contains the caller's --page-token. An endpoint that echoes the request cursor in page_token therefore turns a previously successful single-page read into a CategoryInternal/SubtypeInvalidResponse failure. Move the budget stop before the repeated-cursor check so the check only guards runs that will issue another request.
🐛 Proposed fix: stop at the page budget before diagnosing the cursor
if nextPageToken == "" {
return meta, invalidPageCursor("response reports more pages but returned no page token")
}
+ meta.NextToken = nextPageToken
+ if pageNumber == policy.maxPages {
+ return meta, nil
+ }
if _, repeated := seen[nextPageToken]; repeated {
return meta, invalidPageCursor("response repeated page token %q, which would paginate forever", nextPageToken)
}
-
- meta.NextToken = nextPageToken
- if pageNumber == policy.maxPages {
- return meta, nil
- }paginate_into_test.go:313-349 pins the current behavior and must be updated with this change.
📝 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
| if nextPageToken == "" { | |
| return meta, invalidPageCursor("response reports more pages but returned no page token") | |
| } | |
| if _, repeated := seen[nextPageToken]; repeated { | |
| return meta, invalidPageCursor("response repeated page token %q, which would paginate forever", nextPageToken) | |
| } | |
| meta.NextToken = nextPageToken | |
| if pageNumber == policy.maxPages { | |
| return meta, nil | |
| } | |
| if nextPageToken == "" { | |
| return meta, invalidPageCursor("response reports more pages but returned no page token") | |
| } | |
| meta.NextToken = nextPageToken | |
| if pageNumber == policy.maxPages { | |
| return meta, nil | |
| } | |
| if _, repeated := seen[nextPageToken]; repeated { | |
| return meta, invalidPageCursor("response repeated page token %q, which would paginate forever", nextPageToken) | |
| } |
🤖 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/paginate_into.go` around lines 105 - 115, In the pagination
flow around the maxPages check, return successfully when pageNumber reaches
policy.maxPages before validating nextPageToken against seen. Keep requiring a
non-empty token when another request is allowed, and update the affected
paginate_into tests to expect echoed cursors to succeed for single-page reads.
Comment on lines
+38
to
+40
| factory := newTestFactory() | ||
| factory.IOStreams.In = strings.NewReader("resolved-input") | ||
| cmd := newTestShortcutCmd(s, factory) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate newTestFactory and newTestShortcutCmd, and check how the factory is constructed.
set -euo pipefail
echo "=== newTestFactory definition ==="
ast-grep run --pattern 'func newTestFactory($$$) { $$$ }' --lang go shortcuts/common
echo "=== newTestShortcutCmd definition ==="
ast-grep run --pattern 'func newTestShortcutCmd($$$) { $$$ }' --lang go shortcuts/common
echo "=== mountTestShortcut definition ==="
ast-grep run --pattern 'func mountTestShortcut($$$) { $$$ }' --lang go shortcuts/common
echo "=== TestFactory / LARKSUITE_CLI_CONFIG_DIR usage in shortcuts/common ==="
rg -nP --type=go -C 3 'cmdutil\.TestFactory|LARKSUITE_CLI_CONFIG_DIR' shortcuts/commonRepository: larksuite/cli
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== files under shortcuts/common ==="
git ls-files shortcuts/common
echo "=== symbols containing testFactory/testShortcutCmd/mountTestShortcut ==="
rg -n --type=go 'newTestFactory|newTestShortcutCmd|mountTestShortcut' shortcuts/common || true
echo "=== relevant file around new helper definitions ==="
if [ -f shortcuts/common/runner_normalize_test.go ]; then
wc -l shortcuts/common/runner_normalize_test.go
sed -n '1,120p' shortcuts/common/runner_normalize_test.go | cat -n
fi
echo "=== factory references in shortcuts/common test files ==="
rg -n --type=go 'Factory\{|LARKSUITE_CLI_CONFIG_DIR|test.Config|Config\(\)' shortcuts/common || trueRepository: larksuite/cli
Length of output: 10538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== cmdutil.TestFactory implementation references ==="
rg -n --type=go 'func TestFactory|func NewFactory|LARKSUITE_CLI_CONFIG_DIR' . --glob '*cmdutil*.go' | head -80 || true
echo "=== cmdutil files ==="
git ls-files | rg 'cmdutil|factory' | head -80
echo "=== runner.go config call context ==="
sed -n '880,960p' shortcuts/common/runner.go | cat -n
echo "=== runner_jq_test helper context ==="
sed -n '210,235p' shortcuts/common/runner_jq_test.go | cat -nRepository: larksuite/cli
Length of output: 5869
Use cmdutil.TestFactory in the shortcut tests.
newTestFactory returns &cmdutil.Factory{} directly, so runShortcut reads test-created config instead of isolated CLI config state. Set LARKSUITE_CLI_CONFIG_DIR to t.TempDir() with t.Setenv and create the factory through cmdutil.TestFactory(t, config).
🤖 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_normalize_test.go` around lines 38 - 40, Update the
shortcut test setup around newTestFactory and newTestShortcutCmd to use
cmdutil.TestFactory(t, config) instead of constructing an empty factory
directly. Set LARKSUITE_CLI_CONFIG_DIR to t.TempDir() with t.Setenv before
creating the factory, while preserving the existing input stream and command
setup.
Source: Coding guidelines
Comment on lines
+79
to
+81
| if err := runShortcut(cmd, factory, s, true); err == nil { | ||
| t.Fatal("runShortcut() error = nil") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the error identity, not only that an error occurred.
The test accepts any non-nil error. Identity resolution, configuration load, or the scope check can each fail before Normalize runs. In that case Validate and Execute never run, their t.Fatal guards never fire, and the test passes without exercising the normalization failure path.
Assert the typed metadata and the param, as this file already does at lines 108-114.
💚 Proposed fix to pin the error identity
- if err := runShortcut(cmd, factory, s, true); err == nil {
- t.Fatal("runShortcut() error = nil")
- }
+ err := runShortcut(cmd, factory, s, true)
+ var validationErr *errs.ValidationError
+ if !errors.As(err, &validationErr) {
+ t.Fatalf("error = %T %v, want typed validation error", err, err)
+ }
+ if validationErr.Param != "--legacy" {
+ t.Fatalf("param = %q, want --legacy", validationErr.Param)
+ }As per coding guidelines: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param) and verify cause preservation rather than relying only on message substrings."
📝 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
| if err := runShortcut(cmd, factory, s, true); err == nil { | |
| t.Fatal("runShortcut() error = nil") | |
| } | |
| err := runShortcut(cmd, factory, s, true) | |
| var validationErr *errs.ValidationError | |
| if !errors.As(err, &validationErr) { | |
| t.Fatalf("error = %T %v, want typed validation error", err, err) | |
| } | |
| if validationErr.Param != "--legacy" { | |
| t.Fatalf("param = %q, want --legacy", validationErr.Param) | |
| } |
🤖 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_normalize_test.go` around lines 79 - 81, Update the
error assertion around runShortcut in the normalization failure test to inspect
errs.ProblemOf rather than only checking for a non-nil error. Assert the
expected category, subtype, and param values, and verify the original cause is
preserved, following the existing typed-metadata assertions in this file’s test
around lines 108-114.
Source: Coding guidelines
| } | ||
|
|
||
| func TestMountedShortcutNormalizeDoesNotExpandCobraPreRun(t *testing.T) { | ||
| normalizeCalled := false |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The normalizeCalled assertion cannot fail.
The test calls only ParseFlags and ValidateRequiredFlags. It never calls runShortcut, which is the only caller of Normalize. So normalizeCalled is always false, independent of the implementation. The assertion at lines 147-149 passes even if the normalization ordering is reverted.
The assertions at lines 141-146 are the real contract checks and are correct. Either drop the flag, or make the test invoke the pipeline so the ordering claim is actually exercised.
💚 Proposed fix to remove the tautological assertion
func TestMountedShortcutNormalizeDoesNotExpandCobraPreRun(t *testing.T) {
- normalizeCalled := false
shortcut := Shortcut{
Service: "test", Command: "+normalize-required", Description: "x",
Flags: []Flag{
{Name: "canonical", Required: true},
{Name: "legacy", Hidden: true},
},
Normalize: func(_ context.Context, flags *FlagContext) error {
- normalizeCalled = true
if !flags.Changed("legacy") || flags.Changed("canonical") {
return nil
}
return flags.SetCanonicalFrom("legacy", "canonical", flags.Str("legacy"))
},
Execute: func(context.Context, *RuntimeContext) error { return nil },
}
cmd := mountTestShortcut(t, shortcut)
if err := cmd.ParseFlags([]string{"--legacy", "accepted"}); err != nil {
t.Fatal(err)
}
if cmd.PreRunE != nil || cmd.PreRun != nil {
t.Fatal("Normalize must not install or take over Cobra PreRun hooks")
}
if err := cmd.ValidateRequiredFlags(); err == nil {
t.Fatal("a business Normalize hook must not satisfy Cobra Required")
}
- if normalizeCalled {
- t.Fatal("Normalize ran before Cobra Required validation")
- }
}As per coding guidelines: "contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
Also applies to: 147-149
🤖 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_normalize_test.go` at line 121, Remove the
ineffective normalizeCalled flag and its assertion from the test, since the
current flow never invokes Normalize through runShortcut. Keep the direct
contract assertions around ParseFlags and ValidateRequiredFlags unchanged, or
update the test to exercise runShortcut if ordering coverage is required.
Source: Coding guidelines
Comment on lines
+58
to
+67
| legacy := flags.Str(legacyName) | ||
| if legacy == "" { | ||
| if flags.Changed(canonicalName) { | ||
| return nil | ||
| } | ||
| if err := flags.SetCanonicalFrom(legacyName, canonicalName, ""); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Empty legacy value silently overrides the canonical flag's default.
When --sort-type is explicitly set to an empty string, legacy == "" is not a member of values (same as any other unrecognized value), but this branch skips validation and calls flags.SetCanonicalFrom(legacyName, canonicalName, "") instead of returning an error. This forces the canonical flag from its declared default (e.g. "create_time") to an explicit "".
Contrast this with the non-empty path at Line 79: an unrecognized value such as "unexpected" correctly returns a validation error attributed to --sort-type. An empty value is equally unrecognized but is silently accepted, and the resulting invalid state surfaces later (if at all) as an Enum failure attributed to --sort, not --sort-type, which defeats the "validation attribution" design goal.
Treat an empty explicit legacy value the same way as any other value not in values, or treat it as a no-op (leave the canonical flag at its own default). Do not force the canonical flag into an unsupported empty state.
The test at shortcuts/im/sort_flags_test.go Lines 46-47 currently encodes this behavior and needs updating alongside this fix.
🐛 Proposed fix to validate empty legacy values consistently
legacy := flags.Str(legacyName)
+ allowed := legacySortValues(values)
if legacy == "" {
- if flags.Changed(canonicalName) {
- return nil
- }
- if err := flags.SetCanonicalFrom(legacyName, canonicalName, ""); err != nil {
- return err
- }
- return nil
+ return common.ValidationErrorf("invalid value %q for --%s, allowed: %s", legacy, legacyName, strings.Join(allowed, ", ")).
+ WithParam("--" + legacyName)
}
- allowed := legacySortValues(values)
canonical := ""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/im/sort_flags.go` around lines 58 - 67, Update the legacy-flag
handling around flags.Str and SetCanonicalFrom so an explicitly provided empty
legacy value is not propagated to the canonical flag. Validate it like other
unrecognized values and attribute the error to the legacy flag, or leave the
canonical default unchanged; then update the corresponding test case in
sort_flags_test.go to reflect the corrected behavior.
Source: Coding guidelines
Comment on lines
+109
to
+117
| | [`+chat-messages-list`](references/lark-im-chat-messages-list.md) | List messages in a chat or P2P conversation; user/bot; accepts --chat-id or --user-id, resolves P2P chat_id, supports time range, --order asc|desc sorting, auto-pagination | | ||
| | [`+chat-search`](references/lark-im-chat-search.md) | Search visible group chats by --query keyword and/or --member-ids; user/bot; e.g. look up chat_id by group name; supports type filters, sorting, auto-pagination, and --exclude-muted (user identity only) | | ||
| | [`+chat-update`](references/lark-im-chat-update.md) | Update group chat name or description; user/bot; updates a chat's name or description | | ||
| | [`+messages-mget`](references/lark-im-messages-mget.md) | Batch get messages by IDs; user/bot; fetches up to 50 om_ message IDs, formats sender names, expands thread replies | | ||
| | [`+messages-reply`](references/lark-im-messages-reply.md) | Reply to a message (supports thread replies); user/bot; supports text/markdown/post/media replies, reply-in-thread, idempotency key | | ||
| | [`+messages-resources-download`](references/lark-im-messages-resources-download.md) | Download images/files from a message; user/bot; supports automatic chunked download for large files (8MB chunks), auto-detects file extension from Content-Type | | ||
| | [`+messages-search`](references/lark-im-messages-search.md) | Search messages across chats (supports keyword, sender, time range filters) with user identity; user-only; filters by chat/sender/attachment/time, supports auto-pagination via `--page-all` / `--page-limit`, enriches results via batched mget and chats batch_query | | ||
| | [`+messages-send`](references/lark-im-messages-send.md) | Send a message to a chat or direct message; user/bot; sends to chat-id or user-id with text/markdown/post/media, supports idempotency key | | ||
| | [`+threads-messages-list`](references/lark-im-threads-messages-list.md) | List messages in a thread; user/bot; accepts om_/omt_ input, resolves message IDs to thread_id, supports sort/pagination | | ||
| | [`+threads-messages-list`](references/lark-im-threads-messages-list.md) | List messages in a thread; user/bot; accepts om_/omt_ input, resolves message IDs to thread_id, supports --order asc|desc sorting, auto-pagination | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the literal pipe in "asc|desc" to fix the broken table rows.
Lines 109 and 117 contain unescaped asc|desc inside a table cell. Markdown table parsers split cells on |, so these rows render with an extra, misplaced column instead of the intended 2-column layout.
📝 Proposed fix to escape the pipe character
-| [`+chat-messages-list`](references/lark-im-chat-messages-list.md) | List messages in a chat or P2P conversation; user/bot; accepts --chat-id or --user-id, resolves P2P chat_id, supports time range, --order asc|desc sorting, auto-pagination |
+| [`+chat-messages-list`](references/lark-im-chat-messages-list.md) | List messages in a chat or P2P conversation; user/bot; accepts --chat-id or --user-id, resolves P2P chat_id, supports time range, --order asc\|desc sorting, auto-pagination |-| [`+threads-messages-list`](references/lark-im-threads-messages-list.md) | List messages in a thread; user/bot; accepts om_/omt_ input, resolves message IDs to thread_id, supports --order asc|desc sorting, auto-pagination |
+| [`+threads-messages-list`](references/lark-im-threads-messages-list.md) | List messages in a thread; user/bot; accepts om_/omt_ input, resolves message IDs to thread_id, supports --order asc\|desc sorting, auto-pagination |📝 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
| | [`+chat-messages-list`](references/lark-im-chat-messages-list.md) | List messages in a chat or P2P conversation; user/bot; accepts --chat-id or --user-id, resolves P2P chat_id, supports time range, --order asc|desc sorting, auto-pagination | | |
| | [`+chat-search`](references/lark-im-chat-search.md) | Search visible group chats by --query keyword and/or --member-ids; user/bot; e.g. look up chat_id by group name; supports type filters, sorting, auto-pagination, and --exclude-muted (user identity only) | | |
| | [`+chat-update`](references/lark-im-chat-update.md) | Update group chat name or description; user/bot; updates a chat's name or description | | |
| | [`+messages-mget`](references/lark-im-messages-mget.md) | Batch get messages by IDs; user/bot; fetches up to 50 om_ message IDs, formats sender names, expands thread replies | | |
| | [`+messages-reply`](references/lark-im-messages-reply.md) | Reply to a message (supports thread replies); user/bot; supports text/markdown/post/media replies, reply-in-thread, idempotency key | | |
| | [`+messages-resources-download`](references/lark-im-messages-resources-download.md) | Download images/files from a message; user/bot; supports automatic chunked download for large files (8MB chunks), auto-detects file extension from Content-Type | | |
| | [`+messages-search`](references/lark-im-messages-search.md) | Search messages across chats (supports keyword, sender, time range filters) with user identity; user-only; filters by chat/sender/attachment/time, supports auto-pagination via `--page-all` / `--page-limit`, enriches results via batched mget and chats batch_query | | |
| | [`+messages-send`](references/lark-im-messages-send.md) | Send a message to a chat or direct message; user/bot; sends to chat-id or user-id with text/markdown/post/media, supports idempotency key | | |
| | [`+threads-messages-list`](references/lark-im-threads-messages-list.md) | List messages in a thread; user/bot; accepts om_/omt_ input, resolves message IDs to thread_id, supports sort/pagination | | |
| | [`+threads-messages-list`](references/lark-im-threads-messages-list.md) | List messages in a thread; user/bot; accepts om_/omt_ input, resolves message IDs to thread_id, supports --order asc|desc sorting, auto-pagination | | |
| | [`+chat-messages-list`](references/lark-im-chat-messages-list.md) | List messages in a chat or P2P conversation; user/bot; accepts --chat-id or --user-id, resolves P2P chat_id, supports time range, --order asc\|desc sorting, auto-pagination | | |
| | [`+chat-search`](references/lark-im-chat-search.md) | Search visible group chats by --query keyword and/or --member-ids; user/bot; e.g. look up chat_id by group name; supports type filters, sorting, auto-pagination, and --exclude-muted (user identity only) | | |
| | [`+chat-update`](references/lark-im-chat-update.md) | Update group chat name or description; user/bot; updates a chat's name or description | | |
| | [`+messages-mget`](references/lark-im-messages-mget.md) | Batch get messages by IDs; user/bot; fetches up to 50 om_ message IDs, formats sender names, expands thread replies | | |
| | [`+messages-reply`](references/lark-im-messages-reply.md) | Reply to a message (supports thread replies); user/bot; supports text/markdown/post/media replies, reply-in-thread, idempotency key | | |
| | [`+messages-resources-download`](references/lark-im-messages-resources-download.md) | Download images/files from a message; user/bot; supports automatic chunked download for large files (8MB chunks), auto-detects file extension from Content-Type | | |
| | [`+messages-search`](references/lark-im-messages-search.md) | Search messages across chats (supports keyword, sender, time range filters) with user identity; user-only; filters by chat/sender/attachment/time, supports auto-pagination via `--page-all` / `--page-limit`, enriches results via batched mget and chats batch_query | | |
| | [`+messages-send`](references/lark-im-messages-send.md) | Send a message to a chat or direct message; user/bot; sends to chat-id or user-id with text/markdown/post/media, supports idempotency key | | |
| | [`+threads-messages-list`](references/lark-im-threads-messages-list.md) | List messages in a thread; user/bot; accepts om_/omt_ input, resolves message IDs to thread_id, supports --order asc\|desc sorting, auto-pagination | |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 109-109: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 117-117: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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 `@skills/lark-im/SKILL.md` around lines 109 - 117, Escape the literal pipe in
the “asc|desc” text within the +chat-messages-list and +threads-messages-list
table entries so Markdown treats it as cell content and preserves the intended
table structure.
Source: Linters/SAST tools
Comment on lines
+90
to
+101
| func TestBaseListDryRunValidatesPageSizeAliasAsCanonicalLimit(t *testing.T) { | ||
| setBaseDryRunConfigEnv(t) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| t.Cleanup(cancel) | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{ | ||
| "base", "+table-list", | ||
| "--base-token", "app_x", | ||
| "--limit", "20", | ||
| "--page-size", "40", | ||
| "--dry-run", | ||
| }, | ||
| Args: []string{"base", "+table-list", "--base-token", "app_x", "--page-size", "101", "--dry-run"}, | ||
| DefaultAs: "bot", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 2) | ||
|
|
||
| require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String(), result.Stderr) | ||
| require.Equal(t, "invalid_argument", gjson.Get(result.Stderr, "error.subtype").String(), result.Stderr) | ||
| require.Equal(t, "--page-size", gjson.Get(result.Stderr, "error.param").String(), result.Stderr) | ||
| require.Contains(t, gjson.Get(result.Stderr, "error.message").String(), "mutually exclusive") | ||
| require.Empty(t, result.Stdout) | ||
| require.Equal(t, "--limit", gjson.Get(result.Stderr, "error.param").String(), result.Stderr) | ||
| require.Contains(t, gjson.Get(result.Stderr, "error.message").String(), "must be between 1 and 100") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the complete validation error contract.
This test only checks error.param and the message. It will pass if the error category or subtype changes. It will also pass if validation output pollutes stdout. Assert error.type == "validation", error.subtype == "invalid_argument", and empty stdout.
As per coding guidelines, error-path tests must assert typed metadata. Based on learnings, validation E2E failures must write the typed JSON envelope to stderr and leave stdout empty.
🤖 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 `@tests/cli_e2e/base/base_limit_dryrun_test.go` around lines 90 - 101,
Strengthen TestBaseListDryRunValidatesPageSizeAliasAsCanonicalLimit by asserting
the complete validation error contract: verify error.type is "validation",
error.subtype is "invalid_argument", and stdout is empty, while preserving the
existing error.param and message assertions.
Sources: Coding guidelines, Learnings
Comment on lines
+45
to
+70
| func TestMail_TriageDryRunUsesPageSizeAsExactMaxAlias(t *testing.T) { | ||
| setMailTriageDryRunEnv(t) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| args []string | ||
| want int64 | ||
| }{ | ||
| {name: "alias only", args: []string{"--page-size", "5"}, want: 5}, | ||
| {name: "alias last", args: []string{"--max", "7", "--page-size", "5"}, want: 5}, | ||
| {name: "canonical last", args: []string{"--page-size", "5", "--max", "7"}, want: 7}, | ||
| } | ||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| t.Cleanup(cancel) | ||
| args := []string{"mail", "+triage", "--mailbox", "me"} | ||
| args = append(args, test.args...) | ||
| args = append(args, "--dry-run") | ||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{Args: args, DefaultAs: "user"}) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| require.Equal(t, test.want, clie2e.DryRunGet(result.Stdout, "api.0.params.page_size").Int(), result.Stdout) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 --glob '*_test.go' '\+triage|Mail.*Triage|mail.*triage' tests/cli_e2e/mailRepository: larksuite/cli
Length of output: 6191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mail draft send workflow triage section =="
sed -n '120,175p' tests/cli_e2e/mail/mail_draft_send_workflow_test.go
echo
echo "== mail e2e bootstrap/setup references =="
rg -n --glob '*_test.go' 'mailboxID|Larksuite_CLI_TEST_MAIL_|create|bot|credentials|New.*Mail|TestMain|Setup|teardown|defer\(|cleanup|Tearing|Cleanup' tests/cli_e2e/mail
echo
echo "== triage dryrun test full =="
sed -n '1,90p' tests/cli_e2e/mail/mail_triage_dryrun_test.go
echo
echo "== other +triage usages in e2e tests =="
rg -n '\+triage|DryRunGet|page_size|max_alias|--dry-run' tests/cli_e2e --glob '*_test.go'Repository: larksuite/cli
Length of output: 50370
Add live E2E coverage for the mail triage alias behavior.
These dry-run tests only inspect emitted request params. Add a self-contained live mail +triage workflow that exercises the alias through command execution and asserts the changed behavior.
🤖 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 `@tests/cli_e2e/mail/mail_triage_dryrun_test.go` around lines 45 - 70, Add a
self-contained live E2E test alongside
TestMail_TriageDryRunUsesPageSizeAsExactMaxAlias that executes mail +triage with
the relevant --page-size/--max alias combinations against the configured test
mailbox, rather than inspecting dry-run request parameters. Assert the command
succeeds and verify the resulting triage behavior reflects the alias precedence
and exact page-size/max semantics.
Source: Coding guidelines
Comment on lines
+53
to
+55
| {name: "alias only", args: []string{"--page-size", "5"}, want: 5}, | ||
| {name: "alias last", args: []string{"--max", "7", "--page-size", "5"}, want: 5}, | ||
| {name: "canonical last", args: []string{"--page-size", "5", "--max", "7"}, want: 7}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve canonical --max precedence.
--page-size is documented here as an exact alias of --max. The alias last case expects --page-size to override canonical --max. This conflicts with the PR objective that requires canonical precedence.
Expect 7 when both flags are present, regardless of argument order.
Proposed test update
- {name: "alias last", args: []string{"--max", "7", "--page-size", "5"}, want: 5},
+ {name: "canonical wins when alias is last", args: []string{"--max", "7", "--page-size", "5"}, want: 7},📝 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
| {name: "alias only", args: []string{"--page-size", "5"}, want: 5}, | |
| {name: "alias last", args: []string{"--max", "7", "--page-size", "5"}, want: 5}, | |
| {name: "canonical last", args: []string{"--page-size", "5", "--max", "7"}, want: 7}, | |
| {name: "alias only", args: []string{"--page-size", "5"}, want: 5}, | |
| {name: "canonical wins when alias is last", args: []string{"--max", "7", "--page-size", "5"}, want: 7}, | |
| {name: "canonical last", args: []string{"--page-size", "5", "--max", "7"}, want: 7}, |
🤖 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 `@tests/cli_e2e/mail/mail_triage_dryrun_test.go` around lines 53 - 55, Update
the test cases around the mail triage dry-run flag parsing to preserve canonical
--max precedence: when both --max and its --page-size alias are supplied, expect
the --max value regardless of argument order. Change the “alias last” case to
expect 7 while keeping the single-flag and “canonical last” cases aligned with
this behavior.
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
2 participants
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 a framework-level contract for exact flag-name aliases and a shared, format-aware pagination pipeline for IM list commands. This replaces command-local compatibility plumbing and copied page loops while preserving each domain's ownership of true semantic conversions and keeping one-page behavior as the default.
Changes
--page-all,--page-limit, and--page-delaycontracts to IM chat, chat-search, chat-message, and thread-message lists.Impact
--page-allcallers get consistent safety limits, resumability, and metadata across supported IM commands.Test Plan
make unit-testgo vet ./...gofmt -l .produces no outputgo mod tidyleavesgo.modandgo.sumunchangedgolangci-lint v2.1.6 run --new-from-rev=origin/mainreports 0 issuesmake buildRelated Issues
Summary by CodeRabbit