fix(minutes): make media downloads resilient - #2245
Open
liangshuo-1 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe Minutes media download path now uses the internal ChangesMinutes media download
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant downloadMediaFile
participant download.Open
participant FilenameResolver
participant FileSaving
downloadMediaFile->>download.Open: Open immutable-source stream with 128 MiB parts
download.Open-->>FilenameResolver: Provide response headers
download.Open-->>FileSaving: Provide content type, length, and body
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 |
github-actions
Bot
added
the
size/M
![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
🤖 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/minutes/minutes_download_test.go`:
- Around line 283-332: Update TestDownloadRejectsInvalidResponseBodies in
shortcuts/minutes/minutes_download_test.go:283-332 to inspect errors through
errs.ProblemOf, asserting the network category and expected subtype while
preserving and validating the truncated-response cause; do not use Param on
errs.Problem. Also update the error assertion at
shortcuts/minutes/minutes_download_test.go:889-893 to use errs.ProblemOf, assert
the network category and SubtypeNetworkServer, and retain the existing
retryability assertion.
🪄 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: 455fafad-ffa2-41ad-b2e2-acfd789aaaad
📒 Files selected for processing (2)
shortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
Comment on lines
+283
to
+332
| func TestDownloadRejectsInvalidResponseBodies(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| header http.Header | ||
| body string | ||
| subtype errs.Subtype | ||
| }{ | ||
| { | ||
| name: "truncated", | ||
| header: http.Header{"Content-Length": []string{"10"}}, | ||
| body: "short", | ||
| subtype: errs.SubtypeNetworkProtocol, | ||
| }, | ||
| { | ||
| name: "encoded", | ||
| header: http.Header{"Content-Encoding": []string{"gzip"}}, | ||
| body: "compressed", | ||
| subtype: errs.SubtypeNetworkProtocol, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| chdir(t, t.TempDir()) | ||
| f, _, _, reg := cmdutil.TestFactory(t, defaultConfig()) | ||
| reg.Register(mediaStub("tok001", "https://example.com/media")) | ||
| f.HttpClient = func() (*http.Client, error) { | ||
| return &http.Client{Transport: minutesRoundTripFunc(func(req *http.Request) (*http.Response, error) { | ||
| return &http.Response{ | ||
| StatusCode: http.StatusOK, | ||
| Header: tt.header, | ||
| Body: io.NopCloser(strings.NewReader(tt.body)), | ||
| ContentLength: 10, | ||
| Request: req, | ||
| }, nil | ||
| })}, nil | ||
| } | ||
|
|
||
| err := mountAndRun(t, MinutesDownload, []string{ | ||
| "+download", "--minute-tokens", "tok001", "--output", "out.media", "--as", "bot", | ||
| }, f, nil) | ||
| var networkErr *errs.NetworkError | ||
| if !errors.As(err, &networkErr) || networkErr.Subtype != tt.subtype { | ||
| t.Fatalf("error = %T %v, want network/%s", err, err, tt.subtype) | ||
| } | ||
| if _, statErr := os.Stat("out.media"); !errors.Is(statErr, os.ErrNotExist) { | ||
| t.Fatalf("partial output should not exist: %v", statErr) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete typed error contract.
Both tests bypass errs.ProblemOf, so neither verifies the error category.
shortcuts/minutes/minutes_download_test.go#L283-L332: Assert network category and subtype witherrs.ProblemOf. Assert the truncated-response cause is preserved.shortcuts/minutes/minutes_download_test.go#L889-L893: Assert network category andSubtypeNetworkServerwitherrs.ProblemOf, while retaining the retryability assertion.
As per coding guidelines, error-path tests must assert typed metadata through errs.ProblemOf and preserve applicable causes. Based on learnings, Param is not a field on errs.Problem for these network errors.
📍 Affects 1 file
shortcuts/minutes/minutes_download_test.go#L283-L332(this comment)shortcuts/minutes/minutes_download_test.go#L889-L893
🤖 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/minutes/minutes_download_test.go` around lines 283 - 332, Update
TestDownloadRejectsInvalidResponseBodies in
shortcuts/minutes/minutes_download_test.go:283-332 to inspect errors through
errs.ProblemOf, asserting the network category and expected subtype while
preserving and validating the truncated-response cause; do not use Param on
errs.Problem. Also update the error assertion at
shortcuts/minutes/minutes_download_test.go:889-893 to use errs.ProblemOf, assert
the network category and SubtypeNetworkServer, and retain the existing
retryability assertion.
Sources: Coding guidelines, Learnings
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@06185f7feb7071f0d91201e7d71fd83f0dceee7b🧩 Skill updatenpx skills add larksuite/cli#fix/minutes-resilient-download -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2245 +/- ##
=======================================
Coverage 76.36% 76.37%
=======================================
Files 1011 1011
Lines 111269 111270 +1
=======================================
+ Hits 84970 84978 +8
+ Misses 19815 19811 -4
+ Partials 6484 6481 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Route minute media through the shared sequential range reader, and make the framework describe what it hands back: a multipart stream now reports the whole object's length and drops the first part's Content-Range, so Stream.Header matches the bytes the caller actually receives.
liangshuo-1
force-pushed
the
fix/minutes-resilient-download
branch
from
August 10, 2026 03:32
055ae22 to
06185f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/download/download_test.go`:
- Around line 388-418: Add coverage in TestOpenReportsAssembledStreamHeaders for
Content-Disposition: set a representative filename header on the ranged response
created by testPartial, then assert the assembled stream preserves the same
value alongside the existing Content-Type assertion.
🪄 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: c474cff9-4fc0-44fc-b891-155289e36586
📒 Files selected for processing (4)
internal/download/download.gointernal/download/download_test.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/minutes/minutes_download.go
- shortcuts/minutes/minutes_download_test.go
Comment on lines
+388
to
+418
| // Callers read Content-Type and Content-Disposition off the assembled stream, | ||
| // so those must survive, while the first part's framing must not leak out as a | ||
| // length shorter than the bytes actually delivered. | ||
| func TestOpenReportsAssembledStreamHeaders(t *testing.T) { | ||
| full := []byte("abcdefgh") | ||
| stream, err := openTest(context.Background(), func(_ context.Context, req Request) (*http.Response, error) { | ||
| start := req.Range.Start | ||
| end := min(req.Range.End, int64(len(full))-1) | ||
| resp := testPartial(full[start:end+1], start, end, int64(len(full)), "") | ||
| resp.Header.Set("Content-Length", fmt.Sprint(end-start+1)) | ||
| resp.Header.Set("Content-Type", "video/mp4") | ||
| return resp, nil | ||
| }, testOptions()) | ||
| if err != nil { | ||
| t.Fatalf("Open() error = %v", err) | ||
| } | ||
| defer stream.Body.Close() | ||
|
|
||
| if stream.ContentLength != int64(len(full)) { | ||
| t.Fatalf("ContentLength = %d, want %d", stream.ContentLength, len(full)) | ||
| } | ||
| if got, want := stream.Header.Get("Content-Length"), fmt.Sprint(len(full)); got != want { | ||
| t.Errorf("Content-Length header = %q, want %q (the whole stream, not the first part)", got, want) | ||
| } | ||
| if got := stream.Header.Get("Content-Range"); got != "" { | ||
| t.Errorf("Content-Range header = %q, want it dropped from the assembled stream", got) | ||
| } | ||
| if got := stream.Header.Get("Content-Type"); got != "video/mp4" { | ||
| t.Errorf("Content-Type header = %q, want it preserved", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add a Content-Disposition assertion.
The test comment states that callers depend on Content-Type and Content-Disposition. The fixture and assertions cover only Content-Type. Add a Content-Disposition header to the ranged response and assert that the assembled stream retains it. Otherwise, a regression that drops the filename header can pass this test.
As per coding guidelines, contract tests must assert each changed field or behavior directly.
Proposed test extension
resp.Header.Set("Content-Type", "video/mp4")
+ resp.Header.Set("Content-Disposition", `attachment; filename="minute.mp4"`)
return resp, nil
@@
if got := stream.Header.Get("Content-Type"); got != "video/mp4" {
t.Errorf("Content-Type header = %q, want it preserved", got)
}
+ if got := stream.Header.Get("Content-Disposition"); got != `attachment; filename="minute.mp4"` {
+ t.Errorf("Content-Disposition header = %q, want it preserved", got)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Callers read Content-Type and Content-Disposition off the assembled stream, | |
| // so those must survive, while the first part's framing must not leak out as a | |
| // length shorter than the bytes actually delivered. | |
| func TestOpenReportsAssembledStreamHeaders(t *testing.T) { | |
| full := []byte("abcdefgh") | |
| stream, err := openTest(context.Background(), func(_ context.Context, req Request) (*http.Response, error) { | |
| start := req.Range.Start | |
| end := min(req.Range.End, int64(len(full))-1) | |
| resp := testPartial(full[start:end+1], start, end, int64(len(full)), "") | |
| resp.Header.Set("Content-Length", fmt.Sprint(end-start+1)) | |
| resp.Header.Set("Content-Type", "video/mp4") | |
| return resp, nil | |
| }, testOptions()) | |
| if err != nil { | |
| t.Fatalf("Open() error = %v", err) | |
| } | |
| defer stream.Body.Close() | |
| if stream.ContentLength != int64(len(full)) { | |
| t.Fatalf("ContentLength = %d, want %d", stream.ContentLength, len(full)) | |
| } | |
| if got, want := stream.Header.Get("Content-Length"), fmt.Sprint(len(full)); got != want { | |
| t.Errorf("Content-Length header = %q, want %q (the whole stream, not the first part)", got, want) | |
| } | |
| if got := stream.Header.Get("Content-Range"); got != "" { | |
| t.Errorf("Content-Range header = %q, want it dropped from the assembled stream", got) | |
| } | |
| if got := stream.Header.Get("Content-Type"); got != "video/mp4" { | |
| t.Errorf("Content-Type header = %q, want it preserved", got) | |
| } | |
| } | |
| // Callers read Content-Type and Content-Disposition off the assembled stream, | |
| // so those must survive, while the first part's framing must not leak out as a | |
| // length shorter than the bytes actually delivered. | |
| func TestOpenReportsAssembledStreamHeaders(t *testing.T) { | |
| full := []byte("abcdefgh") | |
| stream, err := openTest(context.Background(), func(_ context.Context, req Request) (*http.Response, error) { | |
| start := req.Range.Start | |
| end := min(req.Range.End, int64(len(full))-1) | |
| resp := testPartial(full[start:end+1], start, end, int64(len(full)), "") | |
| resp.Header.Set("Content-Length", fmt.Sprint(end-start+1)) | |
| resp.Header.Set("Content-Type", "video/mp4") | |
| resp.Header.Set("Content-Disposition", `attachment; filename="minute.mp4"`) | |
| return resp, nil | |
| }, testOptions()) | |
| if err != nil { | |
| t.Fatalf("Open() error = %v", err) | |
| } | |
| defer stream.Body.Close() | |
| if stream.ContentLength != int64(len(full)) { | |
| t.Fatalf("ContentLength = %d, want %d", stream.ContentLength, len(full)) | |
| } | |
| if got, want := stream.Header.Get("Content-Length"), fmt.Sprint(len(full)); got != want { | |
| t.Errorf("Content-Length header = %q, want %q (the whole stream, not the first part)", got, want) | |
| } | |
| if got := stream.Header.Get("Content-Range"); got != "" { | |
| t.Errorf("Content-Range header = %q, want it dropped from the assembled stream", got) | |
| } | |
| if got := stream.Header.Get("Content-Type"); got != "video/mp4" { | |
| t.Errorf("Content-Type header = %q, want it preserved", got) | |
| } | |
| if got := stream.Header.Get("Content-Disposition"); got != `attachment; filename="minute.mp4"` { | |
| t.Errorf("Content-Disposition header = %q, want it preserved", got) | |
| } | |
| } |
🤖 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 388 - 418, Add coverage in
TestOpenReportsAssembledStreamHeaders for Content-Disposition: set a
representative filename header on the ranged response created by testPartial,
then assert the assembled stream preserves the same value alongside the existing
Content-Type assertion.
Source: Coding guidelines
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
Replaces the single unvalidated
GETbehindminutes +downloadwith the sharedinternal/downloadsequential range reader introduced in #2223, so large recording media survives gateway timeouts and mid-transfer interruptions. Command behavior is unchanged: same flags, filename resolution, batch layout, overwrite protection, and JSON output.This also tightens one thing in the shared framework itself. Minutes is its second consumer, and wiring it up surfaced a header that misdescribed multipart streams; the fix is included here rather than deferred because Minutes is the first caller that reads those headers to derive a filename.
Changes
Minutes
download.URL→download.ImmutableSource→download.Openinstead of a bareclient.Dowith ad-hoc status handling.ETag, so declaring it mutable would silently fall back to a single full response and lose ranged recovery.http.Response;resolveFilenameFromResponsenow takes thehttp.Headerit actually reads.Shared download framework
Content-Lengthis the whole object and the first part'sContent-Rangeis dropped. PreviouslyStream.Headerwas a verbatim clone of the opening206, so a caller readingContent-Lengthoff the header saw the first part's size whileStream.ContentLengthheld the total. No current caller reads that header field, so this fixes a latent inconsistency rather than a live defect, and it makes the header agree with what the single-response path already produced.Not included: cross-process checkpointing or resumable downloads. Recovery happens within a single command invocation only.
Test Plan
make unit-test, race enabled, acrosscmd,internal,shortcuts, andextension)go vet ./...,go build ./..., and diff-scopedgolangci-lintare cleanlark-cli minutes +downloadflow works as expectedNew coverage:
TestOpenReportsAssembledStreamHeaderspins the assembled-stream header contract: reverting the change fails it with the first part's length.Verified against live recording media using a temporary, uncommitted transport observer to capture the exact request sequence:
206, media decodes cleanly206with an exactContent-Range, and no fallback full re-fetch occurred.--outputomitted, the filename is derived from the assembled stream's headers; verified on both a single-part and a two-part download, confirmingContent-TypeandContent-Dispositionsurvive the header fix.--overwritereturns a typed validation error and leaves the original file untouched.mainand is unchanged by this PR.Related Issues
Summary by CodeRabbit