fix: include log_id in base attachment media errors#1133
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR extracts backend correlation ids (X-Tt-Logid / Request-Id) from HTTP responses and injects them as ChangesLogid Propagation in Attachment and Media Error Details
Sequence Diagram(s)sequenceDiagram
participant CLI as Test/Caller
participant APIClient as APIClient.DoStream
participant BaseDownload as downloadBaseAttachment
participant HTTP as HTTP Response
participant Wrapper as wrap/attach helpers
CLI->>APIClient: request stream/download
APIClient->>HTTP: receive response (2xx or >=400 + headers)
alt HTTP >= 400
APIClient->>Wrapper: create/receive ExitError
Wrapper->>Wrapper: extract X-Tt-Logid / Request-Id
Wrapper->>APIClient: inject Detail.Detail["log_id"]
APIClient-->>CLI: return ExitError with log_id in detail
else 2xx
APIClient-->>CLI: stream data / success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
domain/base
zgz2048
force-pushed
the
codex/base-download-logid
branch
2 times, most recently
from
May 27, 2026 10:04
46536a4 to
58e6caf
Compare
github-actions
Bot
added
size/L
zgz2048
changed the title
zgz2048
marked this pull request as ready for review
May 27, 2026 10:05
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@197db82c078595834c6c8556bd0d5f75c094bfa5🧩 Skill updatenpx skills add zgz2048/cli#codex/base-download-logid -y -g |
kongenpei
previously approved these changes
May 27, 2026
zgz2048
force-pushed
the
codex/base-download-logid
branch
from
May 27, 2026 10:14
58e6caf to
d4ac47b
Compare
![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/base/record_upload_attachment.go`:
- Around line 785-787: The wrapper around downloadBaseAttachmentResponse
currently rewraps every error as a network error; instead preserve
already-classified pre-request failures by returning them unchanged. Update the
error handling where downloadBaseAttachmentResponse(...) is called (the
occurrences around resp, err := downloadBaseAttachmentResponse in
record_upload_attachment.go) to check the error type first (use errors.As or a
type assertion for *output.ExitError or whatever concrete classified error type
your code uses) and if it matches return nil, err directly; only call
wrapBaseAttachmentDownloadError(err) for genuine transport/network errors. Apply
the same change to the other call site mentioned (the block around lines
831–846).
🪄 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
Run ID: 94343a90-c1ac-44d4-a504-78c9eacb6a84
📒 Files selected for processing (4)
shortcuts/base/base_execute_test.goshortcuts/base/record_upload_attachment.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/common/drive_media_upload_test.go
- shortcuts/base/base_execute_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1133 +/- ##
==========================================
+ Coverage 68.25% 68.63% +0.37%
==========================================
Files 618 625 +7
Lines 57305 58386 +1081
==========================================
+ Hits 39116 40071 +955
- Misses 14953 15027 +74
- Partials 3236 3288 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zgz2048
force-pushed
the
codex/base-download-logid
branch
from
May 27, 2026 10:24
d4ac47b to
14a236b
Compare
zgz2048
changed the title
zgz2048
force-pushed
the
codex/base-download-logid
branch
2 times, most recently
from
May 27, 2026 10:37
faf1c24 to
15dd833
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/base/record_upload_attachment.go (1)
839-847:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the full structured error payload when rewrapping download failures.
This wrapper keeps only a subset of
ErrDetailfields, so downstream consumers can lose existing structured metadata on classified errors.Suggested fix
return &output.ExitError{ Code: exitErr.Code, + Raw: exitErr.Raw, Detail: &output.ErrDetail{ - Type: exitErr.Detail.Type, - Code: exitErr.Detail.Code, - Message: fmt.Sprintf("download failed: %v", err), - Hint: exitErr.Detail.Hint, - Detail: exitErr.Detail.Detail, + Type: exitErr.Detail.Type, + Code: exitErr.Detail.Code, + Message: fmt.Sprintf("download failed: %v", err), + Hint: exitErr.Detail.Hint, + ConsoleURL: exitErr.Detail.ConsoleURL, + Risk: exitErr.Detail.Risk, + Detail: exitErr.Detail.Detail, }, Err: err, }🤖 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/base/record_upload_attachment.go` around lines 839 - 847, The current rewrap builds a new output.ErrDetail manually and drops any extra fields from the original exitErr.Detail; update the Download failure rewrap to preserve the full structured payload by cloning exitErr.Detail (check for nil), then only update or prepend the Message (e.g., set newDetail.Message = fmt.Sprintf("download failed: %v", err) or combine with existing message) and assign Detail: &newDetail on the returned *output.ExitError (keeping ExitError.Code as exitErr.Code); reference the existing symbols exitErr, err, output.ExitError and output.ErrDetail when making this change.
🤖 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/client/client_test.go`:
- Around line 431-443: The test TestDoStream_HTTPErrorIncludesLogID is not using
the standard test factory or isolating config state; replace the manual setup
with cmdutil.TestFactory(t, config) to create the test factory and call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before creating runtime/client
state, then use that factory to obtain the runtime/clients used by APIClient (or
pass the factory's config) so the test uses the standard test harness instead of
constructing APIClient directly; update references around the APIClient
instantiation in TestDoStream_HTTPErrorIncludesLogID to use the factory-created
config/runtime.
---
Duplicate comments:
In `@shortcuts/base/record_upload_attachment.go`:
- Around line 839-847: The current rewrap builds a new output.ErrDetail manually
and drops any extra fields from the original exitErr.Detail; update the Download
failure rewrap to preserve the full structured payload by cloning exitErr.Detail
(check for nil), then only update or prepend the Message (e.g., set
newDetail.Message = fmt.Sprintf("download failed: %v", err) or combine with
existing message) and assign Detail: &newDetail on the returned
*output.ExitError (keeping ExitError.Code as exitErr.Code); reference the
existing symbols exitErr, err, output.ExitError and output.ErrDetail when making
this change.
🪄 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
Run ID: c7e51677-1e89-4944-bc2b-8c0eb82c6c81
📒 Files selected for processing (6)
internal/client/client.gointernal/client/client_test.goshortcuts/base/base_execute_test.goshortcuts/base/record_upload_attachment.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/common/drive_media_upload.go
- shortcuts/base/base_execute_test.go
- shortcuts/common/drive_media_upload_test.go
zgz2048
force-pushed
the
codex/base-download-logid
branch
from
May 28, 2026 03:04
15dd833 to
197db82
Compare
kongenpei
approved these changes
May 28, 2026
tuxedomm
pushed a commit
to zhumiaoxin/cli
that referenced
this pull request
Jun 6, 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by CodeRabbit
Bug Fixes
Tests