feat: add IM read status shortcuts - #2318
Open
os-zzw wants to merge 8 commits into
Open
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds IM shortcuts for paginated message readers and batch message read-status queries. It adds identity-specific validation, pagination safeguards, user-scope error normalization, registration, documentation, and dry-run coverage. ChangesIM read-status operations
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant IMShortcut
participant LarkIMAPI
CLI->>IMShortcut: Validate flags and build request
IMShortcut->>LarkIMAPI: Query read users or batch read status
LarkIMAPI-->>IMShortcut: Return results and pagination data
IMShortcut-->>CLI: Output results and metadata
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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/im
os-zzw
marked this pull request as ready for review
August 12, 2026 07:44
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/im/im_message_read_users_test.go`:
- Around line 49-59: Add live and dry-run E2E tests for ImMessageReadUsers that
exercise read-user retrieval with both user and bot identities against the API,
rather than only validating static scopes. Make the tests self-contained by
creating required resources, invoking the shortcut, asserting results for each
identity and dry-run behavior, and cleaning up all resources afterward.
- Around line 140-144: Update the validation test table and its assertions
around validateMessageReadUsers to include the expected parameter name for each
case. Replace errs.ProblemOf-based checking with errors.As into
*errs.ValidationError, then assert Category, Subtype, and Param for every
validation error; do not add cause assertions for these direct branches.
- Around line 80-121: Extend TestFetchMessageReadUsersAggregatesPages with
regression coverage for both pagination termination guards in
fetchMessageReadUsers: verify a positive page limit stops requests at the
configured cap, and verify an unchanged page token stops pagination without
repeated requests. Add explicit request-count assertions for each scenario so
reverting either guard fails the tests.
In `@shortcuts/im/im_message_read_users.go`:
- Around line 120-127: Update the API response handling in the read-users
shortcut to decode the result into a typed page structure containing typed
read-user items and pagination metadata before aggregation. Replace direct map
access to data["items"] and common.PaginationMeta(data) with the typed fields,
and return decoding/type errors instead of silently treating malformed responses
as empty.
In `@shortcuts/im/im_messages_read_status_test.go`:
- Around line 48-50: Add a table-driven case in the read-status validation tests
alongside “more than fifty” that supplies exactly 50 IDs from
makeReadStatusMessageIDs and expects acceptance. Keep the existing 51-ID
rejection case so the boundary between accepted and rejected counts is covered.
- Around line 43-64: Strengthen
TestBuildMessagesReadStatusBodyRejectsInvalidInputs by asserting
problem.Category is errs.CategoryValidation, then use errors.As to extract an
*errs.ValidationError and verify its Param identifies --message-ids. Keep the
existing invalid-subtype assertion and apply these checks to each validation
error.
In `@tests/cli_e2e/im/message_read_status_dryrun_test.go`:
- Around line 17-84: The dry-run-only coverage for
TestIMMessagesReadStatusDryRun and TestIMMessageReadUsersDryRun is insufficient:
add self-contained live E2E workflows using UAT for +messages-read-status and
all supported identities for +message-read-users, including resource creation,
cleanup, and assertions on actual response fields. In
tests/cli_e2e/im/message_read_status_dryrun_test.go lines 17-84, add the live
tests; in tests/cli_e2e/im/coverage.md lines 31-33, keep these commands marked
uncovered or blocked until live coverage is implemented.
🪄 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: 1ff454dc-058e-45ae-8667-fd32f389ffac
📒 Files selected for processing (9)
shortcuts/im/im_message_read_users.goshortcuts/im/im_message_read_users_test.goshortcuts/im/im_messages_read_status.goshortcuts/im/im_messages_read_status_test.goshortcuts/im/shortcuts.goskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-message-read-status.mdtests/cli_e2e/im/coverage.mdtests/cli_e2e/im/message_read_status_dryrun_test.go
Comment on lines
+49
to
+59
| func TestMessageReadUsersScopesByIdentity(t *testing.T) { | ||
| if !reflect.DeepEqual(ImMessageReadUsers.ScopesForIdentity("user"), []string{"im:message:get_as_user"}) { | ||
| t.Fatalf("user scopes = %v", ImMessageReadUsers.ScopesForIdentity("user")) | ||
| } | ||
| if !reflect.DeepEqual(ImMessageReadUsers.ScopesForIdentity("bot"), []string{"im:message:readonly"}) { | ||
| t.Fatalf("bot scopes = %v", ImMessageReadUsers.ScopesForIdentity("bot")) | ||
| } | ||
| if !reflect.DeepEqual(ImMessageReadUsers.AuthTypes, []string{"user", "bot"}) { | ||
| t.Fatalf("AuthTypes = %v", ImMessageReadUsers.AuthTypes) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add live E2E coverage for this new shortcut.
These tests verify static scope configuration only. They do not verify read-user retrieval with user and bot identities against the API.
Add self-contained live E2E coverage that creates, uses, and cleans up its required resources.
As per coding guidelines, “new shortcuts require live E2E coverage.” Based on learnings, shortcut changes also require dry-run E2E coverage.
🤖 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_message_read_users_test.go` around lines 49 - 59, Add live
and dry-run E2E tests for ImMessageReadUsers that exercise read-user retrieval
with both user and bot identities against the API, rather than only validating
static scopes. Make the tests self-contained by creating required resources,
invoking the shortcut, asserting results for each identity and dry-run behavior,
and cleaning up all resources afterward.
Sources: Coding guidelines, Learnings
Comment on lines
+80
to
+121
| func TestFetchMessageReadUsersAggregatesPages(t *testing.T) { | ||
| calls := 0 | ||
| transport := shortcutRoundTripFunc(func(req *http.Request) (*http.Response, error) { | ||
| if req.URL.Path != "/open-apis/im/v1/messages/om_test/read_users" { | ||
| return nil, fmt.Errorf("unexpected path: %s", req.URL.Path) | ||
| } | ||
| calls++ | ||
| if req.URL.Query().Get("page_token") == "" { | ||
| return shortcutJSONResponse(200, map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{ | ||
| "items": []interface{}{map[string]interface{}{"user_id": "ou_one", "timestamp": "1"}}, | ||
| "has_more": true, | ||
| "page_token": "next", | ||
| }, | ||
| }), nil | ||
| } | ||
| return shortcutJSONResponse(200, map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{ | ||
| "items": []interface{}{map[string]interface{}{"user_id": "ou_two", "tenant_key": "tenant"}}, | ||
| "has_more": false, | ||
| }, | ||
| }), nil | ||
| }) | ||
| runtime := newMessageReadUsersTestRuntime(t, transport, map[string]string{"message-id": "om_test"}, map[string]bool{"page-all": true}, map[string]int{"page-limit": 0}) | ||
|
|
||
| got, err := fetchMessageReadUsers(context.Background(), runtime) | ||
| if err != nil { | ||
| t.Fatalf("fetchMessageReadUsers() error = %v", err) | ||
| } | ||
| if calls != 2 { | ||
| t.Fatalf("calls = %d, want 2", calls) | ||
| } | ||
| items, _ := got["items"].([]interface{}) | ||
| if len(items) != 2 { | ||
| t.Fatalf("len(items) = %d, want 2", len(items)) | ||
| } | ||
| if got["has_more"] != false || got["page_token"] != "" || got["total"] != 2 { | ||
| t.Fatalf("result metadata = %#v", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Cover pagination stop conditions.
This test uses --page-limit=0 and an advancing token. It does not verify the positive page-limit cap or the non-advancing-token guard.
Add request-count assertions for both conditions. Removing either termination guard at Lines 131-137 would currently leave the suite passing.
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 `@shortcuts/im/im_message_read_users_test.go` around lines 80 - 121, Extend
TestFetchMessageReadUsersAggregatesPages with regression coverage for both
pagination termination guards in fetchMessageReadUsers: verify a positive page
limit stops requests at the configured cap, and verify an unchanged page token
stops pagination without repeated requests. Add explicit request-count
assertions for each scenario so reverting either guard fails the tests.
Source: Coding guidelines
Comment on lines
+140
to
+144
| err := validateMessageReadUsers(runtime) | ||
| problem, ok := errs.ProblemOf(err) | ||
| if !ok || problem.Subtype != errs.SubtypeInvalidArgument { | ||
| t.Fatalf("problem = %#v, err = %v", problem, err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert complete validation metadata.
Each case asserts only Subtype. Add expected parameter names to the table. Use errors.As with *errs.ValidationError to assert Category, Subtype, and Param.
No wrapped cause exists in these direct validation branches, so cause preservation is not applicable here.
As per coding guidelines, “Error tests must assert typed metadata and cause preservation rather than message text alone.” Based on learnings, errs.ProblemOf does not expose ValidationError.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/im/im_message_read_users_test.go` around lines 140 - 144, Update
the validation test table and its assertions around validateMessageReadUsers to
include the expected parameter name for each case. Replace errs.ProblemOf-based
checking with errors.As into *errs.ValidationError, then assert Category,
Subtype, and Param for every validation error; do not add cause assertions for
these direct branches.
Sources: Coding guidelines, Learnings
Comment on lines
+120
to
+127
| data, err := runtime.CallAPITyped(http.MethodGet, apiPath, params, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if pageItems, ok := data["items"].([]interface{}); ok { | ||
| items = append(items, pageItems...) | ||
| } | ||
| hasMore, nextToken = common.PaginationMeta(data) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Project the API response into typed structs.
Lines 120-127 consume API fields through map[string]interface{}. An unexpected field type silently produces an empty item list or pagination metadata.
Decode the response at this shortcut boundary into a typed page struct and typed read-user item struct before aggregation.
As per coding guidelines, “project loose-map fields into typed structs at new API boundaries.”
🤖 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_message_read_users.go` around lines 120 - 127, Update the API
response handling in the read-users shortcut to decode the result into a typed
page structure containing typed read-user items and pagination metadata before
aggregation. Replace direct map access to data["items"] and
common.PaginationMeta(data) with the typed fields, and return decoding/type
errors instead of silently treating malformed responses as empty.
Source: Coding guidelines
Comment on lines
+43
to
+64
| func TestBuildMessagesReadStatusBodyRejectsInvalidInputs(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| messageIDs string | ||
| }{ | ||
| {name: "empty", messageIDs: ""}, | ||
| {name: "invalid prefix", messageIDs: "oc_not_message"}, | ||
| {name: "more than fifty", messageIDs: strings.Join(makeReadStatusMessageIDs(51), ",")}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| runtime := newMessagesReadStatusTestRuntime(t, tt.messageIDs) | ||
| _, err := buildMessagesReadStatusBody(runtime) | ||
| problem, ok := errs.ProblemOf(err) | ||
| if !ok { | ||
| t.Fatalf("errs.ProblemOf() ok = false, err = %v", err) | ||
| } | ||
| if problem.Subtype != errs.SubtypeInvalidArgument { | ||
| t.Fatalf("problem.Subtype = %q, want %q", problem.Subtype, errs.SubtypeInvalidArgument) | ||
| } | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the validation category and parameter.
The test accepts an error that loses the --message-ids parameter metadata. Assert CategoryValidation and use errors.As with *errs.ValidationError to assert Param.
Proposed test update
import (
+ "errors"
"fmt"
"reflect"
@@
if problem.Subtype != errs.SubtypeInvalidArgument {
t.Fatalf("problem.Subtype = %q, want %q", problem.Subtype, errs.SubtypeInvalidArgument)
}
+ if problem.Category != errs.CategoryValidation {
+ t.Fatalf("problem.Category = %q, want %q", problem.Category, errs.CategoryValidation)
+ }
+ var validationErr *errs.ValidationError
+ if !errors.As(err, &validationErr) {
+ t.Fatalf("errors.As(*errs.ValidationError) = false, err = %v", err)
+ }
+ if validationErr.Param != "--message-ids" {
+ t.Fatalf("validationErr.Param = %q, want --message-ids", validationErr.Param)
+ }As per coding guidelines, “Error tests must assert typed metadata and cause preservation rather than message text alone.” Based on learnings, use errors.As on *errs.ValidationError to inspect Param.
📝 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
| func TestBuildMessagesReadStatusBodyRejectsInvalidInputs(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| messageIDs string | |
| }{ | |
| {name: "empty", messageIDs: ""}, | |
| {name: "invalid prefix", messageIDs: "oc_not_message"}, | |
| {name: "more than fifty", messageIDs: strings.Join(makeReadStatusMessageIDs(51), ",")}, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| runtime := newMessagesReadStatusTestRuntime(t, tt.messageIDs) | |
| _, err := buildMessagesReadStatusBody(runtime) | |
| problem, ok := errs.ProblemOf(err) | |
| if !ok { | |
| t.Fatalf("errs.ProblemOf() ok = false, err = %v", err) | |
| } | |
| if problem.Subtype != errs.SubtypeInvalidArgument { | |
| t.Fatalf("problem.Subtype = %q, want %q", problem.Subtype, errs.SubtypeInvalidArgument) | |
| } | |
| }) | |
| func TestBuildMessagesReadStatusBodyRejectsInvalidInputs(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| messageIDs string | |
| }{ | |
| {name: "empty", messageIDs: ""}, | |
| {name: "invalid prefix", messageIDs: "oc_not_message"}, | |
| {name: "more than fifty", messageIDs: strings.Join(makeReadStatusMessageIDs(51), ",")}, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| runtime := newMessagesReadStatusTestRuntime(t, tt.messageIDs) | |
| _, err := buildMessagesReadStatusBody(runtime) | |
| problem, ok := errs.ProblemOf(err) | |
| if !ok { | |
| t.Fatalf("errs.ProblemOf() ok = false, err = %v", err) | |
| } | |
| if problem.Subtype != errs.SubtypeInvalidArgument { | |
| t.Fatalf("problem.Subtype = %q, want %q", problem.Subtype, errs.SubtypeInvalidArgument) | |
| } | |
| if problem.Category != errs.CategoryValidation { | |
| t.Fatalf("problem.Category = %q, want %q", problem.Category, errs.CategoryValidation) | |
| } | |
| var validationErr *errs.ValidationError | |
| if !errors.As(err, &validationErr) { | |
| t.Fatalf("errors.As(*errs.ValidationError) = false, err = %v", err) | |
| } | |
| if validationErr.Param != "--message-ids" { | |
| t.Fatalf("validationErr.Param = %q, want --message-ids", 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/im/im_messages_read_status_test.go` around lines 43 - 64,
Strengthen TestBuildMessagesReadStatusBodyRejectsInvalidInputs by asserting
problem.Category is errs.CategoryValidation, then use errors.As to extract an
*errs.ValidationError and verify its Param identifies --message-ids. Keep the
existing invalid-subtype assertion and apply these checks to each validation
error.
Sources: Coding guidelines, Learnings
Comment on lines
+48
to
+50
| {name: "empty", messageIDs: ""}, | ||
| {name: "invalid prefix", messageIDs: "oc_not_message"}, | ||
| {name: "more than fifty", messageIDs: strings.Join(makeReadStatusMessageIDs(51), ",")}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add an exact maximum-boundary acceptance test.
The suite rejects 51 IDs but does not accept 50 IDs. A regression from len(ids) > 50 to len(ids) >= 50 would pass these tests.
Proposed test
+func TestBuildMessagesReadStatusBodyAllowsMaximum(t *testing.T) {
+ runtime := newMessagesReadStatusTestRuntime(t, strings.Join(makeReadStatusMessageIDs(50), ","))
+
+ if _, err := buildMessagesReadStatusBody(runtime); err != nil {
+ t.Fatalf("buildMessagesReadStatusBody() error = %v", err)
+ }
+}
+
func TestBuildMessagesReadStatusBodyRejectsInvalidInputs(t *testing.T) {As per coding guidelines, every behavior change requires a nearby regression test that fails when the implementation is reverted.
📝 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: "empty", messageIDs: ""}, | |
| {name: "invalid prefix", messageIDs: "oc_not_message"}, | |
| {name: "more than fifty", messageIDs: strings.Join(makeReadStatusMessageIDs(51), ",")}, | |
| func TestBuildMessagesReadStatusBodyAllowsMaximum(t *testing.T) { | |
| runtime := newMessagesReadStatusTestRuntime(t, strings.Join(makeReadStatusMessageIDs(50), ",")) | |
| if _, err := buildMessagesReadStatusBody(runtime); err != nil { | |
| t.Fatalf("buildMessagesReadStatusBody() error = %v", err) | |
| } | |
| } | |
| func TestBuildMessagesReadStatusBodyRejectsInvalidInputs(t *testing.T) { | |
| {name: "empty", messageIDs: ""}, | |
| {name: "invalid prefix", messageIDs: "oc_not_message"}, | |
| {name: "more than fifty", messageIDs: strings.Join(makeReadStatusMessageIDs(51), ",")}, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/im/im_messages_read_status_test.go` around lines 48 - 50, Add a
table-driven case in the read-status validation tests alongside “more than
fifty” that supplies exactly 50 IDs from makeReadStatusMessageIDs and expects
acceptance. Keep the existing 51-ID rejection case so the boundary between
accepted and rejected counts is covered.
Source: Coding guidelines
Comment on lines
+17
to
+84
| func TestIMMessagesReadStatusDryRun(t *testing.T) { | ||
| setMessageReadStatusDryRunEnv(t) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| t.Cleanup(cancel) | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{ | ||
| "im", "+messages-read-status", | ||
| "--message-ids", "om_one,om_two", | ||
| "--dry-run", | ||
| }, | ||
| DefaultAs: "user", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| require.Equal(t, "user", clie2e.DryRunGet(result.Stdout, "identity").String()) | ||
| require.Equal(t, http.MethodPost, clie2e.DryRunGet(result.Stdout, "api.0.method").String()) | ||
| require.Equal(t, "/open-apis/im/v1/messages/batch_query_read_status", clie2e.DryRunGet(result.Stdout, "api.0.url").String()) | ||
| require.Equal(t, "om_one", clie2e.DryRunGet(result.Stdout, "api.0.body.message_ids.0").String()) | ||
| require.Equal(t, "om_two", clie2e.DryRunGet(result.Stdout, "api.0.body.message_ids.1").String()) | ||
| } | ||
|
|
||
| func TestIMMessagesReadStatusRejectsBotIdentity(t *testing.T) { | ||
| setMessageReadStatusDryRunEnv(t) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| t.Cleanup(cancel) | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{ | ||
| "im", "+messages-read-status", | ||
| "--message-ids", "om_one", | ||
| "--dry-run", | ||
| }, | ||
| DefaultAs: "bot", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 2) | ||
| require.Empty(t, result.Stdout) | ||
| require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String()) | ||
| require.Equal(t, "invalid_argument", gjson.Get(result.Stderr, "error.subtype").String()) | ||
| require.Equal(t, "--as", gjson.Get(result.Stderr, "error.param").String()) | ||
| } | ||
|
|
||
| func TestIMMessageReadUsersDryRunSupportsUserAndBot(t *testing.T) { | ||
| setMessageReadStatusDryRunEnv(t) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| t.Cleanup(cancel) | ||
|
|
||
| for _, identity := range []string{"user", "bot"} { | ||
| t.Run(identity, func(t *testing.T) { | ||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{ | ||
| "im", "+message-read-users", | ||
| "--message-id", "om_test", | ||
| "--dry-run", | ||
| }, | ||
| DefaultAs: identity, | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| require.Equal(t, identity, clie2e.DryRunGet(result.Stdout, "identity").String()) | ||
| require.Equal(t, http.MethodGet, clie2e.DryRunGet(result.Stdout, "api.0.method").String()) | ||
| require.Equal(t, "/open-apis/im/v1/messages/om_test/read_users", clie2e.DryRunGet(result.Stdout, "api.0.url").String()) | ||
| require.Equal(t, "open_id", clie2e.DryRunGet(result.Stdout, "api.0.params.user_id_type").String()) | ||
| require.Equal(t, int64(100), clie2e.DryRunGet(result.Stdout, "api.0.params.page_size").Int()) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add live E2E coverage before marking the new shortcuts as covered.
Dry-run tests do not verify access-token authentication, scope enforcement, or API response handling. The PR objective also states that access-token smoke tests remain pending.
tests/cli_e2e/im/message_read_status_dryrun_test.go#L17-L84: add self-contained live workflows for+messages-read-statuswith UAT and+message-read-userswith its supported identities. Create, use, and clean up test resources. Assert actual response fields.tests/cli_e2e/im/coverage.md#L31-L33: mark these commands uncovered or blocked until the live workflows exist.
As per coding guidelines, “new shortcuts require live E2E coverage.” Based on learnings, changed shortcut flags and request parameters also require live coverage.
📍 Affects 2 files
tests/cli_e2e/im/message_read_status_dryrun_test.go#L17-L84(this comment)tests/cli_e2e/im/coverage.md#L31-L33
🤖 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/im/message_read_status_dryrun_test.go` around lines 17 - 84,
The dry-run-only coverage for TestIMMessagesReadStatusDryRun and
TestIMMessageReadUsersDryRun is insufficient: add self-contained live E2E
workflows using UAT for +messages-read-status and all supported identities for
+message-read-users, including resource creation, cleanup, and assertions on
actual response fields. In tests/cli_e2e/im/message_read_status_dryrun_test.go
lines 17-84, add the live tests; in tests/cli_e2e/im/coverage.md lines 31-33,
keep these commands marked uncovered or blocked until live coverage is
implemented.
Sources: Coding guidelines, Learnings
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/im/im_messages_read_status_test.go`:
- Around line 30-51: Add a sentinel cause to the PermissionError constructed as
source in TestNormalizeAllowlistedUserScopeErrorRemovesOAuthRecovery, then
assert errors.Is(got, cause) after normalization. Keep the existing typed
metadata assertions unchanged so the test verifies both normalized fields and
cause preservation.
🪄 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: 5dbeddd1-44c8-4964-ba1d-5a3844614b34
📒 Files selected for processing (6)
shortcuts/im/im_allowlisted_scope.goshortcuts/im/im_message_read_users.goshortcuts/im/im_message_read_users_test.goshortcuts/im/im_messages_read_status.goshortcuts/im/im_messages_read_status_test.goskills/lark-im/references/lark-im-message-read-status.md
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/lark-im/references/lark-im-message-read-status.md
- shortcuts/im/im_messages_read_status.go
- shortcuts/im/im_message_read_users.go
- shortcuts/im/im_message_read_users_test.go
Comment on lines
+30
to
+51
| func TestNormalizeAllowlistedUserScopeErrorRemovesOAuthRecovery(t *testing.T) { | ||
| source := errs.NewPermissionError(errs.SubtypeMissingScope, "missing allowlisted scope"). | ||
| WithCode(99991679). | ||
| WithLogID("log-id"). | ||
| WithMissingScopes("im:message.read_status:readonly"). | ||
| WithHint("run auth login") | ||
|
|
||
| got := normalizeAllowlistedUserScopeError(source, core.AsUser, "im:message.read_status:readonly") | ||
| var permissionErr *errs.PermissionError | ||
| if !errors.As(got, &permissionErr) { | ||
| t.Fatalf("errors.As() = false, err = %v", got) | ||
| } | ||
| if len(permissionErr.MissingScopes) != 0 { | ||
| t.Fatalf("MissingScopes = %v, want none", permissionErr.MissingScopes) | ||
| } | ||
| if strings.Contains(permissionErr.Hint, "auth login") || !strings.Contains(permissionErr.Hint, "Scope platform") { | ||
| t.Fatalf("Hint = %q", permissionErr.Hint) | ||
| } | ||
| if permissionErr.Code != 99991679 || permissionErr.LogID != "log-id" { | ||
| t.Fatalf("server evidence was not preserved: %#v", permissionErr) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert cause preservation for the normalized error.
Add a sentinel cause to source. Assert errors.Is(got, cause) after normalization. The current test cannot detect a change that rebuilds the permission error and drops its cause.
As per coding guidelines, “Error tests must assert typed metadata and cause preservation rather than message text alone.” Based on learnings, verify preserved causes with errors.Is.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/im/im_messages_read_status_test.go` around lines 30 - 51, Add a
sentinel cause to the PermissionError constructed as source in
TestNormalizeAllowlistedUserScopeErrorRemovesOAuthRecovery, then assert
errors.Is(got, cause) after normalization. Keep the existing typed metadata
assertions unchanged so the test verifies both normalized fields and cause
preservation.
Sources: Coding guidelines, Learnings
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 two focused IM shortcuts for message read-status APIs that support user access tokens: querying whether the current user read messages, and listing users who read one message. Read-users additionally supports tenant access tokens.
Changes
Test Plan
Local Go tests were intentionally not run; repository CI is the executable Go test gate for this branch.
Related
Merge Order
Summary by CodeRabbit
New Features
Documentation
Tests