fix(slides): local XML precheck, 99991400 backoff, quota code registration#1383
Open
ViperCai wants to merge 1 commit into
Open
fix(slides): local XML precheck, 99991400 backoff, quota code registration#1383ViperCai wants to merge 1 commit into
ViperCai wants to merge 1 commit into
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:
📝 WalkthroughWalkthroughRegisters three new docs/drive quota error codes with per-code Hints and adjusts hint precedence; adds XML fragment well-formedness checks, XML-entity-aware placeholder handling, and exponential-backoff rate-limit retry helpers integrated into Slides create/upload/replace flows; adds tests and updates docs. ChangesQuota Error Code Registration
Slides Reliability: XML Validation and Rate-Limit Retry
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (shortcut)
participant Retry as retryOnRateLimit
participant SlidesAPI as Slides API
CLI->>Retry: call operation (add slide / upload / replace part)
Retry->>SlidesAPI: POST request
SlidesAPI-->>Retry: response (success or rate-limit error)
alt rate-limit error (retryable)
Retry->>Retry: wait (exponential backoff) and log retry
Retry->>SlidesAPI: retry POST
else success or non-retryable error
Retry-->>CLI: return result or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
the
size/M
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1383 +/- ##
==========================================
+ Coverage 72.75% 72.77% +0.02%
==========================================
Files 730 730
Lines 69030 69088 +58
==========================================
+ Hits 50224 50281 +57
- Misses 15034 15035 +1
Partials 3772 3772 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6c79ad95b3693d25f3979db53cc928cb3b98c6d7🧩 Skill updatenpx skills add larksuite/cli#fix/slides-xml-wellformed-precheck -y -g |
ViperCai
force-pushed
the
fix/slides-xml-wellformed-precheck
branch
from
535659c to
27a2836
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 `@internal/errclass/codemeta_drive.go`:
- Around line 16-24: The comment and mapping for error code 90003088 are
misleading: update the comment text near the commercial plan quota block to
state that “free quota” applies only to codes 90003086 and 90003087, and change
the mapping for 90003088 in internal/errclass/codemeta_drive.go from
errs.SubtypeQuotaExceeded to a more appropriate subtype such as
errs.SubtypeFailedPrecondition (or ensure a code-specific hint override is
provided) so that the default quota-exceeded hint is not applied to the “tenant
has not purchased / been granted the docs module” case; adjust the inline
comment for 90003088 accordingly to reflect the non-quota precondition.
🪄 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: e4011aa7-9bd1-4f40-ad1f-0214fee63cdd
📒 Files selected for processing (13)
internal/errclass/codemeta_drive.gointernal/errclass/codemeta_drive_test.gointernal/output/lark_errors.goshortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.goskills/lark-slides/references/xml-format-guide.mdskills/lark-slides/references/xml-schema-quick-ref.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/slides/helpers.go (1)
361-367: ⚡ Quick winPreserve parser causes when wrapping XML parse failures.
Line [363] and Line [367] drop the original parse error. Add
.WithCause(err)(or.WithCause(syn)) so unwrap and root-cause tracing remain intact.Proposed fix
if err != nil { if syn, ok := err.(*xml.SyntaxError); ok { return errs.NewValidationError(errs.SubtypeInvalidArgument, "XML not well-formed at line %d: %s (escape literal & as & and < as < in text)", - syn.Line, syn.Msg) + syn.Line, syn.Msg).WithCause(syn) } - return errs.NewValidationError(errs.SubtypeInvalidArgument, "XML not well-formed: %v", err) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "XML not well-formed: %v", err).WithCause(err) }As per coding guidelines: "Preserve underlying errors with
.WithCause(err)soerrors.Isanderrors.Unwrapcontinue working."🤖 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/slides/helpers.go` around lines 361 - 367, The validation error returned when XML parsing fails (the branch that checks for xml.SyntaxError and the generic err branch calling errs.NewValidationError) currently drops the original parse error; update both return statements to attach the original cause by calling .WithCause(syn) for the xml.SyntaxError branch and .WithCause(err) for the generic branch so unwrap/root-cause tracing (errors.Is/Unwrap) works properly; look for the block that inspects err, the type assertion to xml.SyntaxError, and the calls to errs.NewValidationError and append the corresponding .WithCause(...) before returning.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.
Inline comments:
In `@shortcuts/slides/helpers.go`:
- Around line 49-57: The loop currently calls fn() even when ctx is already
cancelled and returns bare ctx.Err(); update the retry loop in
shortcuts/slides/helpers.go (the block using variables
slidesRateLimitMaxRetries, slidesRateLimitBaseDelay, errOut and calling fn()) to
check ctx.Done() before dispatching fn() on every attempt and return a typed
cancellation error instead of ctx.Err() (use
errs.NewValidationError(errs.SubtypeFailedPrecondition, "...").WithHint(...) or
the appropriate errs.* typed error per guideline). Also replace any bare
ctx.Err() returns inside the select or after cancellation checks with the chosen
typed errs.* error so callers get command-facing typed failures.
---
Nitpick comments:
In `@shortcuts/slides/helpers.go`:
- Around line 361-367: The validation error returned when XML parsing fails (the
branch that checks for xml.SyntaxError and the generic err branch calling
errs.NewValidationError) currently drops the original parse error; update both
return statements to attach the original cause by calling .WithCause(syn) for
the xml.SyntaxError branch and .WithCause(err) for the generic branch so
unwrap/root-cause tracing (errors.Is/Unwrap) works properly; look for the block
that inspects err, the type assertion to xml.SyntaxError, and the calls to
errs.NewValidationError and append the corresponding .WithCause(...) before
returning.
🪄 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: 7368ce9f-cee1-4e7f-9caa-050f08a0c66b
📒 Files selected for processing (13)
internal/errclass/codemeta_drive.gointernal/errclass/codemeta_drive_test.gointernal/output/lark_errors.goshortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.goskills/lark-slides/references/xml-format-guide.mdskills/lark-slides/references/xml-schema-quick-ref.md
✅ Files skipped from review due to trivial changes (4)
- internal/errclass/codemeta_drive.go
- internal/output/lark_errors.go
- skills/lark-slides/references/xml-format-guide.md
- skills/lark-slides/references/xml-schema-quick-ref.md
🚧 Files skipped from review as they are similar to previous changes (8)
- shortcuts/slides/slides_create.go
- shortcuts/slides/slides_replace_slide.go
- shortcuts/slides/slides_replace_slide_test.go
- shortcuts/slides/slides_media_upload.go
- shortcuts/slides/slides_media_upload_test.go
- shortcuts/slides/slides_create_test.go
- internal/errclass/codemeta_drive_test.go
- shortcuts/slides/helpers_test.go
ViperCai
force-pushed
the
fix/slides-xml-wellformed-precheck
branch
2 times, most recently
from
0697b81 to
c76a6fa
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 `@shortcuts/slides/helpers_test.go`:
- Around line 440-492: The tests in TestCheckXMLWellFormed only check error
message substrings; update them to assert the typed validation error and subtype
from checkXMLWellFormed: use errors.As (or errs.ProblemOf) to confirm the
returned error is a *errs.ValidationError and that its Subtype equals
errs.SubtypeInvalidArgument where applicable, then keep the existing substring
checks for line numbers/messages; locate this logic inside the
TestCheckXMLWellFormed table-driven loop and update the error-path branch that
currently inspects err.Error() to perform the typed assertions against
*errs.ValidationError and its Subtype before verifying the message content.
🪄 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: 83854856-c528-4971-9e94-9bc8672fa4d4
📒 Files selected for processing (16)
internal/errclass/classify.gointernal/errclass/classify_test.gointernal/errclass/codemeta.gointernal/errclass/codemeta_drive.gointernal/errclass/codemeta_drive_test.gointernal/output/lark_errors.goshortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.goskills/lark-slides/references/xml-format-guide.mdskills/lark-slides/references/xml-schema-quick-ref.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-slides/references/xml-format-guide.md
🚧 Files skipped from review as they are similar to previous changes (11)
- shortcuts/slides/slides_create.go
- internal/errclass/codemeta_drive.go
- internal/output/lark_errors.go
- internal/errclass/codemeta_drive_test.go
- shortcuts/slides/slides_media_upload.go
- skills/lark-slides/references/xml-schema-quick-ref.md
- shortcuts/slides/slides_replace_slide_test.go
- shortcuts/slides/slides_create_test.go
- shortcuts/slides/slides_media_upload_test.go
- shortcuts/slides/helpers.go
- shortcuts/slides/slides_replace_slide.go
ViperCai
force-pushed
the
fix/slides-xml-wellformed-precheck
branch
2 times, most recently
from
38b1201 to
ebec91e
Compare
…ation
Lands the four agreed fixes from the 2026-06-08 slides write-path
telemetry analysis:
1. Local XML well-formedness precheck (shortcuts/slides/)
- checkXMLWellFormed: pure syntax validation via stdlib encoding/xml
(same parser family as the backend, false-positive risk ~0);
explicitly rejects <?xml ?> declarations; deliberately allows
multiple top-level elements (legal in block_insert fragments)
- wired into +create --slides (at Validate, so a bad slide no longer
leaves a half-built deck) and +replace-slide --parts
replacement/insertion; errors carry line numbers + escaping
guidance, rejected locally with zero API calls
2. 99991400 rate-limit backoff (retryOnRateLimit)
- the code was registered Retryable:true but no slides loop actually
retried, so one frequency-window hit aborted the whole batch
- up to 2 retries with 1s/2s backoff, announced on stderr,
context-cancellable; wired into the +create slide POST loop and
uploadSlidesMedia (+media-upload and the placeholder upload loop)
- upload switched to UploadDriveMediaAllTyped (retry match requires
typed errors; aligns with the slides typed migration)
3. Quota codes 90003086/87/88 registered (internal/errclass + output)
- cloud-space explorer billing codes passed through as HTTP 200 with
body code!=0; previously fell to SubtypeUnknown
- now CategoryAPI + quota_exceeded, not retryable, with per-code
hints (upgrade plan / free quota / ask admin to enable the module)
4. lark-slides skill tag-whitelist ban (skills/lark-slides/)
- quick-ref: never write tags outside the whitelist, name the six
confirmed-rejected tags (audio/video/timeline/animation/trigger/
header), substitution table, escaping rules
- removed <?xml ?> declarations from all examples (contradicted
backend behavior and the new precheck)
Tested with unit + httpmock integration tests, plus live verification
against the real feishu.cn API: all precheck negatives rejected locally,
no false positives on real create/replace, and 18 concurrent uploads hit
3 real 99991400 responses which all retried and succeeded (18/18).
ViperCai
force-pushed
the
fix/slides-xml-wellformed-precheck
branch
from
ebec91e to
6c79ad9
Compare
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.
Background
Based on the 2026-06-08 slides write-path error telemetry analysis (see internal analysis doc), failures on the CLI/OpenAPI path concentrate in four buckets: XML well-formedness errors (bare
&, unclosed tags,<?xml ?>declarations rejected by nodeserver), 99991400 rate limits aborting batch submissions, commercial quota codes 9000308x falling through to the unknown classification with no actionable hint, and skill guidance lacking a tag-whitelist ban. This PR lands all four agreed fixes.Changes
1. Local XML well-formedness precheck (
shortcuts/slides/)checkXMLWellFormed: pure syntax validation using Go's stdlibencoding/xml(same parser family as the backend, false-positive risk ≈ 0); explicitly rejects<?xml ?>declarations (the backend's error for these is unreadable: "?xml not provide the implement"); deliberately allows multiple top-level elements (legal in block_insert fragments)+create --slides(at Validate, so a bad slide no longer leaves a half-built deck behind) and+replace-slide --partsreplacement/insertion2. 99991400 rate-limit backoff (
retryOnRateLimit)+createconsecutive slide POST loop anduploadSlidesMedia(covers+media-uploadand the placeholder image upload loop)UploadDriveMediaAlltoUploadDriveMediaAllTyped(the retry match requires typed errors, and this aligns with the slides typed-error migration in feat(slides): emit typed error envelopes across the slides domain #1349)3. Register commercial quota codes 90003086/87/88 (
internal/errclass+internal/output)4. lark-slides skill tag-whitelist ban (
skills/lark-slides/)xml-schema-quick-ref.md(already mandatory reading before generating any XML per SKILL.md): never write tags outside the whitelist + name the six tags with confirmed online rejections (audio/video/timeline/animation/trigger/header) + substitution table + escaping rules<?xml ?>declarations, contradicting backend behavior (and this PR's precheck); all removedCompatibility notes
+createno longer leaves a partial presentation behind+media-uploadfailure envelopes switch from legacy to typed, aligning with the rest of the slides domain; success paths unchangedTesting
&/<entities created and replaced successfully, read back to confirm persistence — no false positives+media-uploadcalls hit 3 real 99991400 responses; all retried after the 1s backoff and succeeded, 18/18 zero failures (before this change those 3 would have errored out)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests