Skip to content

Commit c76a6fa

Browse files
committed
fix(slides): local XML precheck, 99991400 backoff, quota code registration
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).
1 parent e794fd5 commit c76a6fa

16 files changed

Lines changed: 659 additions & 27 deletions

internal/errclass/classify.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,13 @@ func BuildAPIError(resp map[string]any, cc ClassifyContext) error {
142142
}
143143
case errs.CategoryAPI:
144144
// A server-supplied detail (lifted into base.Hint above) wins over the
145-
// context-free APIHint default; only fall back to APIHint when absent.
145+
// context-free defaults; below that, a per-code CodeMeta.Hint (for codes
146+
// whose recovery is more specific than their subtype's wording) wins
147+
// over the per-subtype APIHint; only fall back to APIHint when both
148+
// are absent.
149+
if base.Hint == "" {
150+
base.Hint = meta.Hint
151+
}
146152
if base.Hint == "" {
147153
base.Hint = APIHint(base.Subtype) // "" for subtypes without a context-free default
148154
}

internal/errclass/classify_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,54 @@ func TestBuildAPIError_TaskInvalidParamsRoutesToAPIError(t *testing.T) {
173173
}
174174
}
175175

176+
// TestBuildAPIError_PerCodeHint pins that the CategoryAPI arm surfaces the
177+
// per-code CodeMeta.Hint on the typed envelope — the commercial-plan codes
178+
// are produced by typed call sites (slides +create, drive import), so without
179+
// this the plan-quota codes would inherit the misleading per-subtype default
180+
// ("retry after the relevant quota resets") and 90003088 would carry no hint
181+
// at all. Also pins precedence: a server-supplied detail still wins.
182+
func TestBuildAPIError_PerCodeHint(t *testing.T) {
183+
t.Run("quota code gets per-code hint over subtype default", func(t *testing.T) {
184+
resp := map[string]any{"code": 90003087, "msg": "A2 create quota exceeded"}
185+
err := errclass.BuildAPIError(resp, errclass.ClassifyContext{})
186+
p, ok := errs.ProblemOf(err)
187+
if !ok {
188+
t.Fatal("ProblemOf returned !ok")
189+
}
190+
if !strings.Contains(p.Hint, "retrying will not help") {
191+
t.Errorf("Hint = %q, want the per-code plan-quota hint, not the generic quota-resets default", p.Hint)
192+
}
193+
})
194+
t.Run("failed-precondition code gets per-code hint despite empty subtype default", func(t *testing.T) {
195+
resp := map[string]any{"code": 90003088, "msg": "docs module unbundled"}
196+
err := errclass.BuildAPIError(resp, errclass.ClassifyContext{})
197+
p, ok := errs.ProblemOf(err)
198+
if !ok {
199+
t.Fatal("ProblemOf returned !ok")
200+
}
201+
if !strings.Contains(p.Hint, "docs module") {
202+
t.Errorf("Hint = %q, want the per-code docs-module hint", p.Hint)
203+
}
204+
})
205+
t.Run("server detail still wins over per-code hint", func(t *testing.T) {
206+
resp := map[string]any{
207+
"code": 90003087,
208+
"msg": "A2 create quota exceeded",
209+
"error": map[string]any{
210+
"details": []any{map[string]any{"value": "server-side specifics"}},
211+
},
212+
}
213+
err := errclass.BuildAPIError(resp, errclass.ClassifyContext{})
214+
p, ok := errs.ProblemOf(err)
215+
if !ok {
216+
t.Fatal("ProblemOf returned !ok")
217+
}
218+
if !strings.Contains(p.Hint, "server-side specifics") {
219+
t.Errorf("Hint = %q, want lifted server detail to take precedence", p.Hint)
220+
}
221+
})
222+
}
223+
176224
// TestBuildAPIError_TroubleshooterLiftedOnAPIArm pins that BuildAPIError lifts
177225
// resp.error.troubleshooter into Problem.Troubleshooter when the response
178226
// routes to the catch-all CategoryAPI arm. troubleshooter is the only

internal/errclass/codemeta.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,25 @@ import (
1010
)
1111

1212
// CodeMeta is the classification metadata attached to a Lark numeric code.
13-
// It does NOT carry Message or Hint — those are derived at the dispatcher
13+
// It does NOT carry Message — that is derived at the dispatcher
1414
// (see BuildAPIError).
1515
//
16+
// Hint is the optional per-code context-free recovery hint for codes whose
17+
// recovery action is more specific than their subtype's APIHint default
18+
// (e.g. a plan-quota code whose quota never resets, so the generic
19+
// "retry after the quota resets" wording would mislead). The CategoryAPI
20+
// dispatcher arm prefers a server-supplied detail, then this per-code hint,
21+
// then APIHint(subtype). Leave empty when the subtype default (or no hint)
22+
// is accurate.
23+
//
1624
// Risk + Action are populated only for codes that route to CategoryConfirmation;
1725
// the dispatcher falls back to RiskUnknown + ctx.LarkCmd when either is empty
1826
// so the envelope is never wire-invalid.
1927
type CodeMeta struct {
2028
Category errs.Category
2129
Subtype errs.Subtype
2230
Retryable bool
31+
Hint string // CategoryAPI arm only; empty → fall back to APIHint(subtype)
2332
Risk string // CategoryConfirmation arm only; empty otherwise
2433
Action string // CategoryConfirmation arm only; empty otherwise
2534
}

internal/errclass/codemeta_drive.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,26 @@ import "github.com/larksuite/cli/errs"
1212
var driveCodeMeta = map[int]CodeMeta{
1313
1061044: {Category: errs.CategoryAPI, Subtype: errs.SubtypeNotFound}, // parent folder does not exist (upload)
1414
1069302: {Category: errs.CategoryAPI, Subtype: errs.SubtypeInvalidParameters}, // comment endpoint "Invalid or missing parameters"
15+
16+
// Commercial plan codes returned by the cloud-space explorer service
17+
// and passed through verbatim by document backends (observed via
18+
// slides_ai create — engine log "A2 create quota exceeded, code=90003087"
19+
// — and drive/v1/import_tasks, both as HTTP 200 with body code≠0).
20+
// Plan/billing limits: retrying can never succeed. 90003086/90003087 are
21+
// plan creation-count quotas (upgrade the plan or free quota); 90003088 is
22+
// not a quota at all — the tenant has not purchased/enabled the docs
23+
// module, so it maps to FailedPrecondition (admin must enable the module)
24+
// rather than QuotaExceeded, whose default hint suggests freeing quota.
25+
// Hint is set per-code because the SubtypeQuotaExceeded default
26+
// ("retry after the relevant quota resets") misleads here: plan quotas
27+
// never reset on their own. Keep the wording in sync with the legacy
28+
// path's legacyHints entries (internal/output/lark_errors.go).
29+
90003086: {Category: errs.CategoryAPI, Subtype: errs.SubtypeQuotaExceeded,
30+
Hint: "document creation quota of the current plan reached: upgrade the plan or delete documents you no longer need to free quota; retrying will not help"}, // premium plan creation count limit reached
31+
90003087: {Category: errs.CategoryAPI, Subtype: errs.SubtypeQuotaExceeded,
32+
Hint: "document creation quota of the current plan reached: upgrade the plan or delete documents you no longer need to free quota; retrying will not help"}, // A2 plan creation count limit reached
33+
90003088: {Category: errs.CategoryAPI, Subtype: errs.SubtypeFailedPrecondition,
34+
Hint: "the tenant has not purchased or enabled the docs module; ask the tenant admin to enable it before creating documents"}, // unbundle: tenant has not purchased / been granted the docs module
1535
}
1636

1737
func init() { mergeCodeMeta(driveCodeMeta, "drive") }

internal/errclass/codemeta_drive_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,14 @@ func TestLookupCodeMeta_DriveCodes(t *testing.T) {
2727
// 1069302: comment endpoint's opaque "Invalid or missing parameters"
2828
// (shortcuts/drive/drive_add_comment.go) → API-side parameter rejection.
2929
{1069302, errs.CategoryAPI, errs.SubtypeInvalidParameters, false},
30+
// 9000308x: cloud-space explorer commercial plan codes, passed
31+
// through by document backends (slides_ai create, drive import_tasks)
32+
// as HTTP 200 with body code≠0. Billing limits → never retryable.
33+
// 86/87 are plan creation-count quotas; 88 is "docs module not
34+
// purchased/enabled" — a precondition, not a quota.
35+
{90003086, errs.CategoryAPI, errs.SubtypeQuotaExceeded, false},
36+
{90003087, errs.CategoryAPI, errs.SubtypeQuotaExceeded, false},
37+
{90003088, errs.CategoryAPI, errs.SubtypeFailedPrecondition, false},
3038
}
3139
for _, tc := range cases {
3240
t.Run(fmt.Sprintf("%d", tc.code), func(t *testing.T) {

internal/output/lark_errors.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,14 @@ const (
8484
LarkErrMailSendQuotaTenantExt = 1236009 // tenant daily external recipient count exceeded
8585
LarkErrMailQuota = 1236010 // mail quota limit
8686
LarkErrTenantStorageLimit = 1236013 // tenant storage limit exceeded
87+
88+
// Docs/slides commercial plan quota codes from the cloud-space explorer
89+
// service, passed through verbatim by document backends as HTTP 200 with
90+
// body code≠0 (observed on slides_ai create and drive/v1/import_tasks).
91+
// Not retryable: these are billing-plan limits.
92+
LarkErrDocsCreatePremiumQuota = 90003086 // premium plan document creation count limit reached
93+
LarkErrDocsCreateA2Quota = 90003087 // A2 plan document creation count limit reached
94+
LarkErrDocsModuleUnbundled = 90003088 // tenant has not purchased / enabled the docs module
8795
)
8896

8997
// legacyHints supplies the per-code actionable hint string for the legacy
@@ -118,6 +126,10 @@ var legacyHints = map[int]string{
118126
"width/height must be >= 20 px; offsets must be >= 0 and less than the anchor cell's width/height",
119127
LarkErrDrivePermApplyRateLimit: "permission-apply quota reached: each user may request access on the same document at most 5 times per day; wait or ask the owner directly",
120128
LarkErrDrivePermApplyNotApplicable: "this document does not accept a permission-apply request (common causes: the document is configured to disallow access requests, the caller already holds the permission, or the target type does not support apply); contact the owner directly",
129+
130+
LarkErrDocsCreatePremiumQuota: "document creation quota of the current plan reached: upgrade the plan or delete documents you no longer need to free quota; retrying will not help",
131+
LarkErrDocsCreateA2Quota: "document creation quota of the current plan reached: upgrade the plan or delete documents you no longer need to free quota; retrying will not help",
132+
LarkErrDocsModuleUnbundled: "the tenant has not purchased or enabled the docs module; ask the tenant admin to enable it before creating documents",
121133
}
122134

123135
// ClassifyLarkError maps a Lark API error code + message to the legacy

shortcuts/slides/helpers.go

Lines changed: 127 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,73 @@
44
package slides
55

66
import (
7+
"context"
8+
"encoding/xml"
79
"fmt"
10+
"io"
811
"net/url"
912
"regexp"
1013
"strings"
14+
"time"
1115

1216
"github.com/larksuite/cli/errs"
1317
"github.com/larksuite/cli/shortcuts/common"
1418
)
1519

20+
const (
21+
// slidesRateLimitMaxRetries is the number of automatic retries (beyond the
22+
// initial request) when the API answers 99991400 "request trigger frequency
23+
// limit". The slides batch paths (+create slide loop, placeholder image
24+
// uploads) fire consecutive POSTs and are the dominant 99991400 producers
25+
// in telemetry; a short backoff absorbs the per-second window without
26+
// masking a genuinely saturated tenant.
27+
slidesRateLimitMaxRetries = 2
28+
)
29+
30+
// slidesRateLimitBaseDelay is the initial backoff delay; subsequent retries
31+
// double it (1s, 2s). Mirrors the wiki +node-create lock-contention pattern
32+
// but with a larger base because frequency limits are per-second windows, not
33+
// sub-second lock races. var (not const) only so tests can shrink it.
34+
var slidesRateLimitBaseDelay = 1 * time.Second
35+
36+
// isRateLimitedErr reports whether err is a typed retryable rate-limit error
37+
// (e.g. 99991400), as classified by errclass.BuildAPIError.
38+
func isRateLimitedErr(err error) bool {
39+
p, ok := errs.ProblemOf(err)
40+
return ok && p.Subtype == errs.SubtypeRateLimit && p.Retryable
41+
}
42+
43+
// retryOnRateLimit runs fn, retrying with exponential backoff (1s, 2s) when it
44+
// returns a retryable rate-limit error. Any other outcome — success or a
45+
// different error — is returned immediately. Progress is announced on errOut
46+
// so a user watching a batch upload understands the pause.
47+
func retryOnRateLimit(ctx context.Context, errOut io.Writer, fn func() error) error {
48+
var lastErr error
49+
for attempt := 0; attempt <= slidesRateLimitMaxRetries; attempt++ {
50+
if attempt > 0 {
51+
delay := slidesRateLimitBaseDelay << uint(attempt-1)
52+
// Report the actual code from the error: the retry predicate matches
53+
// any retryable SubtypeRateLimit, not just 99991400.
54+
code := 0
55+
if p, ok := errs.ProblemOf(lastErr); ok {
56+
code = p.Code
57+
}
58+
fmt.Fprintf(errOut, "Rate limited by the API (%d), retrying (attempt %d/%d) in %v...\n",
59+
code, attempt, slidesRateLimitMaxRetries, delay)
60+
select {
61+
case <-ctx.Done():
62+
return ctx.Err()
63+
case <-time.After(delay):
64+
}
65+
}
66+
lastErr = fn()
67+
if lastErr == nil || !isRateLimitedErr(lastErr) {
68+
return lastErr
69+
}
70+
}
71+
return lastErr
72+
}
73+
1674
// presentationRef holds a parsed --presentation input.
1775
//
1876
// Slides shortcuts accept three input shapes:
@@ -125,8 +183,30 @@ func resolvePresentationID(runtime *common.RuntimeContext, ref presentationRef)
125183
// around `=`); without it we'd silently leave such placeholders unrewritten.
126184
var imgSrcPlaceholderRegex = regexp.MustCompile(`(?s)<img\b[^>]*?\bsrc\s*=\s*(["'])@([^"']+)(["'])`)
127185

186+
// xmlEntityUnescaper reverses the five XML built-in entities in attribute
187+
// values captured from raw slide XML. strings.Replacer scans left-to-right in
188+
// a single pass, so "&amp;lt;" correctly yields "&lt;" (the leading "&amp;"
189+
// is consumed first), matching XML unescape semantics.
190+
var xmlEntityUnescaper = strings.NewReplacer(
191+
"&lt;", "<",
192+
"&gt;", ">",
193+
"&quot;", `"`,
194+
"&apos;", "'",
195+
"&amp;", "&",
196+
)
197+
198+
// placeholderFilePath converts a raw <img src="@..."> capture into the local
199+
// filesystem path it refers to. The capture comes from well-formed XML where
200+
// a literal & must be written &amp; (the precheck enforces this), so the
201+
// entities are decoded before the path touches Stat/upload. Filesystem paths
202+
// containing & are therefore written as e.g. src="@./Q1&amp;Q2.png".
203+
func placeholderFilePath(raw string) string {
204+
return xmlEntityUnescaper.Replace(strings.TrimSpace(raw))
205+
}
206+
128207
// extractImagePlaceholderPaths returns the de-duplicated list of local paths
129-
// referenced via <img src="@path"> in the given slide XML strings.
208+
// referenced via <img src="@path"> in the given slide XML strings, with XML
209+
// built-in entities decoded (see placeholderFilePath).
130210
//
131211
// Order is preserved (first occurrence wins) so dry-run / progress messages are
132212
// stable across runs.
@@ -141,7 +221,7 @@ func extractImagePlaceholderPaths(slideXMLs []string) []string {
141221
// so we filter it here. Treat as malformed XML and skip.
142222
continue
143223
}
144-
path := strings.TrimSpace(m[2])
224+
path := placeholderFilePath(m[2])
145225
if path == "" || seen[path] {
146226
continue
147227
}
@@ -280,6 +360,47 @@ func ensureShapeHasContent(xmlFragment string) string {
280360
return xmlFragment[:m[1]] + "<content/>" + afterOpen
281361
}
282362

363+
// checkXMLWellFormed verifies that fragment parses as well-formed XML, using
364+
// the same parser family as the backend (Go encoding/xml). Syntax only —
365+
// element names and attributes are NOT checked against the SML schema, so
366+
// anything passing here can still be rejected server-side for semantic
367+
// reasons; conversely nothing rejected here could ever have succeeded, which
368+
// keeps the false-positive risk at zero.
369+
//
370+
// The backend reports these failures as an opaque 3350001/4001000
371+
// "invalid param" with no position info; catching them locally turns the
372+
// dominant real-world causes (bare & in text, unclosed tags, attribute
373+
// quoting) into actionable messages with a line number.
374+
//
375+
// An <?xml ?> declaration is rejected explicitly: the rendering backend does
376+
// not accept processing instructions on slide fragments (rejects with
377+
// "?xml not provide the implement"). encoding/xml surfaces it as a regular
378+
// ProcInst token, so it needs its own check.
379+
//
380+
// Multiple top-level elements are deliberately allowed — insertion fragments
381+
// may legitimately carry sibling elements.
382+
func checkXMLWellFormed(fragment string) error {
383+
dec := xml.NewDecoder(strings.NewReader(fragment))
384+
for {
385+
tok, err := dec.Token()
386+
if err == io.EOF {
387+
return nil
388+
}
389+
if err != nil {
390+
if syn, ok := err.(*xml.SyntaxError); ok {
391+
return errs.NewValidationError(errs.SubtypeInvalidArgument,
392+
"XML not well-formed at line %d: %s (escape literal & as &amp; and < as &lt; in text)",
393+
syn.Line, syn.Msg)
394+
}
395+
return errs.NewValidationError(errs.SubtypeInvalidArgument, "XML not well-formed: %v", err)
396+
}
397+
if pi, ok := tok.(xml.ProcInst); ok && strings.EqualFold(pi.Target, "xml") {
398+
return errs.NewValidationError(errs.SubtypeInvalidArgument,
399+
"XML must not contain an <?xml ?> declaration (the slides backend rejects it); remove it and start at the root element")
400+
}
401+
}
402+
}
403+
283404
// replaceImagePlaceholders rewrites <img src="@path"> occurrences in the input
284405
// XML by looking up each path in tokens. Paths missing from the map are left
285406
// untouched (callers should ensure the map is complete).
@@ -294,7 +415,10 @@ func replaceImagePlaceholders(slideXML string, tokens map[string]string) string
294415
// Mismatched quotes — see extractImagePlaceholderPaths.
295416
return match
296417
}
297-
token, ok := tokens[strings.TrimSpace(path)]
418+
// tokens is keyed by the decoded filesystem path (see
419+
// extractImagePlaceholderPaths), while oldQuoted below must use the
420+
// raw capture so the literal XML text is what gets replaced.
421+
token, ok := tokens[placeholderFilePath(path)]
298422
if !ok {
299423
return match
300424
}

0 commit comments

Comments
 (0)