fix: report rate limits and run shutdown hooks on interrupt - #2233
Open
sang-neo03 wants to merge 3 commits into
Open
fix: report rate limits and run shutdown hooks on interrupt#2233sang-neo03 wants to merge 3 commits into
sang-neo03 wants to merge 3 commits into
Conversation
sang-neo03
force-pushed
the
fix/request-lifecycle
branch
from
August 7, 2026 08:10
c650fca to
fbf20ef
Compare
📝 WalkthroughWalkthroughThe PR propagates request cancellation through token refresh and shutdown handling, adds signal-aware root execution, carries retry-after metadata through error classification, removes automatic transport retries, and adds a quality gate for retry loops. ChangesRequest cancellation and shutdown
Rate-limit error classification
Transport retry enforcement
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RootCommand
participant UATRefresh
participant HTTPAPI
participant ErrorClassifier
User->>RootCommand: Send SIGINT or SIGTERM
RootCommand->>UATRefresh: Cancel request context
UATRefresh->>HTTPAPI: Send context-bound refresh request
HTTPAPI-->>UATRefresh: Return cancellation or API response
HTTPAPI->>ErrorClassifier: Pass status and retry-after metadata
ErrorClassifier-->>RootCommand: Return classified error
Possibly related PRs
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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@fbf20efaed613f69cb7124c27f9c50882ed1aa85🧩 Skill updatenpx skills add larksuite/cli#fix/request-lifecycle -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/common/mcp_client.go (1)
128-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrioritize HTTP 429 over unknown payload codes.
classifyMCPHTTPError()andCheckResponseWithContext()classify known business codes before applying the HTTP-status fallback, while the defaultHandleResponsedoes the opposite. For an HTTP 429 body with an unrecognized business code, this path can returnSubtypeUnknownand loseretry_after_seconds.
shortcuts/common/mcp_client.go#L128-L160: Returnerrclass.NewRateLimitError(...)for HTTP 429 when the top-level or JSON-RPC error code is unrecognized.shortcuts/common/runner.go#L357: Add an HTTP 429 fallback after theerrclass.BuildAPIError()result before returning unknown payload errors.shortcuts/common/mcp_client_test.go#L304-L344: Add HTTP 429 cases with unknown top-level and JSON-RPC error codes.shortcuts common/runner_response_test.go#L17-L45: Add an HTTP 429 case with an unknown non-zero business code.internal/client/response.go: Add a test case for HTTP 429 with an unknown non-zero business code flowing throughHandleResponse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/common/mcp_client.go` around lines 128 - 160, Prioritize HTTP 429 rate-limit classification before unknown business-code handling: update classifyMCPHTTPError in shortcuts/common/mcp_client.go:128-160 and the errclass.BuildAPIError flow in shortcuts/common/runner.go:357 to return errclass.NewRateLimitError for unknown top-level or JSON-RPC codes while preserving retry_after_seconds; add coverage for unknown-code 429 responses in shortcuts/common/mcp_client_test.go:304-344, shortcuts/common/runner_response_test.go:17-45, and the HandleResponse tests in internal/client/response.go.internal/client/client.go (1)
506-519: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftProject the response map into a typed error envelope.
Line 507 keeps untyped JSON data across the classification boundary. Add one projection function for the response-error shape before calling
errclass.BuildAPIError. This prevents silent field-shape errors during error classification.As per coding guidelines, “Parse
map[string]interface{}into typed structs at the boundary, use one projection function per shape.”🤖 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/client.go` around lines 506 - 519, Update APIClient.CheckResponseWithContext to project the untyped resultMap into the typed response-error envelope before calling errclass.BuildAPIError. Add and use a single projection function for this response-error shape, handling invalid field shapes explicitly rather than passing raw map data across the classification boundary.Source: Coding guidelines
🤖 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/auth/uat_client_refresh_test.go`:
- Around line 157-201: The cancellation assertion in
TestGetValidAccessTokenCancelsRefreshRequest must validate the typed error
contract as well as preserve context.Canceled. Use errs.ProblemOf(err) to assert
CategoryNetwork and SubtypeNetworkTransport, while retaining the existing
errors.Is(err, context.Canceled) check; do not assert Param for this
*errs.NetworkError path.
In `@internal/auth/uat_client.go`:
- Line 88: Update refreshWithLock so waiting for processLock acquisition
observes ctx cancellation instead of blocking in sync.Mutex.Lock; use the
existing cancellable or timeout-based lock-wait mechanism, then return the
context error when cancellation occurs before acquisition while preserving
normal refresh behavior after the lock is obtained.
In `@internal/client/response.go`:
- Around line 109-116: The response classification in
internal/client/response.go:109-116 must make HTTP 429 authoritative even when
check returns a nonzero business-code error; update check/errclass handling or
add the 429 classification before returning apiErr so the result preserves the
rate-limit subtype and retry delay. Add coverage in
internal/client/response_test.go:367-420 using an unrecognized nonzero JSON code
with HTTP 429, and assert via errs.ProblemOf that the category is API, subtype
is rate_limit, and retry_after_seconds is populated.
In `@internal/cmdutil/factory_default.go`:
- Line 205: Add a regression test targeting buildDirectHTTPTransport directly,
using a test server that returns HTTP 503 and counting received requests; send
one request through the returned transport and assert the server is called
exactly once, ensuring direct transport does not regain retry wrapping.
In `@internal/errclass/retry_after_test.go`:
- Around line 62-71: Update the error-path assertions in
internal/errclass/retry_after_test.go:62-71,
shortcuts/common/mcp_client_test.go:335-341, and
shortcuts/common/runner_response_test.go:36-42 to use errs.ProblemOf(err) for
CategoryAPI and SubtypeRateLimit. Retain errors.As in retry_after_test.go for
validating RetryAfterSeconds, and do not assert Param because these server-side
errors do not populate it.
In `@internal/errclass/retry_after.go`:
- Around line 18-26: Update RetryAfterSeconds in
internal/errclass/retry_after.go to preserve the existing gateway-header
priority, then parse a valid HTTP-date Retry-After value and return its positive
remaining delay. Add a valid HTTP-date test case in
internal/errclass/retry_after_test.go covering the expected positive retry
duration.
In `@internal/qualitygate/rules/transport_retry.go`:
- Around line 25-43: The new checker must return prescribed typed errors instead
of raw filesystem or parser errors. Update CheckTransportRetryLoops and the
related error paths in transportRuleFiles/checkTransportRetrySource to wrap
file-read failures with the repository’s typed file-I/O constructor and parser
or other lower-layer failures with the unknown-error constructor, preserving
each original error via WithCause(err). Add error-path tests asserting the typed
metadata and preserved cause.
In `@tests/plugin_e2e/shutdown_signal_test.go`:
- Around line 65-84: Add a SIGTERM end-to-end test alongside
TestSIGINTRunsShutdownHookAndReturns130 that skips Windows, starts the signal
fork, sends syscall.SIGTERM, and waits for termination. Assert the process exits
with code 143 and verify the shutdown marker contains “shutdown”, mirroring the
existing SIGINT coverage.
---
Outside diff comments:
In `@internal/client/client.go`:
- Around line 506-519: Update APIClient.CheckResponseWithContext to project the
untyped resultMap into the typed response-error envelope before calling
errclass.BuildAPIError. Add and use a single projection function for this
response-error shape, handling invalid field shapes explicitly rather than
passing raw map data across the classification boundary.
In `@shortcuts/common/mcp_client.go`:
- Around line 128-160: Prioritize HTTP 429 rate-limit classification before
unknown business-code handling: update classifyMCPHTTPError in
shortcuts/common/mcp_client.go:128-160 and the errclass.BuildAPIError flow in
shortcuts/common/runner.go:357 to return errclass.NewRateLimitError for unknown
top-level or JSON-RPC codes while preserving retry_after_seconds; add coverage
for unknown-code 429 responses in shortcuts/common/mcp_client_test.go:304-344,
shortcuts/common/runner_response_test.go:17-45, and the HandleResponse tests in
internal/client/response.go.
🪄 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: 62b0c88d-9939-41ab-8c28-691dfe6769ab
📒 Files selected for processing (28)
CHANGELOG.mdcmd/api/api.gocmd/root.gocmd/service/service.gointernal/auth/uat_client.gointernal/auth/uat_client_refresh_test.gointernal/client/client.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/factory_default.gointernal/cmdutil/transport.gointernal/cmdutil/transport_test.gointernal/credential/default_provider.gointernal/credential/tat_fetch.gointernal/errclass/classify.gointernal/errclass/retry_after.gointernal/errclass/retry_after_test.gointernal/identitydiag/diagnostics.gointernal/qualitygate/rules/run.gointernal/qualitygate/rules/transport_retry.gointernal/qualitygate/rules/transport_retry_test.goshortcuts/common/mcp_client.goshortcuts/common/mcp_client_test.goshortcuts/common/runner.goshortcuts/common/runner_response_test.gosidecar/server-multi-tenant-demo/auth_bridge.gosidecar/server-multi-tenant-demo/handler.gotests/plugin_e2e/shutdown_signal_test.go
💤 Files with no reviewable changes (1)
- internal/cmdutil/transport.go
Comment on lines
+157
to
+201
| func TestGetValidAccessTokenCancelsRefreshRequest(t *testing.T) { | ||
| setupStoredTokenTest(t) | ||
| stored := newRefreshTestToken() | ||
| if err := SetStoredToken(stored); err != nil { | ||
| t.Fatalf("SetStoredToken() error = %v", err) | ||
| } | ||
|
|
||
| started := make(chan struct{}, 1) | ||
| var calls atomic.Int32 | ||
| client := &http.Client{Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { | ||
| calls.Add(1) | ||
| started <- struct{}{} | ||
| <-req.Context().Done() | ||
| return nil, req.Context().Err() | ||
| })} | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| resultCh := make(chan error, 1) | ||
| go func() { | ||
| _, err := GetValidAccessToken(ctx, client, newRefreshTestOptions(stored)) | ||
| resultCh <- err | ||
| }() | ||
|
|
||
| select { | ||
| case <-started: | ||
| case <-time.After(2 * time.Second): | ||
| t.Fatal("refresh request did not start") | ||
| } | ||
| cancel() | ||
|
|
||
| select { | ||
| case err := <-resultCh: | ||
| if !errors.Is(err, context.Canceled) { | ||
| t.Fatalf("GetValidAccessToken() error = %v, want context.Canceled cause", err) | ||
| } | ||
| case <-time.After(100 * time.Millisecond): | ||
| t.Fatal("GetValidAccessToken() did not return within 100ms after cancellation") | ||
| } | ||
| if calls.Load() != 1 { | ||
| t.Fatalf("refresh request count = %d, want 1", calls.Load()) | ||
| } | ||
| current := GetStoredToken(stored.AppId, stored.UserOpenId) | ||
| if current == nil || current.RefreshToken != stored.RefreshToken { | ||
| t.Fatalf("stored token = %#v, want canceled refresh to preserve it", current) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the cancellation error classification.
Lines 186-190 verify the context.Canceled cause. They do not verify the new typed error contract. Assert errs.ProblemOf(err) returns errs.CategoryNetwork and errs.SubtypeNetworkTransport.
Param is not applicable because this path returns *errs.NetworkError, not a validation error. Based on learnings, errs.ProblemOf exposes problem-level fields and does not expose Param. As per coding guidelines, error-path tests must assert typed metadata and preserve the cause.
🤖 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/auth/uat_client_refresh_test.go` around lines 157 - 201, The
cancellation assertion in TestGetValidAccessTokenCancelsRefreshRequest must
validate the typed error contract as well as preserve context.Canceled. Use
errs.ProblemOf(err) to assert CategoryNetwork and SubtypeNetworkTransport, while
retaining the existing errors.Is(err, context.Canceled) check; do not assert
Param for this *errs.NetworkError path.
Sources: Coding guidelines, Learnings
| // refreshWithLock serializes the complete refresh transaction with every | ||
| // stored-token writer and remover for this account. | ||
| func refreshWithLock(httpClient *http.Client, opts UATCallOptions) (*StoredUAToken, error) { | ||
| func refreshWithLock(ctx context.Context, httpClient *http.Client, opts UATCallOptions) (*StoredUAToken, error) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline internal/auth --items all --type function --match 'withTokenStorageLock|refreshWithLock|GetValidAccessToken'
rg -n -C 12 '\bwithTokenStorageLock\s*\(' internal/auth
rg -n -C 8 'GetValidAccessTokenCancelsRefreshRequest|ConcurrentRefreshesAreCoalesced' internal/auth/uat_client_refresh_test.goRepository: larksuite/cli
Length of output: 15888
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' internal/auth/uat_client.go
sed -n '49,130p' internal/auth/token_lock.go
sed -n '157,225p' internal/auth/uat_client_refresh_test.go
sed -n '402,470p' internal/auth/uat_client_refresh_test.go
rg -n 'tokenStorageProcessLocks|Lock\(|LockContext|select|<-' internal/auth/token_lock.go internal/auth/token_lock_test.goRepository: larksuite/cli
Length of output: 12482
Make the in-process token lock cancelable.
refreshWithLock passes cancellation only to the refresh HTTP request. The new tests show the HTTP side returns after cancellation, but when refreshWithLock is already queued behind another refresh, processLock.Lock() ignores the context. Use a cancellable/timeout-wait mechanism for the in-process sync.Mutex as well, so GetValidAccessToken stops while still waiting for lock acquisition.
🤖 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/auth/uat_client.go` at line 88, Update refreshWithLock so waiting
for processLock acquisition observes ctx cancellation instead of blocking in
sync.Mutex.Lock; use the existing cancellable or timeout-based lock-wait
mechanism, then return the context error when cancellation occurs before
acquisition while preserving normal refresh behavior after the lock is obtained.
Comment on lines
+109
to
+116
| if apiErr := check(result, identity, cc); apiErr != nil { | ||
| return apiErr | ||
| } | ||
| // CheckResponse treats business code 0 as success, so a 4xx/5xx whose | ||
| // JSON body omits a non-zero code would otherwise be served as a | ||
| // successful result. Classify by HTTP status so it is never swallowed. | ||
| if resp.StatusCode >= 400 { | ||
| return httpStatusError(resp.StatusCode, resp.RawBody) | ||
| return httpStatusError(resp.StatusCode, resp.RawBody, cc) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make HTTP 429 status authoritative for JSON error responses.
A nonzero JSON business code currently returns before the HTTP 429 fallback. This can remove the rate-limit subtype and retry delay.
internal/client/response.go#L109-L116: classifyhttp.StatusTooManyRequestsbefore returning a business-code error, or pass HTTP status intoerrclassclassification.internal/client/response_test.go#L367-L420: add an unrecognized nonzero JSON code with HTTP 429 and assert API category,rate_limitsubtype, andretry_after_secondsthrougherrs.ProblemOf.
📍 Affects 2 files
internal/client/response.go#L109-L116(this comment)internal/client/response_test.go#L367-L420
🤖 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/response.go` around lines 109 - 116, The response
classification in internal/client/response.go:109-116 must make HTTP 429
authoritative even when check returns a nonzero business-code error; update
check/errclass handling or add the 429 classification before returning apiErr so
the result preserves the rate-limit subtype and retry delay. Add coverage in
internal/client/response_test.go:367-420 using an unrecognized nonzero JSON code
with HTTP 429, and assert via errs.ProblemOf that the category is API, subtype
is rate_limit, and retry_after_seconds is populated.
Source: Coding guidelines
|
|
||
| func buildDirectHTTPTransport(base http.RoundTripper, platform bool) http.RoundTripper { | ||
| var builtIn http.RoundTripper = &RetryTransport{Base: base} | ||
| var builtIn http.RoundTripper = base |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a direct-transport single-request test.
Line 205 removes retries from direct HTTP transport. The supplied regression test calls buildSDKTransport only. It cannot fail if buildDirectHTTPTransport regains a retry wrapper. Add an HTTP 503 test that sends a request through buildDirectHTTPTransport and asserts one server call.
🤖 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/cmdutil/factory_default.go` at line 205, Add a regression test
targeting buildDirectHTTPTransport directly, using a test server that returns
HTTP 503 and counting received requests; send one request through the returned
transport and assert the server is called exactly once, ensuring direct
transport does not regain retry wrapping.
Source: Coding guidelines
Comment on lines
+62
to
+71
| var apiErr *errs.APIError | ||
| if !errors.As(err, &apiErr) { | ||
| t.Fatalf("BuildAPIError() = %T %v, want *errs.APIError", err, err) | ||
| } | ||
| if apiErr.RetryAfterSeconds != test.want { | ||
| t.Fatalf("retry_after_seconds = %d, want %d", apiErr.RetryAfterSeconds, test.want) | ||
| } | ||
| if test.want > 0 && (!strings.Contains(apiErr.Hint, "server requested") || !strings.Contains(apiErr.Hint, "safe to repeat")) { | ||
| t.Fatalf("hint = %q, want server delay and repeat-safety guidance", apiErr.Hint) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert common typed metadata through errs.ProblemOf.
These error-path tests inspect *errs.APIError directly. Add errs.ProblemOf(err) assertions for CategoryAPI and SubtypeRateLimit. Retain errors.As for RetryAfterSeconds. Do not assert Param because these server-side errors do not populate it.
internal/errclass/retry_after_test.go#L62-L71: Assert category and subtype througherrs.ProblemOf.shortcuts/common/mcp_client_test.go#L335-L341: Assert category and subtype througherrs.ProblemOf.shortcuts/common/runner_response_test.go#L36-L42: Assert category and subtype througherrs.ProblemOf.
As per coding guidelines, “Error-path tests must assert typed metadata through errs.ProblemOf.” Based on learnings, BuildAPIError server errors do not set Param.
📍 Affects 3 files
internal/errclass/retry_after_test.go#L62-L71(this comment)shortcuts/common/mcp_client_test.go#L335-L341shortcuts/common/runner_response_test.go#L36-L42
🤖 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/retry_after_test.go` around lines 62 - 71, Update the
error-path assertions in internal/errclass/retry_after_test.go:62-71,
shortcuts/common/mcp_client_test.go:335-341, and
shortcuts/common/runner_response_test.go:36-42 to use errs.ProblemOf(err) for
CategoryAPI and SubtypeRateLimit. Retain errors.As in retry_after_test.go for
validating RetryAfterSeconds, and do not assert Param because these server-side
errors do not populate it.
Sources: Coding guidelines, Learnings
Comment on lines
+18
to
+26
| func RetryAfterSeconds(header http.Header) int { | ||
| for _, name := range []string{"X-Ogw-Ratelimit-Reset", "Retry-After"} { | ||
| seconds, err := strconv.Atoi(strings.TrimSpace(header.Get(name))) | ||
| if err == nil && seconds > 0 { | ||
| return seconds | ||
| } | ||
| } | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Support HTTP-date Retry-After values.
Retry-After can contain either delay seconds or an HTTP date. Line 20 discards valid date values, so clients omit a server-provided retry delay.
internal/errclass/retry_after.go#L18-L26: Parse a valid HTTP-dateRetry-Aftervalue into a positive remaining delay after gateway-header handling.internal/errclass/retry_after_test.go#L16-L44: Add a valid HTTP-date case and assert that it produces a positive retry delay.
📍 Affects 2 files
internal/errclass/retry_after.go#L18-L26(this comment)internal/errclass/retry_after_test.go#L16-L44
🤖 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/retry_after.go` around lines 18 - 26, Update
RetryAfterSeconds in internal/errclass/retry_after.go to preserve the existing
gateway-header priority, then parse a valid HTTP-date Retry-After value and
return its positive remaining delay. Add a valid HTTP-date test case in
internal/errclass/retry_after_test.go covering the expected positive retry
duration.
Comment on lines
+25
to
+43
| func CheckTransportRetryLoops(repo string, changedFiles []string, changedOnly bool) ([]report.Diagnostic, error) { | ||
| files, err := transportRuleFiles(repo, changedFiles, changedOnly) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var diagnostics []report.Diagnostic | ||
| for _, path := range files { | ||
| src, err := vfs.ReadFile(filepath.Join(repo, filepath.FromSlash(path))) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fileDiagnostics, err := checkTransportRetrySource(path, src) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| diagnostics = append(diagnostics, fileDiagnostics...) | ||
| } | ||
| return diagnostics, nil |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Return typed errors from the new checker.
These paths return raw file-system and parser errors. Run returns them unchanged. Wrap each failure with the repository typed file-I/O or unknown-error constructor and preserve the cause with .WithCause(err). Add error-path tests that assert the typed metadata and cause preservation.
As per coding guidelines, **/*.go: “Use the prescribed typed error constructors for … file I/O failures, and unknown lower-layer errors.”
Also applies to: 46-50, 137-185
🤖 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/qualitygate/rules/transport_retry.go` around lines 25 - 43, The new
checker must return prescribed typed errors instead of raw filesystem or parser
errors. Update CheckTransportRetryLoops and the related error paths in
transportRuleFiles/checkTransportRetrySource to wrap file-read failures with the
repository’s typed file-I/O constructor and parser or other lower-layer failures
with the unknown-error constructor, preserving each original error via
WithCause(err). Add error-path tests asserting the typed metadata and preserved
cause.
Source: Coding guidelines
Comment on lines
+65
to
+84
| func TestSIGINTRunsShutdownHookAndReturns130(t *testing.T) { | ||
| if runtime.GOOS == "windows" { | ||
| t.Skip("os.Interrupt cannot be sent to Windows processes") | ||
| } | ||
|
|
||
| fork := startSignalFork(t, false) | ||
| if err := fork.cmd.Process.Signal(os.Interrupt); err != nil { | ||
| t.Fatalf("send SIGINT: %v", err) | ||
| } | ||
| waitErr := waitForForkExit(t, fork, 10*time.Second) | ||
|
|
||
| var exitErr *exec.ExitError | ||
| if !errors.As(waitErr, &exitErr) || exitErr.ExitCode() != 130 { | ||
| t.Fatalf("SIGINT exit error = %v, exit code = %d, want 130; stdout=%s stderr=%s", | ||
| waitErr, fork.cmd.ProcessState.ExitCode(), fork.stdout.String(), fork.stderr.String()) | ||
| } | ||
| if data, err := os.ReadFile(fork.shutdownMarker); err != nil || string(data) != "shutdown" { | ||
| t.Fatalf("Shutdown marker = %q, err=%v, want shutdown", data, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add SIGTERM exit-code coverage.
This file covers SIGINT exit code 130 but does not cover SIGTERM exit code 143. Add an end-to-end test that sends syscall.SIGTERM, asserts exit code 143, and verifies the shutdown marker.
As per coding guidelines, “Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure.”
🤖 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/plugin_e2e/shutdown_signal_test.go` around lines 65 - 84, Add a SIGTERM
end-to-end test alongside TestSIGINTRunsShutdownHookAndReturns130 that skips
Windows, starts the signal fork, sends syscall.SIGTERM, and waits for
termination. Assert the process exits with code 143 and verify the shutdown
marker contains “shutdown”, mirroring the existing SIGINT coverage.
Source: Coding guidelines
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2233 +/- ##
==========================================
- Coverage 76.34% 76.34% -0.01%
==========================================
Files 991 993 +2
Lines 106029 106204 +175
==========================================
+ Hits 80952 81082 +130
- Misses 18942 18976 +34
- Partials 6135 6146 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Aug 10, 2026
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
Lark API rate-limit errors currently expose
retryablebut discard the server-providedRetry-Afterdelay, and process interrupts can bypass extension Shutdown hooks. This PR reports the delay in the structured error envelope, removes the dormant transport retry implementation, and makes root-command interruption cancel in-flight work before running Shutdown hooks with the expected signal exit code.Changes
errclass.RetryAfterSecondsand pass response-header context throughinternal/client,shortcuts/common, and MCP response classification so rate-limit errors includeretry_after_secondsonly when the server supplies a positive value. The value is reported without a cap or automatic wait.cmdutil.RetryTransportfrom both SDK transport construction sites. The CLI contract already exposeserror.retryable, code99991400is explicitly retryable, and code1063006is intentionally not short-term retryable; hidden transport retries would conceal those facts from callers.transport_no_automatic_retrytointernal/qualitygate/rulesso aRoundTripimplementation cannot loop over calls to its underlying transport without an explicit documented exception.auth.GetValidAccessTokento the refresh HTTP request, including the multi-tenant sidecar example, and document that signature change inCHANGELOG.md.TestBuildSDKTransportDoesNotRetryServerErrorsand the repository-wide absence ofRetryTransport; cover interrupt ordering withTestSIGINTRunsShutdownHookAndReturns130andTestSecondSIGINTForcesExitDuringShutdown; cover rate-limit delays with the client, shortcut, and MCP response tests; and prevent reintroduction throughTestCheckTransportRetryLoopsand the repository scan test.Test Plan
make unit-testpassed with the race detectorgo vet ./...passedgofmt -l .produced no outputgo mod tidyproduced no changesgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/mainpassed with 0 issuesmake quality-gatepassedgo test -count=1 ./tests/plugin_e2e -run 'Test(SIGINTRunsShutdownHookAndReturns130|SecondSIGINTForcesExitDuringShutdown)'passedgo test -count=1 -tags authsidecar_multi_tenant_demo ./sidecar/server-multi-tenant-demopassedinternal/errclass,internal/credential,internal/client,shortcuts/common,internal/cmdutil,internal/qualitygate/rules,internal/auth, andinternal/identitydiaggo test -count=1 ./...completed with existing live E2E failures; the same failing packages and causes were confirmed on the base commitRelated Issues
N/A
Summary by CodeRabbit