fix: improve identity and rate limit recovery - #2193
Open
zhaojunlin0405 wants to merge 10 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change centralizes API error classification, adds typed rate-limit metadata, introduces resumable pagination errors, standardizes identity validation errors, and coordinates concurrent TAT resolution. ChangesError contracts and pagination
Rate-limit classification and integrations
Identity validation
TAT resolution coordination
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ClassifyAPIResponse
participant ClassifyHTTPRateLimit
participant APIError
Caller->>ClassifyAPIResponse: Decode response and inspect status
ClassifyAPIResponse->>ClassifyHTTPRateLimit: Classify HTTP 429 or business rate limit
ClassifyHTTPRateLimit->>APIError: Attach code, log ID, retry metadata, and hints
APIError-->>Caller: Return typed classified error
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 |
github-actions
Bot
added
domain/ccm
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@9cb2b4d06edd2ddf0f3c1aae6bf482b167c98a05🧩 Skill updatenpx skills add larksuite/cli#fix/identity-rate-limit-recovery -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2193 +/- ##
==========================================
+ Coverage 76.21% 76.26% +0.04%
==========================================
Files 986 989 +3
Lines 104268 104702 +434
==========================================
+ Hits 79472 79852 +380
- Misses 18752 18793 +41
- Partials 6044 6057 +13 ☔ 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: 11
🧹 Nitpick comments (3)
internal/credential/default_provider_test.go (2)
261-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winForce the coalescing path before releasing the resolver.
ready.Wait()only proves each goroutine started. It does not prove each goroutine reachedresolveTATbefore the leader finished. If the leader completes first, later callers read the populated cache and never call the resolver, socalls == 1still passes. The test therefore passes for both single-flight coalescing and plain caching, and it does not fail if coalescing is removed.Wait for
followersto reachcallers-1beforeclose(release), using the same poll pattern as lines 90-102.♻️ Proposed change
ready.Wait() close(begin) select { case <-started: case <-time.After(2 * time.Second): t.Fatal("timed out waiting for TAT resolution to start") } + deadline := time.Now().Add(2 * time.Second) + for { + p.tatMu.Lock() + followers := p.tatFlight.followers + p.tatMu.Unlock() + if followers == callers-1 { + break + } + if time.Now().After(deadline) { + t.Fatalf("in-flight followers = %d, want %d", followers, callers-1) + } + runtime.Gosched() + } close(release)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/credential/default_provider_test.go` around lines 261 - 276, Update the concurrent TAT resolution test around resolveTAT to wait until the followers count reaches callers-1, using the existing polling pattern from the earlier test, before closing release. Keep the current timeout/failure behavior and only release the resolver after all follower calls have entered the coalescing path.
38-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert typed error metadata, not error identity, on the error paths. All three sites compare with
errors.Isonly. The caching decision atdefault_provider.goLine 225 depends onerrs.IsRetryable, which readsProblem.Retryable. IfresolveTATlater wrapped the resolver error and dropped the typed fields,errors.Iswould still pass and the tests would not fail.
internal/credential/default_provider_test.go#L38-L40: read the problem witherrs.ProblemOf(err)and assertCategory == errs.CategoryAPI,Subtype == errs.SubtypeRateLimit, andRetryable == true, in addition toerrors.Is.internal/credential/default_provider_test.go#L105-L114: apply the sameerrs.ProblemOfassertions to each shared-flight outcome, so every follower is proven to receive the retryable classification and not only the same error value.internal/credential/default_provider_test.go#L228-L233: assertCategory == errs.CategoryConfig,Subtype == errs.SubtypeInvalidClient, andRetryable == false, which is the property that makes this error cacheable.Based on the coding guideline "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/credential/default_provider_test.go` around lines 38 - 40, Update all three error-path test sites in internal/credential/default_provider_test.go:38-40, 105-114, and 228-233 to inspect errs.ProblemOf(err) and assert the required category, subtype, and retryable metadata in addition to errors.Is. Use API/RateLimit/true for resolveTAT and shared-flight retryable outcomes, and Config/InvalidClient/false for the cacheable configuration error; retain cause-preservation checks.Source: Coding guidelines
internal/errclass/http_rate_limit.go (1)
188-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename or collapse
resultCodeinto a boolean helper.
resultCodenever returns the response code. It returns99991400or0. Both call sites compare the result to99991400, so the function is a boolean predicate with a numeric signature. Collapse it intoIsBusinessRateLimitto remove the indirection.♻️ Proposed refactor
-func resultCode(result any) int { - resultMap, ok := result.(map[string]any) - if !ok { - return 0 - } - if exactBusinessRateLimitCode(resultMap["code"]) { - return 99991400 - } - return 0 -} - // IsBusinessRateLimit reports whether result carries the exact integer Lark // short-term rate-limit code. func IsBusinessRateLimit(result any) bool { - return resultCode(result) == 99991400 + resultMap, ok := result.(map[string]any) + if !ok { + return false + } + return exactBusinessRateLimitCode(resultMap["code"]) }The
ClassifyHTTPRateLimitcall site at line 35 then becomesbusinessRateLimit := IsBusinessRateLimit(result).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/errclass/http_rate_limit.go` around lines 188 - 203, Collapse resultCode into IsBusinessRateLimit by moving its map assertion and exactBusinessRateLimitCode check into the boolean helper, returning true only for the exact business rate-limit code and false otherwise. Update the ClassifyHTTPRateLimit call site and any other callers to use IsBusinessRateLimit directly, then remove the numeric resultCode helper and comparisons against 99991400.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/auth/auth.go`:
- Around line 86-91: Update both response handlers in cmd/auth/auth.go (lines
86-91 and 168-182): replace each bare parse-error fmt.Errorf with
errs.NewInternalError(errs.SubtypeInvalidResponse, ...).WithCause(err), remove
the unused {"code","msg"} result projection, and pass nil to
client.ClassifyRateLimitResponse in both sites.
In `@errs/pagination.go`:
- Around line 109-113: Update buildJSON in errs/pagination.go at lines 109-113
so every typed fallback, including reserved-field collisions and encoding
failures, retains e.Cause as the wrapped cause while incorporating the collision
or encoding diagnostic into the internal error message. In
errs/pagination_test.go lines 126-150, update the error-path assertions to
verify metadata through errs.ProblemOf, assert SubtypeInvalidResponse, and
confirm errors.Is/errors.As can recover inner.
In `@internal/client/rate_limit.go`:
- Around line 43-62: Unify message selection for business code 99991400 across
the HTTP 429 path in errclass.ClassifyHTTPRateLimit and the shared handling in
the rate-limit flow. Choose either consistently preserving the upstream
result["msg"] or consistently using "request rate limit exceeded", then remove
the conflicting message-selection logic so transport status cannot change the
resulting message.
- Around line 75-93: Update ClassifyRateLimitResponse so
errclass.ParseRateLimitJSON is only called when the response status or
caller-provided classification indicates a rate-limit response, avoiding
reparsing successful payloads; preserve the existing result/classified handling
for malformed rate-limit bodies and the documented trust trade-off for caller
projections.
In `@internal/cmdutil/factory.go`:
- Line 46: Update the comment for identityExplicit to state that it is set
whenever the user explicitly selects any non-auto --as value, including unknown
or invalid identities, so CheckIdentity can attribute those values to the flag.
In `@internal/credential/default_provider.go`:
- Around line 178-227: The resolveTAT implementation must avoid caching results
caused by context cancellation or timeout, even when errs.IsRetryable reports
them as non-retryable; update the cache decision around tatResolver accordingly.
In the existing tatFlight follower path, wait on either flight.done or the
follower’s ctx.Done(), returning the follower’s context error when its context
is canceled while preserving normal flight result sharing otherwise. Scope any
cached result to the leader context as needed so cancellation results cannot be
reused by later ResolveToken calls.
In `@internal/errclass/http_rate_limit_test.go`:
- Around line 49-56: Extend the error-path assertions to validate typed metadata
through errs.ProblemOf while preserving errors.As checks for RetryAfterSeconds:
in internal/errclass/http_rate_limit_test.go:49-56,
internal/client/rate_limit_test.go:123-129,
internal/client/response_test.go:385-398, cmd/auth/auth_test.go:508-518, and
shortcuts/common/call_api_typed_test.go:117-129 assert CategoryAPI and
SubtypeRateLimit; in shortcuts/common/runner_botinfo_test.go:177-215 assert
CategoryAPI for all cases and SubtypeRateLimit for rate-limit cases; in
shortcuts/common/runner_botinfo_test.go:263-265 assert CategoryAPI and a
non-empty subtype. Do not assert Param on APIError.
In `@internal/errclass/http_rate_limit.go`:
- Around line 241-249: Rate-limit guidance construction is duplicated across
internal/errclass/http_rate_limit.go lines 241-249 and
internal/client/rate_limit.go lines 65-73, risking mismatched deduplication.
Export mergeRateLimitHint as MergeRateLimitHint in
internal/errclass/http_rate_limit.go lines 241-249, add an exported
RateLimitGuidance(seconds) constructor for the guidance text at line 63, and
update internal/client/rate_limit.go lines 65-73 and 60 to remove local
duplication and call these errclass helpers.
In `@shortcuts/im/coverage_additional_test.go`:
- Around line 329-332: Update the error assertions in the test around
errs.ProblemOf to validate the complete typed contract: retain the
SubtypeIdentityNotSupported check and also assert the validation category and
--user-id parameter values produced by shortcuts/im/helpers.go. Keep the
existing failure reporting style and use the corresponding errs symbols for
these fields.
In `@shortcuts/mail/large_attachment_test.go`:
- Around line 1225-1228: Update
TestMailRequireUserOpenIDChecksIdentityBeforeConfiguredUser to assert the
returned problem’s Hint field through the existing assertMailIdentityProblem
helper, ensuring the bot-identity recovery hint is verified in addition to the
category and subtype.
In `@shortcuts/mail/template_compose_test.go`:
- Line 10: Replace the os filesystem usage in the test fixture setup, including
the os import and each os.WriteFile call, with the applicable internal/vfs write
helper. Keep the existing fixture paths and contents unchanged while ensuring
all filesystem operations in this Go test use the repository filesystem API.
---
Nitpick comments:
In `@internal/credential/default_provider_test.go`:
- Around line 261-276: Update the concurrent TAT resolution test around
resolveTAT to wait until the followers count reaches callers-1, using the
existing polling pattern from the earlier test, before closing release. Keep the
current timeout/failure behavior and only release the resolver after all
follower calls have entered the coalescing path.
- Around line 38-40: Update all three error-path test sites in
internal/credential/default_provider_test.go:38-40, 105-114, and 228-233 to
inspect errs.ProblemOf(err) and assert the required category, subtype, and
retryable metadata in addition to errors.Is. Use API/RateLimit/true for
resolveTAT and shared-flight retryable outcomes, and Config/InvalidClient/false
for the cacheable configuration error; retain cause-preservation checks.
In `@internal/errclass/http_rate_limit.go`:
- Around line 188-203: Collapse resultCode into IsBusinessRateLimit by moving
its map assertion and exactBusinessRateLimitCode check into the boolean helper,
returning true only for the exact business rate-limit code and false otherwise.
Update the ClassifyHTTPRateLimit call site and any other callers to use
IsBusinessRateLimit directly, then remove the numeric resultCode helper and
comparisons against 99991400.
🪄 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: 09a38343-c21c-4d9a-8fde-f57374ecbe42
📒 Files selected for processing (58)
cmd/api/api_paginate_test.gocmd/api/api_test.gocmd/auth/auth.gocmd/auth/auth_test.gocmd/config/init_probe.gocmd/config/init_probe_test.gocmd/event/runtime.gocmd/event/runtime_test.gocmd/service/service_paginate_test.gocmd/service/service_test.goerrs/ERROR_CONTRACT.mderrs/marshal_test.goerrs/pagination.goerrs/pagination_test.goerrs/subtypes.goerrs/types.goerrs/types_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/rate_limit.gointernal/client/rate_limit_test.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_test.gointernal/credential/default_provider.gointernal/credential/default_provider_test.gointernal/credential/tat_fetch.gointernal/credential/tat_fetch_test.gointernal/errclass/http_rate_limit.gointernal/errclass/http_rate_limit_test.goshortcuts/common/call_api_typed_test.goshortcuts/common/runner.goshortcuts/common/runner_botinfo_test.goshortcuts/contact/contact_get_user.goshortcuts/contact/contact_get_user_test.goshortcuts/drive/drive_member_add.goshortcuts/drive/drive_member_add_test.goshortcuts/im/builders_test.goshortcuts/im/coverage_additional_test.goshortcuts/im/helpers.goshortcuts/im/im_chat_create.goshortcuts/im/im_chat_list.goshortcuts/im/im_chat_list_test.goshortcuts/im/im_chat_messages_list.goshortcuts/mail/helpers.goshortcuts/mail/large_attachment.goshortcuts/mail/large_attachment_test.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/mail/template_compose.goshortcuts/mail/template_compose_test.goshortcuts/wiki/wiki_list_copy_test.goshortcuts/wiki/wiki_member_helpers.goshortcuts/wiki/wiki_member_test.goshortcuts/wiki/wiki_node_create.goshortcuts/wiki/wiki_node_create_test.goshortcuts/wiki/wiki_node_list.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/auth/auth_test.go (1)
559-571: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
cmdutil.TestFactoryfor this mocked HTTP test.This test performs an HTTP-mocked request but constructs
lark.NewClientdirectly. Build the test client throughcmdutil.TestFactory(t, config)so the test uses the standard configuration isolation and HTTP setup.As per coding guidelines, “Use
cmdutil.TestFactory(t, config)for test factories and setLARKSUITE_CLI_CONFIG_DIRtot.TempDir()witht.Setenvto isolate configuration state.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/auth_test.go` around lines 559 - 571, Update the mocked HTTP test around the direct lark.NewClient construction to create the client through cmdutil.TestFactory(t, config). Configure the factory with the test app credentials and mocked HTTP transport, and isolate CLI configuration by setting LARKSUITE_CLI_CONFIG_DIR to t.TempDir() via t.Setenv; preserve the existing request stub and test behavior.Sources: Coding guidelines, Learnings
🤖 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 `@internal/client/rate_limit_test.go`:
- Around line 161-165: Extend the error assertions in the test around
ClassifyRateLimitResponse to verify cause preservation by declaring a
*json.SyntaxError target and asserting errors.As(err, &syntaxErr) succeeds. Keep
the existing errs.ProblemOf metadata checks unchanged, ensuring the trailing
JSON parsing error remains discoverable through the returned error.
In `@internal/credential/default_provider_test.go`:
- Around line 307-350: The leader goroutine in the resolveTAT test currently
sends only the error, so it does not verify the resolved token. Update
leaderDone and its producer to transmit both the returned token and error, then
use a bounded receive after closing release to assert the token equals
"leader-token" and the error is nil; retain the timeout to prevent hangs.
In `@internal/errclass/http_rate_limit.go`:
- Around line 60-61: Update the businessRateLimit branch in the API error
classification flow to set the reused apiErr.Subtype and apiErr.Code to the
rate-limit values alongside RateLimitMessage. Add a regression test covering a
pre-existing non-rate-limit *errs.APIError and assert its message, subtype, and
code are all converted to the business rate-limit classification.
---
Nitpick comments:
In `@cmd/auth/auth_test.go`:
- Around line 559-571: Update the mocked HTTP test around the direct
lark.NewClient construction to create the client through cmdutil.TestFactory(t,
config). Configure the factory with the test app credentials and mocked HTTP
transport, and isolate CLI configuration by setting LARKSUITE_CLI_CONFIG_DIR to
t.TempDir() via t.Setenv; preserve the existing request stub and test behavior.
🪄 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: 3ec25d55-3f07-489d-a097-b93e8e43db45
📒 Files selected for processing (18)
cmd/auth/auth.gocmd/auth/auth_test.goerrs/pagination.goerrs/pagination_test.gointernal/client/rate_limit.gointernal/client/rate_limit_test.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/factory.gointernal/credential/default_provider.gointernal/credential/default_provider_test.gointernal/errclass/http_rate_limit.gointernal/errclass/http_rate_limit_test.goshortcuts/common/call_api_typed_test.goshortcuts/common/runner_botinfo_test.goshortcuts/im/coverage_additional_test.goshortcuts/mail/large_attachment_test.goshortcuts/mail/template_compose_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/client/response.go
- shortcuts/mail/template_compose_test.go
- internal/cmdutil/factory.go
- cmd/auth/auth.go
- shortcuts/common/call_api_typed_test.go
- internal/client/rate_limit.go
- shortcuts/common/runner_botinfo_test.go
- internal/errclass/http_rate_limit_test.go
- internal/client/response_test.go
- shortcuts/im/coverage_additional_test.go
- internal/credential/default_provider.go
- errs/pagination_test.go
- shortcuts/mail/large_attachment_test.go
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 `@internal/errclass/http_rate_limit_test.go`:
- Around line 162-174: Update
TestClassifyHTTPRateLimit_ReclassifiesExistingAPIErrorAsBusinessRateLimit to
inspect the returned error via errs.ProblemOf(err), asserting CategoryAPI and
SubtypeRateLimit. Keep the pointer identity check for in-place mutation, but
remove direct assertions of original.Subtype, original.Code, and
original.Message and do not assert Param on APIError.
🪄 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: 960002f7-8328-4228-9b6d-bdb4ac56b27f
📒 Files selected for processing (5)
cmd/auth/auth_test.gointernal/client/rate_limit_test.gointernal/credential/default_provider_test.gointernal/errclass/http_rate_limit.gointernal/errclass/http_rate_limit_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/client/rate_limit_test.go
- internal/errclass/http_rate_limit.go
- cmd/auth/auth_test.go
- internal/credential/default_provider_test.go
Comment on lines
+162
to
+174
| func TestClassifyHTTPRateLimit_ReclassifiesExistingAPIErrorAsBusinessRateLimit(t *testing.T) { | ||
| original := errs.NewAPIError(errs.SubtypeUnknown, "original classification").WithCode(12345) | ||
| err := ClassifyHTTPRateLimit(http.StatusTooManyRequests, nil, map[string]any{ | ||
| "code": 99991400, | ||
| }, original, time.Now()) | ||
|
|
||
| if err != original { | ||
| t.Fatalf("classification = %T (%v), want original APIError pointer", err, err) | ||
| } | ||
| if original.Subtype != errs.SubtypeRateLimit || original.Code != 99991400 || original.Message != RateLimitMessage { | ||
| t.Fatalf("reclassified API error = %#v, want api/rate_limit code 99991400", original) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the typed API error contract through errs.ProblemOf.
This test checks Subtype, Code, and Message directly on original. The pointer check verifies in-place mutation, but it does not exercise the required Problem metadata contract. Read the returned error with errs.ProblemOf(err) and assert CategoryAPI and SubtypeRateLimit. Do not assert Param for *errs.APIError.
As per coding guidelines, error-path tests must assert typed metadata through errs.ProblemOf. Based on learnings, APIError does not expose Param.
Proposed assertion
if err != original {
t.Fatalf("classification = %T (%v), want original APIError pointer", err, err)
}
- if original.Subtype != errs.SubtypeRateLimit || original.Code != 99991400 || original.Message != RateLimitMessage {
+ problem, ok := errs.ProblemOf(err)
+ if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeRateLimit {
+ t.Fatalf("problem = %#v, want api/rate_limit", problem)
+ }
+ if original.Code != 99991400 || original.Message != RateLimitMessage {
t.Fatalf("reclassified API error = %#v, want api/rate_limit code 99991400", original)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/errclass/http_rate_limit_test.go` around lines 162 - 174, Update
TestClassifyHTTPRateLimit_ReclassifiesExistingAPIErrorAsBusinessRateLimit to
inspect the returned error via errs.ProblemOf(err), asserting CategoryAPI and
SubtypeRateLimit. Keep the pointer identity check for in-place mutation, but
remove direct assertions of original.Subtype, original.Code, and
original.Message and do not assert Param on APIError.
Sources: Coding guidelines, Learnings
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/client/api_response_test.go (1)
195-197: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the concrete trailing-content cause, not just its presence.
errors.Unwrap(err) == nilproves only that some cause is attached. IfDecodeSingleJSONwere changed to wrap a different error, this test would still pass. The sibling test at lines 179-182 already asserts a concrete*json.SyntaxError.Assert the specific error that
errclass.DecodeSingleJSONreturns for trailing content, througherrors.Ison the sentinel orerrors.Ason its type.The repository guidelines require error-path tests to verify cause preservation rather than relying only on message substrings or presence checks.
💚 Proposed tightening
- if errors.Unwrap(err) == nil { - t.Fatalf("error chain does not preserve trailing-content cause: %v", err) - } + if !errors.Is(err, errclass.ErrTrailingJSONContent) { + t.Fatalf("error chain does not preserve the trailing-content cause: %v", err) + }Replace
errclass.ErrTrailingJSONContentwith the actual sentinel or error type thaterrclass.DecodeSingleJSONreturns for trailing content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/api_response_test.go` around lines 195 - 197, Update the trailing-content assertion in the affected test to verify the concrete cause returned by errclass.DecodeSingleJSON, using errors.Is with its actual sentinel or errors.As with its actual error type. Replace the current errors.Unwrap(err) nil-check while preserving the existing failure context.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/client/api_response.go (1)
89-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared rate-limit decoration instead of duplicating it.
Lines 89-104 repeat the business-rate-limit branch of
errclass.ClassifyHTTPRateLimit(internal/errclass/http_rate_limit.go:32-72) almost line for line: the same code overwrite, the sameRateLimitLogIDderivation, the same hint merge, and the sameWithRetryable().WithRetryAfter(...)call.ClassifyHTTPRateLimitcannot be called here only because it early-returns when the status is not 429. Two copies of one classification rule can drift.Extract the decoration into an exported
errclasshelper and call it from both places. Also promote the literal99991400(lines 95, 98, and 118) to a named constant inerrclass.♻️ Sketch of the shared helper
In
internal/errclass/http_rate_limit.go:// BusinessRateLimitCode is the Lark business rate-limit error code. const BusinessRateLimitCode = 99991400 // DecorateBusinessRateLimit normalizes an existing or new APIError into the // canonical business rate-limit shape and attaches retry metadata. func DecorateBusinessRateLimit(header http.Header, result any, classified error, now time.Time) *errs.APIError { var apiErr *errs.APIError if !errors.As(classified, &apiErr) { apiErr = errs.NewAPIError(errs.SubtypeRateLimit, RateLimitMessage) } apiErr.Subtype = errs.SubtypeRateLimit apiErr.Code = BusinessRateLimitCode apiErr.Message = RateLimitMessage apiErr.LogID = RateLimitLogID(result, header) seconds, source := ParseRetryAfter(header, now) apiErr.Hint = MergeRateLimitHint(apiErr.Hint, RateLimitGuidance(seconds)) return apiErr.WithRetryable().WithRetryAfter(seconds, source) }Then in this file:
- var apiErr *errs.APIError - var existing *errs.APIError - if errors.As(classified, &existing) { - apiErr = existing - } - if apiErr == nil { - apiErr = errs.NewAPIError(errs.SubtypeRateLimit, errclass.RateLimitMessage).WithCode(99991400) - } - apiErr.Subtype = errs.SubtypeRateLimit - apiErr.Code = 99991400 - apiErr.Message = errclass.RateLimitMessage - apiErr.LogID = errclass.RateLimitLogID(result, resp.Header) - - seconds, source := errclass.ParseRetryAfter(resp.Header, time.Now()) - apiErr.Hint = errclass.MergeRateLimitHint(apiErr.Hint, errclass.RateLimitGuidance(seconds)) - return apiErr.WithRetryable().WithRetryAfter(seconds, source) + return errclass.DecorateBusinessRateLimit(resp.Header, result, classified, time.Now())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/api_response.go` around lines 89 - 104, Extract the duplicated business rate-limit normalization from the API response flow and errclass.ClassifyHTTPRateLimit into an exported errclass helper, such as DecorateBusinessRateLimit, preserving the shared code, message, log ID, retry hint, and retryability behavior. Define and use a named errclass.BusinessRateLimitCode constant instead of the 99991400 literal throughout both paths, and update the API response branch to call the helper.
🤖 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 `@internal/client/client_test.go`:
- Around line 1082-1086: Extend
TestDoStream_HTTP400RejectsUntrustedFourKiBRateLimitPrefix with a case whose
complete body is exactly maxBody (4096 bytes), without relying on trailing data
or a read error beyond the limit. Assert that DoStream classifies it as
rate-limited and preserves the expected LogID.
---
Outside diff comments:
In `@internal/client/api_response_test.go`:
- Around line 195-197: Update the trailing-content assertion in the affected
test to verify the concrete cause returned by errclass.DecodeSingleJSON, using
errors.Is with its actual sentinel or errors.As with its actual error type.
Replace the current errors.Unwrap(err) nil-check while preserving the existing
failure context.
---
Nitpick comments:
In `@internal/client/api_response.go`:
- Around line 89-104: Extract the duplicated business rate-limit normalization
from the API response flow and errclass.ClassifyHTTPRateLimit into an exported
errclass helper, such as DecorateBusinessRateLimit, preserving the shared code,
message, log ID, retry hint, and retryability behavior. Define and use a named
errclass.BusinessRateLimitCode constant instead of the 99991400 literal
throughout both paths, and update the API response branch to call the helper.
🪄 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: 48f2e22c-8525-414d-9288-4574381f16a2
📒 Files selected for processing (8)
cmd/auth/auth.gocmd/event/runtime.gointernal/client/api_response.gointernal/client/api_response_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/response.goshortcuts/common/runner.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/auth/auth.go
- cmd/event/runtime.go
- internal/client/client.go
- shortcuts/common/runner.go
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improve machine-readable recovery for unsupported identities and platform rate limits. The CLI now preserves structured rate-limit metadata across API, pagination, shortcut, authentication, configuration, and TAT paths without automatically replaying requests.
Changes
identity_not_supportederror subtype and apply it to known bot/user compatibility checks.Retry-After, retryability, request IDs, and quota semantics.Test Plan
make unit-testgo vet ./...Related Issues
Summary by CodeRabbit
New Features
Bug Fixes
Documentation