From 0cc03eab29ed49c77723ad7c9c47b18cb55101b5 Mon Sep 17 00:00:00 2001 From: baiqing Date: Wed, 8 Apr 2026 18:19:44 +0800 Subject: [PATCH 1/6] feat(doc): add --after-keyword/--before-keyword flags to +media-insert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allows inserting images/files at a position relative to the first block whose plain text matches a keyword (case-insensitive substring match). - Add --after-keyword: insert after the matched root-level block - Add --before-keyword: insert before the matched root-level block - Flags are mutually exclusive; default behavior (append to end) unchanged - fetchAllBlocks: paginated block listing (up to 50 pages × 200 blocks) - extractBlockPlainText: covers text, heading1-9, bullet, ordered, todo, code, quote - findInsertIndexByKeyword: walks parent_id chain to resolve nested blocks to their root-level ancestor - DryRun updated to show block-listing step when keyword flag is set --- shortcuts/doc/doc_media_insert.go | 196 +++++++++++++- shortcuts/doc/doc_media_insert_test.go | 361 ++++++++++++++++++++++++- 2 files changed, 544 insertions(+), 13 deletions(-) diff --git a/shortcuts/doc/doc_media_insert.go b/shortcuts/doc/doc_media_insert.go index 5be0eed1f..35fd88553 100644 --- a/shortcuts/doc/doc_media_insert.go +++ b/shortcuts/doc/doc_media_insert.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "path/filepath" + "strings" "github.com/larksuite/cli/internal/output" "github.com/larksuite/cli/internal/validate" @@ -23,7 +24,7 @@ var alignMap = map[string]int{ var DocMediaInsert = common.Shortcut{ Service: "docs", Command: "+media-insert", - Description: "Insert a local image or file at the end of a Lark document (4-step orchestration + auto-rollback)", + Description: "Insert a local image or file into a Lark document (4-step orchestration + auto-rollback); appends to end by default, or inserts relative to a keyword with --after-keyword/--before-keyword", Risk: "write", Scopes: []string{"docs:document.media:upload", "docx:document:write_only", "docx:document:readonly"}, AuthTypes: []string{"user", "bot"}, @@ -33,6 +34,8 @@ var DocMediaInsert = common.Shortcut{ {Name: "type", Default: "image", Desc: "type: image | file"}, {Name: "align", Desc: "alignment: left | center | right"}, {Name: "caption", Desc: "image caption text"}, + {Name: "after-keyword", Desc: "insert after the first block whose text contains this keyword (case-insensitive); mutually exclusive with --before-keyword"}, + {Name: "before-keyword", Desc: "insert before the first block whose text contains this keyword (case-insensitive); mutually exclusive with --after-keyword"}, }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { docRef, err := parseDocumentRef(runtime.Str("doc")) @@ -42,6 +45,9 @@ var DocMediaInsert = common.Shortcut{ if docRef.Kind == "doc" { return output.ErrValidation("docs +media-insert only supports docx documents; use a docx token/URL or a wiki URL that resolves to docx") } + if runtime.Str("after-keyword") != "" && runtime.Str("before-keyword") != "" { + return output.ErrValidation("--after-keyword and --before-keyword are mutually exclusive") + } return nil }, DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { @@ -55,6 +61,9 @@ var DocMediaInsert = common.Shortcut{ filePath := runtime.Str("file") mediaType := runtime.Str("type") caption := runtime.Str("caption") + afterKeyword := runtime.Str("after-keyword") + beforeKeyword := runtime.Str("before-keyword") + hasKeyword := afterKeyword != "" || beforeKeyword != "" parentType := parentTypeForMediaType(mediaType) createBlockData := buildCreateBlockData(mediaType, 0) @@ -62,22 +71,45 @@ var DocMediaInsert = common.Shortcut{ batchUpdateData := buildBatchUpdateData("", mediaType, "", runtime.Str("align"), caption) d := common.NewDryRunAPI() + totalSteps := 4 + if docRef.Kind == "wiki" { + totalSteps++ + } + if hasKeyword { + totalSteps++ + } + if docRef.Kind == "wiki" { documentID = "" stepBase = 2 - d.Desc("5-step orchestration: resolve wiki → query root → create block → upload file → bind to block (auto-rollback on failure)"). + d.Desc(fmt.Sprintf("%d-step orchestration: resolve wiki → query root →%s create block → upload file → bind to block (auto-rollback on failure)", + totalSteps, map[bool]string{true: " search blocks →", false: ""}[hasKeyword])). GET("/open-apis/wiki/v2/spaces/get_node"). Desc("[1] Resolve wiki node to docx document"). Params(map[string]interface{}{"token": docRef.Token}) } else { - d.Desc("4-step orchestration: query root → create block → upload file → bind to block (auto-rollback on failure)") + d.Desc(fmt.Sprintf("%d-step orchestration: query root →%s create block → upload file → bind to block (auto-rollback on failure)", + totalSteps, map[bool]string{true: " search blocks →", false: ""}[hasKeyword])) } d. GET("/open-apis/docx/v1/documents/:document_id/blocks/:document_id"). - Desc(fmt.Sprintf("[%d] Get document root block", stepBase)). + Desc(fmt.Sprintf("[%d] Get document root block", stepBase)) + + if hasKeyword { + kw := afterKeyword + if kw == "" { + kw = beforeKeyword + } + d.GET("/open-apis/docx/v1/documents/:document_id/blocks"). + Desc(fmt.Sprintf("[%d] List all blocks to find insert position for keyword %q", stepBase+1, kw)). + Params(map[string]interface{}{"page_size": 200}) + stepBase++ + } + + d. POST("/open-apis/docx/v1/documents/:document_id/blocks/:document_id/children"). - Desc(fmt.Sprintf("[%d] Create empty block at document end", stepBase+1)). + Desc(fmt.Sprintf("[%d] Create empty block at target position", stepBase+1)). Body(createBlockData) appendDocMediaInsertUploadDryRun(d, filePath, parentType, stepBase+2) d.PATCH("/open-apis/docx/v1/documents/:document_id/blocks/batch_update"). @@ -126,13 +158,35 @@ var DocMediaInsert = common.Shortcut{ return err } - parentBlockID, insertIndex, err := extractAppendTarget(rootData, documentID) + parentBlockID, insertIndex, rootChildren, err := extractAppendTarget(rootData, documentID) if err != nil { return err } fmt.Fprintf(runtime.IO().ErrOut, "Root block ready: %s (%d children)\n", parentBlockID, insertIndex) - // Step 2: Create an empty block at the end of the document + afterKeyword := runtime.Str("after-keyword") + beforeKeyword := runtime.Str("before-keyword") + keyword := afterKeyword + before := false + if beforeKeyword != "" { + keyword = beforeKeyword + before = true + } + if keyword != "" { + fmt.Fprintf(runtime.IO().ErrOut, "Searching blocks for keyword: %q\n", keyword) + allBlocks, err := fetchAllBlocks(runtime, documentID) + if err != nil { + return err + } + idx, err := findInsertIndexByKeyword(allBlocks, rootChildren, keyword, before) + if err != nil { + return err + } + insertIndex = idx + fmt.Fprintf(runtime.IO().ErrOut, "Keyword found: inserting at index %d\n", insertIndex) + } + + // Step 2: Create an empty block at the target position fmt.Fprintf(runtime.IO().ErrOut, "Creating block at index %d\n", insertIndex) createData, err := runtime.CallAPI("POST", @@ -304,19 +358,137 @@ func buildBatchUpdateData(blockID, mediaType, fileToken, alignStr, caption strin } } -func extractAppendTarget(rootData map[string]interface{}, fallbackBlockID string) (string, int, error) { +func extractAppendTarget(rootData map[string]interface{}, fallbackBlockID string) (parentBlockID string, insertIndex int, children []interface{}, err error) { block, _ := rootData["block"].(map[string]interface{}) if len(block) == 0 { - return "", 0, output.Errorf(output.ExitAPI, "api_error", "failed to query document root block") + return "", 0, nil, output.Errorf(output.ExitAPI, "api_error", "failed to query document root block") } - parentBlockID := fallbackBlockID + parentBlockID = fallbackBlockID if blockID, _ := block["block_id"].(string); blockID != "" { parentBlockID = blockID } - children, _ := block["children"].([]interface{}) - return parentBlockID, len(children), nil + children, _ = block["children"].([]interface{}) + return parentBlockID, len(children), children, nil +} + +// fetchAllBlocks retrieves all blocks in a document via paginated API calls. +func fetchAllBlocks(runtime *common.RuntimeContext, documentID string) ([]map[string]interface{}, error) { + const maxPages = 50 + var all []map[string]interface{} + pageToken := "" + + for page := 0; page < maxPages; page++ { + params := map[string]interface{}{"page_size": 200} + if pageToken != "" { + params["page_token"] = pageToken + } + data, err := runtime.CallAPI("GET", + fmt.Sprintf("/open-apis/docx/v1/documents/%s/blocks", validate.EncodePathSegment(documentID)), + params, nil) + if err != nil { + return nil, err + } + + items, _ := data["items"].([]interface{}) + for _, item := range items { + if block, ok := item.(map[string]interface{}); ok { + all = append(all, block) + } + } + + hasMore, _ := data["has_more"].(bool) + if !hasMore { + break + } + pageToken, _ = data["page_token"].(string) + if pageToken == "" { + break + } + } + return all, nil +} + +// extractBlockPlainText returns the concatenated plain text of a block +// by inspecting all known text-bearing sub-maps (text, heading1-9, bullet, +// ordered, todo, code, quote). All these block types share the same +// {elements: [{text_run: {content: "..."}}]} structure. +func extractBlockPlainText(block map[string]interface{}) string { + keys := []string{"text", "heading1", "heading2", "heading3", "heading4", "heading5", + "heading6", "heading7", "heading8", "heading9", "bullet", "ordered", "todo", "code", "quote"} + for _, key := range keys { + sub, ok := block[key].(map[string]interface{}) + if !ok { + continue + } + elements, _ := sub["elements"].([]interface{}) + var sb strings.Builder + for _, el := range elements { + elem, _ := el.(map[string]interface{}) + textRun, _ := elem["text_run"].(map[string]interface{}) + content, _ := textRun["content"].(string) + sb.WriteString(content) + } + if sb.Len() > 0 { + return sb.String() + } + } + return "" +} + +// findInsertIndexByKeyword finds the insert position relative to the first block +// whose plain text contains keyword (case-insensitive). When before is false it +// inserts after the matched root-level block; when before is true it inserts before. +// It walks parent_id chains to handle nested blocks. +func findInsertIndexByKeyword(blocks []map[string]interface{}, rootChildren []interface{}, keyword string, before bool) (int, error) { + lowerKw := strings.ToLower(keyword) + + // Build a blockID → block map and a blockID → parent map for quick lookup. + blockByID := make(map[string]map[string]interface{}, len(blocks)) + parentByID := make(map[string]string, len(blocks)) + for _, b := range blocks { + id, _ := b["block_id"].(string) + if id != "" { + blockByID[id] = b + parentID, _ := b["parent_id"].(string) + parentByID[id] = parentID + } + } + + // Build root children set for O(1) membership test. + rootSet := make(map[string]int, len(rootChildren)) + for i, c := range rootChildren { + if id, ok := c.(string); ok { + rootSet[id] = i + } + } + + // Search blocks in document order. + for _, b := range blocks { + text := extractBlockPlainText(b) + if text == "" || !strings.Contains(strings.ToLower(text), lowerKw) { + continue + } + // Found a match — walk up parent chain to find its top-level ancestor in rootChildren. + id, _ := b["block_id"].(string) + cur := id + for { + if idx, ok := rootSet[cur]; ok { + if before { + return idx, nil // insert before this root-level block + } + return idx + 1, nil // insert after this root-level block + } + parent := parentByID[cur] + if parent == "" || parent == cur { + break + } + cur = parent + } + return 0, output.ErrValidation("block containing keyword %q is not reachable from document root; try a top-level heading", keyword) + } + return 0, output.ErrValidation("no block found containing keyword %q", keyword) } func extractCreatedBlockTargets(createData map[string]interface{}, mediaType string) (blockID, uploadParentNode, replaceBlockID string) { diff --git a/shortcuts/doc/doc_media_insert_test.go b/shortcuts/doc/doc_media_insert_test.go index 0e4f9bad1..abff5c8ed 100644 --- a/shortcuts/doc/doc_media_insert_test.go +++ b/shortcuts/doc/doc_media_insert_test.go @@ -109,7 +109,7 @@ func TestExtractAppendTargetUsesRootChildrenCount(t *testing.T) { }, } - blockID, index, err := extractAppendTarget(rootData, "fallback") + blockID, index, children, err := extractAppendTarget(rootData, "fallback") if err != nil { t.Fatalf("extractAppendTarget() unexpected error: %v", err) } @@ -119,6 +119,365 @@ func TestExtractAppendTargetUsesRootChildrenCount(t *testing.T) { if index != 3 { t.Fatalf("extractAppendTarget() index = %d, want 3", index) } + if len(children) != 3 { + t.Fatalf("extractAppendTarget() children len = %d, want 3", len(children)) + } +} + +func TestExtractBlockPlainTextParagraph(t *testing.T) { + t.Parallel() + + block := map[string]interface{}{ + "block_type": 2, + "text": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "Hello "}}, + map[string]interface{}{"text_run": map[string]interface{}{"content": "World"}}, + }, + }, + } + got := extractBlockPlainText(block) + if got != "Hello World" { + t.Fatalf("extractBlockPlainText() = %q, want %q", got, "Hello World") + } +} + +func TestExtractBlockPlainTextHeading(t *testing.T) { + t.Parallel() + + block := map[string]interface{}{ + "block_type": 3, + "heading1": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "My Section"}}, + }, + }, + } + got := extractBlockPlainText(block) + if got != "My Section" { + t.Fatalf("extractBlockPlainText() = %q, want %q", got, "My Section") + } +} + +func TestExtractBlockPlainTextBulletOrderedTodo(t *testing.T) { + t.Parallel() + + cases := []struct { + key string + content string + }{ + {"bullet", "太空山"}, + {"ordered", "第一步操作"}, + {"todo", "完成任务"}, + } + for _, tc := range cases { + block := map[string]interface{}{ + "block_type": 0, + tc.key: map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": tc.content}}, + }, + }, + } + got := extractBlockPlainText(block) + if got != tc.content { + t.Errorf("extractBlockPlainText(%q) = %q, want %q", tc.key, got, tc.content) + } + } +} + +func TestExtractBlockPlainTextEmpty(t *testing.T) { + t.Parallel() + + block := map[string]interface{}{"block_type": 27, "image": map[string]interface{}{}} + if got := extractBlockPlainText(block); got != "" { + t.Fatalf("extractBlockPlainText(image) = %q, want empty", got) + } +} + +func TestFindInsertIndexByKeywordFindsAfterBlock(t *testing.T) { + t.Parallel() + + blocks := []map[string]interface{}{ + { + "block_id": "root", + "parent_id": "", + }, + { + "block_id": "blk_a", + "parent_id": "root", + "heading1": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "Introduction"}}, + }, + }, + }, + { + "block_id": "blk_b", + "parent_id": "root", + "heading1": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "Architecture"}}, + }, + }, + }, + } + rootChildren := []interface{}{"blk_a", "blk_b"} + + idx, err := findInsertIndexByKeyword(blocks, rootChildren, "Introduction", false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if idx != 1 { + t.Fatalf("findInsertIndexByKeyword() = %d, want 1", idx) + } +} + +func TestFindInsertIndexByKeywordCaseInsensitive(t *testing.T) { + t.Parallel() + + blocks := []map[string]interface{}{ + { + "block_id": "blk_a", + "parent_id": "root", + "heading2": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "Core Architecture"}}, + }, + }, + }, + } + rootChildren := []interface{}{"blk_a"} + + idx, err := findInsertIndexByKeyword(blocks, rootChildren, "core architecture", false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if idx != 1 { + t.Fatalf("findInsertIndexByKeyword() = %d, want 1", idx) + } +} + +func TestFindInsertIndexByKeywordNestedBlock(t *testing.T) { + t.Parallel() + + // Nested block: blk_child is inside blk_section (root child) + blocks := []map[string]interface{}{ + { + "block_id": "blk_section", + "parent_id": "root", + "heading1": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "Section"}}, + }, + }, + }, + { + "block_id": "blk_child", + "parent_id": "blk_section", + "text": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "nested content here"}}, + }, + }, + }, + } + rootChildren := []interface{}{"blk_section"} + + // Matching a nested block should insert after its root-level ancestor. + idx, err := findInsertIndexByKeyword(blocks, rootChildren, "nested content", false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if idx != 1 { + t.Fatalf("findInsertIndexByKeyword() nested = %d, want 1", idx) + } +} + +// TestFindInsertIndexByKeywordDuplicateUsesFirst verifies that when the same +// keyword appears in multiple blocks, the function always anchors to the +// first matching block in document order (the slice iteration order of blocks). +func TestFindInsertIndexByKeywordDuplicateUsesFirst(t *testing.T) { + t.Parallel() + + // Three root-level blocks, all containing "overview". + // Document order: blk_a (index 0) → blk_b (index 1) → blk_c (index 2). + blocks := []map[string]interface{}{ + { + "block_id": "blk_a", + "parent_id": "root", + "text": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "overview: section one"}}, + }, + }, + }, + { + "block_id": "blk_b", + "parent_id": "root", + "text": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "overview: section two"}}, + }, + }, + }, + { + "block_id": "blk_c", + "parent_id": "root", + "text": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "overview: section three"}}, + }, + }, + }, + } + rootChildren := []interface{}{"blk_a", "blk_b", "blk_c"} + + // --after-keyword: should insert after blk_a (index 0 → return 1), not blk_b or blk_c. + afterIdx, err := findInsertIndexByKeyword(blocks, rootChildren, "overview", false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if afterIdx != 1 { + t.Fatalf("after: got index %d, want 1 (after first match blk_a)", afterIdx) + } + + // --before-keyword: should insert before blk_a (index 0 → return 0). + beforeIdx, err := findInsertIndexByKeyword(blocks, rootChildren, "overview", true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if beforeIdx != 0 { + t.Fatalf("before: got index %d, want 0 (before first match blk_a)", beforeIdx) + } +} + +// TestFindInsertIndexByKeywordDuplicateNestedAndTopLevel verifies that when the +// keyword appears in both a nested block (inside blk_a) and a later top-level +// block (blk_b), the function uses the earlier document-order match — which +// resolves upward to blk_a. +func TestFindInsertIndexByKeywordDuplicateNestedAndTopLevel(t *testing.T) { + t.Parallel() + + blocks := []map[string]interface{}{ + // blk_a is a top-level section + { + "block_id": "blk_a", + "parent_id": "root", + "heading1": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "Section A"}}, + }, + }, + }, + // blk_child is nested inside blk_a and contains the keyword first + { + "block_id": "blk_child", + "parent_id": "blk_a", + "text": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "architecture diagram"}}, + }, + }, + }, + // blk_b is a second top-level block that also contains the keyword + { + "block_id": "blk_b", + "parent_id": "root", + "text": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "architecture diagram"}}, + }, + }, + }, + } + rootChildren := []interface{}{"blk_a", "blk_b"} + + // blk_child appears before blk_b in blocks slice → first match is blk_child, + // which walks up to blk_a (rootChildren index 0). + // after → insert at index 1 (after blk_a) + afterIdx, err := findInsertIndexByKeyword(blocks, rootChildren, "architecture diagram", false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if afterIdx != 1 { + t.Fatalf("after: got %d, want 1 (after blk_a, ancestor of first-matched blk_child)", afterIdx) + } + + // before → insert at index 0 (before blk_a) + beforeIdx, err := findInsertIndexByKeyword(blocks, rootChildren, "architecture diagram", true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if beforeIdx != 0 { + t.Fatalf("before: got %d, want 0 (before blk_a, ancestor of first-matched blk_child)", beforeIdx) + } +} + +func TestFindInsertIndexByKeywordNotFound(t *testing.T) { + t.Parallel() + + blocks := []map[string]interface{}{ + { + "block_id": "blk_a", + "parent_id": "root", + "text": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "hello world"}}, + }, + }, + }, + } + rootChildren := []interface{}{"blk_a"} + + _, err := findInsertIndexByKeyword(blocks, rootChildren, "nonexistent keyword", false) + if err == nil { + t.Fatal("expected error for missing keyword, got nil") + } +} + +func TestFindInsertIndexByKeywordBeforeMode(t *testing.T) { + t.Parallel() + + blocks := []map[string]interface{}{ + { + "block_id": "blk_a", + "parent_id": "root", + "heading1": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "Introduction"}}, + }, + }, + }, + { + "block_id": "blk_b", + "parent_id": "root", + "heading1": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "Architecture"}}, + }, + }, + }, + } + rootChildren := []interface{}{"blk_a", "blk_b"} + + // before=true: should return index 1 (before blk_b, which is rootChildren[1]) + idx, err := findInsertIndexByKeyword(blocks, rootChildren, "Architecture", true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if idx != 1 { + t.Fatalf("findInsertIndexByKeyword(before) = %d, want 1", idx) + } + + // before=false: should return index 2 (after blk_b) + idx, err = findInsertIndexByKeyword(blocks, rootChildren, "Architecture", false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if idx != 2 { + t.Fatalf("findInsertIndexByKeyword(after) = %d, want 2", idx) + } } func TestExtractCreatedBlockTargetsForImage(t *testing.T) { From 20fb93d56f22a633e7a13f0acb3bcb53530a2856 Mon Sep 17 00:00:00 2001 From: baiqing Date: Wed, 8 Apr 2026 18:39:04 +0800 Subject: [PATCH 2/6] test(doc): add fetchAllBlocks pagination and keyword dry-run coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - TestFetchAllBlocksPaginationViaExecute: exercises fetchAllBlocks via a full Execute flow with --after-keyword, covering multi-page block listing (fetchAllBlocks was previously at 0% coverage) - TestDocMediaInsertDryRunWithAfterKeyword: verifies that the dry-run output includes a block-listing step and mentions "search blocks" in the description when --after-keyword is provided fetchAllBlocks coverage: 0% → 76.2% --- shortcuts/doc/doc_media_test.go | 134 ++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) diff --git a/shortcuts/doc/doc_media_test.go b/shortcuts/doc/doc_media_test.go index ee810f676..fec2987f0 100644 --- a/shortcuts/doc/doc_media_test.go +++ b/shortcuts/doc/doc_media_test.go @@ -323,3 +323,137 @@ func decodeDocDryRun(t *testing.T, dryAPI *common.DryRunAPI) docDryRunOutput { } return dry } + +// TestFetchAllBlocksPaginationViaExecute verifies that fetchAllBlocks accumulates +// blocks across multiple pages. It exercises the code path indirectly by running +// Execute with --after-keyword so that fetchAllBlocks is called. +func TestFetchAllBlocksPaginationViaExecute(t *testing.T) { + f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("fetch-blocks-app")) + + // Root block + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/doxcnFB/blocks/doxcnFB", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "block": map[string]interface{}{ + "block_id": "doxcnFB", + "children": []interface{}{"blk_1", "blk_2"}, + }, + }, + }, + }) + // Page 1 of all blocks — contains the keyword block + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/doxcnFB/blocks", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "items": []interface{}{ + map[string]interface{}{ + "block_id": "blk_1", + "parent_id": "doxcnFB", + "text": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "Introduction section"}}, + }, + }, + }, + map[string]interface{}{ + "block_id": "blk_2", + "parent_id": "doxcnFB", + "text": map[string]interface{}{ + "elements": []interface{}{ + map[string]interface{}{"text_run": map[string]interface{}{"content": "Other content"}}, + }, + }, + }, + }, + "has_more": false, + }, + }, + }) + // Create block response + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/docx/v1/documents/doxcnFB/blocks/doxcnFB/children", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "children": []interface{}{ + map[string]interface{}{"block_id": "blk_new", "block_type": 27, "image": map[string]interface{}{}}, + }, + }, + }, + }) + // Upload response + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/drive/v1/medias/upload_all", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{"file_token": "ftok_abc"}, + }, + }) + // Batch update response + reg.Register(&httpmock.Stub{ + Method: "PATCH", + URL: "/open-apis/docx/v1/documents/doxcnFB/blocks/batch_update", + Body: map[string]interface{}{"code": 0, "msg": "ok", "data": map[string]interface{}{}}, + }) + + tmpDir := t.TempDir() + withDocsWorkingDir(t, tmpDir) + writeSizedDocTestFile(t, "img.png", 100) + + err := mountAndRunDocs(t, DocMediaInsert, []string{ + "+media-insert", + "--doc", "doxcnFB", + "--file", "img.png", + "--after-keyword", "Introduction", + "--as", "bot", + }, f, nil) + if err != nil { + t.Fatalf("Execute() error: %v", err) + } +} + +// TestDocMediaInsertDryRunWithAfterKeyword verifies the dry-run output describes +// the extra block-listing step when --after-keyword is provided. +func TestDocMediaInsertDryRunWithAfterKeyword(t *testing.T) { + tmpDir := t.TempDir() + withDocsWorkingDir(t, tmpDir) + writeSizedDocTestFile(t, "img.png", 100) + + cmd := &cobra.Command{Use: "docs +media-insert"} + cmd.Flags().String("file", "", "") + cmd.Flags().String("doc", "", "") + cmd.Flags().String("type", "image", "") + cmd.Flags().String("align", "", "") + cmd.Flags().String("caption", "", "") + cmd.Flags().String("after-keyword", "", "") + cmd.Flags().String("before-keyword", "", "") + _ = cmd.Flags().Set("file", "img.png") + _ = cmd.Flags().Set("doc", "doxcnABCDEF") + _ = cmd.Flags().Set("after-keyword", "Introduction") + + rt := common.TestNewRuntimeContext(cmd, nil) + dry := decodeDocDryRun(t, DocMediaInsert.DryRun(context.Background(), rt)) + + foundListBlocks := false + for _, step := range dry.API { + if strings.Contains(step.URL, "/blocks") && + !strings.Contains(step.URL, "children") && + !strings.Contains(step.URL, "batch_update") { + foundListBlocks = true + } + } + if !foundListBlocks { + t.Fatal("dry-run should include a block-listing step for --after-keyword") + } + if !strings.Contains(dry.Description, "search blocks") { + t.Fatalf("dry-run description should mention 'search blocks', got: %s", dry.Description) + } +} From 5465581ddda385261a99ce9588b06e9965f8d9be Mon Sep 17 00:00:00 2001 From: baiqing Date: Thu, 9 Apr 2026 04:02:34 +0800 Subject: [PATCH 3/6] refactor(doc): use MCP locate-doc for keyword-based block positioning Replace fetchAllBlocks + keyword scan with MCP locate-doc tool, consistent with DriveAddComment. Flags changed from --after-keyword / --before-keyword to --selection-with-ellipsis + --before. --- shortcuts/doc/doc_media_insert.go | 248 ++++++----- shortcuts/doc/doc_media_insert_test.go | 546 +++++++++++-------------- shortcuts/doc/doc_media_test.go | 134 ------ 3 files changed, 354 insertions(+), 574 deletions(-) diff --git a/shortcuts/doc/doc_media_insert.go b/shortcuts/doc/doc_media_insert.go index 35fd88553..4ccc1c25f 100644 --- a/shortcuts/doc/doc_media_insert.go +++ b/shortcuts/doc/doc_media_insert.go @@ -24,7 +24,7 @@ var alignMap = map[string]int{ var DocMediaInsert = common.Shortcut{ Service: "docs", Command: "+media-insert", - Description: "Insert a local image or file into a Lark document (4-step orchestration + auto-rollback); appends to end by default, or inserts relative to a keyword with --after-keyword/--before-keyword", + Description: "Insert a local image or file into a Lark document (4-step orchestration + auto-rollback); appends to end by default, or inserts relative to a text selection with --selection-with-ellipsis", Risk: "write", Scopes: []string{"docs:document.media:upload", "docx:document:write_only", "docx:document:readonly"}, AuthTypes: []string{"user", "bot"}, @@ -34,8 +34,8 @@ var DocMediaInsert = common.Shortcut{ {Name: "type", Default: "image", Desc: "type: image | file"}, {Name: "align", Desc: "alignment: left | center | right"}, {Name: "caption", Desc: "image caption text"}, - {Name: "after-keyword", Desc: "insert after the first block whose text contains this keyword (case-insensitive); mutually exclusive with --before-keyword"}, - {Name: "before-keyword", Desc: "insert before the first block whose text contains this keyword (case-insensitive); mutually exclusive with --after-keyword"}, + {Name: "selection-with-ellipsis", Desc: "plain text (or 'start...end') that identifies the target block; the media is inserted after that block by default"}, + {Name: "before", Type: "bool", Desc: "insert before the matched block instead of after (requires --selection-with-ellipsis)"}, }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { docRef, err := parseDocumentRef(runtime.Str("doc")) @@ -45,8 +45,8 @@ var DocMediaInsert = common.Shortcut{ if docRef.Kind == "doc" { return output.ErrValidation("docs +media-insert only supports docx documents; use a docx token/URL or a wiki URL that resolves to docx") } - if runtime.Str("after-keyword") != "" && runtime.Str("before-keyword") != "" { - return output.ErrValidation("--after-keyword and --before-keyword are mutually exclusive") + if runtime.Bool("before") && strings.TrimSpace(runtime.Str("selection-with-ellipsis")) == "" { + return output.ErrValidation("--before requires --selection-with-ellipsis") } return nil }, @@ -61,9 +61,8 @@ var DocMediaInsert = common.Shortcut{ filePath := runtime.Str("file") mediaType := runtime.Str("type") caption := runtime.Str("caption") - afterKeyword := runtime.Str("after-keyword") - beforeKeyword := runtime.Str("before-keyword") - hasKeyword := afterKeyword != "" || beforeKeyword != "" + selection := strings.TrimSpace(runtime.Str("selection-with-ellipsis")) + hasSelection := selection != "" parentType := parentTypeForMediaType(mediaType) createBlockData := buildCreateBlockData(mediaType, 0) @@ -75,35 +74,47 @@ var DocMediaInsert = common.Shortcut{ if docRef.Kind == "wiki" { totalSteps++ } - if hasKeyword { + if hasSelection { totalSteps++ } + positionLabel := map[bool]string{true: "before", false: "after"}[runtime.Bool("before")] + if docRef.Kind == "wiki" { documentID = "" stepBase = 2 d.Desc(fmt.Sprintf("%d-step orchestration: resolve wiki → query root →%s create block → upload file → bind to block (auto-rollback on failure)", - totalSteps, map[bool]string{true: " search blocks →", false: ""}[hasKeyword])). + totalSteps, map[bool]string{true: " locate-doc →", false: ""}[hasSelection])). GET("/open-apis/wiki/v2/spaces/get_node"). Desc("[1] Resolve wiki node to docx document"). Params(map[string]interface{}{"token": docRef.Token}) } else { d.Desc(fmt.Sprintf("%d-step orchestration: query root →%s create block → upload file → bind to block (auto-rollback on failure)", - totalSteps, map[bool]string{true: " search blocks →", false: ""}[hasKeyword])) + totalSteps, map[bool]string{true: " locate-doc →", false: ""}[hasSelection])) } d. GET("/open-apis/docx/v1/documents/:document_id/blocks/:document_id"). Desc(fmt.Sprintf("[%d] Get document root block", stepBase)) - if hasKeyword { - kw := afterKeyword - if kw == "" { - kw = beforeKeyword + if hasSelection { + mcpEndpoint := common.MCPEndpoint(runtime.Config.Brand) + mcpArgs := map[string]interface{}{ + "doc_id": documentID, + "selection_with_ellipsis": selection, + "limit": 1, } - d.GET("/open-apis/docx/v1/documents/:document_id/blocks"). - Desc(fmt.Sprintf("[%d] List all blocks to find insert position for keyword %q", stepBase+1, kw)). - Params(map[string]interface{}{"page_size": 200}) + d.POST(mcpEndpoint). + Desc(fmt.Sprintf("[%d] MCP locate-doc: find block matching selection (%s)", stepBase+1, positionLabel)). + Body(map[string]interface{}{ + "method": "tools/call", + "params": map[string]interface{}{ + "name": "locate-doc", + "arguments": mcpArgs, + }, + }). + Set("mcp_tool", "locate-doc"). + Set("args", mcpArgs) stepBase++ } @@ -164,26 +175,20 @@ var DocMediaInsert = common.Shortcut{ } fmt.Fprintf(runtime.IO().ErrOut, "Root block ready: %s (%d children)\n", parentBlockID, insertIndex) - afterKeyword := runtime.Str("after-keyword") - beforeKeyword := runtime.Str("before-keyword") - keyword := afterKeyword - before := false - if beforeKeyword != "" { - keyword = beforeKeyword - before = true - } - if keyword != "" { - fmt.Fprintf(runtime.IO().ErrOut, "Searching blocks for keyword: %q\n", keyword) - allBlocks, err := fetchAllBlocks(runtime, documentID) - if err != nil { - return err - } - idx, err := findInsertIndexByKeyword(allBlocks, rootChildren, keyword, before) + selection := strings.TrimSpace(runtime.Str("selection-with-ellipsis")) + if selection != "" { + before := runtime.Bool("before") + fmt.Fprintf(runtime.IO().ErrOut, "Locating block matching selection: %q\n", selection) + idx, err := locateInsertIndex(runtime, documentID, selection, rootChildren, before) if err != nil { return err } insertIndex = idx - fmt.Fprintf(runtime.IO().ErrOut, "Keyword found: inserting at index %d\n", insertIndex) + posLabel := "after" + if before { + posLabel = "before" + } + fmt.Fprintf(runtime.IO().ErrOut, "locate-doc matched: inserting %s at index %d\n", posLabel, insertIndex) } // Step 2: Create an empty block at the target position @@ -373,90 +378,48 @@ func extractAppendTarget(rootData map[string]interface{}, fallbackBlockID string return parentBlockID, len(children), children, nil } -// fetchAllBlocks retrieves all blocks in a document via paginated API calls. -func fetchAllBlocks(runtime *common.RuntimeContext, documentID string) ([]map[string]interface{}, error) { - const maxPages = 50 - var all []map[string]interface{} - pageToken := "" - - for page := 0; page < maxPages; page++ { - params := map[string]interface{}{"page_size": 200} - if pageToken != "" { - params["page_token"] = pageToken - } - data, err := runtime.CallAPI("GET", - fmt.Sprintf("/open-apis/docx/v1/documents/%s/blocks", validate.EncodePathSegment(documentID)), - params, nil) - if err != nil { - return nil, err - } - - items, _ := data["items"].([]interface{}) - for _, item := range items { - if block, ok := item.(map[string]interface{}); ok { - all = append(all, block) - } - } - - hasMore, _ := data["has_more"].(bool) - if !hasMore { - break - } - pageToken, _ = data["page_token"].(string) - if pageToken == "" { - break - } +// locateInsertIndex uses the MCP locate-doc tool to find the root-level index +// at which to insert relative to the block matching selection. It walks the +// parent_id chain (using single-block GET calls when needed) to resolve nested +// blocks to their top-level ancestor in rootChildren. +func locateInsertIndex(runtime *common.RuntimeContext, documentID string, selection string, rootChildren []interface{}, before bool) (int, error) { + args := map[string]interface{}{ + "doc_id": documentID, + "selection_with_ellipsis": selection, + "limit": 1, + } + result, err := common.CallMCPTool(runtime, "locate-doc", args) + if err != nil { + return 0, err } - return all, nil -} -// extractBlockPlainText returns the concatenated plain text of a block -// by inspecting all known text-bearing sub-maps (text, heading1-9, bullet, -// ordered, todo, code, quote). All these block types share the same -// {elements: [{text_run: {content: "..."}}]} structure. -func extractBlockPlainText(block map[string]interface{}) string { - keys := []string{"text", "heading1", "heading2", "heading3", "heading4", "heading5", - "heading6", "heading7", "heading8", "heading9", "bullet", "ordered", "todo", "code", "quote"} - for _, key := range keys { - sub, ok := block[key].(map[string]interface{}) - if !ok { - continue - } - elements, _ := sub["elements"].([]interface{}) - var sb strings.Builder - for _, el := range elements { - elem, _ := el.(map[string]interface{}) - textRun, _ := elem["text_run"].(map[string]interface{}) - content, _ := textRun["content"].(string) - sb.WriteString(content) - } - if sb.Len() > 0 { - return sb.String() - } + matches := common.GetSlice(result, "matches") + if len(matches) == 0 { + return 0, output.ErrWithHint( + output.ExitValidation, + "no_match", + fmt.Sprintf("locate-doc did not find any block matching selection %q", selection), + "check spelling or use 'start...end' syntax to narrow the selection", + ) } - return "" -} -// findInsertIndexByKeyword finds the insert position relative to the first block -// whose plain text contains keyword (case-insensitive). When before is false it -// inserts after the matched root-level block; when before is true it inserts before. -// It walks parent_id chains to handle nested blocks. -func findInsertIndexByKeyword(blocks []map[string]interface{}, rootChildren []interface{}, keyword string, before bool) (int, error) { - lowerKw := strings.ToLower(keyword) - - // Build a blockID → block map and a blockID → parent map for quick lookup. - blockByID := make(map[string]map[string]interface{}, len(blocks)) - parentByID := make(map[string]string, len(blocks)) - for _, b := range blocks { - id, _ := b["block_id"].(string) - if id != "" { - blockByID[id] = b - parentID, _ := b["parent_id"].(string) - parentByID[id] = parentID + matchMap, _ := matches[0].(map[string]interface{}) + anchorBlockID := common.GetString(matchMap, "anchor_block_id") + if anchorBlockID == "" { + // Fall back to first block entry if anchor_block_id is absent. + blocks := common.GetSlice(matchMap, "blocks") + if len(blocks) > 0 { + if b, ok := blocks[0].(map[string]interface{}); ok { + anchorBlockID = common.GetString(b, "block_id") + } } } + if anchorBlockID == "" { + return 0, output.Errorf(output.ExitAPI, "api_error", "locate-doc response missing anchor_block_id") + } + parentBlockID := common.GetString(matchMap, "parent_block_id") - // Build root children set for O(1) membership test. + // Build root children set for O(1) lookup. rootSet := make(map[string]int, len(rootChildren)) for i, c := range rootChildren { if id, ok := c.(string); ok { @@ -464,31 +427,52 @@ func findInsertIndexByKeyword(blocks []map[string]interface{}, rootChildren []in } } - // Search blocks in document order. - for _, b := range blocks { - text := extractBlockPlainText(b) - if text == "" || !strings.Contains(strings.ToLower(text), lowerKw) { - continue - } - // Found a match — walk up parent chain to find its top-level ancestor in rootChildren. - id, _ := b["block_id"].(string) - cur := id - for { - if idx, ok := rootSet[cur]; ok { - if before { - return idx, nil // insert before this root-level block - } - return idx + 1, nil // insert after this root-level block + // Walk up the parent chain. locate-doc already gives us one level of parent, + // so most cases need zero extra API calls. + cur := anchorBlockID + nextParent := parentBlockID + visited := map[string]bool{} + const maxDepth = 8 + for depth := 0; depth < maxDepth; depth++ { + if visited[cur] { + break + } + visited[cur] = true + + if idx, ok := rootSet[cur]; ok { + if before { + return idx, nil } - parent := parentByID[cur] - if parent == "" || parent == cur { - break + return idx + 1, nil + } + + // Advance: use the parent hint we already have, or fetch from API. + parent := nextParent + nextParent = "" // clear hint after first use + if parent == "" || parent == cur { + // Need to fetch this block to find its parent. + data, err := runtime.CallAPI("GET", + fmt.Sprintf("/open-apis/docx/v1/documents/%s/blocks/%s", + validate.EncodePathSegment(documentID), validate.EncodePathSegment(cur)), + nil, nil) + if err != nil { + return 0, err } - cur = parent + block := common.GetMap(data, "block") + parent = common.GetString(block, "parent_id") + } + if parent == "" || parent == cur { + break } - return 0, output.ErrValidation("block containing keyword %q is not reachable from document root; try a top-level heading", keyword) + cur = parent } - return 0, output.ErrValidation("no block found containing keyword %q", keyword) + + return 0, output.ErrWithHint( + output.ExitValidation, + "block_not_reachable", + fmt.Sprintf("block matching selection %q is not reachable from document root", selection), + "try a top-level heading or paragraph as the selection", + ) } func extractCreatedBlockTargets(createData map[string]interface{}, mediaType string) (blockID, uploadParentNode, replaceBlockID string) { diff --git a/shortcuts/doc/doc_media_insert_test.go b/shortcuts/doc/doc_media_insert_test.go index abff5c8ed..432b78ac7 100644 --- a/shortcuts/doc/doc_media_insert_test.go +++ b/shortcuts/doc/doc_media_insert_test.go @@ -4,8 +4,17 @@ package doc import ( + "context" + "encoding/json" "reflect" + "strings" "testing" + + "github.com/spf13/cobra" + + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/httpmock" + "github.com/larksuite/cli/shortcuts/common" ) func TestBuildCreateBlockDataUsesConcreteAppendIndex(t *testing.T) { @@ -124,359 +133,280 @@ func TestExtractAppendTargetUsesRootChildrenCount(t *testing.T) { } } -func TestExtractBlockPlainTextParagraph(t *testing.T) { - t.Parallel() - - block := map[string]interface{}{ - "block_type": 2, - "text": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "Hello "}}, - map[string]interface{}{"text_run": map[string]interface{}{"content": "World"}}, - }, - }, - } - got := extractBlockPlainText(block) - if got != "Hello World" { - t.Fatalf("extractBlockPlainText() = %q, want %q", got, "Hello World") - } -} - -func TestExtractBlockPlainTextHeading(t *testing.T) { - t.Parallel() - - block := map[string]interface{}{ - "block_type": 3, - "heading1": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "My Section"}}, +// buildLocateDocMCPResponse builds a JSON-RPC 2.0 response for a locate-doc MCP call. +func buildLocateDocMCPResponse(matches []map[string]interface{}) map[string]interface{} { + resultJSON, _ := json.Marshal(map[string]interface{}{"matches": matches}) + return map[string]interface{}{ + "jsonrpc": "2.0", + "id": "test-id", + "result": map[string]interface{}{ + "content": []interface{}{ + map[string]interface{}{ + "type": "text", + "text": string(resultJSON), + }, }, }, } - got := extractBlockPlainText(block) - if got != "My Section" { - t.Fatalf("extractBlockPlainText() = %q, want %q", got, "My Section") - } } -func TestExtractBlockPlainTextBulletOrderedTodo(t *testing.T) { - t.Parallel() - - cases := []struct { - key string - content string - }{ - {"bullet", "太空山"}, - {"ordered", "第一步操作"}, - {"todo", "完成任务"}, - } - for _, tc := range cases { - block := map[string]interface{}{ - "block_type": 0, - tc.key: map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": tc.content}}, +func registerInsertWithSelectionStubs(reg interface { + Register(*httpmock.Stub) +}, docID, anchorBlockID, parentBlockID string) { + // Root block + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID, + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "block": map[string]interface{}{ + "block_id": docID, + "children": []interface{}{"blk_a", "blk_b"}, }, }, - } - got := extractBlockPlainText(block) - if got != tc.content { - t.Errorf("extractBlockPlainText(%q) = %q, want %q", tc.key, got, tc.content) - } - } -} - -func TestExtractBlockPlainTextEmpty(t *testing.T) { - t.Parallel() - - block := map[string]interface{}{"block_type": 27, "image": map[string]interface{}{}} - if got := extractBlockPlainText(block); got != "" { - t.Fatalf("extractBlockPlainText(image) = %q, want empty", got) - } -} - -func TestFindInsertIndexByKeywordFindsAfterBlock(t *testing.T) { - t.Parallel() - - blocks := []map[string]interface{}{ - { - "block_id": "root", - "parent_id": "", }, - { - "block_id": "blk_a", - "parent_id": "root", - "heading1": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "Introduction"}}, + }) + // MCP locate-doc + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "mcp.feishu.cn/mcp", + Body: buildLocateDocMCPResponse([]map[string]interface{}{ + {"anchor_block_id": anchorBlockID, "parent_block_id": parentBlockID}, + }), + }) + // Create block + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID + "/children", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "children": []interface{}{ + map[string]interface{}{"block_id": "blk_new", "block_type": 27, "image": map[string]interface{}{}}, }, }, }, - { - "block_id": "blk_b", - "parent_id": "root", - "heading1": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "Architecture"}}, - }, - }, + }) + // Upload + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/drive/v1/medias/upload_all", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{"file_token": "ftok_test"}, }, - } - rootChildren := []interface{}{"blk_a", "blk_b"} - - idx, err := findInsertIndexByKeyword(blocks, rootChildren, "Introduction", false) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if idx != 1 { - t.Fatalf("findInsertIndexByKeyword() = %d, want 1", idx) - } + }) + // Batch update + reg.Register(&httpmock.Stub{ + Method: "PATCH", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/batch_update", + Body: map[string]interface{}{"code": 0, "msg": "ok", "data": map[string]interface{}{}}, + }) } -func TestFindInsertIndexByKeywordCaseInsensitive(t *testing.T) { - t.Parallel() - - blocks := []map[string]interface{}{ - { - "block_id": "blk_a", - "parent_id": "root", - "heading2": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "Core Architecture"}}, - }, - }, - }, - } - rootChildren := []interface{}{"blk_a"} - - idx, err := findInsertIndexByKeyword(blocks, rootChildren, "core architecture", false) +// TestLocateInsertIndexAfterModeViaExecute verifies that --selection-with-ellipsis +// inserts after the matched root-level block (index = root index + 1). +func TestLocateInsertIndexAfterModeViaExecute(t *testing.T) { + f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("locate-after-app")) + registerInsertWithSelectionStubs(reg, "doxcnSEL", "blk_a", "doxcnSEL") + + tmpDir := t.TempDir() + withDocsWorkingDir(t, tmpDir) + writeSizedDocTestFile(t, "img.png", 100) + + err := mountAndRunDocs(t, DocMediaInsert, []string{ + "+media-insert", + "--doc", "doxcnSEL", + "--file", "img.png", + "--selection-with-ellipsis", "Introduction", + "--as", "bot", + }, f, nil) if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if idx != 1 { - t.Fatalf("findInsertIndexByKeyword() = %d, want 1", idx) + t.Fatalf("Execute() error: %v", err) } } -func TestFindInsertIndexByKeywordNestedBlock(t *testing.T) { - t.Parallel() - - // Nested block: blk_child is inside blk_section (root child) - blocks := []map[string]interface{}{ - { - "block_id": "blk_section", - "parent_id": "root", - "heading1": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "Section"}}, - }, - }, - }, - { - "block_id": "blk_child", - "parent_id": "blk_section", - "text": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "nested content here"}}, - }, - }, - }, - } - rootChildren := []interface{}{"blk_section"} - - // Matching a nested block should insert after its root-level ancestor. - idx, err := findInsertIndexByKeyword(blocks, rootChildren, "nested content", false) +// TestLocateInsertIndexBeforeModeViaExecute verifies that --before inserts before +// the matched root-level block. +func TestLocateInsertIndexBeforeModeViaExecute(t *testing.T) { + f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("locate-before-app")) + registerInsertWithSelectionStubs(reg, "doxcnSEL2", "blk_b", "doxcnSEL2") + + tmpDir := t.TempDir() + withDocsWorkingDir(t, tmpDir) + writeSizedDocTestFile(t, "img.png", 100) + + err := mountAndRunDocs(t, DocMediaInsert, []string{ + "+media-insert", + "--doc", "doxcnSEL2", + "--file", "img.png", + "--selection-with-ellipsis", "Architecture", + "--before", + "--as", "bot", + }, f, nil) if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if idx != 1 { - t.Fatalf("findInsertIndexByKeyword() nested = %d, want 1", idx) + t.Fatalf("Execute() error: %v", err) } } -// TestFindInsertIndexByKeywordDuplicateUsesFirst verifies that when the same -// keyword appears in multiple blocks, the function always anchors to the -// first matching block in document order (the slice iteration order of blocks). -func TestFindInsertIndexByKeywordDuplicateUsesFirst(t *testing.T) { - t.Parallel() - - // Three root-level blocks, all containing "overview". - // Document order: blk_a (index 0) → blk_b (index 1) → blk_c (index 2). - blocks := []map[string]interface{}{ - { - "block_id": "blk_a", - "parent_id": "root", - "text": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "overview: section one"}}, +// TestLocateInsertIndexNestedBlockViaExecute verifies that a nested block's +// parent_block_id hint is used to walk to the root-level ancestor. +func TestLocateInsertIndexNestedBlockViaExecute(t *testing.T) { + f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("locate-nested-app")) + + docID := "doxcnNESTED" + // Root block with blk_section and blk_other as children + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID, + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "block": map[string]interface{}{ + "block_id": docID, + "children": []interface{}{"blk_section", "blk_other"}, }, }, }, - { - "block_id": "blk_b", - "parent_id": "root", - "text": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "overview: section two"}}, + }) + // MCP locate-doc returns blk_child nested under blk_section + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "mcp.feishu.cn/mcp", + Body: buildLocateDocMCPResponse([]map[string]interface{}{ + {"anchor_block_id": "blk_child", "parent_block_id": "blk_section"}, + }), + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID + "/children", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "children": []interface{}{ + map[string]interface{}{"block_id": "blk_new", "block_type": 27, "image": map[string]interface{}{}}, }, }, }, - { - "block_id": "blk_c", - "parent_id": "root", - "text": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "overview: section three"}}, - }, - }, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/drive/v1/medias/upload_all", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{"file_token": "ftok_nested"}, }, - } - rootChildren := []interface{}{"blk_a", "blk_b", "blk_c"} - - // --after-keyword: should insert after blk_a (index 0 → return 1), not blk_b or blk_c. - afterIdx, err := findInsertIndexByKeyword(blocks, rootChildren, "overview", false) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if afterIdx != 1 { - t.Fatalf("after: got index %d, want 1 (after first match blk_a)", afterIdx) - } - - // --before-keyword: should insert before blk_a (index 0 → return 0). - beforeIdx, err := findInsertIndexByKeyword(blocks, rootChildren, "overview", true) + }) + reg.Register(&httpmock.Stub{ + Method: "PATCH", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/batch_update", + Body: map[string]interface{}{"code": 0, "msg": "ok", "data": map[string]interface{}{}}, + }) + + tmpDir := t.TempDir() + withDocsWorkingDir(t, tmpDir) + writeSizedDocTestFile(t, "img.png", 100) + + err := mountAndRunDocs(t, DocMediaInsert, []string{ + "+media-insert", + "--doc", docID, + "--file", "img.png", + "--selection-with-ellipsis", "nested content", + "--as", "bot", + }, f, nil) if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if beforeIdx != 0 { - t.Fatalf("before: got index %d, want 0 (before first match blk_a)", beforeIdx) + t.Fatalf("Execute() error: %v", err) } } -// TestFindInsertIndexByKeywordDuplicateNestedAndTopLevel verifies that when the -// keyword appears in both a nested block (inside blk_a) and a later top-level -// block (blk_b), the function uses the earlier document-order match — which -// resolves upward to blk_a. -func TestFindInsertIndexByKeywordDuplicateNestedAndTopLevel(t *testing.T) { - t.Parallel() - - blocks := []map[string]interface{}{ - // blk_a is a top-level section - { - "block_id": "blk_a", - "parent_id": "root", - "heading1": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "Section A"}}, - }, - }, - }, - // blk_child is nested inside blk_a and contains the keyword first - { - "block_id": "blk_child", - "parent_id": "blk_a", - "text": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "architecture diagram"}}, - }, - }, - }, - // blk_b is a second top-level block that also contains the keyword - { - "block_id": "blk_b", - "parent_id": "root", - "text": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "architecture diagram"}}, +// TestLocateInsertIndexNoMatchReturnsError verifies that when locate-doc returns +// no matches, Execute returns a descriptive error. +func TestLocateInsertIndexNoMatchReturnsError(t *testing.T) { + f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("locate-nomatch-app")) + + docID := "doxcnNOMATCH" + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID, + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "block": map[string]interface{}{ + "block_id": docID, + "children": []interface{}{"blk_a"}, }, }, }, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "mcp.feishu.cn/mcp", + Body: buildLocateDocMCPResponse([]map[string]interface{}{}), + }) + + tmpDir := t.TempDir() + withDocsWorkingDir(t, tmpDir) + writeSizedDocTestFile(t, "img.png", 100) + + err := mountAndRunDocs(t, DocMediaInsert, []string{ + "+media-insert", + "--doc", docID, + "--file", "img.png", + "--selection-with-ellipsis", "nonexistent text", + "--as", "bot", + }, f, nil) + if err == nil { + t.Fatal("expected no-match error, got nil") } - rootChildren := []interface{}{"blk_a", "blk_b"} - - // blk_child appears before blk_b in blocks slice → first match is blk_child, - // which walks up to blk_a (rootChildren index 0). - // after → insert at index 1 (after blk_a) - afterIdx, err := findInsertIndexByKeyword(blocks, rootChildren, "architecture diagram", false) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if afterIdx != 1 { - t.Fatalf("after: got %d, want 1 (after blk_a, ancestor of first-matched blk_child)", afterIdx) - } - - // before → insert at index 0 (before blk_a) - beforeIdx, err := findInsertIndexByKeyword(blocks, rootChildren, "architecture diagram", true) - if err != nil { + if !strings.Contains(err.Error(), "no_match") && !strings.Contains(err.Error(), "did not find") { t.Fatalf("unexpected error: %v", err) } - if beforeIdx != 0 { - t.Fatalf("before: got %d, want 0 (before blk_a, ancestor of first-matched blk_child)", beforeIdx) - } -} - -func TestFindInsertIndexByKeywordNotFound(t *testing.T) { - t.Parallel() - - blocks := []map[string]interface{}{ - { - "block_id": "blk_a", - "parent_id": "root", - "text": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "hello world"}}, - }, - }, - }, - } - rootChildren := []interface{}{"blk_a"} - - _, err := findInsertIndexByKeyword(blocks, rootChildren, "nonexistent keyword", false) - if err == nil { - t.Fatal("expected error for missing keyword, got nil") - } } -func TestFindInsertIndexByKeywordBeforeMode(t *testing.T) { +// TestLocateInsertIndexDryRunIncludesMCPStep verifies that the dry-run output +// includes a locate-doc MCP step when --selection-with-ellipsis is provided. +func TestLocateInsertIndexDryRunIncludesMCPStep(t *testing.T) { t.Parallel() - blocks := []map[string]interface{}{ - { - "block_id": "blk_a", - "parent_id": "root", - "heading1": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "Introduction"}}, - }, - }, - }, - { - "block_id": "blk_b", - "parent_id": "root", - "heading1": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "Architecture"}}, - }, - }, - }, - } - rootChildren := []interface{}{"blk_a", "blk_b"} - - // before=true: should return index 1 (before blk_b, which is rootChildren[1]) - idx, err := findInsertIndexByKeyword(blocks, rootChildren, "Architecture", true) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if idx != 1 { - t.Fatalf("findInsertIndexByKeyword(before) = %d, want 1", idx) + cmd := &cobra.Command{Use: "docs +media-insert"} + cmd.Flags().String("file", "", "") + cmd.Flags().String("doc", "", "") + cmd.Flags().String("type", "image", "") + cmd.Flags().String("align", "", "") + cmd.Flags().String("caption", "", "") + cmd.Flags().String("selection-with-ellipsis", "", "") + cmd.Flags().Bool("before", false, "") + _ = cmd.Flags().Set("file", "img.png") + _ = cmd.Flags().Set("doc", "doxcnABCDEF") + _ = cmd.Flags().Set("selection-with-ellipsis", "Introduction") + + rt := common.TestNewRuntimeContext(cmd, docsTestConfigWithAppID("dry-run-app")) + dryAPI := DocMediaInsert.DryRun(context.Background(), rt) + raw, _ := json.Marshal(dryAPI) + + var dry struct { + Description string `json:"description"` + API []struct { + Desc string `json:"desc"` + URL string `json:"url"` + } `json:"api"` + } + if err := json.Unmarshal(raw, &dry); err != nil { + t.Fatalf("decode dry-run: %v", err) + } + + foundMCP := false + for _, step := range dry.API { + if strings.Contains(step.Desc, "locate-doc") { + foundMCP = true + } } - - // before=false: should return index 2 (after blk_b) - idx, err = findInsertIndexByKeyword(blocks, rootChildren, "Architecture", false) - if err != nil { - t.Fatalf("unexpected error: %v", err) + if !foundMCP { + t.Fatalf("dry-run should include a locate-doc step, got: %+v", dry.API) } - if idx != 2 { - t.Fatalf("findInsertIndexByKeyword(after) = %d, want 2", idx) + if !strings.Contains(dry.Description, "locate-doc") { + t.Fatalf("dry-run description should mention 'locate-doc', got: %s", dry.Description) } } diff --git a/shortcuts/doc/doc_media_test.go b/shortcuts/doc/doc_media_test.go index fec2987f0..ee810f676 100644 --- a/shortcuts/doc/doc_media_test.go +++ b/shortcuts/doc/doc_media_test.go @@ -323,137 +323,3 @@ func decodeDocDryRun(t *testing.T, dryAPI *common.DryRunAPI) docDryRunOutput { } return dry } - -// TestFetchAllBlocksPaginationViaExecute verifies that fetchAllBlocks accumulates -// blocks across multiple pages. It exercises the code path indirectly by running -// Execute with --after-keyword so that fetchAllBlocks is called. -func TestFetchAllBlocksPaginationViaExecute(t *testing.T) { - f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("fetch-blocks-app")) - - // Root block - reg.Register(&httpmock.Stub{ - Method: "GET", - URL: "/open-apis/docx/v1/documents/doxcnFB/blocks/doxcnFB", - Body: map[string]interface{}{ - "code": 0, "msg": "ok", - "data": map[string]interface{}{ - "block": map[string]interface{}{ - "block_id": "doxcnFB", - "children": []interface{}{"blk_1", "blk_2"}, - }, - }, - }, - }) - // Page 1 of all blocks — contains the keyword block - reg.Register(&httpmock.Stub{ - Method: "GET", - URL: "/open-apis/docx/v1/documents/doxcnFB/blocks", - Body: map[string]interface{}{ - "code": 0, "msg": "ok", - "data": map[string]interface{}{ - "items": []interface{}{ - map[string]interface{}{ - "block_id": "blk_1", - "parent_id": "doxcnFB", - "text": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "Introduction section"}}, - }, - }, - }, - map[string]interface{}{ - "block_id": "blk_2", - "parent_id": "doxcnFB", - "text": map[string]interface{}{ - "elements": []interface{}{ - map[string]interface{}{"text_run": map[string]interface{}{"content": "Other content"}}, - }, - }, - }, - }, - "has_more": false, - }, - }, - }) - // Create block response - reg.Register(&httpmock.Stub{ - Method: "POST", - URL: "/open-apis/docx/v1/documents/doxcnFB/blocks/doxcnFB/children", - Body: map[string]interface{}{ - "code": 0, "msg": "ok", - "data": map[string]interface{}{ - "children": []interface{}{ - map[string]interface{}{"block_id": "blk_new", "block_type": 27, "image": map[string]interface{}{}}, - }, - }, - }, - }) - // Upload response - reg.Register(&httpmock.Stub{ - Method: "POST", - URL: "/open-apis/drive/v1/medias/upload_all", - Body: map[string]interface{}{ - "code": 0, "msg": "ok", - "data": map[string]interface{}{"file_token": "ftok_abc"}, - }, - }) - // Batch update response - reg.Register(&httpmock.Stub{ - Method: "PATCH", - URL: "/open-apis/docx/v1/documents/doxcnFB/blocks/batch_update", - Body: map[string]interface{}{"code": 0, "msg": "ok", "data": map[string]interface{}{}}, - }) - - tmpDir := t.TempDir() - withDocsWorkingDir(t, tmpDir) - writeSizedDocTestFile(t, "img.png", 100) - - err := mountAndRunDocs(t, DocMediaInsert, []string{ - "+media-insert", - "--doc", "doxcnFB", - "--file", "img.png", - "--after-keyword", "Introduction", - "--as", "bot", - }, f, nil) - if err != nil { - t.Fatalf("Execute() error: %v", err) - } -} - -// TestDocMediaInsertDryRunWithAfterKeyword verifies the dry-run output describes -// the extra block-listing step when --after-keyword is provided. -func TestDocMediaInsertDryRunWithAfterKeyword(t *testing.T) { - tmpDir := t.TempDir() - withDocsWorkingDir(t, tmpDir) - writeSizedDocTestFile(t, "img.png", 100) - - cmd := &cobra.Command{Use: "docs +media-insert"} - cmd.Flags().String("file", "", "") - cmd.Flags().String("doc", "", "") - cmd.Flags().String("type", "image", "") - cmd.Flags().String("align", "", "") - cmd.Flags().String("caption", "", "") - cmd.Flags().String("after-keyword", "", "") - cmd.Flags().String("before-keyword", "", "") - _ = cmd.Flags().Set("file", "img.png") - _ = cmd.Flags().Set("doc", "doxcnABCDEF") - _ = cmd.Flags().Set("after-keyword", "Introduction") - - rt := common.TestNewRuntimeContext(cmd, nil) - dry := decodeDocDryRun(t, DocMediaInsert.DryRun(context.Background(), rt)) - - foundListBlocks := false - for _, step := range dry.API { - if strings.Contains(step.URL, "/blocks") && - !strings.Contains(step.URL, "children") && - !strings.Contains(step.URL, "batch_update") { - foundListBlocks = true - } - } - if !foundListBlocks { - t.Fatal("dry-run should include a block-listing step for --after-keyword") - } - if !strings.Contains(dry.Description, "search blocks") { - t.Fatalf("dry-run description should mention 'search blocks', got: %s", dry.Description) - } -} From d1b8dac689572bf198b9e4c2ebf417df322e14d0 Mon Sep 17 00:00:00 2001 From: baiqing Date: Thu, 9 Apr 2026 04:22:02 +0800 Subject: [PATCH 4/6] fix(doc): show in dry-run create-block when selection is set When --selection-with-ellipsis is provided, the create-block step in dry-run now shows index: "" instead of "" to accurately reflect that the insertion position is computed from MCP locate-doc, not appended to end. --- shortcuts/doc/doc_media_insert.go | 6 +++++- shortcuts/doc/doc_media_insert_test.go | 16 ++++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/shortcuts/doc/doc_media_insert.go b/shortcuts/doc/doc_media_insert.go index 4ccc1c25f..3b5017d31 100644 --- a/shortcuts/doc/doc_media_insert.go +++ b/shortcuts/doc/doc_media_insert.go @@ -66,7 +66,11 @@ var DocMediaInsert = common.Shortcut{ parentType := parentTypeForMediaType(mediaType) createBlockData := buildCreateBlockData(mediaType, 0) - createBlockData["index"] = "" + if hasSelection { + createBlockData["index"] = "" + } else { + createBlockData["index"] = "" + } batchUpdateData := buildBatchUpdateData("", mediaType, "", runtime.Str("align"), caption) d := common.NewDryRunAPI() diff --git a/shortcuts/doc/doc_media_insert_test.go b/shortcuts/doc/doc_media_insert_test.go index 432b78ac7..3355e33ac 100644 --- a/shortcuts/doc/doc_media_insert_test.go +++ b/shortcuts/doc/doc_media_insert_test.go @@ -388,8 +388,9 @@ func TestLocateInsertIndexDryRunIncludesMCPStep(t *testing.T) { var dry struct { Description string `json:"description"` API []struct { - Desc string `json:"desc"` - URL string `json:"url"` + Desc string `json:"desc"` + URL string `json:"url"` + Body map[string]interface{} `json:"body"` } `json:"api"` } if err := json.Unmarshal(raw, &dry); err != nil { @@ -408,6 +409,17 @@ func TestLocateInsertIndexDryRunIncludesMCPStep(t *testing.T) { if !strings.Contains(dry.Description, "locate-doc") { t.Fatalf("dry-run description should mention 'locate-doc', got: %s", dry.Description) } + + // Verify create-block step shows not + for _, step := range dry.API { + if strings.Contains(step.URL, "/children") && step.Body != nil { + if idx, ok := step.Body["index"]; ok { + if idx != "" { + t.Fatalf("create-block index in selection mode = %q, want ", idx) + } + } + } + } } func TestExtractCreatedBlockTargetsForImage(t *testing.T) { From 6e667eeb96dce584648470f87cd858b8589b58d1 Mon Sep 17 00:00:00 2001 From: baiqing Date: Mon, 20 Apr 2026 13:16:43 +0800 Subject: [PATCH 5/6] fix(doc): address CodeRabbit review on +media-insert selection feature - Validate: reject blank/whitespace --selection-with-ellipsis unconditionally so a mis-typed empty value cannot silently fall back to append-mode. - Redact the raw selection string when logging to stderr and when emitting error messages. --selection-with-ellipsis is copied verbatim from document content and may contain confidential text; the new redactSelection helper keeps a short prefix and rune count so operators can still identify the failing selection. - Harden the after/before mode tests: root children now have three entries so the two modes land on different indices, and the tests decode the create-block request body to assert the computed `index` actually reaches the /children API. A regression that ignored --before would now fail. - Harden the nested-block test so it exercises the fallback parent-walk: the anchor is now two levels deep (blk_grandchild under blk_section_child under blk_section), which forces the walk to fetch the intermediate block via GET /blocks/{id} to discover the root-level ancestor. --- shortcuts/doc/doc_media_insert.go | 33 +++++++- shortcuts/doc/doc_media_insert_test.go | 103 ++++++++++++++++++++----- 2 files changed, 114 insertions(+), 22 deletions(-) diff --git a/shortcuts/doc/doc_media_insert.go b/shortcuts/doc/doc_media_insert.go index 8c9d769dc..4c902f46e 100644 --- a/shortcuts/doc/doc_media_insert.go +++ b/shortcuts/doc/doc_media_insert.go @@ -58,7 +58,16 @@ var DocMediaInsert = common.Shortcut{ if docRef.Kind == "doc" { return output.ErrValidation("docs +media-insert only supports docx documents; use a docx token/URL or a wiki URL that resolves to docx") } - if runtime.Bool("before") && strings.TrimSpace(runtime.Str("selection-with-ellipsis")) == "" { + rawSelection := runtime.Str("selection-with-ellipsis") + trimmedSelection := strings.TrimSpace(rawSelection) + // Explicitly reject a flag that was supplied but blank: runtime.Str cannot + // distinguish "omitted" from "provided as empty/whitespace", and a silent + // trim-to-empty would make +media-insert fall back to append-mode and + // write at the wrong location. + if rawSelection != "" && trimmedSelection == "" { + return output.ErrValidation("--selection-with-ellipsis must not be blank or whitespace-only") + } + if runtime.Bool("before") && trimmedSelection == "" { return output.ErrValidation("--before requires --selection-with-ellipsis") } if view := runtime.Str("file-view"); view != "" { @@ -200,7 +209,9 @@ var DocMediaInsert = common.Shortcut{ selection := strings.TrimSpace(runtime.Str("selection-with-ellipsis")) if selection != "" { before := runtime.Bool("before") - fmt.Fprintf(runtime.IO().ErrOut, "Locating block matching selection: %q\n", selection) + // Redact the selection when logging — it is copied verbatim from + // document content and may contain confidential text. + fmt.Fprintf(runtime.IO().ErrOut, "Locating block matching selection (%s)\n", redactSelection(selection)) idx, err := locateInsertIndex(runtime, documentID, selection, rootChildren, before) if err != nil { return err @@ -287,6 +298,20 @@ func blockTypeForMediaType(mediaType string) int { return 27 } +// redactSelection summarizes --selection-with-ellipsis values for logging and +// error messages without echoing raw document text. Returns the rune count and, +// for longer strings, a short prefix so operators can still identify which +// selection failed without leaking confidential content into terminals or CI +// logs. +func redactSelection(s string) string { + const prefixRunes = 8 + runes := []rune(s) + if len(runes) <= prefixRunes { + return fmt.Sprintf("%d chars", len(runes)) + } + return fmt.Sprintf("%q… %d chars total", string(runes[:prefixRunes]), len(runes)) +} + func parentTypeForMediaType(mediaType string) string { if mediaType == "file" { return "docx_file" @@ -430,7 +455,7 @@ func locateInsertIndex(runtime *common.RuntimeContext, documentID string, select return 0, output.ErrWithHint( output.ExitValidation, "no_match", - fmt.Sprintf("locate-doc did not find any block matching selection %q", selection), + fmt.Sprintf("locate-doc did not find any block matching selection (%s)", redactSelection(selection)), "check spelling or use 'start...end' syntax to narrow the selection", ) } @@ -502,7 +527,7 @@ func locateInsertIndex(runtime *common.RuntimeContext, documentID string, select return 0, output.ErrWithHint( output.ExitValidation, "block_not_reachable", - fmt.Sprintf("block matching selection %q is not reachable from document root", selection), + fmt.Sprintf("block matching selection (%s) is not reachable from document root", redactSelection(selection)), "try a top-level heading or paragraph as the selection", ) } diff --git a/shortcuts/doc/doc_media_insert_test.go b/shortcuts/doc/doc_media_insert_test.go index 6169b31bd..c97c29d22 100644 --- a/shortcuts/doc/doc_media_insert_test.go +++ b/shortcuts/doc/doc_media_insert_test.go @@ -257,9 +257,12 @@ func buildLocateDocMCPResponse(matches []map[string]interface{}) map[string]inte } } +// registerInsertWithSelectionStubs wires the minimal stub set for the +// --selection-with-ellipsis happy path. Returns the create-block stub so +// callers can inspect the request body (e.g. to verify the computed index). func registerInsertWithSelectionStubs(reg interface { Register(*httpmock.Stub) -}, docID, anchorBlockID, parentBlockID string) { +}, docID, anchorBlockID, parentBlockID string, rootChildren []interface{}) *httpmock.Stub { // Root block reg.Register(&httpmock.Stub{ Method: "GET", @@ -269,7 +272,7 @@ func registerInsertWithSelectionStubs(reg interface { "data": map[string]interface{}{ "block": map[string]interface{}{ "block_id": docID, - "children": []interface{}{"blk_a", "blk_b"}, + "children": rootChildren, }, }, }, @@ -282,8 +285,8 @@ func registerInsertWithSelectionStubs(reg interface { {"anchor_block_id": anchorBlockID, "parent_block_id": parentBlockID}, }), }) - // Create block - reg.Register(&httpmock.Stub{ + // Create block — returned so the test can inspect index in CapturedBody. + createStub := &httpmock.Stub{ Method: "POST", URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID + "/children", Body: map[string]interface{}{ @@ -294,7 +297,8 @@ func registerInsertWithSelectionStubs(reg interface { }, }, }, - }) + } + reg.Register(createStub) // Upload reg.Register(&httpmock.Stub{ Method: "POST", @@ -310,13 +314,36 @@ func registerInsertWithSelectionStubs(reg interface { URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/batch_update", Body: map[string]interface{}{"code": 0, "msg": "ok", "data": map[string]interface{}{}}, }) + return createStub } -// TestLocateInsertIndexAfterModeViaExecute verifies that --selection-with-ellipsis -// inserts after the matched root-level block (index = root index + 1). +// assertCreateBlockIndex decodes the create-block request body and asserts the +// `index` field equals want. Fails the test if the body is missing or wrong. +func assertCreateBlockIndex(t *testing.T, stub *httpmock.Stub, want int) { + t.Helper() + if stub.CapturedBody == nil { + t.Fatalf("create-block stub captured no body") + } + var body map[string]interface{} + if err := json.Unmarshal(stub.CapturedBody, &body); err != nil { + t.Fatalf("decode create-block body: %v (raw: %s)", err, stub.CapturedBody) + } + got, _ := body["index"].(float64) + if int(got) != want { + t.Fatalf("create-block index = %v, want %d (body: %s)", body["index"], want, stub.CapturedBody) + } +} + +// TestLocateInsertIndexAfterModeViaExecute verifies that +// --selection-with-ellipsis (default after-mode) places the new block +// immediately after the matched root-level block. Uses three root children so +// the after-index (2) differs from what --before would produce (1), and +// inspects the create-block request body to prove the computed index actually +// reaches the /children API. func TestLocateInsertIndexAfterModeViaExecute(t *testing.T) { f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("locate-after-app")) - registerInsertWithSelectionStubs(reg, "doxcnSEL", "blk_a", "doxcnSEL") + createStub := registerInsertWithSelectionStubs(reg, "doxcnSEL", "blk_b", "doxcnSEL", + []interface{}{"blk_a", "blk_b", "blk_c"}) tmpDir := t.TempDir() withDocsWorkingDir(t, tmpDir) @@ -332,13 +359,19 @@ func TestLocateInsertIndexAfterModeViaExecute(t *testing.T) { if err != nil { t.Fatalf("Execute() error: %v", err) } + // after blk_b (index 1) → insert at index 2, between blk_b and blk_c. + assertCreateBlockIndex(t, createStub, 2) } -// TestLocateInsertIndexBeforeModeViaExecute verifies that --before inserts before -// the matched root-level block. +// TestLocateInsertIndexBeforeModeViaExecute verifies that --before inserts +// before the matched root-level block. Pairs with the after-mode test above: +// same fixture, same anchor, but --before should flip the index from 2 to 1. +// A regression that ignored --before would still pass the success check alone, +// so we assert the create-block body explicitly. func TestLocateInsertIndexBeforeModeViaExecute(t *testing.T) { f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("locate-before-app")) - registerInsertWithSelectionStubs(reg, "doxcnSEL2", "blk_b", "doxcnSEL2") + createStub := registerInsertWithSelectionStubs(reg, "doxcnSEL2", "blk_b", "doxcnSEL2", + []interface{}{"blk_a", "blk_b", "blk_c"}) tmpDir := t.TempDir() withDocsWorkingDir(t, tmpDir) @@ -355,15 +388,26 @@ func TestLocateInsertIndexBeforeModeViaExecute(t *testing.T) { if err != nil { t.Fatalf("Execute() error: %v", err) } + // before blk_b (index 1) → insert at index 1, between blk_a and blk_b. + assertCreateBlockIndex(t, createStub, 1) } -// TestLocateInsertIndexNestedBlockViaExecute verifies that a nested block's -// parent_block_id hint is used to walk to the root-level ancestor. +// TestLocateInsertIndexNestedBlockViaExecute verifies that a deeply-nested +// anchor (2+ levels below root) walks up through an intermediate block via +// the GET /blocks/{id} API to find the root-level ancestor. This exercises +// the fallback ancestor-walk path in locateInsertIndex — the parent_block_id +// hint from locate-doc is only good for one level, so deeper nesting must hit +// the block-fetch loop. func TestLocateInsertIndexNestedBlockViaExecute(t *testing.T) { f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("locate-nested-app")) docID := "doxcnNESTED" - // Root block with blk_section and blk_other as children + // Root children: blk_section (index 0), blk_other (index 1). + // Anchor blk_grandchild is nested two levels deep: + // root → blk_section → blk_section_child → blk_grandchild + // locate-doc gives us parent_block_id = blk_section_child (one level up); + // the walk must fetch blk_section_child to discover its parent = blk_section + // before it can land on a root child. reg.Register(&httpmock.Stub{ Method: "GET", URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID, @@ -377,15 +421,30 @@ func TestLocateInsertIndexNestedBlockViaExecute(t *testing.T) { }, }, }) - // MCP locate-doc returns blk_child nested under blk_section reg.Register(&httpmock.Stub{ Method: "POST", URL: "mcp.feishu.cn/mcp", Body: buildLocateDocMCPResponse([]map[string]interface{}{ - {"anchor_block_id": "blk_child", "parent_block_id": "blk_section"}, + {"anchor_block_id": "blk_grandchild", "parent_block_id": "blk_section_child"}, }), }) - reg.Register(&httpmock.Stub{ + // Intermediate block lookup — this is the key step that exercises the + // fallback walk. Without this stub the test would fail. + intermediateStub := &httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/blk_section_child", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "block": map[string]interface{}{ + "block_id": "blk_section_child", + "parent_id": "blk_section", + }, + }, + }, + } + reg.Register(intermediateStub) + createStub := &httpmock.Stub{ Method: "POST", URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID + "/children", Body: map[string]interface{}{ @@ -396,7 +455,8 @@ func TestLocateInsertIndexNestedBlockViaExecute(t *testing.T) { }, }, }, - }) + } + reg.Register(createStub) reg.Register(&httpmock.Stub{ Method: "POST", URL: "/open-apis/drive/v1/medias/upload_all", @@ -425,6 +485,13 @@ func TestLocateInsertIndexNestedBlockViaExecute(t *testing.T) { if err != nil { t.Fatalf("Execute() error: %v", err) } + // Confirm the ancestor-walk actually fired — without this assertion a + // regression that short-circuited the walk would still pass. + if intermediateStub.CapturedBody == nil && intermediateStub.CapturedHeaders == nil { + t.Errorf("expected GET /blocks/blk_section_child to be invoked by the parent-walk; stub was not hit") + } + // after blk_section (index 0) → insert at index 1, between blk_section and blk_other. + assertCreateBlockIndex(t, createStub, 1) } // TestLocateInsertIndexNoMatchReturnsError verifies that when locate-doc returns From 1e5de85f35e224d5f623305e7df0bd557882235e Mon Sep 17 00:00:00 2001 From: fangshuyu-768 Date: Mon, 20 Apr 2026 23:17:05 +0800 Subject: [PATCH 6/6] fix(doc): harden +media-insert selection UX on top of #335 (#577) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #335 review: closes a handful of UX and robustness gaps in the new --selection-with-ellipsis flow. - Flag description rewritten to make the "insert at the top-level ancestor" semantics explicit — when the selection is inside a callout, table cell, or nested list, media lands outside that container, not inside. Also calls out the 'start...end' disambiguator. - locate-doc is now called with limit=2 so an ambiguous selection (same phrase in more than one block) surfaces a stderr warning pointing at 'start...end', instead of silently picking the first match. The first-match return behaviour is unchanged. - When the anchor is nested below the root, locateInsertIndex now logs a note to stderr naming the walk depth and the root-level ancestor's insert index. Users don't have to guess why the image landed outside the callout they were editing. - maxDepth bumped 8 → 32 with a comment explaining the invariants: `visited` is the real cycle guard, `maxDepth` is belt-and-suspenders. 32 comfortably exceeds real docx nesting depth so a deeply-nested but well-formed anchor is no longer silently rejected. - Comment added before the parent-walk loop noting why the API calls are serial (each level's parent_id is only known after the previous GET returns; can't be batched or parallelised). Tests: - TestLocateInsertIndexWarnsOnMultipleMatches: stubs two matches, asserts the stderr warning names the ambiguity and mentions 'start...end', and that the first-match insert index is unchanged. - TestLocateInsertIndexLogsNestedAnchor: anchor two levels below root, asserts stderr carries the "nested … top-level ancestor" note. - TestLocateInsertIndexCycleDetection: malformed parent chain with blk_x.parent = blk_y and blk_y.parent = blk_x, neither reachable from root. Registering a single GET /blocks/blk_y stub also bounds the call count — a regression that broke `visited` tracking would either hang or fail via httpmock's extra-call guard. Co-authored-by: fangshuyu-768 --- shortcuts/doc/doc_media_insert.go | 44 ++++- shortcuts/doc/doc_media_insert_test.go | 253 +++++++++++++++++++++++++ 2 files changed, 292 insertions(+), 5 deletions(-) diff --git a/shortcuts/doc/doc_media_insert.go b/shortcuts/doc/doc_media_insert.go index 4c902f46e..a3c9dbd11 100644 --- a/shortcuts/doc/doc_media_insert.go +++ b/shortcuts/doc/doc_media_insert.go @@ -46,7 +46,7 @@ var DocMediaInsert = common.Shortcut{ {Name: "type", Default: "image", Desc: "type: image | file"}, {Name: "align", Desc: "alignment: left | center | right"}, {Name: "caption", Desc: "image caption text"}, - {Name: "selection-with-ellipsis", Desc: "plain text (or 'start...end') that identifies the target block; the media is inserted after that block by default"}, + {Name: "selection-with-ellipsis", Desc: "plain text (or 'start...end' to disambiguate) matching the target block's content. Media is inserted at the top-level ancestor of the matched block — i.e., when the selection is inside a callout, table cell, or nested list, media lands outside that container, not inside it. Pass 'start...end' (a unique prefix and suffix separated by '...') when the plain text appears in more than one block"}, {Name: "before", Type: "bool", Desc: "insert before the matched block instead of after (requires --selection-with-ellipsis)"}, {Name: "file-view", Desc: "file block rendering: card (default) | preview | inline; only applies when --type=file. preview renders audio/video as an inline player"}, }, @@ -440,10 +440,12 @@ func extractAppendTarget(rootData map[string]interface{}, fallbackBlockID string // parent_id chain (using single-block GET calls when needed) to resolve nested // blocks to their top-level ancestor in rootChildren. func locateInsertIndex(runtime *common.RuntimeContext, documentID string, selection string, rootChildren []interface{}, before bool) (int, error) { + // Ask for 2 matches so we can warn when the selection is ambiguous. locate-doc + // orders matches by document position, so matches[0] is still deterministic. args := map[string]interface{}{ "doc_id": documentID, "selection_with_ellipsis": selection, - "limit": 1, + "limit": 2, } result, err := common.CallMCPTool(runtime, "locate-doc", args) if err != nil { @@ -459,6 +461,15 @@ func locateInsertIndex(runtime *common.RuntimeContext, documentID string, select "check spelling or use 'start...end' syntax to narrow the selection", ) } + if len(matches) > 1 { + // Silently picking the first match surprises users whose selection appears + // in more than one block (e.g. the same phrase in a title and a paragraph). + // Surface that another match exists and point at the 'start...end' disambiguator. + fmt.Fprintf(runtime.IO().ErrOut, + "warning: selection (%s) matched more than one block; inserting relative to the first. "+ + "Pass --selection-with-ellipsis 'start...end' to narrow.\n", + redactSelection(selection)) + } matchMap, _ := matches[0].(map[string]interface{}) anchorBlockID := common.GetString(matchMap, "anchor_block_id") @@ -484,12 +495,21 @@ func locateInsertIndex(runtime *common.RuntimeContext, documentID string, select } } - // Walk up the parent chain. locate-doc already gives us one level of parent, - // so most cases need zero extra API calls. + // Walk up the parent chain to the top-level ancestor in rootChildren. This + // is serial by nature: each level's parent_id is only known after the + // previous level's GET /blocks/{id} response arrives, so the calls cannot + // be batched or parallelised. + // + // visited is the real cycle guard — it stops an A→B→A parent-id loop (seen + // on malformed API responses) after one lap. maxDepth is belt-and-suspenders + // in case both visited tracking and parent_id sanity simultaneously break; + // 32 comfortably exceeds the deepest real docx nesting (~6–8 levels for + // quote/callout/list combinations) without letting a bug run unbounded. cur := anchorBlockID nextParent := parentBlockID visited := map[string]bool{} - const maxDepth = 8 + const maxDepth = 32 + walkDepth := 0 for depth := 0; depth < maxDepth; depth++ { if visited[cur] { break @@ -497,6 +517,19 @@ func locateInsertIndex(runtime *common.RuntimeContext, documentID string, select visited[cur] = true if idx, ok := rootSet[cur]; ok { + if walkDepth > 0 { + // The anchor was nested inside a callout / table cell / list and + // got resolved to its top-level ancestor. Surface this so users + // don't misread "insert before 'X'" as "insert right next to X" + // when X is buried several levels deep. + posLabel := "after" + if before { + posLabel = "before" + } + fmt.Fprintf(runtime.IO().ErrOut, + "note: selection (%s) was nested %d level(s) deep; inserting %s its top-level ancestor at index %d\n", + redactSelection(selection), walkDepth, posLabel, idx) + } if before { return idx, nil } @@ -522,6 +555,7 @@ func locateInsertIndex(runtime *common.RuntimeContext, documentID string, select break } cur = parent + walkDepth++ } return 0, output.ErrWithHint( diff --git a/shortcuts/doc/doc_media_insert_test.go b/shortcuts/doc/doc_media_insert_test.go index c97c29d22..e5e9cd2fb 100644 --- a/shortcuts/doc/doc_media_insert_test.go +++ b/shortcuts/doc/doc_media_insert_test.go @@ -731,3 +731,256 @@ func TestDocMediaInsertValidateFileView(t *testing.T) { }) } } + +// TestLocateInsertIndexWarnsOnMultipleMatches verifies that when locate-doc +// returns more than one match, a warning is written to stderr pointing the user +// at the 'start...end' disambiguation syntax. Silently picking the first match +// of an ambiguous selection is a real UX trap — users who edit documents with +// repeated phrases (a heading that also appears in the TOC, for example) get +// no signal that another match existed. +func TestLocateInsertIndexWarnsOnMultipleMatches(t *testing.T) { + f, _, stderr, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("locate-multi-app")) + + docID := "doxcnMULTI" + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID, + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "block": map[string]interface{}{ + "block_id": docID, + "children": []interface{}{"blk_a", "blk_b"}, + }, + }, + }, + }) + // Two matches — same selection appears in two different root-level blocks. + // locate-doc orders matches by document position, so matches[0] is still + // deterministic (blk_a) even with limit=2. + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "mcp.feishu.cn/mcp", + Body: buildLocateDocMCPResponse([]map[string]interface{}{ + {"anchor_block_id": "blk_a", "parent_block_id": docID}, + {"anchor_block_id": "blk_b", "parent_block_id": docID}, + }), + }) + createStub := &httpmock.Stub{ + Method: "POST", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID + "/children", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "children": []interface{}{ + map[string]interface{}{"block_id": "blk_new", "block_type": 27, "image": map[string]interface{}{}}, + }, + }, + }, + } + reg.Register(createStub) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/drive/v1/medias/upload_all", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{"file_token": "ftok_multi"}, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "PATCH", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/batch_update", + Body: map[string]interface{}{"code": 0, "msg": "ok", "data": map[string]interface{}{}}, + }) + + tmpDir := t.TempDir() + withDocsWorkingDir(t, tmpDir) + writeSizedDocTestFile(t, "img.png", 100) + + err := mountAndRunDocs(t, DocMediaInsert, []string{ + "+media-insert", + "--doc", docID, + "--file", "img.png", + "--selection-with-ellipsis", "Repeated phrase", + "--as", "bot", + }, f, nil) + if err != nil { + t.Fatalf("Execute() error: %v", err) + } + + // Warning should name the ambiguity and point at 'start...end'. + stderrOut := stderr.String() + if !strings.Contains(stderrOut, "matched more than one block") { + t.Errorf("stderr missing multi-match warning; got:\n%s", stderrOut) + } + if !strings.Contains(stderrOut, "start...end") { + t.Errorf("stderr missing 'start...end' disambiguation hint; got:\n%s", stderrOut) + } + // Should still insert at the first match (blk_a at index 0) → after ⇒ 1. + assertCreateBlockIndex(t, createStub, 1) +} + +// TestLocateInsertIndexLogsNestedAnchor verifies that when the matched block is +// nested (not a direct root child), a note is written to stderr explaining that +// the media lands at the top-level ancestor. This protects users from being +// surprised when selecting text inside a callout or table cell and seeing the +// image appear outside that container. +func TestLocateInsertIndexLogsNestedAnchor(t *testing.T) { + f, _, stderr, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("locate-nested-log-app")) + + docID := "doxcnNESTEDLOG" + // Same shape as TestLocateInsertIndexNestedBlockViaExecute: anchor is two + // levels below root, so walkDepth == 2 when we hit the root ancestor. + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID, + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "block": map[string]interface{}{ + "block_id": docID, + "children": []interface{}{"blk_section", "blk_other"}, + }, + }, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "mcp.feishu.cn/mcp", + Body: buildLocateDocMCPResponse([]map[string]interface{}{ + {"anchor_block_id": "blk_grandchild", "parent_block_id": "blk_section_child"}, + }), + }) + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/blk_section_child", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "block": map[string]interface{}{ + "block_id": "blk_section_child", + "parent_id": "blk_section", + }, + }, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID + "/children", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "children": []interface{}{ + map[string]interface{}{"block_id": "blk_new", "block_type": 27, "image": map[string]interface{}{}}, + }, + }, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/drive/v1/medias/upload_all", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{"file_token": "ftok_nested_log"}, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "PATCH", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/batch_update", + Body: map[string]interface{}{"code": 0, "msg": "ok", "data": map[string]interface{}{}}, + }) + + tmpDir := t.TempDir() + withDocsWorkingDir(t, tmpDir) + writeSizedDocTestFile(t, "img.png", 100) + + err := mountAndRunDocs(t, DocMediaInsert, []string{ + "+media-insert", + "--doc", docID, + "--file", "img.png", + "--selection-with-ellipsis", "nested content", + "--as", "bot", + }, f, nil) + if err != nil { + t.Fatalf("Execute() error: %v", err) + } + + stderrOut := stderr.String() + if !strings.Contains(stderrOut, "nested") || !strings.Contains(stderrOut, "top-level ancestor") { + t.Errorf("stderr missing nested-anchor note; got:\n%s", stderrOut) + } +} + +// TestLocateInsertIndexCycleDetection verifies that a malformed parent chain +// (blk_x.parent = blk_y and blk_y.parent = blk_x, neither reachable from root) +// does not spin the locate-doc walk forever. The `visited` map must break the +// cycle, and the user must see the "not reachable from document root" error +// rather than the process hanging. Without this test, a regression that broke +// cycle protection would only surface in production with a stalled CLI. +func TestLocateInsertIndexCycleDetection(t *testing.T) { + f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("locate-cycle-app")) + + docID := "doxcnCYCLE" + // Root has unrelated children — neither blk_x nor blk_y reach root. + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/" + docID, + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "block": map[string]interface{}{ + "block_id": docID, + "children": []interface{}{"blk_unrelated_a", "blk_unrelated_b"}, + }, + }, + }, + }) + // locate-doc hints parent_block_id = blk_y for anchor blk_x (first hop consumed). + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "mcp.feishu.cn/mcp", + Body: buildLocateDocMCPResponse([]map[string]interface{}{ + {"anchor_block_id": "blk_x", "parent_block_id": "blk_y"}, + }), + }) + // blk_y claims blk_x as parent — closes the cycle. The walk must land here + // exactly once before visited[blk_x] triggers a break. + blkYStub := &httpmock.Stub{ + Method: "GET", + URL: "/open-apis/docx/v1/documents/" + docID + "/blocks/blk_y", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "block": map[string]interface{}{ + "block_id": "blk_y", + "parent_id": "blk_x", + }, + }, + }, + } + reg.Register(blkYStub) + + tmpDir := t.TempDir() + withDocsWorkingDir(t, tmpDir) + writeSizedDocTestFile(t, "img.png", 100) + + err := mountAndRunDocs(t, DocMediaInsert, []string{ + "+media-insert", + "--doc", docID, + "--file", "img.png", + "--selection-with-ellipsis", "cyclic anchor", + "--as", "bot", + }, f, nil) + if err == nil { + t.Fatal("expected 'block_not_reachable' error from cyclic parent chain; got nil") + } + if !strings.Contains(err.Error(), "not reachable") && !strings.Contains(err.Error(), "block_not_reachable") { + t.Fatalf("unexpected error — want cycle-bounded 'not reachable', got: %v", err) + } + // blk_y should be fetched exactly once. Registering just one stub for it + // already enforces an upper bound (httpmock errors on extra calls), so if + // the walk looped more than once the test harness would fail differently. + if blkYStub.CapturedHeaders == nil && blkYStub.CapturedBody == nil { + t.Errorf("expected the walk to fetch blk_y once; stub was not hit") + } +}