fix(im): resume ranged downloads from the response, not the request - #2176
Open
sang-neo03 wants to merge 11 commits into
Open
fix(im): resume ranged downloads from the response, not the request#2176sang-neo03 wants to merge 11 commits into
sang-neo03 wants to merge 11 commits into
Conversation
A ranged resource download tracked its next offset from the range it asked for instead of the range the server said it sent. A 206 carrying a different slice of the same length was therefore written at the wrong offset: the delivered byte count still matched the total, so the final size check passed and the command reported success on a corrupt file. Every offset now comes from the response's Content-Range. A slice that starts where we resumed from is accepted whatever its end, so a server that returns the whole object in one 206, or serves smaller slices than requested, still completes; a slice that starts elsewhere, or reports a different total, is rejected before any byte reaches disk. Later chunks carry If-Range pinned to the probe's validator, so a resource replaced mid-download comes back as 200 and is rejected instead of splicing two versions into one file. Protocol violations get their own network subtype so they are not taken for retryable connection errors: replaying the same request cannot fix a server that answered with the wrong range. The Content-Range parser and its table test come from #1178.
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 IM resource download path validates ChangesRange download protocol handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant rangeChunkReader
participant HTTPResourceServer
participant OutputFile
CLI->>rangeChunkReader: Start resource download
rangeChunkReader->>HTTPResourceServer: Request initial range
HTTPResourceServer-->>rangeChunkReader: Return Content-Range, ETag, and body
rangeChunkReader->>rangeChunkReader: Validate range and response body
rangeChunkReader->>OutputFile: Write validated bytes
rangeChunkReader->>HTTPResourceServer: Request next range with If-Range
HTTPResourceServer-->>rangeChunkReader: Return next range response
rangeChunkReader->>OutputFile: Append validated bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
shortcuts/im/helpers_network_test.go (1)
788-794: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
md5.Sumwithbytes.Equalfor payload comparison.The repository ast-grep rule
weak-hash-md5-goflags everymd5.Sumcall, including these test comparisons. The tests only need byte equality, so a direct comparison removes the lint findings and reports mismatches without hashing. The same change applies at Lines 827-828.♻️ Proposed change
- if md5.Sum(got) != md5.Sum(payload) { - t.Fatalf("payload MD5 = %x, want %x", md5.Sum(got), md5.Sum(payload)) + if !bytes.Equal(got, payload) { + t.Fatalf("downloaded payload does not match the served payload (got %d bytes, want %d)", len(got), len(payload)) }Remove the
crypto/md5import after this 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 `@shortcuts/im/helpers_network_test.go` around lines 788 - 794, Update the payload assertions in the relevant network tests, including the comparison near the second referenced location, to use bytes.Equal(got, payload) instead of md5.Sum-based comparisons while preserving the failure assertion. Remove the now-unused crypto/md5 import.Source: Linters/SAST tools
shortcuts/im/im_messages_resources_download.go (2)
205-214: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReturn the overflow error even when
Closefails.In the
r.delivered > r.totalSizebranch a non-nilcloseErrreplaces the protocol error. The caller then sees a transport-level close failure instead of the response-contract violation, and the error loses itsSubtypeNetworkProtocolclassification. The adjacent EOF branch at Line 229 already gives the integrity failure precedence overcloseErr; apply the same order here.♻️ Proposed change
if r.delivered > r.totalSize { if err == io.EOF { - closeErr := r.current.Close() - r.current = nil - if closeErr != nil { - return 0, closeErr - } + _ = r.current.Close() + r.current = nil } return 0, errs.NewNetworkError(errs.SubtypeNetworkProtocol, "chunk overflow: delivered %d, expected %d", r.delivered, r.totalSize) }🤖 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 205 - 214, Update the delivered-overflow handling in the reader method containing r.delivered and r.totalSize so it always returns the existing SubtypeNetworkProtocol chunk-overflow error, even when r.current.Close() fails. Preserve closing and clearing r.current for io.EOF, but do not return closeErr from this branch.
548-595: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
parseContentRangerejects a valid empty-resource range.Line 585 rejects
total <= 0. A zero-length representation cannot appear in a206, so the check is safe today. But the message "invalid total size" also covers a legitimatebytes 0-0/1boundary only by accident of the later checks. Consider narrowing the guard tototal < 0and keeping theend >= totalcheck as the real bound, so the parser stays a faithful RFC 9110 parser and the callers own the policy.This is optional; current behavior is correct for this download path.
🤖 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 548 - 595, Optionally update parseContentRange to reject only negative total sizes by narrowing the total validation from total <= 0 to total < 0, while preserving the existing end >= total bounds check and caller-specific policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/im/helpers_network_test.go`:
- Around line 958-961: Strengthen the four range-rejection tests in
shortcuts/im/helpers_network_test.go at lines 958-961, 699-701, 925-927, and
988-990 by asserting errs.ProblemOf(err) reports CategoryNetwork and
SubtypeNetworkProtocol after the existing message checks; additionally assert
Retryable is false at lines 925-927. Reuse the pattern demonstrated by
TestDownloadIMResourceToPathRejectsChunkAtWrongOffset.
---
Nitpick comments:
In `@shortcuts/im/helpers_network_test.go`:
- Around line 788-794: Update the payload assertions in the relevant network
tests, including the comparison near the second referenced location, to use
bytes.Equal(got, payload) instead of md5.Sum-based comparisons while preserving
the failure assertion. Remove the now-unused crypto/md5 import.
In `@shortcuts/im/im_messages_resources_download.go`:
- Around line 205-214: Update the delivered-overflow handling in the reader
method containing r.delivered and r.totalSize so it always returns the existing
SubtypeNetworkProtocol chunk-overflow error, even when r.current.Close() fails.
Preserve closing and clearing r.current for io.EOF, but do not return closeErr
from this branch.
- Around line 548-595: Optionally update parseContentRange to reject only
negative total sizes by narrowing the total validation from total <= 0 to total
< 0, while preserving the existing end >= total bounds check and caller-specific
policy.
🪄 Autofix (Beta)
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: ad6b605a-8998-4495-b878-a54ddd737d14
📒 Files selected for processing (4)
errs/subtypes.goshortcuts/im/helpers_network_test.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_resources_download.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1bd1c48a1daf79c9ea8a63e319568039f7bc5c83🧩 Skill updatenpx skills add larksuite/cli#fix/im-range-resume-from-response -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2176 +/- ##
==========================================
+ Coverage 75.70% 75.74% +0.03%
==========================================
Files 944 944
Lines 100288 100457 +169
==========================================
+ Hits 75926 76088 +162
- Misses 18565 18570 +5
- Partials 5797 5799 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review of the previous commit found that the If-Range pinning it added was opportunistic: when the first 206 carried no usable validator, later requests went out with only a Range header. A same-length replacement between two requests then passed every check — matching start, matching total, matching byte count — and produced a file whose first half came from one version and second half from another, reported as success. An adversarial run confirmed it: prefix 'A', suffix 'B', exit code 0. Parts may only be combined when one strong validator ties them together (RFC 9110 15.3.7.3), so that is now a precondition rather than a bonus: - rangeValidator accepts a strong entity-tag only. A weak tag must not be sent in If-Range, and holding any entity-tag rules out sending a date instead (RFC 9110 13.1.5); a date is only a strong validator under conditions a client cannot confirm from the response (8.8.2.2). - When more ranges are needed and no strong validator exists, the probe is dropped and the resource is re-read as one rangeless stream, which needs no combining. A server that answers that request with a partial response too leaves no safe way to assemble the file, so the download fails. - Every later 206 must carry the first response's validator. If-Range is the server's job; checking it ourselves is what catches a server that ignores it. Two more findings from the same review: - Accepting shorter slices than requested had no ceiling, so a server answering one byte at a time turned one download into one request per byte: 2048 requests for 2048 bytes, and it succeeded. The response count is now bounded by max(64, 4x the expected chunk count). - An If-Range mismatch answered with 200 is correct HTTP, not a protocol violation, and re-running the command does fix it. It now uses a dedicated representation_changed subtype, marked retryable, with the retry advice moved into the hint. - A failing Close no longer masks the protocol error that ended the transfer; it is attached as the cause instead. The masked error was surfacing as internal/file_io, losing the reason entirely. Error assertions in the download tests now check category, subtype and retryable through one helper instead of matching message text, so a regression to network/transport cannot pass unnoticed. New cases cover the fallback, the mixed-version scenario, a missing and a changed validator on a later chunk, and the response-count ceiling. Adds the live E2E the shortcut never had: upload a 320 KiB fixture with `+messages-send --file`, read its file_key off the message, download it and compare MD5. The size crosses the probe chunk so the ranged path runs against the real endpoint, whose ETag and Content-Range behaviour no fake server can prove.
github-actions
Bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shortcuts/im/helpers_network_test.go (2)
1137-1196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
imRangeServerfor these hand-rolled round-trip closures.Lines 926-946, 957-974, 1140-1162, and 1172-1191 each rebuild the same probe/range handler that
imRangeServeralready provides. ExtendingimRangeServerwith per-attempt header control would remove four near-identical closures and keep theContent-Rangefixture format in one place.🤖 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/helpers_network_test.go` around lines 1137 - 1196, Refactor the validator-change tests around TestDownloadIMResourceToPathRejectsValidatorChangeOnLaterChunk and TestDownloadIMResourceToPathRejectsMissingValidatorOnLaterChunk to reuse imRangeServer instead of duplicating probe, range parsing, and Content-Range response logic. Extend imRangeServer with per-attempt response-header control so these tests can return v1 then v2, or omit the validator on later chunks, while preserving their existing assertions.
1056-1101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert version uniformity instead of absence of mixing.
The current check passes when the file contains only
Abytes, onlyBbytes, or arbitrary other content. A stronger assertion pins the intended contract: the output equals exactly one of the two versions.♻️ Proposed stronger assertion
- if bytes.Contains(got, []byte("A")) && bytes.Contains(got, []byte("B")) { - t.Fatalf("file mixes both versions: first=%q last=%q", got[0:1], got[len(got)-1:]) - } + if !bytes.Equal(got, versionA) && !bytes.Equal(got, versionB) { + t.Fatalf("file is not a single version: len=%d first=%q last=%q", len(got), got[0:1], got[len(got)-1:]) + }🤖 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/helpers_network_test.go` around lines 1056 - 1101, Strengthen the output validation in TestDownloadIMResourceToPathNeverMixesVersionsWithoutValidator by asserting that got exactly equals either versionA or versionB. Replace the current bytes.Contains-based mixed-content check so arbitrary or partially corrupted output cannot pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/im/helpers_network_test.go`:
- Around line 1232-1249: The large-file case in TestMaxRangeResponses has an
incorrect expected value. Update the “large file scales with chunk count” test
case to expect 28 for 400 * normalChunkSize, matching maxRangeResponses’
fallback behavior.
---
Nitpick comments:
In `@shortcuts/im/helpers_network_test.go`:
- Around line 1137-1196: Refactor the validator-change tests around
TestDownloadIMResourceToPathRejectsValidatorChangeOnLaterChunk and
TestDownloadIMResourceToPathRejectsMissingValidatorOnLaterChunk to reuse
imRangeServer instead of duplicating probe, range parsing, and Content-Range
response logic. Extend imRangeServer with per-attempt response-header control so
these tests can return v1 then v2, or omit the validator on later chunks, while
preserving their existing assertions.
- Around line 1056-1101: Strengthen the output validation in
TestDownloadIMResourceToPathNeverMixesVersionsWithoutValidator by asserting that
got exactly equals either versionA or versionB. Replace the current
bytes.Contains-based mixed-content check so arbitrary or partially corrupted
output cannot pass.
🪄 Autofix (Beta)
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: 21e0f95b-75f4-4658-9b27-003ed658fc12
📒 Files selected for processing (6)
errs/subtypes.goshortcuts/im/helpers_network_test.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_resources_download.gotests/cli_e2e/im/coverage.mdtests/cli_e2e/im/message_resource_download_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- errs/subtypes.go
- shortcuts/im/im_messages_resources_download.go
Comment on lines
+1232
to
1249
| func TestMaxRangeResponses(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| total int64 | ||
| want int | ||
| }{ | ||
| {name: "small file uses the floor", total: 1024, want: 64}, | ||
| {name: "exactly one chunk uses the floor", total: normalChunkSize, want: 64}, | ||
| {name: "large file scales with chunk count", total: 400 * normalChunkSize, want: 4 * 401}, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if got := maxRangeResponses(tt.total); got != tt.want { | ||
| t.Fatalf("maxRangeResponses(%d) = %d, want %d", tt.total, got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect maxRangeResponses and the chunk-size constants it depends on.
fd -t f 'im_messages_resources_download.go' shortcuts | while IFS= read -r f; do
ast-grep run --pattern 'func maxRangeResponses($$$) { $$$ }' --lang go "$f"
rg -n -C3 'probeChunkSize|normalChunkSize|maxRangeResponses' "$f"
doneRepository: larksuite/cli
Length of output: 1942
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="$(fd -t f 'im_messages_resources_download.go' shortcuts | head -n 1)"
sed -n '613,625p' "$f"
python3 - <<'PY'
from math import ceil
normal_chunk_size = 8 * 1024 * 1024
def max_range_responses(total_size):
expected = total_size // normal_chunk_size + 1
generous = 4 * expected
return generous if generous > 64 else 64
for total, expected in [
("small file uses the floor", 1024, 64),
("exactly one chunk uses the floor", normal_chunk_size, 64),
("large file scales with chunk count", 400 * normal_chunk_size, 28),
]:
print(f"{total}: max_range_responses({expected}) = {max_range_responses(expected)}")
PYRepository: larksuite/cli
Length of output: 830
Fix the “large file” expected value in TestMaxRangeResponses.
maxRangeResponses returns 4 * (total/normalChunkSize + 1) only when that value exceeds 64; the function then falls back to 64 for 400 chunks. Set the expected value to 28 for 400 * normalChunkSize.
🤖 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/helpers_network_test.go` around lines 1232 - 1249, The
large-file case in TestMaxRangeResponses has an incorrect expected value. Update
the “large file scales with chunk count” test case to expect 28 for 400 *
normalChunkSize, matching maxRangeResponses’ fallback behavior.
… fallback bodyclose cannot see that the caller closes the returned response body through the io.ReadCloser it assigns, so returning the body and its length directly keeps ownership of the close unambiguous at the call site.
…ator Requiring a validator before combining range responses is only worth anything if the value is one. The check accepted any ETag that was neither empty nor weak, so a server answering `ETag: not-a-valid-entity-tag` on every response passed it: two parts "matched" on a string that identifies nothing, and a same-length replacement between the two requests still produced a file with one version at the front and another at the back. The value must now parse as RFC 9110 8.8.3 opaque-tag = DQUOTE *etagc DQUOTE, in a field carrying exactly one value. Bare strings, unterminated quotes, embedded quotes, control characters, an empty opaque-tag, and multi-valued fields all read as "no validator", which routes the download to the rangeless single-stream path instead. Later chunks compare through the same parser, so a malformed tag mid-download stops the transfer too.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli_e2e/im/message_resource_download_workflow_test.go (1)
42-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister IM live-chat cleanup in
createChat.
createChatreturns a new private chat but does not register its deletion withparentT.Cleanup. Register cleanup after chat creation, or expose a cleanup routine and register it in this test. Chat delete isn’t available via theimCLI, so this workflow cannot be self-contained unless e2e cleanup happens through the test account.🤖 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` around lines 42 - 54, The createChat flow must register deletion of the newly created private chat with parentT.Cleanup. Update createChat, or expose a suitable cleanup routine and register it immediately after chatID is created, using the test account because the im CLI lacks chat deletion support.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.
Nitpick comments:
In `@tests/cli_e2e/im/message_resource_download_workflow_test.go`:
- Around line 42-54: The createChat flow must register deletion of the newly
created private chat with parentT.Cleanup. Update createChat, or expose a
suitable cleanup routine and register it immediately after chatID is created,
using the test account because the im CLI lacks chat deletion support.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e67a4530-5875-4a55-b86c-f2489c193e16
📒 Files selected for processing (7)
errs/ERROR_CONTRACT.mdshortcuts/im/helpers_network_test.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_resources_download.gotests/cli_e2e/im/coverage.mdtests/cli_e2e/im/message_resource_download_content_test.gotests/cli_e2e/im/message_resource_download_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/cli_e2e/im/coverage.md
- shortcuts/im/helpers_test.go
- shortcuts/im/im_messages_resources_download.go
- shortcuts/im/helpers_network_test.go
The live workflow read the key with gjson, assuming the raw platform JSON
`{"file_key":"..."}`. `im +messages-mget` converts message content to the
CLI display form first, so a file message arrives as
`<file key="file_v3_..." name="resource.bin"/>` and the lookup returned an
empty string: the e2e-live job failed three times before the download
subtest ever ran. The key is now read out of that attribute, with a table
test pinning both shapes so the assumption cannot drift again.
Failure and skip messages no longer echo stdout or stderr. This job runs on
a public repository and the mget envelope carries live chat, message,
sender, app-link and tenant identifiers; the diagnostics keep the exit
code, error subtype and whether the field was present.
The no-validator fallback throws its probe response away and streams a second one, but the file name, extension and Content-Type were still read off the probe. A resource replaced between the two requests — the case the fallback exists for — would be saved under the abandoned version name: a probe advertising stale.txt/text/plain against a body delivered as current.bin/application/octet-stream landed on disk as stale.txt. The body, its expected size and its headers now travel together as one value, and the path and save options are computed after the branch picks a source, so there is one response to read all three from. Also from the same review pass: - The range unit is case-insensitive. RFC 9110 14.1 spells bytes-unit as a plain ABNF string, and ABNF strings match either case unless marked %s, so `Content-Range: Bytes 0-9/10` is valid and was being rejected. - A later 206 with no usable validator is not the same failure as one whose validator changed. A changed strong tag means the resource was replaced and starting over resolves it, so it stays retryable representation_changed. No usable validator means the server cannot support safe combining and asking again gets the same answer, so it is now network/protocol and not retryable. - ERROR_CONTRACT.md lists both new subtypes with their retry semantics. - The initial invalid-Content-Range and chunk-overflow tests assert category, subtype and retryable instead of message text; the close-error-as-cause behaviour is now pinned by a test rather than a throwaway check; and the response-ceiling test asserts the exact request count rather than a range. - The live E2E comment and coverage note no longer claim the ranged path is proven. The command output does not say whether the endpoint answered 206 or ignored the Range, so the test proves the round trip and the unit tests prove the path.
sang-neo03
force-pushed
the
fix/im-range-resume-from-response
branch
from
August 4, 2026 09:27
efc0bec to
30379a9
Compare
A body that stops short of the length it framed reports io.ErrUnexpectedEOF, not a clean io.EOF, so it never reached the short-body check: the reader returned the raw error, the save wrapper re-labelled it, and the envelope read `internal/file_io: cannot create file: unexpected EOF` — pointing at the local disk for what is a truncated response. Truncation is now network/protocol with the original error as the cause, and any other read error becomes network/transport rather than travelling untyped into the save path. Also recall the fixture message when the live workflow finishes, so a run no longer leaves a file message in the test account on every CI cycle. The chat still stays behind: lark-cli exposes no chat-delete command, which is why the shared createChatAs helper registers an empty cleanup. Recalling the message is the part of create -> use -> cleanup this suite can honour.
Requiring a strong validator before combining ranges was measured against the real endpoint and turned out to disable ranged downloads outright: a probe for a 320 KiB file answers 206 Content-Range: bytes 0-131071/327680 (no ETag at all) so every `--type file` download fell through to the rangeless fallback and none of the per-chunk checks this branch added could ever run in production. That trade is the wrong way round. Chunking is what keeps each request inside its own timeout budget; one stream means one budget for the whole body, which is what would actually break a large file on a slow link — and it would break it every time, to guard against a replacement that the resource cannot undergo, since an IM attachment is fixed once its message is sent. Ranges now continue when no validator is offered, with every other check kept: the start of each response must be where the transfer resumed from, the total size must not change, the body must deliver the length its Content-Range declared, and the response count stays bounded. If-Range and the per-chunk validator comparison apply only when the probe actually produced a strong entity-tag. What is lost without one is narrow and now stated in the code: a replacement of exactly the same length cannot be detected. The single-stream fallback and the resourceStream value it needed are gone with it — with one response there is nothing for the file name and MIME type to diverge from. Verified end to end against the live endpoint with the branch build: a 3 MiB file downloads as probe + one range, If-Range correctly absent, and the bytes match the upload exactly.
The shared clie2e.Result assertions and ReportCleanupFailure print the full stdout and stderr when they fail, and this suite's envelopes carry live chat ids, message ids, resource keys, app links, sender profiles and tenant identifiers. e2e-live runs on a public repository, so a failing assertion published all of it — which contradicted the comment two lines above saying content and stdout are deliberately not echoed. Assertions now go through one helper that logs the exit code and the error type and subtype only, and the cleanup path reports the same instead of the whole command result.
Without a validator the later requests carry only a Range, so a new total is the server correctly reporting a resource that changed under us — and running the command again reads the new version. That is representation_changed and retryable, not the flat protocol error it was reporting, which told callers to give up on something a retry fixes. With If-Range on the wire a changed resource has to come back as 200 instead, so a 206 describing a different total means the condition was ignored: that stays a non-retryable protocol failure, now named as such. Also accept an empty opaque-tag. RFC 9110 8.8.3 lists `ETag: ""` among its valid examples, and a server using it is still making a strong-comparison promise; the length guard was rejecting it as if the field were absent. Adds the dry-run E2E this shortcut never had, which AGENTS.md requires for every shortcut change. The existing im_download_resources_dryrun_test.go covers `+chat-messages-list --download-resources`, a different command. The new test pins the method, the resource URL, the type parameter and the echoed message id, file key and output path, plus the output-path rejection for absolute and parent-escaping paths.
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
A ranged
im +messages-resources-downloadtracked its next offset from the range it asked for instead of the range the server said it sent, so a206carrying a different slice of the same length was written at the wrong offset. The delivered byte count still matched the total, the final size check passed, and the command reported success with exit code 0 on a corrupt file. Every offset now comes from the response'sContent-Range, and range responses are only combined when one strong validator ties them together.Changes
rangeChunkReaderderivesnextOffsetand the per-chunk expected length from the response'sContent-Rangeinstead of from the requested range. A slice that starts where we resumed from is accepted whatever its end, so a server that returns the whole object in one206— or serves smaller slices than requested — still completes.Content-Rangecontradicts.rangeValidatoraccepts only a well-formed strongETag(RFC 9110 §8.8.3opaque-tag): a weak tag must not be sent inIf-Range, holding any entity-tag rules out sending a date instead (§13.1.5), and a bare or unterminated string is not a validator at all. When one is present,If-Rangegoes out on every later chunk and each response's validator is compared against the first.206 Content-Range: bytes 0-131071/327680with noETagat all. Requiring one would disable ranged downloads outright and put every file behind a single request — one timeout budget for the whole body instead of one per chunk, which is what would actually break a large file on a slow link, every time, to guard against a replacement an IM attachment cannot undergo once its message is sent. Every other check is kept: each response must start where the transfer resumed from, the total size must not change, the body must deliver the length itsContent-Rangedeclared, and the response count stays bounded. The one thing that cannot be detected without a validator — a replacement of exactly the same length — is stated in the code.If-Rangeon every chunk after the probe, and check the returned validator against the first response's.If-Rangeis the server's job; checking it ourselves is what catches a server that ignores it. A missing or changed validator ends the download.max(64, 4 × expected chunk count). Accepting shorter slices than requested otherwise has no ceiling: a server answering one byte at a time turns one download into one request per byte.network / protocolsubtype and are not retryable — replaying the same request cannot fix a server that answered with the wrong range. A resource that changed mid-transfer uses a separaterepresentation_changedsubtype, marked retryable, with the "run it again" advice in the hint: the peer behaved correctly there, so an agent should retry rather than give up. A failingCloseno longer masks the protocol error that ended the transfer (it surfaced asinternal/file_io, losing the reason); it is attached as the cause.parseTotalSizewith a fullContent-Rangeparser (contentRange+parseContentRange), taken from fix(im): validate Content-Range for resource download chunks #1178 along with its table test.Test Plan
go test -count=1 ./errs/... ./internal/... ./shortcuts/...— all green, including the full./shortcuts/impackagego vet ./...clean;gofmt -l .cleancd lint && go run . .. --changed-from main) reports the same 76 pre-existing findings as cleanmain, none in the files this PR touchesrequireDownloadProblemhelper that checks category, subtype and retryable, so a regression tonetwork/transportcannot pass on message text aloneexit 0, file with prefixAand suffixB; now: single-version file via the rangeless fallback206for a 2048-byte file → before: 2048 requests, success; now: stops at 64 withnetwork/protocolClosefailing on an over-long body → before:internal/file_io: close failed; now:network/protocolwith the close error as causeendwith no start check (today'smain):TestDownloadIMResourceToPathRejectsChunkAtWrongOffsetreportserror = <nil>— the download "succeeds" on a corrupt file, which is the bug this PR fixesTestDownloadIMResourceToPathAcceptsWholeFileInOneRangeResponsefails withunexpected initial Content-Range: got bytes 0-135167/135168, want bytes 0-131071/135168skipped: tenant test credentials not set); it runs in thee2e-livejob206 bytes 0-131071/327680, noETag; download exits 0 and the bytes match the upload (cmpclean)131072-3145727),If-Rangecorrectly absent since there is no validator to pin to, bytes match exactlyNew unit coverage: whole file in one
206; shorter slices than requested (asserting every follow-up resumes exactly where the previous response ended); chunk at the wrong offset;If-Rangepinning; rangeless fallback when no strong validator exists; mixed-version scenario; server that refuses to serve the whole resource; missing and changed validator on a later chunk; response-count ceiling with output cleanup; resource changed mid-download; total size changed mid-download; body longer than its declaredContent-Range;parseContentRange,rangeValidatorandmaxRangeResponsestable tests.New live E2E (
TestIM_MessageResourceDownloadWorkflowAsBot): uploads a 320 KiB fixture withim +messages-send --file, reads itsfile_keyback off the message, downloads it and compares MD5. The size crosses the 128 KiB probe chunk so the ranged path runs against the real endpoint, whoseETagandContent-Rangebehaviour no fake server can prove.tests/cli_e2e/im/coverage.mdmoves the shortcut from uncovered to covered (11 → 12, 36.7% → 40.0%).Not included, deliberately: the output-path normalization and the HTTP-status error-subtype changes from #1178. The latter edits
downloadResponseError, which is unreachable —internal/client.DoStreamalready turns any non-2xx into a typed error (5xx →server_error, status written toerror.code), so a response with status ≥ 400 never reaches that helper.Related Issues
206, so the ranged path is real, but it never offers a validator — so a design that gates ranges on one is designing against an imagined server rather than the one in production. That is the same mistake fix(im): validate Content-Range for resource download chunks #1178 makes in the other direction.206, or smaller slices) and leaves the harder integrity gap open: chunks coming from two different versions of the file pass everyContent-Rangecheck.