From 6ac31eae87b6bbd11b2e5beb75b73b622d054fbd Mon Sep 17 00:00:00 2001 From: "zhuyuanbin.gdut" Date: Tue, 18 Aug 2026 17:26:16 +0800 Subject: [PATCH] feat(slides): lint slide writes server-side, add --no-lint to opt out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit +create, +add-slide, +update-slide and +replace-slide are the four shortcuts that change slide content, so they are the four that can ask the backend to check the page before accepting the write. All four now send lint_xml=true by default and lint_xml=false when --no-lint is passed. The subject of the check is the page the write produces, not the payload it was handed: +replace-slide submits fragments, and a fragment that is correct on its own can still push a neighbour off the canvas. The switch travels in the request body rather than the query string. A query parameter has to be declared in the gateway's own api meta before it is bound to a field, and the published definition of these endpoints does not list one — so an undeclared parameter is dropped, the field arrives unset, and the server reads it as "not requested". Verified against a live backend: pages that asked to be linted were written unlinted, with nothing anywhere to say so. Body fields ride along with the JSON already being sent and need no registration. The value is sent explicitly in both directions rather than omitted when on. The parameter is newer than the registry, so the server-side default is not something this CLI can read anywhere, and a request that states the value keeps meaning the same thing if that default ever moves. A refusal is passed through verbatim. The message field carries the lint report itself — the same document the lint tool writes when it is run by hand — and the same refusal reaches callers through `lark-cli api` as well, where nothing rewrites it. Rendering it to prose here would give one refusal two formats depending on which command produced it. Each finding carries the numbers behind its own rule, and which numbers those are differs per rule, so nothing is decoded that is not used: the report is what the caller reads. The refusal is recognised by its error code, 4000153, which the engine raises for nothing else and which reaches the CLI unchanged. Matching on the shape of the message instead would mean claiming any JSON that resembles a report, and a false positive there rewrites the hint of an error this code does not understand. What the backend cannot say goes in the hint instead: how many findings refused the write, that the page did not land, and --no-lint, which is a CLI flag the server has never heard of. The count is summary.error_count rather than the number of findings, because errors are what refuse a page — the same line the lint tool draws when it is run by hand, exiting non-zero on error_count alone. A report can arrive with warnings beside its errors, and counting those too would send the caller hunting for blockers that are not there. A message that does not parse still gets the hint: the escape hatch is the half of it they cannot get anywhere else, and withholding it over a missing number helps nobody. The hint names no page. Every write path submits exactly one page, so a finding's slide_number is its position inside that submission and is always 1 — which is not the page the caller is looking for. On +create it is actively wrong: it would read "on slide 1" next to a progress line saying "adding slide 2/3 failed". The page number has one source, and it is that line. Findings that did not refuse the write come back the other way. The backend returns them in an issues field on a response that succeeded, and all four shortcuts now pass that field through untouched rather than dropping it. It only ever arrives on a page that was written: anything serious enough to refuse the write left as the error above, carrying the same report. Dropping it would leave the caller believing the deck says exactly what they wrote, with no way to learn otherwise short of looking at the rendered page. It is passed through rather than reformatted so that one field reads the same however the page was written. The reference docs say which of the two a caller is holding. A refusal is 4000153 and the page is not there; an issues field means the page is there and the backend still had something to say about it. Both carry the same report, so the docs describe the shape once and link to it from each command. +create keeps adding its pages one at a time, so a refusal there can arrive with the presentation and some of its pages already written. It is reported as such: the error carries the lint report and, next to it, which page was refused and how many landed before it, so the retry adds the rest instead of building a second deck. +replace-pages does the same for the items in its plan. A batch that was told to keep going reports its failures only through the per-item records, so those carry the report, the code and the flag hint as well; a record built from the error's message alone would have named neither the refusal nor the way past it. The position stays on the returning path, where it is the only thing that says how far the batch got — beside a per-item record it would describe a batch that did not stop. Tests assert on the wire — the body the stub actually received — rather than on the builder's return value, so a command that stops calling its own builder still fails. --- shortcuts/slides/slides_add_slide.go | 14 +- shortcuts/slides/slides_create.go | 64 ++- shortcuts/slides/slides_create_test.go | 121 +++-- shortcuts/slides/slides_lint_error.go | 120 +++++ shortcuts/slides/slides_lint_error_test.go | 301 +++++++++++++ shortcuts/slides/slides_lint_param.go | 66 +++ shortcuts/slides/slides_lint_param_test.go | 425 ++++++++++++++++++ shortcuts/slides/slides_replace_pages.go | 71 ++- shortcuts/slides/slides_replace_pages_test.go | 94 ++++ shortcuts/slides/slides_replace_slide.go | 27 +- shortcuts/slides/slides_replace_slide_test.go | 41 ++ shortcuts/slides/slides_update_slide.go | 24 +- shortcuts/slides/slides_update_slide_test.go | 65 +++ .../references/cli/lark-slides-add-slide.md | 6 +- .../references/cli/lark-slides-create.md | 5 +- .../cli/lark-slides-replace-slide.md | 3 + .../cli/lark-slides-update-slide.md | 3 + .../references/workflow/error-handling.md | 1 + 18 files changed, 1363 insertions(+), 88 deletions(-) create mode 100644 shortcuts/slides/slides_lint_error.go create mode 100644 shortcuts/slides/slides_lint_error_test.go create mode 100644 shortcuts/slides/slides_lint_param.go create mode 100644 shortcuts/slides/slides_lint_param_test.go diff --git a/shortcuts/slides/slides_add_slide.go b/shortcuts/slides/slides_add_slide.go index d20ade334c..99b05c4786 100644 --- a/shortcuts/slides/slides_add_slide.go +++ b/shortcuts/slides/slides_add_slide.go @@ -53,6 +53,7 @@ var SlidesAddSlide = common.Shortcut{ {Name: "slide", Desc: "one complete XML document", Required: true, Input: []string{common.File, common.Stdin}}, {Name: "before-slide-id", Desc: "insert before this slide_id (default: append after the last page)"}, {Name: "revision-id", Type: "int", Default: "-1", Desc: "presentation revision (-1 = latest; pass a specific number for optimistic locking)"}, + noLintFlag(), }, Tips: []string{ " placeholders resolve against the current directory, not the directory of the --slide file, and are deduplicated per call: a page-by-page loop re-uploads a shared image once per page, so upload it once with slides +media-upload and reuse the file_token instead.", @@ -139,7 +140,7 @@ var SlidesAddSlide = common.Shortcut{ )). Desc(fmt.Sprintf("[%d/%d] Add page%s", step, total, descSuffix)). Params(addSlideQuery(runtime)). - Body(addSlideBody(slideXML, runtime.Str("before-slide-id"))) + Body(addSlideBody(slideXML, runtime.Str("before-slide-id"), runtime)) return dry.Set("images_to_upload", len(placeholders)) }, @@ -181,7 +182,7 @@ var SlidesAddSlide = common.Shortcut{ validate.EncodePathSegment(presentationID), ), addSlideQuery(runtime), - addSlideBody(slideXML, beforeSlideID), + addSlideBody(slideXML, beforeSlideID, runtime), ) if err != nil { if len(placeholders) > 0 { @@ -189,7 +190,10 @@ var SlidesAddSlide = common.Shortcut{ // a retry silently uploads a second copy of every file. err = appendSlidesProgressHint(err, fmt.Sprintf("%d image(s) were uploaded before the page failed; re-running will upload them again", len(placeholders))) } - return enrichSlidesReplaceError(err) + // Lint first: it names the actual finding, and enrichSlidesReplaceError + // only fills an empty hint, so its generic checklist stays out of the + // way when the backend already said what was wrong. + return enrichSlidesReplaceError(enrichSlidesLintError(err)) } slideID := common.GetString(data, "slide_id") @@ -234,12 +238,12 @@ func addSlideQuery(runtime *common.RuntimeContext) map[string]interface{} { // addSlideBody builds the request body shared by dry-run and execute. // before_slide_id is omitted when empty: the backend appends to the end only // if the key is absent, and an empty string is rejected as an unknown slide. -func addSlideBody(slideXML, beforeSlideID string) map[string]interface{} { +func addSlideBody(slideXML, beforeSlideID string, runtime *common.RuntimeContext) map[string]interface{} { body := map[string]interface{}{ "slide": map[string]interface{}{"content": slideXML}, } if id := strings.TrimSpace(beforeSlideID); id != "" { body["before_slide_id"] = id } - return body + return withLintXML(body, runtime) } diff --git a/shortcuts/slides/slides_create.go b/shortcuts/slides/slides_create.go index 56b5684676..3486e43a13 100644 --- a/shortcuts/slides/slides_create.go +++ b/shortcuts/slides/slides_create.go @@ -23,6 +23,12 @@ const ( ) // SlidesCreate creates a new Lark Slides presentation with bot auto-grant. +// +// The presentation is created first as an empty shell, then the pages are added +// one at a time. Each page is linted on its own way in, so a bad page is refused +// with the findings for that page — and the run stops there, leaving the +// presentation and the pages added before it. The error says so, so the caller +// knows what exists and where the run stopped. var SlidesCreate = common.Shortcut{ Service: "slides", Command: "+create", @@ -44,6 +50,7 @@ var SlidesCreate = common.Shortcut{ // by the framework, so it is not spelled out in Desc. {Name: "slides", Desc: "slide content JSON array (each element is a XML string, max 10; for more pages, create first then add them one at a time with slides +add-slide). placeholders are auto-uploaded and replaced with file_token.", Input: []string{common.File, common.Stdin}}, {Name: "slide", Type: "string_array", Desc: "one complete XML document, or @path to read one from a file; repeat once per page (max 10) and the CLI assembles the array for you, so no JSON escaping is needed. placeholders are handled as with --slides. Mutually exclusive with --slides."}, + noLintFlag(), }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { slides, param, err := createSlideContents(runtime) @@ -66,16 +73,25 @@ var SlidesCreate = common.Shortcut{ createBody := map[string]interface{}{ "xml_presentation": map[string]interface{}{"content": buildPresentationXML(title)}, } + placeholders := extractImagePlaceholderPaths(slides) + + // The note belongs to the create step, which is what the grant follows. + // Adding it at the end instead would land on whichever step happened to + // be last and overwrite that step's own description. + botNote := "" + if runtime.IsBot() { + botNote = " After creation succeeds in bot mode, the CLI will also try to grant the current CLI user full_access on the new presentation." + } dry := common.NewDryRunAPI() if len(slides) == 0 { dry.Desc("Create empty presentation"). POST("/open-apis/slides_ai/v1/xml_presentations"). + Desc(strings.TrimSpace(botNote)). Body(createBody) } else { n := len(slides) - placeholders := extractImagePlaceholderPaths(slides) total := n + 1 + len(placeholders) descSuffix := "" @@ -84,7 +100,7 @@ var SlidesCreate = common.Shortcut{ } dry.Desc(fmt.Sprintf("Create presentation%s + add %d slide(s)", descSuffix, n)). POST("/open-apis/slides_ai/v1/xml_presentations"). - Desc(fmt.Sprintf("[1/%d] Create presentation", total)). + Desc(fmt.Sprintf("[1/%d] Create presentation.%s", total, botNote)). Body(createBody) // Upload steps come right after creation so they can use the new @@ -101,15 +117,11 @@ var SlidesCreate = common.Shortcut{ for i, slideXML := range slides { dry.POST("/open-apis/slides_ai/v1/xml_presentations//slide"). Desc(fmt.Sprintf("[%d/%d] Add slide %d%s", slideStepStart+i, total, i+1, slideDescSuffix)). - Body(map[string]interface{}{ - "slide": map[string]interface{}{"content": slideXML}, - }) + Params(createSlideQuery()). + Body(createSlideBody(slideXML, runtime)) } } - if runtime.IsBot() { - dry.Desc("After creation succeeds in bot mode, the CLI will also try to grant the current CLI user full_access on the new presentation.") - } return dry }, Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { @@ -121,8 +133,10 @@ var SlidesCreate = common.Shortcut{ if err != nil { return err } + placeholders := extractImagePlaceholderPaths(slides) - // Step 1: Create presentation + // Step 1: Create presentation. The shell carries no page, so there is + // nothing here for the server to lint and no lint switch to send. data, err := runtime.CallAPITyped( "POST", "/open-apis/slides_ai/v1/xml_presentations", @@ -158,7 +172,6 @@ var SlidesCreate = common.Shortcut{ // Step 1.5: Upload any @path placeholders, then rewrite slide XML // with the resulting file_tokens. Uploads run after creation so // they can use the new presentation_id as parent_node. - placeholders := extractImagePlaceholderPaths(slides) if len(placeholders) > 0 { tokens, uploaded, err := uploadSlidesPlaceholders(runtime, presentationID, placeholders, param) if err != nil { @@ -170,6 +183,9 @@ var SlidesCreate = common.Shortcut{ result["images_uploaded"] = uploaded } + // Each page is linted on its way in, so the first refusal stops the + // run with the pages before it already on the server — which is what + // the progress hint on the error spells out. slideURL := fmt.Sprintf( "/open-apis/slides_ai/v1/xml_presentations/%s/slide", validate.EncodePathSegment(presentationID), @@ -181,13 +197,11 @@ var SlidesCreate = common.Shortcut{ slideData, err := runtime.CallAPITyped( "POST", slideURL, - map[string]interface{}{"revision_id": -1}, - map[string]interface{}{ - "slide": map[string]interface{}{"content": slideXML}, - }, + createSlideQuery(), + createSlideBody(slideXML, runtime), ) 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)) + return appendSlidesProgressHint(enrichSlidesLintError(err), fmt.Sprintf("adding slide %d/%d failed; presentation %s was created, %d slide(s) added before failure", i+1, len(slides), presentationID, i)) } sid := common.GetString(slideData, "slide_id") if sid != "" { @@ -352,6 +366,26 @@ func effectiveTitle(title string) string { return title } +// createSlideQuery builds the query for the per-page calls +create makes after +// the presentation exists. revision_id is pinned to -1 (latest) rather than +// exposed: the deck was created by this same command a moment ago, so there is +// no earlier revision a caller could sensibly target. +func createSlideQuery() map[string]interface{} { + return map[string]interface{}{"revision_id": -1} +} + +// createSlideBody builds the per-page body shared by dry-run and execute, so +// the two cannot drift on the lint switch the way two literals would. +// +// The presentation-create call has no body of its own to stamp: it sends the +// title-only shell from buildPresentationXML, and there is no +// page in it for the server to lint. +func createSlideBody(slideXML string, runtime *common.RuntimeContext) map[string]interface{} { + return withLintXML(map[string]interface{}{ + "slide": map[string]interface{}{"content": slideXML}, + }, runtime) +} + // buildPresentationXML builds the minimal XML for a new empty presentation. func buildPresentationXML(title string) string { escapedTitle := xmlEscape(title) diff --git a/shortcuts/slides/slides_create_test.go b/shortcuts/slides/slides_create_test.go index 87249740a2..d676dfb727 100644 --- a/shortcuts/slides/slides_create_test.go +++ b/shortcuts/slides/slides_create_test.go @@ -298,42 +298,7 @@ func TestSlidesCreateWithSlides(t *testing.T) { t.Parallel() f, stdout, _, 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, - "msg": "ok", - "data": map[string]interface{}{ - "xml_presentation_id": "pres_with_slides", - "revision_id": 1, - }, - }, - }) - reg.Register(&httpmock.Stub{ - Method: "POST", - URL: "/open-apis/slides_ai/v1/xml_presentations/pres_with_slides/slide", - Body: map[string]interface{}{ - "code": 0, - "msg": "ok", - "data": map[string]interface{}{ - "slide_id": "slide_001", - "revision_id": 2, - }, - }, - }) - reg.Register(&httpmock.Stub{ - Method: "POST", - URL: "/open-apis/slides_ai/v1/xml_presentations/pres_with_slides/slide", - Body: map[string]interface{}{ - "code": 0, - "msg": "ok", - "data": map[string]interface{}{ - "slide_id": "slide_002", - "revision_id": 3, - }, - }, - }) + createStubPresentation(t, reg, "pres_with_slides", 2) slidesJSON := `["",""]` err := runSlidesCreateShortcut(t, f, stdout, []string{ @@ -354,14 +319,18 @@ func TestSlidesCreateWithSlides(t *testing.T) { if !ok || len(slideIDs) != 2 { t.Fatalf("slide_ids = %v, want 2 elements", data["slide_ids"]) } - if slideIDs[0] != "slide_001" || slideIDs[1] != "slide_002" { - t.Fatalf("slide_ids = %v, want [slide_001, slide_002]", slideIDs) + if slideIDs[0] != "s_1" || slideIDs[1] != "s_2" { + t.Fatalf("slide_ids = %v, want [s_1, s_2]", slideIDs) } if data["slides_added"] != float64(2) { t.Fatalf("slides_added = %v, want 2", data["slides_added"]) } } +// TestSlidesCreatePreservesSchemaIssues keeps the advisories from every call +// that produced them. Each page is judged by its own call, so the per-page +// findings are collected under slide_issues with the page they belong to, and +// the presentation-level ones stay separate. func TestSlidesCreatePreservesSchemaIssues(t *testing.T) { t.Parallel() @@ -412,6 +381,70 @@ func TestSlidesCreatePreservesSchemaIssues(t *testing.T) { } } +// TestSlidesCreateRefusedPageSaysWhatLanded is what a lint refusal mid-deck has +// to tell the caller. The pages go in one at a time, so page 2 is refused with +// page 1 already on the server and a presentation that exists — the run cannot +// undo that, so the error has to name the presentation and say how far it got, +// or the caller retries into a duplicate deck. +func TestSlidesCreateRefusedPageSaysWhatLanded(t *testing.T) { + t.Parallel() + + f, stdout, _, 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_refused", "revision_id": 1}, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_refused/slide", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{"slide_id": "s_1", "revision_id": 2}}, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_refused/slide", + Body: map[string]interface{}{"code": 4000153, "msg": lintBlockMessage}, + }) + + page1 := `` + page2 := `` + err := runSlidesCreateShortcut(t, f, stdout, []string{ + "+create", + "--title", "Partial", + "--slide", page1, + "--slide", page2, + "--as", "user", + }) + if err == nil { + t.Fatal("expected the refused page to surface, got nil") + } + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected a typed errs.* error, got %v", err) + } + if p.Category != errs.CategoryAPI { + t.Fatalf("category = %q, want %q", p.Category, errs.CategoryAPI) + } + // The lint report reaches the caller verbatim, so the findings are there to + // read and parse the same way `lark-cli api` would deliver them. + if p.Message != lintBlockMessage { + t.Fatalf("message = %q, want the lint report verbatim", p.Message) + } + // Both halves of the hint matter: how to fix the page, and what already + // exists so the retry adds the rest instead of starting over. + if !strings.Contains(p.Hint, lintRemediationHint) { + t.Fatalf("hint = %q, want the lint remediation wording", p.Hint) + } + for _, want := range []string{"pres_refused", "slide 2/2", "1 slide(s) added"} { + if !strings.Contains(p.Hint, want) { + t.Fatalf("hint lost %q, got: %s", want, p.Hint) + } + } +} + // TestSlidesCreateWithSlidesPartialFailure verifies error reporting when a slide fails to create. func TestSlidesCreateWithSlidesPartialFailure(t *testing.T) { t.Parallel() @@ -481,14 +514,10 @@ func TestSlidesCreateWithSlidesPartialFailure(t *testing.T) { // The presentation was created but a slide add failed; the recovery hint // carries the partial-progress context (which presentation exists, how many // slides landed) so the caller can resume without recreating. - if !strings.Contains(p.Hint, "pres_partial") { - t.Fatalf("hint should contain presentation ID, got: %s", p.Hint) - } - if !strings.Contains(p.Hint, "slide 2/2") { - t.Fatalf("hint should indicate slide 2/2 failed, got: %s", p.Hint) - } - if !strings.Contains(p.Hint, "1 slide(s) added") { - t.Fatalf("hint should report 1 slide added before failure, got: %s", p.Hint) + for _, want := range []string{"pres_partial", "slide 2/2", "1 slide(s) added"} { + if !strings.Contains(p.Hint, want) { + t.Fatalf("hint lost %q, got: %s", want, p.Hint) + } } } diff --git a/shortcuts/slides/slides_lint_error.go b/shortcuts/slides/slides_lint_error.go new file mode 100644 index 0000000000..bf53a0e645 --- /dev/null +++ b/shortcuts/slides/slides_lint_error.go @@ -0,0 +1,120 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package slides + +import ( + "encoding/json" + "fmt" + "strings" + + "github.com/larksuite/cli/errs" +) + +// lintBlockedCode is the code the slide engine answers a layout-lint refusal +// with. It is the engine's own, not a generic validation code, so it identifies +// the refusal on its own and nothing about the message has to be guessed at. +const lintBlockedCode = 4000153 + +// lintBlockDetail is the part of the lint report this file reads. The message is +// the report the lint tool produced, forwarded whole, and the report is large: +// a per-level split, a per-slide grouping, and a set of measurements that differ +// from rule to rule. None of that is decoded here because nothing here would do +// anything with it — it reaches the caller through the message itself, which is +// left verbatim. What is decoded is the two facts the hint is built from. +type lintBlockDetail struct { + Summary lintBlockSummary `json:"summary"` + // SchemaIssues is the schema-sanitize report, which the engine merges into + // the lint report as its own top-level field so the message stays parseable + // as a whole. + SchemaIssues string `json:"schema_issues"` +} + +// lintBlockSummary is the report's own verdict over the page. Only error_count +// is read: it is the count that refused the write, and it is the same line the +// lint script draws when it is run by hand — it exits non-zero on error_count +// and says nothing about warnings or infos. A report can therefore arrive with +// warnings in it and still be a refusal that only the errors caused, so +// counting findings instead would overstate what has to be fixed. +type lintBlockSummary struct { + ErrorCount int `json:"error_count"` +} + +// lintRemediationHint names the escape hatch. The backend cannot write this line +// itself: --no-lint is a CLI flag and the server has never heard of it. +// The opening says "the page" and not "nothing": on +create the refusal can +// arrive after the presentation and some of its pages already exist, and a +// blanket "nothing was written" would contradict the progress hint sitting next +// to it. One page is the unit the gate refuses on every path, so it is what this +// says. +const lintRemediationHint = "the page was not written. Fix the error-level findings in the message and retry;" + + " if the lint is wrong about a page that has to ship as-is, re-run with --no-lint" + +// enrichSlidesLintError adds what the CLI knows about a layout-lint refusal, and +// leaves every other error alone. +// +// The refusal arrives as the lint report in the message field and it stays there +// verbatim. The same refusal reaches callers two ways: through these shortcuts, +// and through `lark-cli api` calling the endpoint directly, where nothing +// rewrites it. Rendering the report to prose here would mean the same refusal +// has two different message formats depending on which command produced it, so a +// caller could not parse one field one way. Verbatim also matches +// ERROR_CONTRACT.md's "propagate typed errors unchanged": the server named the +// offending element, the page it sits on, and how to fix it. +// +// What the backend cannot say is added as a hint instead: how many findings +// refused the page, that the page did not land, and --no-lint, which is a CLI +// flag the server has never heard of. That is the one thing this layer knows and +// the report does not, which is the only reason this enrichment exists at all. +// +// Detection is by code. Every other error is returned untouched, so the helper is +// safe on write paths whose backend does not lint. +func enrichSlidesLintError(err error) error { + if err == nil { + return nil + } + p, ok := errs.ProblemOf(err) + if !ok || p.Code != lintBlockedCode { + return err + } + // Reuses the progress-hint helper for its append-preserving-classification + // behaviour, not for its orchestration meaning. + return appendSlidesProgressHint(err, lintBlockHint(parseLintBlockDetail(p.Message))) +} + +// parseLintBlockDetail reads the two fields the hint needs out of the report. A +// message that does not decode is not an error here: the code already said this +// is a refusal, and the hint is worth writing even without a count in it — the +// escape hatch is the half of it the caller cannot get anywhere else. +func parseLintBlockDetail(message string) lintBlockDetail { + var detail lintBlockDetail + if err := json.Unmarshal([]byte(strings.TrimSpace(message)), &detail); err != nil { + return lintBlockDetail{} + } + return detail +} + +// lintBlockHint summarises the refusal without restating it. The findings +// themselves stay in the message, so this says only what a caller needs before +// deciding whether to read them: how many refused the write, and what to do next. +// +// It deliberately does not name a page. Every write path submits one page, so +// the slide_number on each finding is its position inside that submission and is +// always 1 — which is not the page the caller is looking for. On +create it is +// actively wrong: the progress hint sitting next to this one says "adding slide +// 2/3 failed", and a "on slide 1" next to it sends the reader to the wrong page. +// The real page number has exactly one source, and it is that other line. +// +// The schema findings get their own note because they travel in their own field +// and are easy to miss inside a long report. +func lintBlockHint(detail lintBlockDetail) string { + head := "xml lint blocked" + if detail.Summary.ErrorCount > 0 { + head = fmt.Sprintf("%s: %d error(s)", head, detail.Summary.ErrorCount) + } + parts := []string{head} + if detail.SchemaIssues != "" { + parts = append(parts, "the message also carries schema_issues") + } + return strings.Join(parts, ", ") + ". " + lintRemediationHint +} diff --git a/shortcuts/slides/slides_lint_error_test.go b/shortcuts/slides/slides_lint_error_test.go new file mode 100644 index 0000000000..15ff7a4306 --- /dev/null +++ b/shortcuts/slides/slides_lint_error_test.go @@ -0,0 +1,301 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package slides + +import ( + "strings" + "testing" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/httpmock" +) + +// lintBlockMessage is the report the backend forwards when the layout lint +// refuses a write — the same document the lint script writes when it is run by +// hand. Written out as literal JSON rather than marshalled from the parser's own +// structs so a change to the server's field names has to be noticed here instead +// of being absorbed silently by both sides at once. +// +// It carries a warning alongside the two errors, because that is the report a +// real refusal produces and it is what proves the hint counts the errors rather +// than the findings. The second error carries a measurement and a +// related_objects pair, which this parser has no field for and which differ from +// rule to rule — they are here because the verbatim assertion below is what +// proves the CLI does not have to model a finding in order to deliver one. +const lintBlockMessage = `{"schema_version":"2.0","tool":"xml_lint","file":null,` + + `"slide_size":{"width":960,"height":540},` + + `"summary":{"slide_count":1,"error_count":2,"warning_count":1,"info_count":0,` + + `"status":"blocked","release_ready":false,"screenshot_review_required":true},` + + `"document":{"errors":[],"warnings":[],"infos":[]},` + + `"slides":[{"slide_number":1,"status":"blocked","element_count":7,"errors":[` + + `{"level":"error","code":"text_overflow","slide_number":1,"path":"presentation/slide/data/shape[2]",` + + `"message":"text is 340pt tall in a 180pt shape","hint":"shorten the text or raise height"},` + + `{"level":"error","code":"bbox_overlap","slide_number":1,` + + `"message":"element ends at x=1080, slide is 960 wide",` + + `"measurement":{"intersection_area":94.848,"width":9.88,"height":9.6},` + + `"related_objects":[{"element_id":"bhT","xml_path":"slide[1]/data/shape[3]",` + + `"bbox":{"x":36,"y":36,"width":9.88,"height":9.6}}]}],` + + `"warnings":[{"level":"warning","code":"sparse_slide_content","slide_number":1,` + + `"message":"coverage 12.8% below 15.0%"}],"infos":[]}]}` + +// lintBlockedError builds the error the API layer produces for a refusal, which +// is a typed error carrying the engine's code and the report as its message. +func lintBlockedError(message string) error { + return errs.NewAPIError(errs.SubtypeInvalidParameters, "%s", message).WithCode(lintBlockedCode) +} + +// TestLintErrorKeepsTheReportVerbatimAndAddsTheEscapeHatch pins the division of +// labour: the finding is the server's to word and travels untouched, the flag is +// the CLI's to mention and travels in the hint. +// +// The message is asserted byte-for-byte because the same refusal also reaches +// callers through `lark-cli api`, where nothing rewrites it. Rendering it here +// would give one refusal two message formats depending on which command produced +// it, and a caller could not parse the field one way. +func TestLintErrorKeepsTheReportVerbatimAndAddsTheEscapeHatch(t *testing.T) { + t.Parallel() + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_lint/slide", + Body: map[string]interface{}{"code": lintBlockedCode, "msg": lintBlockMessage}, + }) + + err := runSlidesShortcut(t, f, stdout, SlidesAddSlide, []string{ + "+add-slide", + "--presentation", "pres_lint", + "--slide", testPageXML, + "--as", "user", + }) + if err == nil { + t.Fatal("expected the lint block to surface as an error") + } + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("err = %v, want typed problem metadata", err) + } + if problem.Category != errs.CategoryAPI { + t.Fatalf("Category = %q, want %q: the backend rejection must survive enrichment", + problem.Category, errs.CategoryAPI) + } + // Byte-for-byte, not "contains": every field of every finding reaches the + // caller in the shape the server chose, so the same parse works whether the + // refusal came through a shortcut or through `lark-cli api`. + if problem.Message != lintBlockMessage { + t.Fatalf("Message = %q, want the server document verbatim:\n%q", problem.Message, lintBlockMessage) + } + // The hint carries only what the document cannot say for itself. + for _, want := range []string{ + "xml lint blocked: 2 error(s)", + "--no-lint", + } { + if !strings.Contains(problem.Hint, want) { + t.Fatalf("Hint lost %q\ngot: %q", want, problem.Hint) + } + } + // The hint summarises, it does not restate: repeating the findings would put + // two copies of the same list in one error, and the copies drift. + if strings.Contains(problem.Hint, "text is 340pt tall") { + t.Fatalf("Hint = %q, want a summary rather than a second copy of the findings", problem.Hint) + } +} + +// TestLintErrorCountsOnlyTheErrorsThatRefusedThePage covers a report spread +// across levels. Only errors refuse a write — the same line the lint script +// draws when it is run by hand — so a report that also carries a warning and an +// info is still a one-error refusal, and saying "3" would send the caller +// hunting for two more blockers that are not there. +func TestLintErrorCountsOnlyTheErrorsThatRefusedThePage(t *testing.T) { + t.Parallel() + + msg := `{"schema_version":"2.0","tool":"xml_lint",` + + `"summary":{"slide_count":3,"error_count":1,"warning_count":1,"info_count":1,` + + `"status":"blocked","release_ready":false,"screenshot_review_required":true},` + + `"schema_issues":"dropped unknown attribute shadow on shape[1]",` + + `"slides":[{"slide_number":1,"status":"blocked","errors":[` + + `{"level":"error","code":"out_of_bounds","message":"element ends at x=1080, slide is 960 wide"}],` + + `"warnings":[{"level":"warning","code":"sparse_slide_content","message":"coverage 12.8% below 15.0%"}],` + + `"infos":[{"level":"info","code":"table_resolved_size_mismatch",` + + `"message":"table is 12pt narrower than declared"}]}]}` + + enriched := enrichSlidesLintError(lintBlockedError(msg)) + problem, ok := errs.ProblemOf(enriched) + if !ok { + t.Fatalf("err = %v, want typed problem metadata", enriched) + } + if problem.Message != msg { + t.Fatalf("Message = %q, want the server document verbatim", problem.Message) + } + for _, want := range []string{ + // One error refused the page; the warning and the info came along. + "xml lint blocked: 1 error(s)", + // schema_issues travels in its own field, so a caller reading the message + // top to bottom can miss it. The hint says it is there. + "the message also carries schema_issues", + } { + if !strings.Contains(problem.Hint, want) { + t.Fatalf("Hint lost %q\ngot: %q", want, problem.Hint) + } + } + if strings.Contains(problem.Hint, "3 ") { + t.Fatalf("Hint = %q, want the error count rather than the finding count", problem.Hint) + } +} + +// TestLintErrorLeavesOtherFailuresAlone guards the detection. The helper runs on +// every write path including ones the backend does not lint, so a false positive +// would staple "re-run with --no-lint" onto unrelated failures — advice that does +// nothing and sends the caller down the wrong path. +func TestLintErrorLeavesOtherFailuresAlone(t *testing.T) { + t.Parallel() + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_other/slide", + Body: map[string]interface{}{"code": 3350001, "msg": "invalid slide xml"}, + }) + + err := runSlidesShortcut(t, f, stdout, SlidesAddSlide, []string{ + "+add-slide", + "--presentation", "pres_other", + "--slide", testPageXML, + "--as", "user", + }) + if err == nil { + t.Fatal("expected the backend rejection to surface") + } + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("err = %v, want typed problem metadata", err) + } + if strings.Contains(problem.Hint, "--no-lint") { + t.Fatalf("Hint = %q, want no lint advice on a non-lint failure", problem.Hint) + } + // The 3350001 checklist still lands, so lint enrichment running first has not + // displaced the existing fallback. + if !strings.Contains(problem.Hint, "block_id not found in current slide") { + t.Fatalf("Hint = %q, want the existing 3350001 checklist", problem.Hint) + } +} + +// TestLintErrorClaimsOnlyItsOwnCode is the other half of the detection. The +// backend reports other failures through the same field as JSON, and one of them +// could one day look like a lint report; the code is what says this refusal came +// from the lint gate, so a message alone must not be enough to claim it. +func TestLintErrorClaimsOnlyItsOwnCode(t *testing.T) { + t.Parallel() + + for name, msg := range map[string]string{ + "a report under a foreign code": lintBlockMessage, + "node server shape": `{"code":"SCHEMA_INVALID","errors":[{"line":3,"detail":"unexpected element"}]}`, + "not json": "invalid param", + } { + t.Run(name, func(t *testing.T) { + original := errs.NewAPIError(errs.SubtypeInvalidParameters, "%s", msg).WithCode(4001000) + enriched := enrichSlidesLintError(original) + problem, ok := errs.ProblemOf(enriched) + if !ok { + t.Fatalf("err = %v, want typed problem metadata", enriched) + } + if problem.Message != msg { + t.Fatalf("Message = %q, want it passed through unchanged", problem.Message) + } + if strings.Contains(problem.Hint, "--no-lint") { + t.Fatalf("Hint = %q, want no lint advice on a message this helper does not own", problem.Hint) + } + }) + } +} + +// TestLintErrorStillHelpsWhenTheReportCannotBeRead covers the code arriving with +// a message this parser cannot decode. The count is the half of the hint that +// needs the report; the escape hatch is the half the caller cannot get anywhere +// else, so dropping the whole hint over a missing number would withhold the only +// thing this layer knows. +func TestLintErrorStillHelpsWhenTheReportCannotBeRead(t *testing.T) { + t.Parallel() + + enriched := enrichSlidesLintError(lintBlockedError("xml lint blocked")) + problem, ok := errs.ProblemOf(enriched) + if !ok { + t.Fatalf("err = %v, want typed problem metadata", enriched) + } + if !strings.Contains(problem.Hint, "--no-lint") { + t.Fatalf("Hint = %q, want the escape hatch even without a readable report", problem.Hint) + } + if strings.Contains(problem.Hint, "error(s)") { + t.Fatalf("Hint = %q, want no invented count when the report did not parse", problem.Hint) + } +} + +// TestLintErrorSurvivesTheImageProgressHint covers the two enrichers landing on +// the same error. Both write to Hint, and a caller that uploaded images needs +// both facts: the pages were rejected, and the uploads already happened. +func TestLintErrorSurvivesTheImageProgressHint(t *testing.T) { + t.Parallel() + + err := lintBlockedError(lintBlockMessage) + enriched := appendSlidesProgressHint(enrichSlidesLintError(err), "2 image(s) were uploaded before the page failed") + + problem, ok := errs.ProblemOf(enriched) + if !ok { + t.Fatalf("err = %v, want typed problem metadata", enriched) + } + if !strings.Contains(problem.Hint, "--no-lint") { + t.Fatalf("Hint = %q, want the lint escape hatch", problem.Hint) + } + if !strings.Contains(problem.Hint, "2 image(s) were uploaded") { + t.Fatalf("Hint = %q, want the upload progress note", problem.Hint) + } +} + +// TestLintErrorScopesTheRefusalToThePage pins the wording against the case that +// exposed it. On +create the refusal can arrive after the deck and some of its +// pages are already on the server, and the orchestration hint says exactly that +// — so a lint hint claiming nothing was written puts two contradictory +// statements in the same message and the caller cannot tell which is true. +func TestLintErrorScopesTheRefusalToThePage(t *testing.T) { + t.Parallel() + + err := lintBlockedError(lintBlockMessage) + // The order +create produces: the lint enricher first, then the progress + // note about what already landed. + enriched := appendSlidesProgressHint(enrichSlidesLintError(err), + "adding slide 2/3 failed; presentation pres_abc was created, 1 slide(s) added before failure") + + problem, ok := errs.ProblemOf(enriched) + if !ok { + t.Fatalf("err = %v, want typed problem metadata", enriched) + } + if strings.Contains(problem.Hint, "nothing was written") { + t.Fatalf("Hint = %q, want no blanket claim that nothing was written: the same hint reports a created presentation and added slides", problem.Hint) + } + if !strings.Contains(problem.Hint, "the page was not written") { + t.Fatalf("Hint = %q, want the refusal scoped to the page the gate rejected", problem.Hint) + } + if !strings.Contains(problem.Hint, "was created") { + t.Fatalf("Hint = %q, want the orchestration progress preserved alongside it", problem.Hint) + } + // The findings in lintBlockMessage carry slide_number 1, because slide_number + // counts inside the submission and every write path submits one page. Naming + // it here would put "on slide 1" next to "adding slide 2/3 failed" and send + // the caller to a page that linted clean. The page number has one source, and + // it is the progress note. + if strings.Contains(problem.Hint, "on slide") { + t.Fatalf("Hint = %q, want no page named by the lint half: slide_number is the position inside the submitted page, not the deck", problem.Hint) + } +} + +// TestLintErrorPassesNilThrough keeps the helper safe to fold into a return +// expression on a path that may not have failed. +func TestLintErrorPassesNilThrough(t *testing.T) { + t.Parallel() + + if got := enrichSlidesLintError(nil); got != nil { + t.Fatalf("enrichSlidesLintError(nil) = %v, want nil", got) + } +} diff --git a/shortcuts/slides/slides_lint_param.go b/shortcuts/slides/slides_lint_param.go new file mode 100644 index 0000000000..54d9313b9a --- /dev/null +++ b/shortcuts/slides/slides_lint_param.go @@ -0,0 +1,66 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package slides + +import "github.com/larksuite/cli/shortcuts/common" + +// The server-side XML lint switch, carried by every shortcut that writes slide +// content: +create, +add-slide, +update-slide and +replace-slide. +// +// The backend lints the page the write produces and refuses it when that page +// fails. Note "produces", not "is handed": +replace-slide submits fragments, and +// the subject of the lint is the assembled page, so a fragment that is valid on +// its own is still refused when it pushes a neighbour off the canvas. The CLI +// asks for the check on every call, so a caller who never touches the flag gets +// the checked path; --no-lint is the escape hatch for the case the lint is wrong +// about a page that must still ship. +// +// Sent explicitly in both directions rather than omitted when on: the parameter +// is newer than internal/registry/meta_data.json, so the server-side default is +// not something this CLI can read anywhere, and a request that states the value +// means the same thing before and after that default ever changes. +const noLintFlagName = "no-lint" + +// lintXMLBodyKey is the wire name the switch binds to, and it travels in the +// request body rather than the query string. +// +// That is not a style choice. A query parameter has to be declared in the +// gateway's own api meta before it is bound to a field, and the published +// definition of these endpoints lists only xml_presentation_id, revision_id +// and idempotency_key. Until that publish happens the gateway drops an +// undeclared parameter on the floor: the request succeeds, the field arrives +// unset, and the server reads it as "lint not requested". Verified against a +// live backend — pages that asked to be linted were written unlinted, with no +// error anywhere to say so. Body fields ride along with the JSON already being +// sent and need no separate registration. +// +// snake_case matches every other body key these endpoints take: slide, +// before_slide_id, parts, comment. +// +// Worth knowing when this is next touched: a wrong name here still fails +// silently, for the same reason it did in the query. Tests and --dry-run only +// prove what the CLI sent, not what was read. +const lintXMLBodyKey = "lint_xml" + +// noLintFlag is the shared flag definition, so the three commands cannot drift +// apart on the name or the wording. +func noLintFlag() common.Flag { + return common.Flag{ + Name: noLintFlagName, + Type: "bool", + Desc: "submit the XML without the server-side lint; by default the server lints every page and rejects the write when it fails", + } +} + +// withLintXML stamps the switch onto a request body and returns it, so the body +// builders stay one-liners and dry-run and execute cannot disagree about the +// value. A nil map is allocated rather than rejected, so a caller with nothing +// else to send still gets a well-formed body. +func withLintXML(body map[string]interface{}, runtime *common.RuntimeContext) map[string]interface{} { + if body == nil { + body = map[string]interface{}{} + } + body[lintXMLBodyKey] = !runtime.Bool(noLintFlagName) + return body +} diff --git a/shortcuts/slides/slides_lint_param_test.go b/shortcuts/slides/slides_lint_param_test.go new file mode 100644 index 0000000000..c2ddc5daf2 --- /dev/null +++ b/shortcuts/slides/slides_lint_param_test.go @@ -0,0 +1,425 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package slides + +import ( + "encoding/json" + "io" + "net/http" + "net/url" + "strconv" + "strings" + "testing" + + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/httpmock" + "github.com/larksuite/cli/shortcuts/common" +) + +// The lint switch is asserted on the wire rather than on the body-builder +// return value. What matters is what the backend receives, and only a real +// request proves the switch reached it: a builder test would still pass if a +// command stopped calling its own builder. +// +// Where it sits on the wire is asserted too, not just its value. The switch +// used to ride the query string, where the gateway dropped it silently because +// its api meta never declared the parameter — the request succeeded and the +// page went in unlinted. A test that only read the value would have been just +// as green then as it is now. + +// captureLintBody records the decoded request body of a stub. +func captureLintBody(t *testing.T, into *map[string]interface{}) func(*http.Request) { + t.Helper() + return func(req *http.Request) { + raw, err := io.ReadAll(req.Body) + if err != nil { + t.Errorf("read request body: %v", err) + return + } + var decoded map[string]interface{} + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Errorf("decode request body %q: %v", raw, err) + return + } + *into = decoded + } +} + +// assertLintSwitch checks the switch is in the body, carries the expected +// value, and left the query alone. +func assertLintSwitch(t *testing.T, body map[string]interface{}, query url.Values, want bool) { + t.Helper() + got, ok := body[lintXMLBodyKey] + if !ok { + t.Fatalf("body = %v, want a %s key", body, lintXMLBodyKey) + } + if got != want { + t.Fatalf("body[%s] = %v, want %v", lintXMLBodyKey, got, want) + } + if q := query.Get(lintXMLBodyKey); q != "" { + t.Fatalf("query carried %s = %q; the gateway does not bind it there, so a value in the query is a silent no-op", + lintXMLBodyKey, q) + } +} + +// TestAddSlideLintXMLTravels pins both directions on +add-slide. +func TestAddSlideLintXMLTravels(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + extra []string + want bool + }{ + {name: "default lints", want: true}, + {name: "--no-lint disables", extra: []string{"--no-lint"}, want: false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + var body map[string]interface{} + var query url.Values + capture := captureLintBody(t, &body) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_abc/slide", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{"slide_id": "slide_001"}}, + OnMatch: func(req *http.Request) { + query = req.URL.Query() + capture(req) + }, + }) + + args := append([]string{ + "+add-slide", + "--presentation", "pres_abc", + "--slide", testPageXML, + "--as", "user", + }, tc.extra...) + if err := runSlidesShortcut(t, f, stdout, SlidesAddSlide, args); err != nil { + t.Fatalf("unexpected error: %v", err) + } + assertLintSwitch(t, body, query, tc.want) + // The switch is added to the body, not substituted for it. + if _, ok := body["slide"]; !ok { + t.Fatalf("body = %v, want the slide payload alongside the switch", body) + } + }) + } +} + +// TestUpdateSlideLintXMLTravels pins both directions on +update-slide, and that +// the switch does not displace the parts payload. +func TestUpdateSlideLintXMLTravels(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + extra []string + want bool + }{ + {name: "default lints", want: true}, + {name: "--no-lint disables", extra: []string{"--no-lint"}, want: false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + var body map[string]interface{} + var query url.Values + capture := captureLintBody(t, &body) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_abc/slide/replace", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{"revision_id": 9}}, + OnMatch: func(req *http.Request) { + query = req.URL.Query() + capture(req) + }, + }) + + args := append([]string{ + "+update-slide", + "--presentation", "pres_abc", + "--slide-id", "bUn", + "--content", testPageXML, + "--as", "user", + }, tc.extra...) + if err := runSlidesShortcut(t, f, stdout, SlidesUpdateSlide, args); err != nil { + t.Fatalf("unexpected error: %v", err) + } + assertLintSwitch(t, body, query, tc.want) + if _, ok := body["parts"]; !ok { + t.Fatalf("body = %v, want the parts payload alongside the switch", body) + } + // slide_id genuinely is a query parameter here, and the gateway does + // declare it. Moving the switch must not have disturbed it. + if got := query.Get("slide_id"); got != "bUn" { + t.Fatalf("slide_id = %q, want bUn", got) + } + }) + } +} + +// TestCreateLintXMLTravels pins both directions on +create. +// +// +create makes the presentation first and then sends the pages one at a time, +// so the switch has to reach every page call — a deck where only the first page +// is linted is the failure this guards against. The create call itself sends the +// title-only shell, which has no page in it to lint, so it carries no switch at +// all. +func TestCreateLintXMLTravels(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + extra []string + want bool + }{ + {name: "default lints", want: true}, + {name: "--no-lint disables", extra: []string{"--no-lint"}, want: false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + var createBody map[string]interface{} + type slideCall struct { + body map[string]interface{} + query url.Values + } + var slideCalls []slideCall + 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_lint", + "revision_id": 1, + }}, + OnMatch: captureLintBody(t, &createBody), + }) + for i := 0; i < 2; i++ { + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_lint/slide", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{ + "slide_id": "s_1", + "revision_id": i + 2, + }}, + OnMatch: func(req *http.Request) { + call := slideCall{query: req.URL.Query()} + captureLintBody(t, &call.body)(req) + slideCalls = append(slideCalls, call) + }, + }) + } + + args := append([]string{ + "+create", + "--title", "Lint", + "--slide", testPageXML, + "--slide", testPageXML, + "--as", "user", + }, tc.extra...) + if err := runSlidesCreateShortcut(t, f, stdout, args); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // The shell holds the title and nothing else, so there is nothing for a + // switch on it to decide. + presentation, _ := createBody["xml_presentation"].(map[string]interface{}) + content, _ := presentation["content"].(string) + if strings.Contains(content, ""}]`, + "--as", "user", + }, tc.extra...) + if err := runSlidesShortcut(t, f, stdout, SlidesReplaceSlide, args); err != nil { + t.Fatalf("unexpected error: %v", err) + } + assertLintSwitch(t, body, query, tc.want) + if _, ok := body["parts"]; !ok { + t.Fatalf("body = %v, want the parts payload alongside the switch", body) + } + }) + } +} + +// TestReplacePagesLintXMLTravels pins both directions on +replace-pages. +// +// This path deletes the old page once the new one lands, so an unlinted create +// here is the only write that can destroy existing content on the strength of a +// page nobody checked. The switch has to reach every item, not just the first, +// for the same reason as on +create. +func TestReplacePagesLintXMLTravels(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + extra []string + want bool + }{ + {name: "default lints", want: true}, + {name: "--no-lint disables", extra: []string{"--no-lint"}, want: false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + type slideCall struct { + body map[string]interface{} + query url.Values + } + var createCalls []slideCall + for i := 0; i < 2; i++ { + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_abc/slide", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{ + "slide_id": "new_1", + "revision_id": i*2 + 11, + }}, + OnMatch: func(req *http.Request) { + call := slideCall{query: req.URL.Query()} + captureLintBody(t, &call.body)(req) + createCalls = append(createCalls, call) + }, + }) + reg.Register(&httpmock.Stub{ + Method: "DELETE", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_abc/slide", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{ + "revision_id": i*2 + 12, + }}, + }) + } + + pages := `[{"slide_id":"old1","content":` + strconv.Quote(testPageXML) + + `},{"slide_id":"old2","content":` + strconv.Quote(testPageXML) + `}]` + args := append([]string{ + "+replace-pages", + "--presentation", "pres_abc", + "--pages", pages, + "--as", "user", + }, tc.extra...) + if err := runSlidesShortcut(t, f, stdout, SlidesReplacePages, args); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(createCalls) != 2 { + t.Fatalf("create calls = %d, want 2", len(createCalls)) + } + for _, call := range createCalls { + assertLintSwitch(t, call.body, call.query, tc.want) + // The switch is added to the body, not substituted for it. + if _, ok := call.body["before_slide_id"]; !ok { + t.Fatalf("body = %v, want before_slide_id alongside the switch", call.body) + } + } + }) + } +} + +// TestLintXMLFlagIsOnEverySlideContentWriter guards the set. A new writer that +// forgets the flag ships pages the backend never checked, and the omission is +// invisible until something renders wrong. +func TestLintXMLFlagIsOnEverySlideContentWriter(t *testing.T) { + t.Parallel() + + for _, sc := range []struct { + name string + want bool + have []string + }{ + {name: "+create", want: true, have: lintFlagNames(SlidesCreate.Flags)}, + {name: "+add-slide", want: true, have: lintFlagNames(SlidesAddSlide.Flags)}, + {name: "+update-slide", want: true, have: lintFlagNames(SlidesUpdateSlide.Flags)}, + // +replace-slide sends fragments rather than a whole , which is + // exactly why it needs the flag: the lint's subject is the page they + // assemble into, so this is the path where a fragment that is correct + // on its own can still break the page. + {name: "+replace-slide", want: true, have: lintFlagNames(SlidesReplaceSlide.Flags)}, + // +replace-pages deletes the old page once the new one lands, so it is + // the one writer where skipping the lint can cost content that already + // existed. + {name: "+replace-pages", want: true, have: lintFlagNames(SlidesReplacePages.Flags)}, + } { + if got := flagListHas(sc.have, noLintFlagName); got != sc.want { + t.Fatalf("%s has --%s = %v, want %v", sc.name, noLintFlagName, got, sc.want) + } + } +} + +func lintFlagNames(flags []common.Flag) []string { + names := make([]string, 0, len(flags)) + for _, f := range flags { + names = append(names, f.Name) + } + return names +} + +func flagListHas(haystack []string, needle string) bool { + for _, s := range haystack { + if s == needle { + return true + } + } + return false +} diff --git a/shortcuts/slides/slides_replace_pages.go b/shortcuts/slides/slides_replace_pages.go index 39e9d53b01..1688fc38ba 100644 --- a/shortcuts/slides/slides_replace_pages.go +++ b/shortcuts/slides/slides_replace_pages.go @@ -33,6 +33,7 @@ var SlidesReplacePages = common.Shortcut{ {Name: "pages", Desc: "JSON array of page replacements (each: {slide_id, content}); supports @file or -", Required: true, Input: []string{common.File, common.Stdin}}, {Name: "continue-on-error", Type: "bool", Desc: "continue with later pages after a create/delete failure; default false"}, {Name: "validate-only", Type: "bool", Desc: "validate input and build the create/delete plan without write calls"}, + noLintFlag(), }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { ref, err := parsePresentationRef(runtime.Str("presentation")) @@ -56,7 +57,7 @@ var SlidesReplacePages = common.Shortcut{ if err != nil { return dry.Set("error", err.Error()) } - appendReplacePagesDryRunCalls(dry, resolved) + appendReplacePagesDryRunCalls(dry, resolved, runtime) return dry. Set("xml_presentation_id", resolved.PresentationID). Set("pages_count", len(resolved.Plan)). @@ -83,6 +84,17 @@ var SlidesReplacePages = common.Shortcut{ results := make([]replacePageResult, 0, len(resolved.Plan)) for i, item := range resolved.Plan { result, err := replaceOnePage(runtime, resolved.PresentationID, item, revisionID) + if err != nil { + // The lint refusal is enriched here for the same reason as on every + // other write path: --no-lint is a CLI flag the backend cannot name. + // It happens before the --continue-on-error branch below because a + // refused page needs the same recovery information either way; the + // batch that keeps going only ever sees the per-item record, so + // enriching inside the returning branch would have hidden the flag + // from the callers most likely to have several pages to fix. + err = enrichSlidesLintError(err) + recordReplacePageError(&result, err) + } results = append(results, result) if result.RevisionID != nil { revisionID = *result.RevisionID @@ -91,6 +103,11 @@ var SlidesReplacePages = common.Shortcut{ if runtime.Bool("continue-on-error") { continue } + // The progress hint wrapping the error carries the page, which the + // lint hint deliberately does not — inside one item the report + // always says slide 1. It is appended only on the returning path: + // the per-item records already say which items ran and how each + // ended, so the batch that keeps going has no use for a position. return appendSlidesProgressHint(err, fmt.Sprintf("slides +replace-pages stopped at item %d/%d; %d page(s) completed before failure; old page is kept when create failed", i+1, len(resolved.Plan), countReplacedPages(results))) } } @@ -136,10 +153,27 @@ type replacePageResult struct { NewSlideID string Status string Error string + // ErrorCode and ErrorHint carry the typed metadata that Error alone drops: + // Problem.Error() renders the message and nothing else, so an item recorded + // from the string would arrive without the code that identifies it or the + // hint that says how to recover from it. + ErrorCode int + ErrorHint string Issues interface{} RevisionID *int } +// recordReplacePageError is the one place a failed item is written down, so the +// stop-on-error path and the --continue-on-error path cannot record a failure +// differently. Untyped errors keep only their message, which is all they have. +func recordReplacePageError(result *replacePageResult, err error) { + result.Error = err.Error() + if p, ok := errs.ProblemOf(err); ok { + result.ErrorCode = p.Code + result.ErrorHint = p.Hint + } +} + func prepareReplacePages(runtime *common.RuntimeContext) (*replacePagesPrepared, error) { ref, err := parsePresentationRef(runtime.Str("presentation")) if err != nil { @@ -240,16 +274,13 @@ func buildReplacePagesPlan(pages []replacePageInput) ([]replacePagePlanItem, err return plan, nil } -func appendReplacePagesDryRunCalls(dry *common.DryRunAPI, resolved *replacePagesPrepared) { +func appendReplacePagesDryRunCalls(dry *common.DryRunAPI, resolved *replacePagesPrepared, runtime *common.RuntimeContext) { dry.Desc("Batch replace pages in-place: create each new page before old page, then delete old page (not atomic)") for i, item := range resolved.Plan { dry.POST(fmt.Sprintf("/open-apis/slides_ai/v1/xml_presentations/%s/slide", validate.EncodePathSegment(resolved.PresentationID))). Desc(fmt.Sprintf("[%d/%d] Create replacement before old slide %s", i*2+1, len(resolved.Plan)*2, item.OldSlideID)). Params(map[string]interface{}{"revision_id": ""}). - Body(map[string]interface{}{ - "slide": map[string]interface{}{"content": item.Content}, - "before_slide_id": item.OldSlideID, - }) + Body(replacePageCreateBody(item, runtime)) dry.DELETE(fmt.Sprintf("/open-apis/slides_ai/v1/xml_presentations/%s/slide", validate.EncodePathSegment(resolved.PresentationID))). Desc(fmt.Sprintf("[%d/%d] Delete old slide %s after create succeeds", i*2+2, len(resolved.Plan)*2, item.OldSlideID)). Params(map[string]interface{}{ @@ -259,6 +290,20 @@ func appendReplacePagesDryRunCalls(dry *common.DryRunAPI, resolved *replacePages } } +// replacePageCreateBody builds the create request body shared by dry-run and +// execute, so the previewed request and the sent one cannot drift apart on +// whether the page gets linted. +func replacePageCreateBody(item replacePagePlanItem, runtime *common.RuntimeContext) map[string]interface{} { + return withLintXML(map[string]interface{}{ + "slide": map[string]interface{}{"content": item.Content}, + "before_slide_id": item.OldSlideID, + }, runtime) +} + +// replaceOnePage reports how far the item got through Status and returns the +// failure itself untouched. Recording it belongs to the caller, which is where +// the error is enriched — writing the message here as well would have produced +// a per-item record built from a different error than the one the caller sees. func replaceOnePage(runtime *common.RuntimeContext, presentationID string, item replacePagePlanItem, revisionID int) (replacePageResult, error) { result := replacePageResult{ OldSlideID: item.OldSlideID, @@ -269,21 +314,16 @@ func replaceOnePage(runtime *common.RuntimeContext, presentationID string, item "POST", slideURL, map[string]interface{}{"revision_id": revisionID}, - map[string]interface{}{ - "slide": map[string]interface{}{"content": item.Content}, - "before_slide_id": item.OldSlideID, - }, + replacePageCreateBody(item, runtime), ) if err != nil { result.Status = "create_failed" - result.Error = err.Error() return result, err } newSlideID := common.GetString(createData, "slide_id") if newSlideID == "" { err := errs.NewInternalError(errs.SubtypeInvalidResponse, "slide.create returned no slide_id for replacement of slide_id %q", item.OldSlideID) result.Status = "create_failed" - result.Error = err.Error() return result, err } result.NewSlideID = newSlideID @@ -306,7 +346,6 @@ func replaceOnePage(runtime *common.RuntimeContext, presentationID string, item ) if err != nil { result.Status = "delete_failed" - result.Error = err.Error() return result, err } if rev, ok := revisionFromData(deleteData); ok { @@ -342,6 +381,12 @@ func replacePageResultsOutput(results []replacePageResult) []map[string]interfac if result.Error != "" { m["error"] = result.Error } + if result.ErrorCode != 0 { + m["error_code"] = result.ErrorCode + } + if result.ErrorHint != "" { + m["hint"] = result.ErrorHint + } if result.Issues != nil { m["issues"] = result.Issues } diff --git a/shortcuts/slides/slides_replace_pages_test.go b/shortcuts/slides/slides_replace_pages_test.go index c7322aedd9..ffa0443311 100644 --- a/shortcuts/slides/slides_replace_pages_test.go +++ b/shortcuts/slides/slides_replace_pages_test.go @@ -200,6 +200,100 @@ func TestReplacePagesContinueOnErrorReturnsPartialFailure(t *testing.T) { } } +// TestReplacePagesContinueOnErrorKeepsLintRecoveryPerItem pins the one thing +// --continue-on-error changes about a refusal: nothing. A batch that keeps going +// reports its failures only through the per-item records, so if the enrichment +// lived on the returning path the caller with the most pages to fix would be the +// one told least about how to fix them. +// +// The report travels verbatim in the item for the same reason it does in the +// error: it is the server's document and one parse has to work on it wherever it +// surfaced. The code and the hint travel beside it because Problem.Error() is +// the message alone, so an item built from the string would name neither the +// refusal nor the flag that gets past it. +func TestReplacePagesContinueOnErrorKeepsLintRecoveryPerItem(t *testing.T) { + t.Parallel() + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_abc/slide", + Body: map[string]interface{}{"code": lintBlockedCode, "msg": lintBlockMessage}, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_abc/slide", + Body: map[string]interface{}{ + "code": 0, + "data": map[string]interface{}{"slide_id": "new2", "revision_id": 11}, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "DELETE", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_abc/slide", + Body: map[string]interface{}{ + "code": 0, + "data": map[string]interface{}{"revision_id": 12}, + }, + }) + + pages := `[ + {"slide_id":"old1","content":""}, + {"slide_id":"old2","content":""} + ]` + err := runSlidesShortcut(t, f, stdout, SlidesReplacePages, []string{ + "+replace-pages", + "--presentation", "pres_abc", + "--pages", pages, + "--continue-on-error", + "--as", "user", + }) + var pfErr *output.PartialFailureError + if !errors.As(err, &pfErr) { + t.Fatalf("err = %T %v, want *output.PartialFailureError", err, err) + } + + env := decodeReplacePagesEnvelope(t, stdout) + results, _ := env.Data["results"].([]interface{}) + if len(results) != 2 { + t.Fatalf("results len = %d, want 2", len(results)) + } + refused, _ := results[0].(map[string]interface{}) + if refused["status"] != "create_failed" { + t.Fatalf("refused item status = %v, want create_failed", refused["status"]) + } + if refused["error_code"] != float64(lintBlockedCode) { + t.Fatalf("refused item error_code = %v, want %d: the item cannot be recognised as a lint refusal without it", + refused["error_code"], lintBlockedCode) + } + if refused["error"] != lintBlockMessage { + t.Fatalf("refused item error = %v, want the server report verbatim", refused["error"]) + } + hint, _ := refused["hint"].(string) + for _, want := range []string{"xml lint blocked: 2 error(s)", "--no-lint"} { + if !strings.Contains(hint, want) { + t.Fatalf("refused item hint lost %q\ngot: %q", want, hint) + } + } + // The position belongs to the stop-on-error path. Here every item has its own + // record, so a "stopped at item 1/2" would describe a batch that did not stop. + if strings.Contains(hint, "stopped at item") { + t.Fatalf("refused item hint = %q, want no stop-on-error progress line", hint) + } + // A refusal is not a partial write: the page the batch moved on from must not + // look like it left a new page behind. + if _, ok := refused["new_slide_id"]; ok { + t.Fatalf("refused item = %#v, want no new_slide_id", refused) + } + survivor, _ := results[1].(map[string]interface{}) + if survivor["status"] != "replaced" || survivor["new_slide_id"] != "new2" { + t.Fatalf("second result = %#v, want replaced with new2", survivor) + } + if _, ok := survivor["error_code"]; ok { + t.Fatalf("second result = %#v, want no error_code on a page that landed", survivor) + } +} + func TestReplacePagesContinueOnErrorDeleteFailureIncludesNewSlideID(t *testing.T) { t.Parallel() diff --git a/shortcuts/slides/slides_replace_slide.go b/shortcuts/slides/slides_replace_slide.go index 1b98a0fe82..7d90db745b 100644 --- a/shortcuts/slides/slides_replace_slide.go +++ b/shortcuts/slides/slides_replace_slide.go @@ -34,6 +34,8 @@ 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 asks the backend to lint the page these parts produce, and renders the +// refusal when the lint blocks the write. --no-lint opts out. // // `str_replace` is intentionally NOT exposed: product direction is that // slide edits go through structural (block-level) operations only. The backend @@ -53,6 +55,7 @@ var SlidesReplaceSlide = common.Shortcut{ {Name: "parts", Desc: "JSON array of replace parts; accepts replace/insert action aliases, target_id for block_id, and block/content/shape/element for the action's XML payload; max 200", Required: true, Input: []string{common.File, common.Stdin}}, {Name: "revision-id", Type: "int", Default: "-1", Desc: "presentation revision (-1 = latest; pass a specific number for optimistic locking)"}, {Name: "tid", Desc: "transaction id for concurrent-edit locking (usually empty)"}, + noLintFlag(), }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { ref, err := parsePresentationRef(runtime.Str("presentation")) @@ -103,7 +106,7 @@ var SlidesReplaceSlide = common.Shortcut{ if tid := runtime.Str("tid"); tid != "" { query["tid"] = tid } - body := map[string]interface{}{"parts": injected} + body := replaceSlideBody(injected, runtime) dry := common.NewDryRunAPI() presentationID := ref.Token @@ -155,11 +158,14 @@ var SlidesReplaceSlide = common.Shortcut{ if tid := strings.TrimSpace(runtime.Str("tid")); tid != "" { query["tid"] = tid } - body := map[string]interface{}{"parts": injected} + body := replaceSlideBody(injected, runtime) data, err := runtime.CallAPITyped("POST", slideReplaceAPIPath(presentationID), query, body) if err != nil { - return enrichSlidesReplaceError(err) + // Lint first: enrichSlidesReplaceError only fills an empty hint, so + // running it second leaves the specific lint finding in place and + // the generic 3350001 checklist for everything else. + return enrichSlidesReplaceError(enrichSlidesLintError(err)) } result := map[string]interface{}{ @@ -183,12 +189,27 @@ var SlidesReplaceSlide = common.Shortcut{ if raw, ok := data["failed_reason"]; ok { result["failed_reason"] = raw } + // issues points the other way from failed_reason: the parts were applied + // and committed, and the backend still had something to say about the page + // they produced. A finding serious enough to refuse the write leaves as an + // error carrying the same report, so whatever arrives here describes a page + // that is already stored. + if raw, ok := data["issues"]; ok { + result["issues"] = raw + } runtime.Out(result, nil) return nil }, } +// replaceSlideBody builds the request body shared by dry-run and execute, so the +// two cannot disagree about the lint switch — the failure mode being a --dry-run +// that shows a linted request and an execute that sends an unlinted one. +func replaceSlideBody(parts []map[string]interface{}, runtime *common.RuntimeContext) map[string]interface{} { + return withLintXML(map[string]interface{}{"parts": parts}, runtime) +} + // replacePart is the normalized (post-JSON) representation of one entry in the // parts array. Fields are nullable so we can tell "not provided" from "empty". type replacePart struct { diff --git a/shortcuts/slides/slides_replace_slide_test.go b/shortcuts/slides/slides_replace_slide_test.go index 23a06993df..5cee37a9e3 100644 --- a/shortcuts/slides/slides_replace_slide_test.go +++ b/shortcuts/slides/slides_replace_slide_test.go @@ -614,6 +614,47 @@ func TestReplaceSlidePassThroughFailureFields(t *testing.T) { } } +// TestReplaceSlidePassesThroughIssues covers the field pointing the other way +// from failed_reason: the parts were applied and committed, and the backend +// still had something to say about the page they produced. A finding serious +// enough to refuse the write leaves as an error carrying the same report, so +// what arrives here always describes a page that is already stored. +func TestReplaceSlidePassesThroughIssues(t *testing.T) { + t.Parallel() + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/slide/replace", + Body: map[string]interface{}{ + "code": 0, + "data": map[string]interface{}{ + "revision_id": 4, + "issues": "[issue=text_overflows_container id=bgD level=warning]", + }, + }, + }) + + parts := `[{"action":"block_replace","block_id":"bxx","replacement":""}]` + err := runSlidesShortcut(t, f, stdout, SlidesReplaceSlide, []string{ + "+replace-slide", + "--presentation", "pres_abc", + "--slide-id", "s", + "--parts", parts, + "--as", "user", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + data := decodeShortcutData(t, stdout) + if issues, _ := data["issues"].(string); issues != "[issue=text_overflows_container id=bgD level=warning]" { + t.Fatalf("issues = %#v, want the backend report passed through verbatim", data["issues"]) + } + if _, ok := data["failed_reason"]; ok { + t.Fatalf("failed_reason should stay absent on a write that succeeded: %#v", data) + } +} + func TestReplaceSlide3350001ErrorEnrichment(t *testing.T) { t.Parallel() diff --git a/shortcuts/slides/slides_update_slide.go b/shortcuts/slides/slides_update_slide.go index 9ee1304573..34e98abf59 100644 --- a/shortcuts/slides/slides_update_slide.go +++ b/shortcuts/slides/slides_update_slide.go @@ -99,6 +99,7 @@ var updateSlideFlags = []common.Flag{ {Name: "content", Aliases: contentFlagAliases, Desc: "the page's full target XML, one root; elements omitted here are removed from the page", Required: true, Input: []string{common.File, common.Stdin}}, {Name: "revision-id", Type: "int", Default: "-1", Desc: "revision to apply against; -1 (default) means latest. Pinning an older revision rebuilds the page from that snapshot and discards newer edits to it"}, {Name: "tid", Desc: "transaction id for concurrent-edit locking (usually empty)"}, + noLintFlag(), } func updateSlideValidate(_ context.Context, runtime *common.RuntimeContext) error { @@ -187,7 +188,7 @@ func updateSlideDryRun(_ context.Context, runtime *common.RuntimeContext) *commo dry.POST(slideReplaceAPIPath(presentationID)). Desc(fmt.Sprintf("[%d/%d] Replace slide%s", step, total, descSuffix)). Params(updateSlideQuery(runtime, slideID)). - Body(map[string]interface{}{"parts": updateSlideParts(slideID, content)}) + Body(updateSlideBody(slideID, content, runtime)) return dry.Set("slide_id", slideID). Set("content_bytes", len(content)). Set("images_to_upload", len(placeholders)) @@ -233,14 +234,17 @@ func updateSlideExecute(_ context.Context, runtime *common.RuntimeContext) error data, err := runtime.CallAPITyped("POST", slideReplaceAPIPath(presentationID), updateSlideQuery(runtime, slideID), - map[string]interface{}{"parts": updateSlideParts(slideID, content)}) + updateSlideBody(slideID, content, runtime)) if err != nil { if len(placeholders) > 0 { // The images are already in the deck's media store; say so, or a // retry silently uploads a second copy of every file. err = appendSlidesProgressHint(err, fmt.Sprintf("%d image(s) were uploaded before the slide failed; re-running will upload them again", len(placeholders))) } - return enrichUpdateSlideError(err) + // .../slide/replace is gated, and the subject is the page the write + // produces rather than the payload sent, so a page that is invalid only + // in combination is caught here too. + return enrichUpdateSlideError(enrichSlidesLintError(err)) } // A single part carries the whole page, so any failed_reason means the page @@ -263,6 +267,14 @@ func updateSlideExecute(_ context.Context, runtime *common.RuntimeContext) error if v, ok := data["revision_id"]; ok { result["revision_id"] = v } + // issues is advisory and only ever arrives on a page that was written: a + // finding serious enough to refuse the write leaves as an error carrying the + // same report, and the failed_reason branch above has already returned. Passed + // through untouched, as +add-slide and +create do, so a caller reads one field + // however it wrote the page. + if issues, ok := data["issues"]; ok { + result["issues"] = issues + } runtime.Out(result, nil) return nil } @@ -316,6 +328,12 @@ func updateSlideQuery(runtime *common.RuntimeContext, slideID string) map[string return query } +// updateSlideBody builds the request body shared by dry-run and execute, so the +// two cannot disagree about the lint switch. +func updateSlideBody(slideID, content string, runtime *common.RuntimeContext) map[string]interface{} { + return withLintXML(map[string]interface{}{"parts": updateSlideParts(slideID, content)}, runtime) +} + // updateSlideParts builds the one part the command ever sends. block_id is the // page id, which is what makes the backend swap the whole out. func updateSlideParts(slideID, content string) []map[string]interface{} { diff --git a/shortcuts/slides/slides_update_slide_test.go b/shortcuts/slides/slides_update_slide_test.go index 66b84c5a6b..a63193c130 100644 --- a/shortcuts/slides/slides_update_slide_test.go +++ b/shortcuts/slides/slides_update_slide_test.go @@ -419,6 +419,71 @@ func TestUpdateSlideRevisionAndTIDTravel(t *testing.T) { } } +// TestUpdateSlidePassesThroughIssues keeps the backend's lint report visible on +// a page that was written. A finding serious enough to refuse the write comes +// back as an error carrying the same report, so anything arriving here describes +// a page that is already stored — dropping it would leave the caller believing +// the deck says exactly what they wrote. +func TestUpdateSlidePassesThroughIssues(t *testing.T) { + t.Parallel() + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_abc/slide/replace", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{ + "revision_id": 9, + "issues": "[issue=text_overflows_container id=v5 level=warning]", + }}, + }) + + if err := runSlidesShortcut(t, f, stdout, SlidesUpdateSlide, []string{ + "+update-slide", + "--presentation", "pres_abc", + "--slide-id", "pYw", + "--content", testPageXML, + "--as", "user", + }); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + data := decodeShortcutData(t, stdout) + if issues, _ := data["issues"].(string); issues != "[issue=text_overflows_container id=v5 level=warning]" { + t.Fatalf("issues = %#v, want the backend report passed through verbatim", data["issues"]) + } + if data["revision_id"] != float64(9) { + t.Fatalf("revision_id = %#v, want 9 alongside the report", data["revision_id"]) + } +} + +// TestUpdateSlideOmitsIssuesWhenAbsent keeps the key out of the output rather +// than reporting an empty report, so a caller can test for presence. +func TestUpdateSlideOmitsIssuesWhenAbsent(t *testing.T) { + t.Parallel() + + f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/slides_ai/v1/xml_presentations/pres_abc/slide/replace", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{"revision_id": 9}}, + }) + + if err := runSlidesShortcut(t, f, stdout, SlidesUpdateSlide, []string{ + "+update-slide", + "--presentation", "pres_abc", + "--slide-id", "pYw", + "--content", testPageXML, + "--as", "user", + }); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + data := decodeShortcutData(t, stdout) + if _, ok := data["issues"]; ok { + t.Fatalf("issues should be omitted when the backend sent none: %#v", data) + } +} + // TestUpdateSlideOmitsEmptyTID keeps an unset --tid out of the query rather than // sending tid="". func TestUpdateSlideOmitsEmptyTID(t *testing.T) { diff --git a/skills/lark-slides/references/cli/lark-slides-add-slide.md b/skills/lark-slides/references/cli/lark-slides-add-slide.md index 3d94542823..c81d1abdec 100644 --- a/skills/lark-slides/references/cli/lark-slides-add-slide.md +++ b/skills/lark-slides/references/cli/lark-slides-add-slide.md @@ -45,6 +45,7 @@ lark-cli slides +add-slide --presentation "$PRES_ID" --slide @page3.xml --dry-ru | `--slide` | 是 | 一个完整的 `...` 文档；支持字面量、`@file`、stdin `-` | | `--before-slide-id` | 否 | 插到该 `slide_id` 之前；**不传就是追加到末尾** | | `--revision-id` | 否 | 演示文稿版本号，默认 `-1`（最新）；传具体版本号做乐观锁 | +| `--no-lint` | 否 | 跳过服务端版式校验（默认开启）；仅在确认校验误判、该页必须原样发布时使用 | | `--dry-run` | 否 | 打印将要发起的请求（含图片上传步骤），不写入 | `@file` 路径**必须在 CWD 内**（如 `@./plan/page3.xml`）；绝对路径和 `../` 会被拒绝并报 `unsafe file path`。 @@ -73,14 +74,14 @@ lark-cli slides +add-slide --as user \ "revision_id": 42, "before_slide_id": "slide_example_target_id", "images_uploaded": 1, - "issues": "[issue=unsupported_attr tag= attr=style]" + "issues": "<服务端返回的发现>" } ``` | 字段 | 说明 | |------|------| | `slide_id` | 新创建页面的唯一标识 | -| `issues` | 字符串，**只在服务端丢弃过内容时才出现**：页面创建成功，但括号里列出的标签/属性没写进去。出现就必须 `+screenshot` 复核，别当纯警告忽略；干净提交时这个字段不返回 | +| `issues` | 字符串，仅在**页面已写入成功**且服务端有发现时返回，干净提交时不返回，不影响本次调用的成功状态。两种来源：提交的 XML 里有服务端不支持的标签/属性被丢弃（**页面内容与提交的不一致**），或未达阻断级的版式校验发现。格式不固定，不要解析；出现就用 `+screenshot` 复核该页，不要按普通告警忽略 | ## 常见错误 @@ -89,4 +90,5 @@ lark-cli slides +add-slide --as user \ | `--slide is not a single complete document` | 传了 `` 整份 XML，或多个 `` 拼在一起 | 一次只传一页，根元素必须是 `` | | `--slide cannot be empty` | `@file` 指向空文件，或 stdin 没内容 | 检查文件内容 | | 3350001 | XML 结构/转义有问题；**或 `--before-slide-id` 不是有效 `slide_id`** | 优先改用 `--slide @file` 绕开 shell 转义；插页失败先 `+xml-get` 回读确认 `slide_id`；再按 [workflow/error-handling.md](../workflow/error-handling.md) 排查 | +| 4000153 `xml lint blocked` | 服务端版式校验拒绝了这一页，页面未写入 | `error.message` 是完整的校验报告，按其中每条发现给出的修改建议修正后重试 | | 1061004 / 403 | 当前身份对这份 PPT 没有编辑权限 | 检查是否拥有 `slides:presentation:update` 或 `slides:presentation:write_only` scope；wiki 链接另需 `wiki:node:read`，`@` 占位符另需 `docs:document.media:upload`；`--as bot` 还要求该 bot 对目标 PPT 有编辑权限 | diff --git a/skills/lark-slides/references/cli/lark-slides-create.md b/skills/lark-slides/references/cli/lark-slides-create.md index a2c51a1fe4..50e8816206 100644 --- a/skills/lark-slides/references/cli/lark-slides-create.md +++ b/skills/lark-slides/references/cli/lark-slides-create.md @@ -54,11 +54,12 @@ lark-cli slides +create --title "项目汇报" --slide @./slide-01.xml --dry-run - **`slides_added`**（integer，可选）：带页面创建时返回，成功添加的页面数量 - **`images_uploaded`**（integer，可选）：页面 XML 中含 `@<本地路径>` 占位符时返回，已上传的去重后图片数量 - **`permission_grant`**（object，可选）：仅 `--as bot` 时返回，说明是否已自动为当前 CLI 用户授予可管理权限 +- **`slide_issues`**（数组，可选）：带页面创建时才可能返回，逐项对应一个**已写入成功的页面**（标明页序和 `slide_id`），内容是服务端对该页的发现，不影响本次调用的成功状态。两种来源：页面 XML 里有服务端不支持的标签/属性被丢弃（**页面内容与提交的不一致**），或未达阻断级的版式校验发现。格式不固定，不要解析；出现就用 `+screenshot` 复核对应页 > [!IMPORTANT] > 不带页面参数时，`slides +create` 只创建空白演示文稿。创建后用 [`+add-slide`](lark-slides-add-slide.md) 逐页添加 slide 内容。 > -> 带了页面时，CLI 先创建空白演示文稿，再逐页调用 slide 创建接口添加页面。如果某一页添加失败，CLI 会停止并报错，已创建的演示文稿和已添加的页面会保留。 +> 带了页面时，CLI 先创建空白演示文稿，再逐页调用 slide 创建接口添加页面，每页各过一次服务端版式校验。如果某一页失败，CLI 会停止并报错，已创建的演示文稿和已添加的页面会保留，报错会指明失败页序和此前已成功写入的页数。 > > 如果演示文稿是**以应用身份（bot）创建**的，如 `lark-cli slides +create --as bot`，CLI 会**尝试为当前 CLI 用户自动授予该演示文稿的 `full_access`（可管理权限）**。 > @@ -76,6 +77,7 @@ lark-cli slides +create --title "项目汇报" --slide @./slide-01.xml --dry-run | `--title` | 否 | 演示文稿标题（不传则默认 "Untitled"） | | `--slide` | 否 | 一页 `` XML，或 `@路径`；可重复，最多 10 次。格式见[页面输入形式](#页面输入形式) | | `--slides` | 否 | 页面 XML 的 JSON 字符串数组，最多 10 个；支持 `@文件` 和 `-`（stdin）。格式见[页面输入形式](#页面输入形式) | +| `--no-lint` | 否 | 跳过服务端版式校验（默认开启，每页各校验一次）；仅在确认校验误判、页面必须原样发布时使用 | 10 页是 CLI 的上限，服务端每次只接收一页。超过 10 页时先用 `+create` 创建空白 PPT，再用 [`+add-slide`](lark-slides-add-slide.md) 逐页添加。 @@ -169,6 +171,7 @@ lark-cli slides +add-slide --as user \ |--------|------|----------| | 400 | 参数错误 | 检查参数格式是否正确 | | 403 | 权限不足 | 检查是否拥有 `slides:presentation:create` 和 `slides:presentation:write_only` scope | +| 4000153 `xml lint blocked` | 服务端版式校验拒绝了该页；演示文稿及其之前的页面已写入成功 | `error.message` 是完整的校验报告，按其中每条发现给出的修改建议修正后，用 `+add-slide` 从该页续接，无需重建整份演示文稿 | ## 相关命令 diff --git a/skills/lark-slides/references/cli/lark-slides-replace-slide.md b/skills/lark-slides/references/cli/lark-slides-replace-slide.md index 08dae90306..10982689a8 100644 --- a/skills/lark-slides/references/cli/lark-slides-replace-slide.md +++ b/skills/lark-slides/references/cli/lark-slides-replace-slide.md @@ -51,6 +51,7 @@ lark-cli slides +replace-slide --as user \ | `--parts` | 是 | JSON 数组（`[{...}, ...]`），单次最多 200 条。支持 `@` 和 `-`（stdin）读取 | | `--revision-id` | 否 | 基础版本号；默认 `-1` 表示基于最新版执行；传具体版本号时，服务端以该版本为 base 执行；**传不存在的版本号（超过当前 revision）返回 3350002** | | `--tid` | 否 | 并发事务 ID；多人协作长事务才用，单次单人调用留空 | +| `--no-lint` | 否 | 跳过服务端版式校验（默认开启）。**校验主体是这些 parts 拼装后的整页**，不是片段本身 | ## parts 元素结构 @@ -172,6 +173,7 @@ lark-cli slides +replace-slide --as user \ | `revision_id` | 成功后的新版本号，下次做乐观锁时用 | | `failed_part_index` | 有部分失败时存在，指向第几条 part 失败 | | `failed_reason` | 失败原因文字描述 | +| `issues` | 与 `failed_reason` 相对：parts 已全部生效，服务端只是仍有发现，不影响本次调用的成功状态。内容是未达阻断级的版式校验发现；校验主体是**拼装后的整页**，因此可能报出页面上原有的元素。格式不固定，不要解析；出现就用 `+screenshot` 复核该页 | 整批作为原子事务：任一 part 失败则整批不生效，服务端通过 `failed_part_index` / `failed_reason` 告诉你是哪条；按此定位修正后重发。 @@ -249,6 +251,7 @@ lark-cli slides +replace-slide --as user \ | `--parts[i] (block_replace) requires non-empty block_id` / `replacement` | 字段名对，但值缺失或是空串 | 按 parts 元素结构补齐值 | | `` 不显示 / 显示破图 | `src` 写了外链 URL | 换成通过 [`+media-upload`](lark-slides-media-upload.md) 拿到的 `file_token` | | 3350001 | `replacement` 不是合法单根 XML 片段，或 `block_id` 不存在 | CLI 已自动注入 `id` 和 ``；如果仍报错，重新 `slide.get` 拿最新 XML 确认 `block_id` 存在；检查 XML 结构是否合法；坐标是否超出 960×540 | +| 4000153 `xml lint blocked` | 服务端版式校验拒绝了本次提交，页面维持原状；校验主体是拼装后的整页，因此片段自身合法也可能因与相邻元素重叠、或页面原有元素越界而被拒 | `error.message` 是完整的校验报告，按其中每条发现给出的修改建议修正；需要调整既有元素时，在同一批 `--parts` 内一并提交 | | 403 | 权限不足 | 需要 `slides:presentation:update` 或 `slides:presentation:write_only`；wiki URL 还需要 `wiki:node:read` | ## 相关命令 diff --git a/skills/lark-slides/references/cli/lark-slides-update-slide.md b/skills/lark-slides/references/cli/lark-slides-update-slide.md index 4e9dc6ee0f..7ded368b96 100644 --- a/skills/lark-slides/references/cli/lark-slides-update-slide.md +++ b/skills/lark-slides/references/cli/lark-slides-update-slide.md @@ -34,6 +34,7 @@ lark-cli slides +update-slide --as user \ | `--content` | 是 | 这一页的完整目标 XML，单一 `` 根；支持字面量、`@file`、stdin `-`。别名：`--xml` / `--slide-xml` / `--slide-content` / `--content-xml` | | `--revision-id` | 否 | 默认 `-1`（最新）。它只选择服务端执行所基于的快照，不是“页面有新编辑就拒绝”的乐观锁；传旧版本号会以旧快照重建页面并丢弃其后的编辑 | | `--tid` | 否 | 调用方提供的任务/事务标识，CLI 原样透传；用于关联同一编辑任务或重试，不等同于版本前置条件，不能单独保证并发冲突时拒绝写入。一般留空 | +| `--no-lint` | 否 | 跳过服务端版式校验（默认开启）；仅在确认校验误判、该页必须原样发布时使用 | `@file` 和 `+xml-get --output` 一样**只接受当前目录下的相对路径**，绝对路径会被拒。 命令别名：`slides +update`（隐藏）；服务别名：`lark-cli slide …` 等价于 `lark-cli slides …`。 @@ -147,6 +148,7 @@ lark-cli slides +xml-get --as user \ | `slide_id` | 与传入相同——整页覆盖不换页 id | | `revision_id` | 写入后的新版本号 | | `images_uploaded` | 仅当 `--content` 带 `@` 占位符时出现：本次去重后实际上传的图片张数 | +| `issues` | 仅在**页面已写入成功**且服务端有发现时返回，不影响本次调用的成功状态。内容是未达阻断级的版式校验发现。格式不固定，不要解析；出现就用 `+screenshot` 复核该页 | 服务端拒绝这次写入时（`failed_reason` 非空）**不会**返回成功输出，而是报错并带上原因——单个 part 承载整页，任何失败都意味着页面没被写入。 @@ -160,4 +162,5 @@ lark-cli slides +xml-get --as user \ | 3350001，原因包含 `not found` | `--presentation` 不匹配，或 `--slide-id` 对应的页面已被删除 | 检查 `--presentation` 和 `--slide-id`，再用 `slides +xml-get` 回读当前页面 ID | | 3350001，其他 invalid param | `--content` 的 XML 结构有问题（如 `` 缺 ``、包含服务端不支持的元素） | 按 [error-handling.md](../workflow/error-handling.md) 检查 `--content` 的 XML 结构 | | 3350002 not found | `--revision-id` 传了不存在的版本号 | 用 `-1` 或真实存在的 `revision_id` | +| 4000153 `xml lint blocked` | 服务端版式校验拒绝了本次写回，页面维持原状 | `error.message` 是完整的校验报告，按其中每条发现给出的修改建议修正后重试 | | 1061004 / 403 | 当前身份对这份 PPT 没有编辑权限 | 检查是否拥有 `slides:presentation:update` 或 `slides:presentation:write_only` scope；wiki 链接另需 `wiki:node:read`，`@` 占位符另需 `docs:document.media:upload`；`--as bot` 还要求该 bot 对目标 PPT 有编辑权限 | diff --git a/skills/lark-slides/references/workflow/error-handling.md b/skills/lark-slides/references/workflow/error-handling.md index f89385d88c..58bb90494b 100644 --- a/skills/lark-slides/references/workflow/error-handling.md +++ b/skills/lark-slides/references/workflow/error-handling.md @@ -53,6 +53,7 @@ | 1061004 forbidden | 当前用户对演示文稿无编辑权限 | 确认当前用户对目标 PPT 有编辑权限 | | 3350001 | XML 非 well-formed、XML 结构不符合服务端要求，或 replace 片段问题 | 优先检查未转义字符；replace 场景再看 `block_id` 和 `` | | 3350002 | `revision_id` 大于当前版本 | 用 `-1` 取当前版本，或重新用 `slides +xml-get` 取最新 `revision_id` | +| 4000153 `xml lint blocked` | 服务端版式校验拒绝了本次写入，被拒的页面未写入（`+create` 逐页提交，之前的页面保留） | 完整的校验报告在 `error.message`；`+replace-pages --continue-on-error` 不中断整批，被拒的那几页改在 `results[]` 里以 `error_code: 4000153` + `error`（同一份报告）+ `hint` 给出。按报告里每条发现的修改建议修正后重试。只有阻断级发现会拦截，未拦截的发现不会丢失——写入成功时通过返回值里的 `issues` 字段给出。确认误判时才用 `--no-lint` | | validation: unsafe file path | `--file` 给了绝对路径或上层路径 | `--file` 必须是 CWD 内相对路径；先 `cd` 到素材目录再执行 | ## Command-Specific References