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
📝 WalkthroughWalkthroughRegisters three new quota error codes and maps them to API classifications; adds XML well-formedness prechecks and exponential-backoff rate-limit retry helpers, integrates them into SlidesCreate, SlidesMediaUpload, and ReplaceSlide, adds tests, and updates slides XML 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 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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1383 +/- ##
==========================================
+ Coverage 71.95% 72.00% +0.04%
==========================================
Files 699 700 +1
Lines 66285 66351 +66
==========================================
+ Hits 47695 47775 +80
+ Misses 14883 14871 -12
+ Partials 3707 3705 -2 ☔ 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@c76a6fab76e7b9bbeb8fc05dcb852754692f4f6b🧩 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
Comment on lines
+49
to
+57
| for attempt := 0; attempt <= slidesRateLimitMaxRetries; attempt++ { | ||
| if attempt > 0 { | ||
| delay := slidesRateLimitBaseDelay << uint(attempt-1) | ||
| fmt.Fprintf(errOut, "Rate limited by the API (99991400), retrying (attempt %d/%d) in %v...\n", | ||
| attempt, slidesRateLimitMaxRetries, delay) | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-time.After(delay): |
There was a problem hiding this comment.
Honor cancellation before dispatch and return typed cancel errors.
Line [60] still calls fn() on attempt 0 even if ctx is already canceled, which can send an extra write request. Also, Line [56] returns bare ctx.Err() instead of a typed errs.* error.
Proposed fix
func retryOnRateLimit(ctx context.Context, errOut io.Writer, fn func() error) error {
var lastErr error
for attempt := 0; attempt <= slidesRateLimitMaxRetries; attempt++ {
+ if err := ctx.Err(); err != nil {
+ return errs.NewValidationError(errs.SubtypeFailedPrecondition, "request canceled before API call").
+ WithHint("Retry after the cancellation condition is cleared.").
+ WithCause(err)
+ }
if attempt > 0 {
delay := slidesRateLimitBaseDelay << uint(attempt-1)
fmt.Fprintf(errOut, "Rate limited by the API (99991400), retrying (attempt %d/%d) in %v...\n",
attempt, slidesRateLimitMaxRetries, delay)
select {
case <-ctx.Done():
- return ctx.Err()
+ return errs.NewValidationError(errs.SubtypeFailedPrecondition, "request canceled during rate-limit backoff").
+ WithHint("Retry after the cancellation condition is cleared.").
+ WithCause(ctx.Err())
case <-time.After(delay):
}
}
lastErr = fn()As per coding guidelines: "Command-facing failures must be typed errs.* errors" and "Use errs.NewValidationError(errs.SubtypeFailedPrecondition, ...).WithHint(...) for valid requests with wrong system state."
Also applies to: 60-63
🤖 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 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.
Source: Coding guidelines
Collaborator
Author
There was a problem hiding this comment.
Keeping as-is, for two reasons: (1) returning bare ctx.Err() from a backoff select is the established repo-wide pattern for internal retry helpers — see wiki_node_create.go:320, wiki_async_task.go:151, wiki_move.go:576, minutes_download.go:181, okr_cycle_detail.go — and this helper follows the wiki_node_create.go precedent directly; cancellation here propagates up and is normalized at the command boundary like in those call sites. (2) The "extra write request on attempt 0" concern doesn't materialize: fn() wraps CallAPITyped, whose HTTP request carries the same ctx — Go's http client fails immediately on an already-canceled context without dispatching the request.
There was a problem hiding this comment.
@ViperCai, understood — both points are well-taken. The established repo-wide convention of propagating bare ctx.Err() from retry helpers (with normalization at the command boundary) and the fact that CallAPITyped carries ctx through to Go's HTTP client (which short-circuits immediately on a pre-cancelled context without dispatching) are good reasons to keep the current shape. Noting these patterns for future reviews.
✏️ Learnings added
Learnt from: ViperCai
Repo: larksuite/cli PR: 1383
File: shortcuts/slides/helpers.go:49-57
Timestamp: 2026-06-10T09:26:44.921Z
Learning: In larksuite/cli Go retry helpers (e.g. `retryOnRateLimit` in `shortcuts/slides/helpers.go`), returning bare `ctx.Err()` from a `select { case <-ctx.Done() }` branch is the established repo-wide convention. The error propagates up and is normalized at the command boundary, matching the pattern used in `wiki_node_create.go`, `wiki_async_task.go`, `wiki_move.go`, `minutes_download.go`, and `okr_cycle_detail.go`. Do NOT flag bare `ctx.Err()` returns inside retry-helper select blocks as needing typed `errs.*` wrapping — the normalization is intentional and happens at the command level.
Learnt from: ViperCai
Repo: larksuite/cli PR: 1383
File: shortcuts/slides/helpers.go:49-57
Timestamp: 2026-06-10T09:26:44.921Z
Learning: In larksuite/cli, `fn()` closures inside retry helpers such as `retryOnRateLimit` (`shortcuts/slides/helpers.go`) typically wrap `CallAPITyped`, which carries the caller's `context.Context` into Go's `net/http` client. A pre-cancelled context causes the HTTP client to return immediately without dispatching a network request, so there is no "extra write request on attempt 0" risk. Do NOT flag these retry loops for missing an upfront `ctx.Err()` guard on attempt 0 — the context is already threaded through `CallAPITyped` and the HTTP layer handles cancellation correctly.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: JayYoung2021
Repo: larksuite/cli PR: 410
File: cmd/auth/login_scope_cache.go:47-54
Timestamp: 2026-04-11T23:06:23.250Z
Learning: In larksuite/cli Go code, use `validate.SafeInputPath` only for user-supplied file path inputs (e.g., `--file` flags). Do NOT require `validate.SafeInputPath` for internally constructed, application-managed filesystem paths when: (1) their root directory is already validated via `validate.SafeEnvDirPath()` inside helpers like `appdir.CacheDir()`, `appdir.StateDir()`, or `appdir.LogDir()`, and (2) the filename/path components are sanitized by helpers such as `sanitizeLoginScopeCacheKey()`. In code reviews, missing `SafeInputPath` calls should not be flagged for these app-managed paths if the above safeguards are already in place.
Learnt from: JayYoung2021
Repo: larksuite/cli PR: 410
File: internal/update/update.go:147-147
Timestamp: 2026-04-11T23:06:28.717Z
Learning: In this repo’s Go code (larksuite/cli), treat `validate.SafeInputPath` as a guard for **user-supplied local file paths only**. When you build a path by joining a previously validated base directory (e.g., from `appdir.StateDir()`, `appdir.CacheDir()`, `appdir.ConfigDir()` which already validate env overrides via `validate.SafeEnvDirPath()`) with a compile-time constant filename (e.g., `"update-state.json"`, `"config.json"`), do **not** require an additional `validate.SafeInputPath` call before using `vfs.*` or `validate.AtomicWrite`. Reviewers should avoid flagging/adding redundant `validate.SafeInputPath` in these computed-path cases; only require it when the path ultimately comes from user input.
Learnt from: ViperCai
Repo: larksuite/cli PR: 450
File: shortcuts/slides/slides_create.go:50-60
Timestamp: 2026-04-14T03:13:09.459Z
Learning: When reviewing this Go codebase, do not require explicit `validate.SafeInputPath` / `validate.SafeOutputPath` calls for user-supplied paths if the code uses the `runtime.FileIO()` abstraction methods (`Stat`, `Open`, `Save`, `ResolvePath`). `runtime.FileIO()` is backed by `LocalFileIO` (in `localfileio.go`), which enforces `SafeInputPath`/`SafeOutputPath` before any filesystem access and reports violations as `fileio.PathValidationError`. Only require explicit `validate.SafeInputPath` when bypassing `runtime.FileIO()` and calling `vfs.*` or `os.*` directly with user-influenced paths.
Learnt from: ViperCai
Repo: larksuite/cli PR: 450
File: shortcuts/slides/slides_media_upload.go:44-49
Timestamp: 2026-04-14T03:13:09.234Z
Learning: In larksuite/cli (Go), treat calls that validate user-supplied `--file` paths via `runtime.FileIO().Stat(path)` as already enforcing path-safety. `runtime.FileIO().Stat()` routes to `LocalFileIO.Stat()` (internal/vfs/localfileio/localfileio.go), which calls `SafeInputPath(name)` and wraps failures as `fileio.PathValidationError`. Therefore, when reviewing `shortcuts` code, do NOT flag missing explicit `validate.SafeInputPath` calls for `--file` flags if the code calls `runtime.FileIO().Stat()` on that same `--file` path during its Validate or Execute path (i.e., the safety is already enforced by the `Stat` call path).
Learnt from: herbertliu
Repo: larksuite/cli PR: 508
File: shortcuts/doc/clipboard.go:29-58
Timestamp: 2026-04-16T09:26:48.292Z
Learning: In Go CLI code, it is acceptable (and intentional) to use the standard library `os.*` APIs (e.g., `os.CreateTemp`, `os.Stat`, `os.Remove`) for **CLI-internal ephemeral temp files**—files created by the CLI itself (not provided by the user), used only within a single call frame, and cleaned up via an explicit deferred cleanup (e.g., a `cleanup()` closure). Reserve `vfs.*`/`fileio` abstractions for **user-facing input/output paths** such as user-supplied flags (e.g., `--file`) or atomic writes to user-specified destinations. Do not flag `os.*` usage when it is strictly for these caller-managed internal temp files; routing such internal temp paths through `fileio` may fail path validation by design (e.g., temp paths outside the working directory), even though the usage is correct.
Learnt from: fangshuyu-768
Repo: larksuite/cli PR: 696
File: shortcuts/drive/drive_pull.go:0-0
Timestamp: 2026-04-28T13:21:10.653Z
Learning: In the larksuite/cli repo, the golangci-lint depguard rule `shortcuts-no-vfs` prohibits anything under `shortcuts/` from importing `internal/vfs` directly; shortcuts should use `runtime.FileIO()` / `runtime.ValidatePath()` instead. If a shortcut needs to delete files or walk a user-supplied local directory tree, it may use `os.Remove` and `filepath.WalkDir` as the current accepted fallback (with `//nolint:forbidigo` on the violating call(s) and a brief inline explanation). Do not flag `os.Remove` or `filepath.WalkDir` in `shortcuts/` as a vfs-abstraction violation while `runtime.FileIO()` still lacks directory-walking and remove capabilities. If/when runtime adds directory-walking/remove support to `runtime.FileIO()`, update shortcuts to use the runtime APIs instead of `os.Remove`/`filepath.WalkDir`.
Learnt from: evandance
Repo: larksuite/cli PR: 984
File: internal/lintcheck/scan_test.go:0-0
Timestamp: 2026-05-20T08:44:00.596Z
Learning: In Go, the "declared and not used" compiler error is triggered only for variables that are never referenced after declaration. A statement like `x++` counts as a use, so a variable that is only incremented (e.g., `rejectCount++`) will still compile even if it is never read. When reviewing Go code, don’t assume `x++`-only variables would fail to compile—treat them as potentially semantically dead code instead (remove/avoid unless it’s required for later reads, logging, metrics, or other side effects).
Learnt from: evandance
Repo: larksuite/cli PR: 1287
File: shortcuts/contact/contact_search_user.go:241-252
Timestamp: 2026-06-06T07:37:51.822Z
Learning: In larksuite/cli, per-domain API decoder helpers under `shortcuts/` should not be flagged for missing “nil `data`” guards when their input comes from `runtime.ClassifyAPIResponse`. `ClassifyAPIResponse` intentionally returns a nil `map[string]interface{}` for successful responses with no `data` field (e.g. `{"code":0,"msg":"ok"}`), and existing decode helpers may safely pass that nil map through `json.Marshal`/`json.Unmarshal` to produce zero-value structs / empty results (matching prior behavior). If “missing `data`” ever needs to become an invalid-response error, make that change in the shared `runtime.ClassifyAPIResponse`, not in individual domain decode helpers.
27a2836 to
0697b81
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
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
Comment on lines
+440
to
+492
| func TestCheckXMLWellFormed(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| in string | ||
| wantErr string | ||
| }{ | ||
| {name: "simple element", in: `<shape type="rect"><content/></shape>`}, | ||
| {name: "nested with attributes", in: `<slide><shape type="text"><content><p>hi</p></content></shape></slide>`}, | ||
| // Insertion fragments may carry sibling top-level elements; the decoder | ||
| // must not enforce a single document element. | ||
| {name: "multiple top-level elements", in: `<p>a</p><p>b</p>`}, | ||
| {name: "escaped entities", in: `<p>A & B <tag> "q"</p>`}, | ||
| {name: "CDATA with raw ampersand", in: `<p><![CDATA[a & b < c]]></p>`}, | ||
| {name: "comment", in: `<!-- note --><shape/>`}, | ||
| {name: "img placeholder attr", in: `<img src="@./local.png" width="100"/>`}, | ||
| {name: "unicode text", in: `<p>项目汇报 🎯</p>`}, | ||
|
|
||
| // Top CLI-path failure cause in engine logs: bare & in text. | ||
| {name: "bare ampersand", in: `<p>Q & A</p>`, wantErr: "line 1"}, | ||
| {name: "bare ampersand multiline", in: "<slide>\n<p>R&D</p>\n</slide>", wantErr: "line 2"}, | ||
| {name: "unclosed tag", in: `<shape><content></shape>`, wantErr: "not well-formed"}, | ||
| {name: "unquoted attribute", in: `<shape type=rect/>`, wantErr: "not well-formed"}, | ||
| {name: "stray closing tag", in: `<p>hi</p></div>`, wantErr: "not well-formed"}, | ||
| {name: "undefined entity", in: `<p>a b</p>`, wantErr: "not well-formed"}, | ||
|
|
||
| // nodeserver rejects processing instructions ("?xml not provide the | ||
| // implement"); reject the declaration locally regardless of position. | ||
| {name: "xml declaration", in: `<?xml version="1.0"?><shape/>`, wantErr: "declaration"}, | ||
| {name: "xml declaration with encoding", in: `<?xml version="1.0" encoding="UTF-8"?><slide/>`, wantErr: "declaration"}, | ||
| {name: "uppercase xml declaration", in: `<?XML version="1.0"?><shape/>`, wantErr: "declaration"}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| err := checkXMLWellFormed(tt.in) | ||
| if tt.wantErr == "" { | ||
| if err != nil { | ||
| t.Fatalf("unexpected err: %v", err) | ||
| } | ||
| return | ||
| } | ||
| if err == nil { | ||
| t.Fatalf("want error containing %q, got nil", tt.wantErr) | ||
| } | ||
| if !strings.Contains(err.Error(), tt.wantErr) { | ||
| t.Fatalf("want error containing %q, got %q", tt.wantErr, err.Error()) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert typed error metadata, not just message substrings.
The error-path test cases only verify that err.Error() contains expected substrings (e.g., "line 1", "not well-formed", "declaration"). Per coding guidelines, error-path tests must assert typed metadata via errs.ProblemOf or errors.As, not message substrings alone.
Since checkXMLWellFormed maps syntax errors to line-numbered validation errors (per PR objectives), the test should verify:
- The error is a
*errs.ValidationError(viaerrors.As) - The error has the correct
Subtype(e.g.,errs.SubtypeInvalidArgument) - The error message contains expected line numbers/guidance (current substring checks)
Example assertion pattern for typed validation errors
if tt.wantErr == "" {
if err != nil {
t.Fatalf("unexpected err: %v", err)
}
return
}
if err == nil {
t.Fatalf("want error containing %q, got nil", tt.wantErr)
}
+ var ve *errs.ValidationError
+ if !errors.As(err, &ve) {
+ t.Fatalf("want ValidationError, got: %T", err)
+ }
+ if ve.Subtype != errs.SubtypeInvalidArgument {
+ t.Fatalf("want SubtypeInvalidArgument, got: %v", ve.Subtype)
+ }
if !strings.Contains(err.Error(), tt.wantErr) {
t.Fatalf("want error containing %q, got %q", tt.wantErr, err.Error())
}🤖 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_test.go` around lines 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.
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.
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