Skip to content

Commit c313f8b

Browse files
committed
fix(slides): local XML precheck, 99991400 backoff
Lands three of the four agreed fixes from the 2026-06-08 slides write-path telemetry analysis (the commercial quota-code registration is deferred to a separate change): 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. 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). CCM-Harness: set-lark-cli-dev-env,spec
1 parent 824aa9e commit c313f8b

10 files changed

Lines changed: 563 additions & 25 deletions

shortcuts/slides/helpers.go

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

66
import (
7+
"context"
8+
"encoding/xml"
9+
"errors"
710
"fmt"
11+
"io"
812
"net/url"
913
"regexp"
1014
"strings"
15+
"time"
1116

1217
"github.com/larksuite/cli/errs"
1318
"github.com/larksuite/cli/shortcuts/common"
1419
)
1520

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

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

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

shortcuts/slides/helpers_test.go

Lines changed: 174 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,15 @@
44
package slides
55

66
import (
7+
"bytes"
8+
"context"
9+
"errors"
710
"reflect"
811
"strings"
912
"testing"
13+
"time"
14+
15+
"github.com/larksuite/cli/errs"
1016
)
1117

1218
func TestParsePresentationRef(t *testing.T) {
@@ -216,6 +222,15 @@ func TestExtractImagePlaceholderPaths(t *testing.T) {
216222
in: []string{`<img src = "@./spaced.png" />`},
217223
want: []string{"./spaced.png"},
218224
},
225+
{
226+
// Regression: the well-formedness precheck forces a literal & in a
227+
// filename to be written &amp; in the XML; the captured path must
228+
// be entity-decoded before it reaches Stat/upload so the file is
229+
// actually found on disk.
230+
name: "decodes XML entities in path",
231+
in: []string{`<img src="@./Q1&amp;Q2.png"/>`},
232+
want: []string{"./Q1&Q2.png"},
233+
},
219234
}
220235

221236
for _, tt := range tests {
@@ -233,8 +248,9 @@ func TestReplaceImagePlaceholders(t *testing.T) {
233248
t.Parallel()
234249

235250
tokens := map[string]string{
236-
"./pic.png": "tok_abc",
237-
"./b.png": "tok_b",
251+
"./pic.png": "tok_abc",
252+
"./b.png": "tok_b",
253+
"./Q1&Q2.png": "tok_amp", // keyed by decoded filesystem path
238254
}
239255

240256
tests := []struct {
@@ -280,6 +296,13 @@ func TestReplaceImagePlaceholders(t *testing.T) {
280296
in: `<img src = "@./pic.png" topLeftX="10"/>`,
281297
want: `<img src = "tok_abc" topLeftX="10"/>`,
282298
},
299+
{
300+
// Regression: tokens are keyed by the decoded filesystem path, but
301+
// the literal XML text (with &amp;) is what must be rewritten.
302+
name: "decodes XML entities when looking up token",
303+
in: `<img src="@./Q1&amp;Q2.png" topLeftX="10"/>`,
304+
want: `<img src="tok_amp" topLeftX="10"/>`,
305+
},
283306
}
284307

285308
for _, tt := range tests {
@@ -413,3 +436,152 @@ func TestEnsureXMLRootID(t *testing.T) {
413436
})
414437
}
415438
}
439+
440+
func TestCheckXMLWellFormed(t *testing.T) {
441+
t.Parallel()
442+
443+
tests := []struct {
444+
name string
445+
in string
446+
wantErr string
447+
}{
448+
{name: "simple element", in: `<shape type="rect"><content/></shape>`},
449+
{name: "nested with attributes", in: `<slide><shape type="text"><content><p>hi</p></content></shape></slide>`},
450+
// Insertion fragments may carry sibling top-level elements; the decoder
451+
// must not enforce a single document element.
452+
{name: "multiple top-level elements", in: `<p>a</p><p>b</p>`},
453+
{name: "escaped entities", in: `<p>A &amp; B &lt;tag&gt; &quot;q&quot;</p>`},
454+
{name: "CDATA with raw ampersand", in: `<p><![CDATA[a & b < c]]></p>`},
455+
{name: "comment", in: `<!-- note --><shape/>`},
456+
{name: "img placeholder attr", in: `<img src="@./local.png" width="100"/>`},
457+
{name: "unicode text", in: `<p>项目汇报 🎯</p>`},
458+
459+
// Top CLI-path failure cause in engine logs: bare & in text.
460+
{name: "bare ampersand", in: `<p>Q & A</p>`, wantErr: "line 1"},
461+
{name: "bare ampersand multiline", in: "<slide>\n<p>R&D</p>\n</slide>", wantErr: "line 2"},
462+
{name: "unclosed tag", in: `<shape><content></shape>`, wantErr: "not well-formed"},
463+
{name: "unquoted attribute", in: `<shape type=rect/>`, wantErr: "not well-formed"},
464+
{name: "stray closing tag", in: `<p>hi</p></div>`, wantErr: "not well-formed"},
465+
{name: "undefined entity", in: `<p>a&nbsp;b</p>`, wantErr: "not well-formed"},
466+
467+
// nodeserver rejects processing instructions ("?xml not provide the
468+
// implement"); reject the declaration locally regardless of position.
469+
{name: "xml declaration", in: `<?xml version="1.0"?><shape/>`, wantErr: "declaration"},
470+
{name: "xml declaration with encoding", in: `<?xml version="1.0" encoding="UTF-8"?><slide/>`, wantErr: "declaration"},
471+
{name: "uppercase xml declaration", in: `<?XML version="1.0"?><shape/>`, wantErr: "declaration"},
472+
}
473+
474+
for _, tt := range tests {
475+
t.Run(tt.name, func(t *testing.T) {
476+
t.Parallel()
477+
err := checkXMLWellFormed(tt.in)
478+
if tt.wantErr == "" {
479+
if err != nil {
480+
t.Fatalf("unexpected err: %v", err)
481+
}
482+
return
483+
}
484+
if err == nil {
485+
t.Fatalf("want error containing %q, got nil", tt.wantErr)
486+
}
487+
var ve *errs.ValidationError
488+
if !errors.As(err, &ve) {
489+
t.Fatalf("want *errs.ValidationError, got %T: %v", err, err)
490+
}
491+
if ve.Subtype != errs.SubtypeInvalidArgument {
492+
t.Fatalf("want SubtypeInvalidArgument, got %v", ve.Subtype)
493+
}
494+
if !strings.Contains(err.Error(), tt.wantErr) {
495+
t.Fatalf("want error containing %q, got %q", tt.wantErr, err.Error())
496+
}
497+
})
498+
}
499+
}
500+
501+
// TestRetryOnRateLimit verifies the 99991400 backoff helper: retryable
502+
// rate-limit errors are retried with backoff, anything else returns
503+
// immediately, and exhaustion surfaces the last rate-limit error.
504+
//
505+
// Not parallel: shrinks the package-level slidesRateLimitBaseDelay.
506+
func TestRetryOnRateLimit(t *testing.T) {
507+
restore := slidesRateLimitBaseDelay
508+
slidesRateLimitBaseDelay = time.Millisecond
509+
t.Cleanup(func() { slidesRateLimitBaseDelay = restore })
510+
511+
rateLimitErr := func() error {
512+
return errs.NewAPIError(errs.SubtypeRateLimit, "request trigger frequency limit").WithRetryable()
513+
}
514+
515+
t.Run("success without retry", func(t *testing.T) {
516+
var errOut bytes.Buffer
517+
calls := 0
518+
err := retryOnRateLimit(context.Background(), &errOut, func() error {
519+
calls++
520+
return nil
521+
})
522+
if err != nil || calls != 1 {
523+
t.Fatalf("err=%v calls=%d, want nil/1", err, calls)
524+
}
525+
if errOut.Len() != 0 {
526+
t.Fatalf("no retry message expected, got: %s", errOut.String())
527+
}
528+
})
529+
530+
t.Run("succeeds after transient rate limit", func(t *testing.T) {
531+
var errOut bytes.Buffer
532+
calls := 0
533+
err := retryOnRateLimit(context.Background(), &errOut, func() error {
534+
calls++
535+
if calls <= 2 {
536+
return rateLimitErr()
537+
}
538+
return nil
539+
})
540+
if err != nil || calls != 3 {
541+
t.Fatalf("err=%v calls=%d, want nil/3", err, calls)
542+
}
543+
if !strings.Contains(errOut.String(), "retrying") {
544+
t.Fatalf("expected retry announcement, got: %s", errOut.String())
545+
}
546+
})
547+
548+
t.Run("exhaustion returns last rate-limit error", func(t *testing.T) {
549+
var errOut bytes.Buffer
550+
calls := 0
551+
err := retryOnRateLimit(context.Background(), &errOut, func() error {
552+
calls++
553+
return rateLimitErr()
554+
})
555+
if err == nil || !isRateLimitedErr(err) {
556+
t.Fatalf("want rate-limit error after exhaustion, got: %v", err)
557+
}
558+
if calls != slidesRateLimitMaxRetries+1 {
559+
t.Fatalf("calls=%d, want %d", calls, slidesRateLimitMaxRetries+1)
560+
}
561+
})
562+
563+
t.Run("non-rate-limit error returns immediately", func(t *testing.T) {
564+
var errOut bytes.Buffer
565+
calls := 0
566+
boom := errs.NewAPIError(errs.SubtypeNotFound, "not found")
567+
err := retryOnRateLimit(context.Background(), &errOut, func() error {
568+
calls++
569+
return boom
570+
})
571+
if !errors.Is(err, boom) || calls != 1 {
572+
t.Fatalf("err=%v calls=%d, want boom/1", err, calls)
573+
}
574+
})
575+
576+
t.Run("cancelled context aborts the backoff wait", func(t *testing.T) {
577+
var errOut bytes.Buffer
578+
ctx, cancel := context.WithCancel(context.Background())
579+
cancel()
580+
err := retryOnRateLimit(ctx, &errOut, func() error {
581+
return rateLimitErr()
582+
})
583+
if !errors.Is(err, context.Canceled) {
584+
t.Fatalf("want context.Canceled, got: %v", err)
585+
}
586+
})
587+
}

0 commit comments

Comments
 (0)