feat: add framework flag aliases and unified IM pagination - #2146
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:
📝 WalkthroughWalkthroughThe change adds declarative flag aliases, shared shortcut normalization, automatic IM pagination, pagination-aware output metadata, manifest alias support, and flag-contract linting. It 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: 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 |
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.
a435d6f to
e32d7cb
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@a33b0911d76502d1310ae9c4903db2e15ae94145🧩 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 940 +9
Lines 99162 99876 +714
==========================================
+ Hits 74937 75564 +627
- Misses 18501 18530 +29
- Partials 5724 5782 +58 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/root.go`:
- Around line 606-614: The flagDidYouMean validation error must preserve the
original parse error and verify its typed metadata. In cmd/root.go lines
606-614, attach ferr as the cause when constructing validationErr. In
cmd/flag_suggest_test.go lines 129-140, assert category and subtype via
errs.ProblemOf, extract *errs.ValidationError with errors.As to verify Param,
and assert the returned error preserves parseErr as its cause.
In `@lint/flagcontract/scan_test.go`:
- Around line 32-37: Extend the assertions in the test around the existing
got[0].Rule check to verify that got[0].File equals "shortcuts/demo/demo.go".
Keep the current violation-count and rule assertions, and directly validate the
expected source file for the reported violation.
In `@shortcuts/mail/flag_suggest.go`:
- Around line 57-65: Update the fallback path in the SetFlagErrorFunc callback
to handle a nil result from svc.FlagErrorFunc(). After the
parseUnknownToken/flagSuggestErrorFunc path, return err directly when inherited
is nil; otherwise continue delegating to inherited(c, err).
In `@tests/cli_e2e/im/im_list_page_all_dryrun_test.go`:
- Around line 60-69: The existing dry-run coverage only checks the initial
request and must be supplemented with live E2E coverage for --page-all. Add a
bot-authenticated create/use/cleanup workflow in the relevant test flow that
produces multiple pages, then verify continuation requests use the returned
cursor and that the final output accumulates results across pages.
🪄 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: 671b2e38-c06f-4a41-82b6-e1a8c28cb767
📒 Files selected for processing (51)
cmd/flag_suggest_test.gocmd/root.gointernal/flagalias/error_attribution.gointernal/flagalias/error_attribution_test.gointernal/flagalias/flagalias.gointernal/flagalias/flagalias_test.gointernal/output/envelope.gointernal/output/format.gointernal/output/format_test.golint/flagcontract/scan.golint/flagcontract/scan_test.goshortcuts/base/base_execute_test.goshortcuts/common/flag_error_attribution.goshortcuts/common/page_all_flags.goshortcuts/common/paginate_into.goshortcuts/common/paginate_into_test.goshortcuts/common/runner.goshortcuts/common/runner_flag_alias_test.goshortcuts/im/helpers.goshortcuts/im/im_chat_list.goshortcuts/im/im_chat_members_list.goshortcuts/im/im_chat_members_list_test.goshortcuts/im/im_chat_messages_list.goshortcuts/im/im_chat_search.goshortcuts/im/im_feed_group_item_test.goshortcuts/im/im_feed_group_list.goshortcuts/im/im_feed_group_list_item.goshortcuts/im/im_feed_group_list_test.goshortcuts/im/im_flag_aliases_test.goshortcuts/im/im_flag_list.goshortcuts/im/im_flag_test.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_threads_messages_list.goshortcuts/im/im_threads_messages_list_test.goshortcuts/mail/flag_suggest.goshortcuts/mail/flag_suggest_test.goskills/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-group-list-item.mdskills/lark-im/references/lark-im-feed-group-list.mdskills/lark-im/references/lark-im-flag-list.mdskills/lark-im/references/lark-im-messages-search.mdskills/lark-im/references/lark-im-threads-messages-list.mdtests/cli_e2e/base/base_limit_dryrun_test.gotests/cli_e2e/im/im_list_page_all_dryrun_test.gotests/cli_e2e/im/im_page_all_live_test.go
💤 Files with no reviewable changes (1)
- shortcuts/im/im_threads_messages_list_test.go
🚧 Files skipped from review as they are similar to previous changes (22)
- shortcuts/base/base_execute_test.go
- skills/lark-im/references/lark-im-chat-search.md
- shortcuts/im/im_messages_search.go
- skills/lark-im/references/lark-im-chat-list.md
- shortcuts/common/paginate_into_test.go
- shortcuts/im/helpers.go
- shortcuts/im/im_chat_messages_list.go
- shortcuts/common/page_all_flags.go
- shortcuts/im/im_messages_mget.go
- tests/cli_e2e/base/base_limit_dryrun_test.go
- shortcuts/im/im_threads_messages_list.go
- internal/output/envelope.go
- skills/lark-im/references/lark-im-chat-messages-list.md
- shortcuts/im/im_chat_list.go
- shortcuts/common/runner.go
- shortcuts/im/im_list_pagination.go
- shortcuts/common/paginate_into.go
- shortcuts/im/im_flag_aliases_test.go
- shortcuts/im/im_list_page_all_test.go
- internal/flagalias/flagalias_test.go
- shortcuts/im/im_chat_search.go
- tests/cli_e2e/im/im_page_all_live_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/im/im_threads_messages_list_test.go (1)
32-33: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle mounted-runtime errors before using the runtimes.
newMountedIMRuntimereturns an error, but these calls discard it. If mounting or argument parsing fails, the test can panic or report an unclear assertion failure instead of the setup error. Capture the error and fail witht.Fatalfbefore callingDryRunorStr.Proposed fix
- newRT, _ := newMountedIMRuntime(t, &ImThreadsMessagesList, "--thread", "omt_test", "--order", dir) - oldRT, _ := newMountedIMRuntime(t, &ImThreadsMessagesList, "--thread", "omt_test", "--sort", dir) + newRT, err := newMountedIMRuntime(t, &ImThreadsMessagesList, "--thread", "omt_test", "--order", dir) + if err != nil { + t.Fatalf("mount canonical runtime: %v", err) + } + oldRT, err := newMountedIMRuntime(t, &ImThreadsMessagesList, "--thread", "omt_test", "--sort", dir) + if err != nil { + t.Fatalf("mount alias runtime: %v", err) + } ... - rt, _ := newMountedIMRuntime(t, &ImThreadsMessagesList, test.args...) + rt, err := newMountedIMRuntime(t, &ImThreadsMessagesList, test.args...) + if err != nil { + t.Fatalf("mount runtime: %v", err) + }Also applies to: 52-52
🤖 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_threads_messages_list_test.go` around lines 32 - 33, Update the test setup around newMountedIMRuntime calls to capture each returned error and call t.Fatalf with the setup error before using newRT or oldRT in DryRun or Str. Apply the same handling to both mounted-runtime initializations, including the additional occurrence referenced by the review.
🤖 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.
Outside diff comments:
In `@shortcuts/im/im_threads_messages_list_test.go`:
- Around line 32-33: Update the test setup around newMountedIMRuntime calls to
capture each returned error and call t.Fatalf with the setup error before using
newRT or oldRT in DryRun or Str. Apply the same handling to both mounted-runtime
initializations, including the additional occurrence referenced by the review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c51d1e28-ad43-4e5d-bb38-902bc2427f57
📒 Files selected for processing (11)
internal/qualitygate/cmd/manifest-export/main_test.golint/README.mdshortcuts/im/im_chat_messages_list.goshortcuts/im/im_chat_messages_list_test.goshortcuts/im/im_flag_aliases_test.goshortcuts/im/im_threads_messages_list.goshortcuts/im/im_threads_messages_list_test.goshortcuts/im/sort_flags.goskills/lark-im/SKILL.mdskills/lark-mail/references/lark-mail-triage.mdtests/cli_e2e/im/im_flag_aliases_dryrun_test.go
💤 Files with no reviewable changes (1)
- shortcuts/im/sort_flags.go
🚧 Files skipped from review as they are similar to previous changes (8)
- lint/README.md
- skills/lark-im/SKILL.md
- internal/qualitygate/cmd/manifest-export/main_test.go
- tests/cli_e2e/im/im_flag_aliases_dryrun_test.go
- shortcuts/im/im_threads_messages_list.go
- shortcuts/im/im_chat_messages_list.go
- shortcuts/im/im_flag_aliases_test.go
- shortcuts/im/im_chat_messages_list_test.go
Summary
Add framework-level exact flag aliases and a shared, format-aware pagination pipeline for IM list commands. Semantic conversions remain business-owned.
Changes
--page-all,--page-limit, and--page-delayhandling to supported IM list commands.Behavior changes
--page-tokensets the start cursor;--page-allcontinues from it.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