fix(shortcuts): classify media stream recovery errors - #2299
Conversation
github-actions
Bot
added
domain/ccm
📝 WalkthroughWalkthroughDoc and Drive media streams now classify transport-formatted permission and rate-limit errors. The flows preserve original causes and metadata, add shared recovery hints, and expand tests for app-scope and retryable failures. ChangesMedia error recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DriveDownload
participant classifyDriveFileReadStreamError
participant DriveFileReadRecovery
DriveDownload->>classifyDriveFileReadStreamError: final stream error
classifyDriveFileReadStreamError->>DriveFileReadRecovery: typed API error and original cause
DriveFileReadRecovery-->>DriveDownload: classified error with recovery hint
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
shortcuts/doc/doc_media_test.go (2)
743-748: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
"1 minute"assertions remain after the switch to exact hint equality. Each site now compares the hint to a golden constant and then also checks that the hint does not contain"1 minute". A string equal to the constant cannot contain that substring, so the second check can never fail. The shared root cause is the migration from substring assertions to exact-equality assertions without removing the superseded checks.
shortcuts/doc/doc_media_test.go#L743-L748: remove thestrings.Contains(problem.Hint, "1 minute")check that follows the equality assertion.shortcuts/drive/drive_io_test.go#L1832-L1837: remove the same trailing check.shortcuts/drive/drive_io_test.go#L1867-L1872: remove the same trailing check.🤖 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/doc/doc_media_test.go` around lines 743 - 748, Remove the redundant strings.Contains(problem.Hint, "1 minute") assertions following exact hint equality checks in shortcuts/doc/doc_media_test.go lines 743-748, shortcuts/drive/drive_io_test.go lines 1832-1837, and shortcuts/drive/drive_io_test.go lines 1867-1872; retain each golden constant comparison.
879-903: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd log-ID coverage to match the Drive test.
classifyDocMediaStreamErrorcopiesproblem.LogIDinto the reconstructed response header at doc_errors.go lines 58-60. This stub sets no log-ID header, so that branch stays untested for Doc. The Drive equivalent inshortcuts/drive/drive_io_test.gosetslarkcore.HttpHeaderKeyLogIdand assertsproblem.LogID. Mirror it here.💚 Proposed additions
}, + Headers: http.Header{ + "Content-Type": []string{"application/json"}, + larkcore.HttpHeaderKeyLogId: []string{"log-doc-stream"}, + }, })problem, ok := errs.ProblemOf(err) - if !ok || problem.Code != apiFailure.code { - t.Fatalf("problem=%+v ok=%v, want code=%d", problem, ok, apiFailure.code) + if !ok || problem.Code != apiFailure.code || problem.LogID != "log-doc-stream" { + t.Fatalf("problem=%+v ok=%v, want code=%d and stream log id", problem, ok, apiFailure.code) }This requires the
larkcoreimport in the test file.🤖 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/doc/doc_media_test.go` around lines 879 - 903, Add log-ID coverage to the Doc media error test around the HTTP stub and problem assertions: import the existing larkcore package, set larkcore.HttpHeaderKeyLogId on the stub response using a test log ID, and assert the resulting problem.LogID matches it, mirroring the Drive test while preserving the existing error checks.shortcuts/drive/drive_errors.go (2)
123-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared hint block.
withDrivePreviewRecoveryHintrepeats the app-scope and rate-limit logic fromwithDriveDownloadRecoveryHintlines 109-120. A single helper keeps the two paths aligned and removes the repeated literal guard fragments.🤖 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/drive/drive_errors.go` around lines 123 - 136, Extract the shared app-scope and rate-limit hint logic from withDrivePreviewRecoveryHint and withDriveDownloadRecoveryHint into one helper, then have both recovery functions reuse it. Preserve the existing guards, hint constants, and return behavior while removing the duplicated literal checks.
116-119: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDo not discard the
okflag before dereferencingproblem.Line 116 ignores the second return of
errs.ProblemOfand then readsproblem.Hint. This is safe today only becausedriveFileReadIsRateLimitreturns false whenProblemOffails. The guard lives in a different function, so the safety is implicit. If the predicate ever changes, this line panics. The same pattern exists at line 131 inwithDrivePreviewRecoveryHint.🛡️ Proposed guard
- problem, _ := errs.ProblemOf(err) - if strings.Contains(problem.Hint, "exponential backoff") { + problem, ok := errs.ProblemOf(err) + if !ok || problem == nil || strings.Contains(problem.Hint, "exponential backoff") { return err } return appendDriveExportRecoveryHint(err, driveFileReadRateLimitHint)Apply the same change at line 131.
🤖 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/drive/drive_errors.go` around lines 116 - 119, Check the success flag returned by errs.ProblemOf in both the driveFileReadIsRateLimit path and withDrivePreviewRecoveryHint before accessing problem.Hint. Only evaluate the exponential-backoff hint when conversion succeeds; preserve the existing return behavior otherwise.shortcuts/doc/doc_errors.go (3)
102-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared hint block.
withDocMediaDownloadRecoveryHintandwithDocMediaPreviewRecoveryHintrepeat the same app-scope and rate-limit logic. The idempotency guards also repeat literal fragments of the hint constants ("stop retrying now","exponential backoff"). A single helper removes the drift risk between the guard text and the constant text.♻️ Proposed helper
+func appendDocMediaSharedRecoveryHints(problem *errs.Problem) { + if problem.Code == 99991672 && !strings.Contains(problem.Hint, docMediaAppScopeHint) { + appendDocRecoveryHint(problem, docMediaAppScopeHint) + } + if docMediaIsRateLimit(problem) && !strings.Contains(problem.Hint, docMediaRateLimitHint) { + appendDocRecoveryHint(problem, docMediaRateLimitHint) + } +}Then call it from both functions.
🤖 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/doc/doc_errors.go` around lines 102 - 124, Extract the duplicated app-scope and rate-limit hint logic from withDocMediaDownloadRecoveryHint and withDocMediaPreviewRecoveryHint into one shared helper. Have the helper perform the existing problem validation, append both hints, and use the hint constants or shared guard values instead of repeating literal fragments; then call it from both recovery functions while preserving their current error return behavior.
72-81: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAttach the cause for every classified error type.
If
classifiedis neither*errs.PermissionErrornor*errs.APIError, the function returns it without a cause. The original transport error and its HTTP status then disappear from the error chain. TodayClassifyAPIResponsemaps 99991672 and 99991400 to those two types, so this is a defensive gap only. Consider returning the originalerrwhen no cause can be attached.♻️ Proposed fallback
var apiErr *errs.APIError if errors.As(classified, &apiErr) { apiErr.WithCause(err) + return classified } - return classified + // No known typed carrier for the cause; keep the transport error intact. + return 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/doc/doc_errors.go` around lines 72 - 81, Update the error-classification flow around the PermissionError and APIError handling so every classified error preserves the original err as its cause. If classified is neither supported type, return or wrap the original err using the established fallback rather than returning classified without a cause, while preserving the existing typed-error behavior.
33-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe media-read recovery contract is duplicated across the
docanddrivepackages.classifyDocMediaStreamErrorandclassifyDriveFileReadStreamErrorhave identical bodies, and the four recovery-hint constants carry identical text. The shared root cause is that no package owns the "DoStream transport message to typed business error" contract, so each domain package carries its own copy. Both copies also share the same cause-attachment gap: a classified error that is neither*errs.PermissionErrornor*errs.APIErroris returned without the original transport cause.The repository guidelines ask you to fix root causes at the narrowest cohesive owner boundary and to reuse existing machinery. Two identical shims in two packages is the widest possible boundary for one contract. Consider moving the classifier and the hint constants into
shortcuts/common, whereRuntimeContextandClassifyAPIResponsealready live, and keeping only the domain-specific hint composition in each package. If you prefer to keep the shims local whileDoStreamlacks a structured body, record that decision in both file comments so the next reader knows the duplication is intentional and time-boxed.
shortcuts/doc/doc_errors.go#L33-L83: moveclassifyDocMediaStreamErrorto a shared helper, or document why the Doc copy must stay local.shortcuts/drive/drive_errors.go#L40-L84: replaceclassifyDriveFileReadStreamErrorwith the shared helper, or document the same rationale.shortcuts/doc/doc_errors.go#L18-L21: sourcedocMediaAppScopeHintanddocMediaRateLimitHintfrom the shared location.shortcuts/drive/drive_errors.go#L19-L21: sourcedriveFileReadAppScopeHintanddriveFileReadRateLimitHintfrom the same shared location.🤖 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/doc/doc_errors.go` around lines 33 - 83, The duplicated media-stream classifier and recovery-hint constants should be owned by shortcuts/common, with the shared helper also attaching the original transport cause to every classified error it returns. In shortcuts/doc/doc_errors.go#L33-83 and shortcuts/drive/drive_errors.go#L40-84, replace the local classifiers with the shared helper; in shortcuts/doc/doc_errors.go#L18-21 and shortcuts/drive/drive_errors.go#L19-21, source all four hints from the shared common definitions while preserving each package’s domain-specific hint composition.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 `@shortcuts/doc/doc_errors.go`:
- Around line 102-124: Extract the duplicated app-scope and rate-limit hint
logic from withDocMediaDownloadRecoveryHint and withDocMediaPreviewRecoveryHint
into one shared helper. Have the helper perform the existing problem validation,
append both hints, and use the hint constants or shared guard values instead of
repeating literal fragments; then call it from both recovery functions while
preserving their current error return behavior.
- Around line 72-81: Update the error-classification flow around the
PermissionError and APIError handling so every classified error preserves the
original err as its cause. If classified is neither supported type, return or
wrap the original err using the established fallback rather than returning
classified without a cause, while preserving the existing typed-error behavior.
- Around line 33-83: The duplicated media-stream classifier and recovery-hint
constants should be owned by shortcuts/common, with the shared helper also
attaching the original transport cause to every classified error it returns. In
shortcuts/doc/doc_errors.go#L33-83 and shortcuts/drive/drive_errors.go#L40-84,
replace the local classifiers with the shared helper; in
shortcuts/doc/doc_errors.go#L18-21 and shortcuts/drive/drive_errors.go#L19-21,
source all four hints from the shared common definitions while preserving each
package’s domain-specific hint composition.
In `@shortcuts/doc/doc_media_test.go`:
- Around line 743-748: Remove the redundant strings.Contains(problem.Hint, "1
minute") assertions following exact hint equality checks in
shortcuts/doc/doc_media_test.go lines 743-748, shortcuts/drive/drive_io_test.go
lines 1832-1837, and shortcuts/drive/drive_io_test.go lines 1867-1872; retain
each golden constant comparison.
- Around line 879-903: Add log-ID coverage to the Doc media error test around
the HTTP stub and problem assertions: import the existing larkcore package, set
larkcore.HttpHeaderKeyLogId on the stub response using a test log ID, and assert
the resulting problem.LogID matches it, mirroring the Drive test while
preserving the existing error checks.
In `@shortcuts/drive/drive_errors.go`:
- Around line 123-136: Extract the shared app-scope and rate-limit hint logic
from withDrivePreviewRecoveryHint and withDriveDownloadRecoveryHint into one
helper, then have both recovery functions reuse it. Preserve the existing
guards, hint constants, and return behavior while removing the duplicated
literal checks.
- Around line 116-119: Check the success flag returned by errs.ProblemOf in both
the driveFileReadIsRateLimit path and withDrivePreviewRecoveryHint before
accessing problem.Hint. Only evaluate the exponential-backoff hint when
conversion succeeds; preserve the existing return behavior otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 497813a9-8bcb-4928-aa31-c80614261149
📒 Files selected for processing (9)
shortcuts/doc/doc_errors.goshortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_preview.goshortcuts/doc/doc_media_test.goshortcuts/drive/drive_download.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_preview_common.goshortcuts/drive/drive_preview_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@29f599e0d41d3fd596a0a0704e386410f81e449e🧩 Skill updatenpx skills add wittam-01/cli#fix/media-stream-recovery-hints -y -g |
1 participant
Summary
Classify scope and rate-limit failures returned by streaming media requests in Docs and Drive shortcuts, while keeping the change inside the owning domain packages.
Changes
Test Plan
Related Issues
Summary by CodeRabbit