You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(apps): answer rate-limited calls with backoff instead of request hints - #2882
When the OpenAPI gateway throttles an apps call (code 99991400), lark-cli already classifies the error correctly as api/rate_limit, retryable: true. But the apps hint fallback then fills the empty hint with the command's request-shaped advice. A rate-limited +db-execute tells the caller to "verify table/column names with +db-table-get … target the dev database with --environment dev". +trace-get tells it to "verify --app-id", and +session-messages-list tells it to "verify --session-id / --turn-id".
The server never evaluated those requests, so that advice cannot be right. An agent that follows it changes a correct request and sends more calls into the same limit. This PR answers a rate limit with backoff guidance instead. It changes only shortcuts/apps.
Root cause
internal/errclass/codemeta.go maps 99991400 to api/rate_limit (retryable). APIHint has no wording for rate_limit, so the error leaves the classifier with hint == "".
withAppsHint (shortcuts/apps/common.go) fills an empty hint with the caller's fallback whenever hintExplainsFailure is true. That gate only excluded failed_precondition, so rate_limit got the request-shaped hint.
withDBSyncHint (shortcuts/apps/db_common.go), used by the +db-sync-* commands, has no subtype gate at all and did the same.
Generic dispatch does not replay requests (errs/ERROR_CONTRACT.md), and this path carries no retry_after_seconds. The hint is therefore the caller's only guidance, and it pointed the wrong way.
Before (+db-execute, error envelope):
{"type":"api","subtype":"rate_limit","code":99991400,"message":"request trigger frequency limit","hint":"verify table/column names with `lark-cli apps +db-table-get --app-id app_x --table <table>`; for day-to-day debugging target the dev database with `--environment dev`","retryable":true}
After:
{"type":"api","subtype":"rate_limit","code":99991400,"message":"request trigger frequency limit","hint":"the request was rate limited, not rejected for its arguments, so do not change them; stop immediate retries, then rerun the same command later with exponential backoff and jitter, and send fewer calls in parallel","retryable":true}
Changes
The PR has five commits:
The failing tests only.
The fix.
Run the rate-limit check before the message heuristics, with its own regression cases.
Narrow a test comment that claimed every apps command goes through these helpers.
Narrow the same claim in the hintExplainsFailure comment.
TestAppsDBExecute_RateLimitHintIsBackoffNotRequestFix runs +db-execute against a 99991400 envelope (HTTP 200 and 429) through the real classification path. It asserts that the classification is unchanged, that the hint no longer contains +db-table-get, --environment dev or table/column, and that it contains backoff.
TestAppsHints_RateLimitDoesNotInheritRequestHint drives a real 99991400 classification through withAppsHint (with the +trace-get, +db-execute and +session-messages-list hints), withObservabilityHint and withDBSyncHint. It asserts the same error value, unchanged classification, no inherited request hint, and backoff guidance.
TestWithAppsHint_RateLimitKeepsUpstreamHint is a guard: a hint the upstream already supplied (e.g. a retry-after sentence) is kept. It passes before and after the fix.
fix(apps):
hintExplainsFailure now also returns false for SubtypeRateLimit. The gate stays per subtype, so authentication, authorization and server errors keep their command hints, as the existing tests require. The comment above it names the failing tests, as it asks before the gate is widened.
withAppsHint fills an empty hint on a rate limit with a new shared appRateLimitHint.
withDBSyncHint applies the same hint after its "upstream hint wins" check and before its code table and fallback.
The backoff wording follows the existing rate-limit hints in internal/client and internal/credential and the api/rate_limit entry in errs/ERROR_CONTRACT.md.
fix(apps): prioritize rate-limit classification over message heuristics:
withAppsHint and withObservabilityHint recognise some failures from message text (no database yet, a database subcode, no running container) and rewrite the message, the hint and, for database subcodes, the classification. They used to do that before looking at the subtype, so a throttle whose text happened to contain one of those markers was turned into "create a database" or "deploy the app" advice, and a database subcode even reclassified it as failed_precondition and dropped retryable.
A throttle was never evaluated by those services, so the rate-limit subtype is now checked first in both helpers. The message heuristics run only for other classes.
TestWithAppsHint_RateLimitWinsOverDatabaseMessageMarkers and TestWithObservabilityHint_RateLimitWinsOverContainerMessageMarker cover the three marker paths. The gateway's current throttle text does not contain these markers, so this closes an invariant gap rather than a reported case.
The apps commands that attach a request-shaped hint reach one of these helpers (withObservabilityHint, withRoleErrorHint and the icon helper delegate to withAppsHint), so no command file changes. Commands that return the client error as is, such as +list, keep the classifier's rate_limit with an empty hint, which does not mislead. Commands with their own error helpers are not changed here.
Not changed:type, subtype, code, retryable, log_id, the cause chain and the exit code. Only error.hint differs, and only for rate_limit errors that arrive without a hint. A rate_limit error is also no longer rewritten by the message heuristics, so its message and classification stay as the classifier produced them. internal/errclass and other domains are untouched. A global APIHint entry was considered and rejected: 1063006 (a daily, non-retryable quota) shares the rate_limit subtype and would get contradictory wording, and some drive/doc download helpers skip their own, more specific wording when the hint already mentions backoff.
Test Plan
New tests fail on the first commit, which contains only the tests. 7 subtests fail and the guard passes:
TestAppsDBExecute_RateLimitHintIsBackoffNotRequestFix: HTTP 200 and 429
TestAppsHints_RateLimitDoesNotInheritRequestHint: all 5 helper paths
New tests pass on the second commit, which contains the fix (3 tests and 7 subtests PASS).
go test ./shortcuts/apps/... ./internal/errclass/ -count=1 passes. This includes the existing gate contracts: TestWithAppsHint_WithholdsHintForFailedPrecondition, TestWithAppsHint_FillsHintForOtherClasses (authentication 99991663 and 503 keep their hints), the no-database, db-subcode, db-sync, role and observability hint tests, and TestAppsFeatureEnable_RateLimit*.
go test -count=1 ./cmd/... ./internal/... ./shortcuts/... ./extension/... ./errs/... passes: 135 packages ok and 0 failures. This is the make unit-test package set plus ./errs/..., run without the race and -N -l flags.
The three new regression cases fail on the second commit (the message heuristics rewrite the throttle: failed_precondition with retryable=false, the no-database message, the no-container message) and pass on the third.
make vet and make fmt-check pass.
go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=<base> with Go 1.23.0 (the CI toolchain): 0 issues. The diff-scoped lint module and its tests pass.
make quality-gate with this PR's title, body and branch as public-content metadata: no REJECT.
Agent-trajectory check against a simulated gateway throttle: 12 clean agent runs, all passing. See "Agent trajectory check" below.
Not run: tests/ live e2e (needs live credentials) and any check against a real gateway throttle, which cannot be triggered on demand. In the unit tests the 99991400 envelope is simulated with httpmock (HTTP 200 and 429 bodies).
Agent trajectory check
This change is about what an agent does after a throttle, and unit tests cannot show that. So both binaries were also driven by clean agent sessions.
Setup:
Two binaries with the same version string: one from the base commit (f31194b4), one from this branch (b09fbcb1).
A local mock of the OpenAPI gateway answers with the real 99991400 envelope (HTTP 400, request trigger frequency limit). It does so for every apps call in the first 45 seconds, and for any call that comes less than 2 seconds after the previous one. After that, it serves SQLite copies of an online database and a dev database.
Each run is a fresh agent session:
no repository access and no project instructions;
file reads limited to the binary and the shipped skills/;
network limited to loopback.
The prompt only states the goal: count the paid orders in the app's online database. It names no command or flag.
Assertions come from the mock's request log, not from the agent's own account:
the reported number matches the online data and comes from a successful online query;
no query targets another environment;
there are no writes.
Results: 3 runs per binary per model, 12 runs in total. All 12 pass the assertions. The runs used b09fbcb1; the two later commits only reword comments.
Model
Binary
Runs that called +db-table-get right after a throttled +db-execute (what the old hint says to do)
apps calls per run (mean)
Claude Haiku 5.5
base
2 of 3
5.0
Claude Haiku 5.5
this PR
0 of 3
4.3
Claude Sonnet 5.5
base
0 of 3
3.3
Claude Sonnet 5.5
this PR
0 of 3
3.0
Base binary: one run says it is following the hint to check the table. It also notes the hint's advice to switch to the dev database, and declines only because the task named the online database. These extra calls land in the same rate limit.
This PR: every run waits and retries the same command.
Larger model: it already acts on subtype: rate_limit and backs off with both binaries, so the difference only shows with the smaller model.
Sample size: 6 runs per model is small. The result shows that agents act on the hint; it is not a rate.
Not covered: a task that does not name an environment. With such a task, the old hint's --environment dev advice could change which data is read.
Related Issues
None
Summary by CodeRabbit
Bug Fixes
Rate-limit errors now retain their classification and existing guidance across app, observability, and database-sync responses.
When rate-limit guidance is missing, clearer backoff advice is provided instead of generic request-fix or deployment suggestions.
Existing upstream hints remain unchanged, and rate-limit messages are handled consistently across successful and error responses.
Rate-limit errors are prioritized over database and container-related messages when determining the guidance shown.
It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.
No actionable comments were generated in the recent review.
Walkthrough
Rate-limited errors now receive backoff guidance through the Apps and DB-sync hint helpers. Existing upstream hints remain intact, and request-specific or message-based advice does not replace rate-limit handling. Tests cover classification, causes, and HTTP 200 and 429 envelopes.
Changes: Apps rate-limit hints
Layer / File(s)
Summary
Rate-limit hint handling shortcuts/apps/common.go, shortcuts/apps/db_common.go, shortcuts/apps/apps_rate_limit_hint_test.go
The Apps and DB-sync hint helpers handle rate limits before applying other hint logic. They preserve existing upstream hints or provide backoff guidance. Tests verify classification and cause preservation, and check that request-shaped hints and message markers do not override rate-limit guidance.
Rate-limited calls receive backoff guidance without replacing existing upstream hints or changing error classification. No actionable merge-blocking risk was identified.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly and concisely describes the main change: rate-limited Apps calls now receive backoff guidance instead of request-specific hints.
Description check
The description includes all required sections and provides detailed motivation, changes, test coverage, related issues, and explicitly identifies unrun live tests.
Docstring Coverage
Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 10 functions across 3 files.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
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.
Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 940a6a89-68d0-47a4-8b5d-4530ff69bdd8
Commits
Reviewing files that changed from the base of the PR and between 612a20e and c144f12.
Files selected for processing: 1
shortcuts/apps/common.go
Files skipped from review as they are similar to previous changes: 1
shortcuts/apps/common.go
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
…st hints
The OpenAPI gateway answers throttled calls with code 99991400, which
internal/errclass already classifies as api/rate_limit with retryable=true
and no hint. withAppsHint then fills the empty hint with the command's
request-shaped fallback, because its gate only withholds that fallback for
failed_precondition. A rate-limited +db-execute therefore tells the caller to
"verify table/column names with +db-table-get" and to "target the dev
database with --environment dev"; +trace-get says to verify --app-id. None of
that can explain a call the server never evaluated, and an agent that follows
the advice sends more requests into the same limit. withDBSyncHint, used by
the db-sync commands, has no subtype gate at all and does the same.
These cases fail on the current code and are committed ahead of the fix, as
hintExplainsFailure asks before its gate is widened:
- TestAppsDBExecute_RateLimitHintIsBackoffNotRequestFix runs +db-execute
against a 99991400 envelope (HTTP 200 and 429) through the real
classification path and requires the hint to drop the table/column and
--environment dev advice in favour of backoff guidance
- TestAppsHints_RateLimitDoesNotInheritRequestHint drives a real 99991400
classification through withAppsHint (+trace-get, +db-execute,
+session-messages-list), withObservabilityHint and withDBSyncHint, and
requires the same error value, unchanged classification, no inherited
request hint and backoff guidance
- TestWithAppsHint_RateLimitKeepsUpstreamHint guards that a hint the upstream
already supplied (e.g. a retry-after sentence) is kept; it passes today
…hints
When the OpenAPI gateway throttles an apps call (code 99991400), the error
is already classified as api/rate_limit with retryable=true, but it reaches
withAppsHint with an empty hint and leaves carrying the command's fallback:
"verify table/column names with +db-table-get ... target the dev database
with --environment dev" on +db-execute, "verify --app-id" on +trace-get,
"verify --session-id / --turn-id" on +session-messages-list. The server never
evaluated those requests, so none of that advice can be right, and an agent
that follows it changes a correct request and sends more calls into the same
limit. The db-sync commands do the same through withDBSyncHint, which has no
subtype gate.
- Withhold the caller's hint for SubtypeRateLimit in hintExplainsFailure,
next to failed_precondition. The gate stays per subtype: authentication,
authorization and server errors keep their command hints as before
- Fill the empty hint with a shared appRateLimitHint instead: the arguments
are not the problem, stop immediate retries, rerun later with exponential
backoff and jitter, and send fewer calls in parallel. The backoff wording
follows the existing rate-limit hints in internal/client and
internal/credential and the api/rate_limit row of errs/ERROR_CONTRACT.md
- Apply the same answer in withDBSyncHint before its code table and fallback
- Keep any hint the upstream already supplied, as for every other class
Classification, code, retryable, log_id, the cause chain and the exit code
are unchanged; only error.hint differs. All apps commands pick this up through
the two helpers, so no command file changes.
Not every apps command goes through withAppsHint, withObservabilityHint or
withDBSyncHint: +list returns the client error as is, so a throttle there
keeps rate_limit with an empty hint. Say which commands the test covers
instead of claiming all of them.
…t hints
The hintExplainsFailure comment said the gateway's 99991400 reached every
apps command through the request-hint fallback. Commands that return the
client error as is, such as +list, never get a request hint, so say which
commands it reached.
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
size/MSingle-domain feat or fix with limited business impact
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.
github-actions
Bot
added
the
size/M
Summary
When the OpenAPI gateway throttles an
appscall (code99991400), lark-cli already classifies the error correctly asapi/rate_limit,retryable: true. But theappshint fallback then fills the empty hint with the command's request-shaped advice. A rate-limited+db-executetells the caller to "verify table/column names with+db-table-get… target the dev database with--environment dev".+trace-gettells it to "verify --app-id", and+session-messages-listtells it to "verify --session-id / --turn-id".The server never evaluated those requests, so that advice cannot be right. An agent that follows it changes a correct request and sends more calls into the same limit. This PR answers a rate limit with backoff guidance instead. It changes only
shortcuts/apps.Root cause
internal/errclass/codemeta.gomaps99991400toapi/rate_limit(retryable).APIHinthas no wording forrate_limit, so the error leaves the classifier withhint == "".withAppsHint(shortcuts/apps/common.go) fills an empty hint with the caller's fallback wheneverhintExplainsFailureis true. That gate only excludedfailed_precondition, sorate_limitgot the request-shaped hint.withDBSyncHint(shortcuts/apps/db_common.go), used by the+db-sync-*commands, has no subtype gate at all and did the same.errs/ERROR_CONTRACT.md), and this path carries noretry_after_seconds. The hint is therefore the caller's only guidance, and it pointed the wrong way.Before (
+db-execute, error envelope):{"type":"api","subtype":"rate_limit","code":99991400,"message":"request trigger frequency limit","hint":"verify table/column names with `lark-cli apps +db-table-get --app-id app_x --table <table>`; for day-to-day debugging target the dev database with `--environment dev`","retryable":true}After:
{"type":"api","subtype":"rate_limit","code":99991400,"message":"request trigger frequency limit","hint":"the request was rate limited, not rejected for its arguments, so do not change them; stop immediate retries, then rerun the same command later with exponential backoff and jitter, and send fewer calls in parallel","retryable":true}Changes
The PR has five commits:
appscommand goes through these helpers.hintExplainsFailurecomment.shortcuts/apps/apps_rate_limit_hint_test.go.TestAppsDBExecute_RateLimitHintIsBackoffNotRequestFixruns+db-executeagainst a99991400envelope (HTTP 200 and 429) through the real classification path. It asserts that the classification is unchanged, that the hint no longer contains+db-table-get,--environment devortable/column, and that it containsbackoff.TestAppsHints_RateLimitDoesNotInheritRequestHintdrives a real99991400classification throughwithAppsHint(with the+trace-get,+db-executeand+session-messages-listhints),withObservabilityHintandwithDBSyncHint. It asserts the same error value, unchanged classification, no inherited request hint, and backoff guidance.TestWithAppsHint_RateLimitKeepsUpstreamHintis a guard: a hint the upstream already supplied (e.g. a retry-after sentence) is kept. It passes before and after the fix.hintExplainsFailurenow also returns false forSubtypeRateLimit. The gate stays per subtype, so authentication, authorization and server errors keep their command hints, as the existing tests require. The comment above it names the failing tests, as it asks before the gate is widened.withAppsHintfills an empty hint on a rate limit with a new sharedappRateLimitHint.withDBSyncHintapplies the same hint after its "upstream hint wins" check and before its code table and fallback.internal/clientandinternal/credentialand theapi/rate_limitentry inerrs/ERROR_CONTRACT.md.withAppsHintandwithObservabilityHintrecognise some failures from message text (no database yet, a database subcode, no running container) and rewrite the message, the hint and, for database subcodes, the classification. They used to do that before looking at the subtype, so a throttle whose text happened to contain one of those markers was turned into "create a database" or "deploy the app" advice, and a database subcode even reclassified it asfailed_preconditionand droppedretryable.TestWithAppsHint_RateLimitWinsOverDatabaseMessageMarkersandTestWithObservabilityHint_RateLimitWinsOverContainerMessageMarkercover the three marker paths. The gateway's current throttle text does not contain these markers, so this closes an invariant gap rather than a reported case.The
appscommands that attach a request-shaped hint reach one of these helpers (withObservabilityHint,withRoleErrorHintand the icon helper delegate towithAppsHint), so no command file changes. Commands that return the client error as is, such as+list, keep the classifier'srate_limitwith an empty hint, which does not mislead. Commands with their own error helpers are not changed here.Not changed:
type,subtype,code,retryable,log_id, the cause chain and the exit code. Onlyerror.hintdiffers, and only forrate_limiterrors that arrive without a hint. Arate_limiterror is also no longer rewritten by the message heuristics, so its message and classification stay as the classifier produced them.internal/errclassand other domains are untouched. A globalAPIHintentry was considered and rejected:1063006(a daily, non-retryable quota) shares therate_limitsubtype and would get contradictory wording, and some drive/doc download helpers skip their own, more specific wording when the hint already mentions backoff.Test Plan
TestAppsDBExecute_RateLimitHintIsBackoffNotRequestFix: HTTP 200 and 429TestAppsHints_RateLimitDoesNotInheritRequestHint: all 5 helper pathsgo test ./shortcuts/apps/... ./internal/errclass/ -count=1passes. This includes the existing gate contracts:TestWithAppsHint_WithholdsHintForFailedPrecondition,TestWithAppsHint_FillsHintForOtherClasses(authentication 99991663 and 503 keep their hints), the no-database, db-subcode, db-sync, role and observability hint tests, andTestAppsFeatureEnable_RateLimit*.go test -count=1 ./cmd/... ./internal/... ./shortcuts/... ./extension/... ./errs/...passes: 135 packages ok and 0 failures. This is themake unit-testpackage set plus./errs/..., run without the race and-N -lflags.failed_preconditionwithretryable=false, the no-database message, the no-container message) and pass on the third.make vetandmake fmt-checkpass.go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=<base>with Go 1.23.0 (the CI toolchain): 0 issues. The diff-scopedlintmodule and its tests pass.make quality-gatewith this PR's title, body and branch as public-content metadata: no REJECT.tests/live e2e (needs live credentials) and any check against a real gateway throttle, which cannot be triggered on demand. In the unit tests the 99991400 envelope is simulated withhttpmock(HTTP 200 and 429 bodies).Agent trajectory check
This change is about what an agent does after a throttle, and unit tests cannot show that. So both binaries were also driven by clean agent sessions.
f31194b4), one from this branch (b09fbcb1).99991400envelope (HTTP 400,request trigger frequency limit). It does so for everyappscall in the first 45 seconds, and for any call that comes less than 2 seconds after the previous one. After that, it serves SQLite copies of an online database and a dev database.skills/;b09fbcb1; the two later commits only reword comments.+db-table-getright after a throttled+db-execute(what the old hint says to do)appscalls per run (mean)subtype: rate_limitand backs off with both binaries, so the difference only shows with the smaller model.--environment devadvice could change which data is read.Related Issues
Summary by CodeRabbit