From c313f8bd1a0d7df1a1bc986c7c8e27b2f2a8e938 Mon Sep 17 00:00:00 2001 From: "caichengjie.viper" Date: Wed, 10 Jun 2026 16:56:59 +0800 Subject: [PATCH] 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 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 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 --- shortcuts/slides/helpers.go | 132 ++++++++++++- shortcuts/slides/helpers_test.go | 176 +++++++++++++++++- shortcuts/slides/slides_create.go | 33 +++- shortcuts/slides/slides_create_test.go | 96 +++++++++- shortcuts/slides/slides_media_upload.go | 25 ++- shortcuts/slides/slides_media_upload_test.go | 47 +++++ shortcuts/slides/slides_replace_slide.go | 11 ++ shortcuts/slides/slides_replace_slide_test.go | 38 ++++ .../references/xml-format-guide.md | 6 +- .../references/xml-schema-quick-ref.md | 24 ++- 10 files changed, 563 insertions(+), 25 deletions(-) diff --git a/shortcuts/slides/helpers.go b/shortcuts/slides/helpers.go index 2c2212264..20d86d819 100644 --- a/shortcuts/slides/helpers.go +++ b/shortcuts/slides/helpers.go @@ -4,15 +4,74 @@ package slides import ( + "context" + "encoding/xml" + "errors" "fmt" + "io" "net/url" "regexp" "strings" + "time" "github.com/larksuite/cli/errs" "github.com/larksuite/cli/shortcuts/common" ) +const ( + // slidesRateLimitMaxRetries is the number of automatic retries (beyond the + // initial request) when the API answers 99991400 "request trigger frequency + // limit". The slides batch paths (+create slide loop, placeholder image + // uploads) fire consecutive POSTs and are the dominant 99991400 producers + // in telemetry; a short backoff absorbs a transient burst without masking a + // genuinely saturated tenant. + slidesRateLimitMaxRetries = 2 +) + +// slidesRateLimitBaseDelay is the initial backoff delay; subsequent retries +// double it (1s, 2s). Mirrors the wiki +node-create lock-contention pattern +// but with a larger base because a frequency window takes longer to clear than +// a sub-second lock race. var (not const) only so tests can shrink it. +var slidesRateLimitBaseDelay = 1 * time.Second + +// isRateLimitedErr reports whether err is a typed retryable rate-limit error +// (e.g. 99991400), as classified by errclass.BuildAPIError. +func isRateLimitedErr(err error) bool { + p, ok := errs.ProblemOf(err) + return ok && p.Subtype == errs.SubtypeRateLimit && p.Retryable +} + +// retryOnRateLimit runs fn, retrying with exponential backoff (1s, 2s) when it +// returns a retryable rate-limit error. Any other outcome — success or a +// different error — is returned immediately. Progress is announced on errOut +// so a user watching a batch upload understands the pause. +func retryOnRateLimit(ctx context.Context, errOut io.Writer, fn func() error) error { + var lastErr error + for attempt := 0; attempt <= slidesRateLimitMaxRetries; attempt++ { + if attempt > 0 { + delay := slidesRateLimitBaseDelay << uint(attempt-1) + // Report the actual code from the error: the retry predicate matches + // any retryable SubtypeRateLimit, not just 99991400. + code := 0 + if p, ok := errs.ProblemOf(lastErr); ok { + code = p.Code + } + fmt.Fprintf(errOut, "Rate limited by the API (%d), retrying (attempt %d/%d) in %v...\n", + code, attempt, slidesRateLimitMaxRetries, delay) + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(delay): + } + } + lastErr = fn() + if lastErr == nil || !isRateLimitedErr(lastErr) { + return lastErr + } + } + return lastErr +} + // presentationRef holds a parsed --presentation input. // // Slides shortcuts accept three input shapes: @@ -125,8 +184,30 @@ func resolvePresentationID(runtime *common.RuntimeContext, ref presentationRef) // around `=`); without it we'd silently leave such placeholders unrewritten. var imgSrcPlaceholderRegex = regexp.MustCompile(`(?s)]*?\bsrc\s*=\s*(["'])@([^"']+)(["'])`) +// xmlEntityUnescaper reverses the five XML built-in entities in attribute +// values captured from raw slide XML. strings.Replacer scans left-to-right in +// a single pass, so "&lt;" correctly yields "<" (the leading "&" +// is consumed first), matching XML unescape semantics. +var xmlEntityUnescaper = strings.NewReplacer( + "<", "<", + ">", ">", + """, `"`, + "'", "'", + "&", "&", +) + +// placeholderFilePath converts a raw capture into the local +// filesystem path it refers to. The capture comes from well-formed XML where +// a literal & must be written & (the precheck enforces this), so the +// entities are decoded before the path touches Stat/upload. Filesystem paths +// containing & are therefore written as e.g. src="@./Q1&Q2.png". +func placeholderFilePath(raw string) string { + return xmlEntityUnescaper.Replace(strings.TrimSpace(raw)) +} + // extractImagePlaceholderPaths returns the de-duplicated list of local paths -// referenced via in the given slide XML strings. +// referenced via in the given slide XML strings, with XML +// built-in entities decoded (see placeholderFilePath). // // Order is preserved (first occurrence wins) so dry-run / progress messages are // stable across runs. @@ -141,7 +222,7 @@ func extractImagePlaceholderPaths(slideXMLs []string) []string { // so we filter it here. Treat as malformed XML and skip. continue } - path := strings.TrimSpace(m[2]) + path := placeholderFilePath(m[2]) if path == "" || seen[path] { continue } @@ -280,6 +361,48 @@ func ensureShapeHasContent(xmlFragment string) string { return xmlFragment[:m[1]] + "" + afterOpen } +// checkXMLWellFormed verifies that fragment parses as well-formed XML, using +// the same parser family as the backend (Go encoding/xml). Syntax only — +// element names and attributes are NOT checked against the SML schema, so +// anything passing here can still be rejected server-side for semantic +// reasons; conversely nothing rejected here could ever have succeeded, which +// keeps the false-positive risk at zero. +// +// The backend reports these failures as an opaque 3350001/4001000 +// "invalid param" with no position info; catching them locally turns the +// dominant real-world causes (bare & in text, unclosed tags, attribute +// quoting) into actionable messages with a line number. +// +// An declaration is rejected explicitly: the rendering backend does +// not accept processing instructions on slide fragments (rejects with +// "?xml not provide the implement"). encoding/xml surfaces it as a regular +// ProcInst token, so it needs its own check. +// +// Multiple top-level elements are deliberately allowed — insertion fragments +// may legitimately carry sibling elements. +func checkXMLWellFormed(fragment string) error { + dec := xml.NewDecoder(strings.NewReader(fragment)) + for { + tok, err := dec.Token() + if errors.Is(err, io.EOF) { + return nil + } + if err != nil { + var syn *xml.SyntaxError + if errors.As(err, &syn) { + return errs.NewValidationError(errs.SubtypeInvalidArgument, + "XML not well-formed at line %d: %s (escape literal & as & and < as < in text)", + syn.Line, syn.Msg) + } + return errs.NewValidationError(errs.SubtypeInvalidArgument, "XML not well-formed: %v", err) + } + if pi, ok := tok.(xml.ProcInst); ok && strings.EqualFold(pi.Target, "xml") { + return errs.NewValidationError(errs.SubtypeInvalidArgument, + "XML must not contain an declaration (the slides backend rejects it); remove it and start at the root element") + } + } +} + // replaceImagePlaceholders rewrites occurrences in the input // XML by looking up each path in tokens. Paths missing from the map are left // untouched (callers should ensure the map is complete). @@ -294,7 +417,10 @@ func replaceImagePlaceholders(slideXML string, tokens map[string]string) string // Mismatched quotes — see extractImagePlaceholderPaths. return match } - token, ok := tokens[strings.TrimSpace(path)] + // tokens is keyed by the decoded filesystem path (see + // extractImagePlaceholderPaths), while oldQuoted below must use the + // raw capture so the literal XML text is what gets replaced. + token, ok := tokens[placeholderFilePath(path)] if !ok { return match } diff --git a/shortcuts/slides/helpers_test.go b/shortcuts/slides/helpers_test.go index bd1a740b7..53b16238c 100644 --- a/shortcuts/slides/helpers_test.go +++ b/shortcuts/slides/helpers_test.go @@ -4,9 +4,15 @@ package slides import ( + "bytes" + "context" + "errors" "reflect" "strings" "testing" + "time" + + "github.com/larksuite/cli/errs" ) func TestParsePresentationRef(t *testing.T) { @@ -216,6 +222,15 @@ func TestExtractImagePlaceholderPaths(t *testing.T) { in: []string{``}, want: []string{"./spaced.png"}, }, + { + // Regression: the well-formedness precheck forces a literal & in a + // filename to be written & in the XML; the captured path must + // be entity-decoded before it reaches Stat/upload so the file is + // actually found on disk. + name: "decodes XML entities in path", + in: []string{``}, + want: []string{"./Q1&Q2.png"}, + }, } for _, tt := range tests { @@ -233,8 +248,9 @@ func TestReplaceImagePlaceholders(t *testing.T) { t.Parallel() tokens := map[string]string{ - "./pic.png": "tok_abc", - "./b.png": "tok_b", + "./pic.png": "tok_abc", + "./b.png": "tok_b", + "./Q1&Q2.png": "tok_amp", // keyed by decoded filesystem path } tests := []struct { @@ -280,6 +296,13 @@ func TestReplaceImagePlaceholders(t *testing.T) { in: ``, want: ``, }, + { + // Regression: tokens are keyed by the decoded filesystem path, but + // the literal XML text (with &) is what must be rewritten. + name: "decodes XML entities when looking up token", + in: ``, + want: ``, + }, } for _, tt := range tests { @@ -413,3 +436,152 @@ func TestEnsureXMLRootID(t *testing.T) { }) } } + +func TestCheckXMLWellFormed(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + wantErr string + }{ + {name: "simple element", in: ``}, + {name: "nested with attributes", in: `

hi

`}, + // Insertion fragments may carry sibling top-level elements; the decoder + // must not enforce a single document element. + {name: "multiple top-level elements", in: `

a

b

`}, + {name: "escaped entities", in: `

A & B <tag> "q"

`}, + {name: "CDATA with raw ampersand", in: `

`}, + {name: "comment", in: ``}, + {name: "img placeholder attr", in: ``}, + {name: "unicode text", in: `

项目汇报 🎯

`}, + + // Top CLI-path failure cause in engine logs: bare & in text. + {name: "bare ampersand", in: `

Q & A

`, wantErr: "line 1"}, + {name: "bare ampersand multiline", in: "\n

R&D

\n", wantErr: "line 2"}, + {name: "unclosed tag", in: ``, wantErr: "not well-formed"}, + {name: "unquoted attribute", in: ``, wantErr: "not well-formed"}, + {name: "stray closing tag", in: `

hi

`, wantErr: "not well-formed"}, + {name: "undefined entity", in: `

a b

`, wantErr: "not well-formed"}, + + // nodeserver rejects processing instructions ("?xml not provide the + // implement"); reject the declaration locally regardless of position. + {name: "xml declaration", in: ``, wantErr: "declaration"}, + {name: "xml declaration with encoding", in: ``, wantErr: "declaration"}, + {name: "uppercase xml declaration", in: ``, 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) + } + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("want *errs.ValidationError, got %T: %v", err, 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()) + } + }) + } +} + +// TestRetryOnRateLimit verifies the 99991400 backoff helper: retryable +// rate-limit errors are retried with backoff, anything else returns +// immediately, and exhaustion surfaces the last rate-limit error. +// +// Not parallel: shrinks the package-level slidesRateLimitBaseDelay. +func TestRetryOnRateLimit(t *testing.T) { + restore := slidesRateLimitBaseDelay + slidesRateLimitBaseDelay = time.Millisecond + t.Cleanup(func() { slidesRateLimitBaseDelay = restore }) + + rateLimitErr := func() error { + return errs.NewAPIError(errs.SubtypeRateLimit, "request trigger frequency limit").WithRetryable() + } + + t.Run("success without retry", func(t *testing.T) { + var errOut bytes.Buffer + calls := 0 + err := retryOnRateLimit(context.Background(), &errOut, func() error { + calls++ + return nil + }) + if err != nil || calls != 1 { + t.Fatalf("err=%v calls=%d, want nil/1", err, calls) + } + if errOut.Len() != 0 { + t.Fatalf("no retry message expected, got: %s", errOut.String()) + } + }) + + t.Run("succeeds after transient rate limit", func(t *testing.T) { + var errOut bytes.Buffer + calls := 0 + err := retryOnRateLimit(context.Background(), &errOut, func() error { + calls++ + if calls <= 2 { + return rateLimitErr() + } + return nil + }) + if err != nil || calls != 3 { + t.Fatalf("err=%v calls=%d, want nil/3", err, calls) + } + if !strings.Contains(errOut.String(), "retrying") { + t.Fatalf("expected retry announcement, got: %s", errOut.String()) + } + }) + + t.Run("exhaustion returns last rate-limit error", func(t *testing.T) { + var errOut bytes.Buffer + calls := 0 + err := retryOnRateLimit(context.Background(), &errOut, func() error { + calls++ + return rateLimitErr() + }) + if err == nil || !isRateLimitedErr(err) { + t.Fatalf("want rate-limit error after exhaustion, got: %v", err) + } + if calls != slidesRateLimitMaxRetries+1 { + t.Fatalf("calls=%d, want %d", calls, slidesRateLimitMaxRetries+1) + } + }) + + t.Run("non-rate-limit error returns immediately", func(t *testing.T) { + var errOut bytes.Buffer + calls := 0 + boom := errs.NewAPIError(errs.SubtypeNotFound, "not found") + err := retryOnRateLimit(context.Background(), &errOut, func() error { + calls++ + return boom + }) + if !errors.Is(err, boom) || calls != 1 { + t.Fatalf("err=%v calls=%d, want boom/1", err, calls) + } + }) + + t.Run("cancelled context aborts the backoff wait", func(t *testing.T) { + var errOut bytes.Buffer + ctx, cancel := context.WithCancel(context.Background()) + cancel() + err := retryOnRateLimit(ctx, &errOut, func() error { + return rateLimitErr() + }) + if !errors.Is(err, context.Canceled) { + t.Fatalf("want context.Canceled, got: %v", err) + } + }) +} diff --git a/shortcuts/slides/slides_create.go b/shortcuts/slides/slides_create.go index 115f04cd1..4d1f1b2b0 100644 --- a/shortcuts/slides/slides_create.go +++ b/shortcuts/slides/slides_create.go @@ -50,6 +50,15 @@ var SlidesCreate = common.Shortcut{ if len(slides) > maxSlidesPerCreate { return errs.NewValidationError(errs.SubtypeInvalidArgument, "--slides array exceeds maximum of %d slides; create the presentation first, then add slides via xml_presentation.slide.create", maxSlidesPerCreate).WithParam("--slides") } + // Well-formedness precheck before any API call: a syntax error in + // slide N would otherwise create the presentation and then fail on + // the slide POST with an opaque backend "invalid param", leaving a + // partially-built deck behind. + for i, slideXML := range slides { + if err := checkXMLWellFormed(slideXML); err != nil { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--slides[%d]: %v", i, err).WithParam("--slides").WithCause(err) + } + } // Validate placeholder paths up front so we don't create a presentation // only to fail mid-way on a missing local file. for _, path := range extractImagePlaceholderPaths(slides) { @@ -183,14 +192,22 @@ var SlidesCreate = common.Shortcut{ var slideIDs []string for i, slideXML := range slides { - slideData, err := runtime.CallAPITyped( - "POST", - slideURL, - map[string]interface{}{"revision_id": -1}, - map[string]interface{}{ - "slide": map[string]interface{}{"content": slideXML}, - }, - ) + var slideData map[string]interface{} + // Consecutive slide POSTs are the main 99991400 producer in + // telemetry; absorb the per-second frequency window with a + // short backoff instead of aborting the whole batch. + err := retryOnRateLimit(ctx, runtime.IO().ErrOut, func() error { + var callErr error + slideData, callErr = runtime.CallAPITyped( + "POST", + slideURL, + map[string]interface{}{"revision_id": -1}, + map[string]interface{}{ + "slide": map[string]interface{}{"content": slideXML}, + }, + ) + return callErr + }) if err != nil { return appendSlidesProgressHint(err, fmt.Sprintf("adding slide %d/%d failed; presentation %s was created, %d slide(s) added before failure", i+1, len(slides), presentationID, i)) } diff --git a/shortcuts/slides/slides_create_test.go b/shortcuts/slides/slides_create_test.go index 36b964a07..309b941dd 100644 --- a/shortcuts/slides/slides_create_test.go +++ b/shortcuts/slides/slides_create_test.go @@ -10,6 +10,7 @@ import ( "os" "strings" "testing" + "time" "github.com/spf13/cobra" @@ -392,7 +393,10 @@ func TestSlidesCreateWithSlidesPartialFailure(t *testing.T) { }, }) - slidesJSON := `["",""]` + // The second slide is well-formed XML (so it passes the local precheck) + // but uses an element the backend rejects — partial failure must come from + // the API layer, not validation. + slidesJSON := `["",""]` err := runSlidesCreateShortcut(t, f, stdout, []string{ "+create", "--title", "Partial", @@ -918,3 +922,93 @@ func TestSlidesCreateWithPlaceholdersDryRun(t *testing.T) { t.Fatalf("dry-run header should describe upload count, got: %s", out) } } + +// TestSlidesCreateRejectsMalformedSlideXML verifies the well-formedness +// precheck fires before any API call — no presentation should be created when +// a slide fragment has a syntax error, so no httpmock stubs are registered. +func TestSlidesCreateRejectsMalformedSlideXML(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + slides string + wantErr string + }{ + {"bare ampersand", `["

Q & A

"]`, "--slides[0]"}, + {"unclosed tag", `["

ok

",""]`, "--slides[1]"}, + {"xml declaration", `[""]`, "declaration"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + err := runSlidesCreateShortcut(t, f, stdout, []string{ + "+create", + "--title", "precheck", + "--slides", tt.slides, + "--as", "user", + }) + if err == nil { + t.Fatalf("expected validation error for %s, got nil", tt.name) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error = %q, want substring %q", err.Error(), tt.wantErr) + } + if !strings.Contains(err.Error(), "well-formed") && !strings.Contains(err.Error(), "declaration") { + t.Fatalf("error should explain the XML problem, got %q", err.Error()) + } + }) + } +} + +// TestSlidesCreateRetriesSlideRateLimit verifies the +create slide loop +// retries a 99991400 "request trigger frequency limit" slide POST with +// backoff instead of aborting the batch (one-shot stubs: first slide POST +// answers 99991400, the second answers success — both must be consumed). +// +// Not parallel: shrinks the package-level slidesRateLimitBaseDelay. +func TestSlidesCreateRetriesSlideRateLimit(t *testing.T) { + restore := slidesRateLimitBaseDelay + slidesRateLimitBaseDelay = time.Millisecond + t.Cleanup(func() { slidesRateLimitBaseDelay = restore }) + + f, stdout, stderr, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations", + Body: map[string]interface{}{ + "code": 0, + "data": map[string]interface{}{"xml_presentation_id": "pres_rl", "revision_id": 1}, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_rl/slide", + Status: 400, + Body: map[string]interface{}{"code": 99991400, "msg": "request trigger frequency limit"}, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_rl/slide", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{"slide_id": "s1", "revision_id": 2}}, + }) + + err := runSlidesCreateShortcut(t, f, stdout, []string{ + "+create", + "--title", "RL test", + "--slides", `[""]`, + "--as", "user", + }) + if err != nil { + t.Fatalf("unexpected error (rate limit should have been retried): %v", err) + } + + data := decodeSlidesCreateEnvelope(t, stdout) + if data["slides_added"] != float64(1) { + t.Fatalf("slides_added = %v, want 1", data["slides_added"]) + } + if !strings.Contains(stderr.String(), "retrying") { + t.Fatalf("expected retry announcement on stderr, got: %s", stderr.String()) + } +} diff --git a/shortcuts/slides/slides_media_upload.go b/shortcuts/slides/slides_media_upload.go index bd611f177..1884c1f9a 100644 --- a/shortcuts/slides/slides_media_upload.go +++ b/shortcuts/slides/slides_media_upload.go @@ -128,13 +128,26 @@ func uploadSlidesMedia(runtime *common.RuntimeContext, filePath, fileName string fileName, common.FormatSize(fileSize)) } parent := presentationID - return common.UploadDriveMediaAllTyped(runtime, common.DriveMediaUploadAllConfig{ - FilePath: filePath, - FileName: fileName, - FileSize: fileSize, - ParentType: slidesMediaParentType, - ParentNode: &parent, + var fileToken string + // upload_all is rate-limited per second; consecutive placeholder uploads + // from +create (and rapid repeated +media-upload calls) can hit 99991400. + // A failed rate-limited attempt creates nothing server-side, and each + // attempt re-opens the file from FilePath, so the retry is safe. + // The Typed variant is required here: retryOnRateLimit matches on the + // typed subtype, and slides error wrapping (appendSlidesProgressHint) + // already expects typed errs.* envelopes. + err := retryOnRateLimit(runtime.Ctx(), runtime.IO().ErrOut, func() error { + var callErr error + fileToken, callErr = common.UploadDriveMediaAllTyped(runtime, common.DriveMediaUploadAllConfig{ + FilePath: filePath, + FileName: fileName, + FileSize: fileSize, + ParentType: slidesMediaParentType, + ParentNode: &parent, + }) + return callErr }) + return fileToken, err } // appendSlidesUploadDryRun renders the upload_all step for a single file. diff --git a/shortcuts/slides/slides_media_upload_test.go b/shortcuts/slides/slides_media_upload_test.go index a46205bb4..20a301cda 100644 --- a/shortcuts/slides/slides_media_upload_test.go +++ b/shortcuts/slides/slides_media_upload_test.go @@ -12,6 +12,7 @@ import ( "os" "strings" "testing" + "time" "github.com/spf13/cobra" @@ -367,3 +368,49 @@ func readAll(t *testing.T, r interface { } return buf.Bytes() } + +// TestSlidesMediaUploadRetriesRateLimit verifies uploadSlidesMedia retries a +// 99991400 "request trigger frequency limit" upload_all response with backoff +// (one-shot stubs: the rate-limited response is consumed first, then the +// success response) and still returns the file_token. +// +// Not parallel: uses os.Chdir and shrinks slidesRateLimitBaseDelay. +func TestSlidesMediaUploadRetriesRateLimit(t *testing.T) { + restore := slidesRateLimitBaseDelay + slidesRateLimitBaseDelay = time.Millisecond + t.Cleanup(func() { slidesRateLimitBaseDelay = restore }) + + dir := t.TempDir() + withSlidesTestWorkingDir(t, dir) + if err := os.WriteFile("rl.png", []byte("x"), 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/drive/v1/medias/upload_all", + Status: 400, + Body: map[string]interface{}{"code": 99991400, "msg": "request trigger frequency limit"}, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/drive/v1/medias/upload_all", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{"file_token": "tok_rl"}}, + }) + + err := runSlidesShortcut(t, f, stdout, SlidesMediaUpload, []string{ + "+media-upload", + "--file", "rl.png", + "--presentation", "pres_rl_upload", + "--as", "user", + }) + if err != nil { + t.Fatalf("unexpected error (rate limit should have been retried): %v", err) + } + + data := decodeShortcutData(t, stdout) + if data["file_token"] != "tok_rl" { + t.Fatalf("file_token = %v, want tok_rl", data["file_token"]) + } +} diff --git a/shortcuts/slides/slides_replace_slide.go b/shortcuts/slides/slides_replace_slide.go index a69107288..f14ee7745 100644 --- a/shortcuts/slides/slides_replace_slide.go +++ b/shortcuts/slides/slides_replace_slide.go @@ -34,6 +34,9 @@ const maxReplaceParts = 200 // it triggers 3350001. // 4. On 3350001 errors it enriches the hint with context-specific guidance // so AI agents can self-correct. +// 5. It rejects non-well-formed replacement/insertion XML before any API +// call, with a line number and escaping hint — the backend reports these +// only as an opaque 3350001/4001000 "invalid param". // // `str_replace` is intentionally NOT exposed: product direction is that // slide edits go through structural (block-level) operations only. The backend @@ -278,6 +281,8 @@ func enrichSlidesReplaceError(err error) error { // - size is within [1, 200] // - action is one of the exposed actions (block_replace / block_insert) // - per-action required fields are present +// - replacement / insertion fragments are well-formed XML (syntax only; +// see checkXMLWellFormed) func validateReplaceParts(parts []replacePart) error { if len(parts) == 0 { return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts must contain at least 1 item").WithParam("--parts") @@ -294,10 +299,16 @@ func validateReplaceParts(parts []replacePart) error { if p.Replacement == nil || strings.TrimSpace(*p.Replacement) == "" { return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d] (block_replace) requires non-empty replacement", i).WithParam("--parts") } + if err := checkXMLWellFormed(*p.Replacement); err != nil { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].replacement: %v", i, err).WithParam("--parts").WithCause(err) + } case "block_insert": if p.Insertion == nil || strings.TrimSpace(*p.Insertion) == "" { return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d] (block_insert) requires non-empty insertion", i).WithParam("--parts") } + if err := checkXMLWellFormed(*p.Insertion); err != nil { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].insertion: %v", i, err).WithParam("--parts").WithCause(err) + } case "str_replace": // Backend still accepts str_replace, but product decision is to // force structural edits through the CLI. Block it up-front so diff --git a/shortcuts/slides/slides_replace_slide_test.go b/shortcuts/slides/slides_replace_slide_test.go index a4c37db39..287c454c6 100644 --- a/shortcuts/slides/slides_replace_slide_test.go +++ b/shortcuts/slides/slides_replace_slide_test.go @@ -731,3 +731,41 @@ func TestReplaceSlideValidationParam(t *testing.T) { }) } } + +// TestReplaceSlideRejectsMalformedFragmentXML verifies the well-formedness +// precheck on replacement / insertion fragments fires at validation time, +// before wiki resolution or the replace POST. +func TestReplaceSlideRejectsMalformedFragmentXML(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + parts string + wantErr string + }{ + {"replacement bare ampersand", `[{"action":"block_replace","block_id":"bUn","replacement":"

R & D

"}]`, "--parts[0].replacement"}, + {"replacement unclosed tag", `[{"action":"block_replace","block_id":"bUn","replacement":""}]`, "--parts[0].replacement"}, + {"insertion xml declaration", `[{"action":"block_insert","insertion":""}]`, "declaration"}, + {"second part malformed", `[{"action":"block_insert","insertion":"

ok

"},{"action":"block_insert","insertion":"

Q & A

"}]`, "--parts[1].insertion"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + err := runSlidesShortcut(t, f, stdout, SlidesReplaceSlide, []string{ + "+replace-slide", + "--presentation", "pres_abc", + "--slide-id", "s", + "--parts", tt.parts, + "--as", "user", + }) + if err == nil { + t.Fatalf("expected validation error for %s, got nil", tt.name) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error = %q, want substring %q", err.Error(), tt.wantErr) + } + }) + } +} diff --git a/skills/lark-slides/references/xml-format-guide.md b/skills/lark-slides/references/xml-format-guide.md index 7e5c31f5f..fca486e31 100644 --- a/skills/lark-slides/references/xml-format-guide.md +++ b/skills/lark-slides/references/xml-format-guide.md @@ -2,10 +2,11 @@ 本文档基于 [slides_xml_schema_definition.xml](slides_xml_schema_definition.xml) 整理，说明飞书 Slides XML Schema（SML 2.0）的核心结构和常用写法。 +> **注意**：所有提交给 API 的 XML（整篇或片段）都**不要带 `` 声明**——slides 后端会拒绝它，直接从根元素写起。标签白名单与文本转义规则见 [xml-schema-quick-ref.md](xml-schema-quick-ref.md)。 + ## 基本结构 ```xml - 演示文稿标题 @@ -224,7 +225,7 @@ | `src` 形式 | 说明 | |---|---| | `file_token`（如 `boxcnXXXXXXXXXXXXXXXXXXXXXX`） | 通过 `slides +media-upload` 上传后返回的 token | -| `@<本地路径>`（如 `@./assets/chart.png`） | **仅在 `slides +create --slides` 中可用**：CLI 会自动上传该文件并替换为 file_token | +| `@<本地路径>`（如 `@./assets/chart.png`） | **仅在 `slides +create --slides` 中可用**：CLI 会自动上传该文件并替换为 file_token。路径也要按 XML 规则转义：文件名含 `&` 时写 `@./Q1&Q2.png`，CLI 反转义后查找文件 | > **禁止使用 http(s) 外链 URL**：飞书 slides 渲染端不会代理外链图片，`src="https://..."` 在 PPT 里通常显示破图。要用网图必须先 `curl`/下载到 CWD 内，再走上传流程拿 `file_token`。 @@ -305,7 +306,6 @@ ## 完整示例 ```xml - 季度报告 diff --git a/skills/lark-slides/references/xml-schema-quick-ref.md b/skills/lark-slides/references/xml-schema-quick-ref.md index 6ac38d9d6..9405c5132 100644 --- a/skills/lark-slides/references/xml-schema-quick-ref.md +++ b/skills/lark-slides/references/xml-schema-quick-ref.md @@ -9,10 +9,30 @@ 3. `` 直接子元素只有 `