diff --git a/shortcuts/doc/doc_media_insert.go b/shortcuts/doc/doc_media_insert.go index cb24485b3..a3c9dbd11 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/extension/fileio" "github.com/larksuite/cli/internal/output" @@ -35,7 +36,7 @@ var fileViewMap = 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 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"}, @@ -45,6 +46,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: "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"}, }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { @@ -55,6 +58,18 @@ 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") } + 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 != "" { if _, ok := fileViewMap[view]; !ok { return output.ErrValidation("invalid --file-view value %q, expected one of: card | preview | inline", view) @@ -76,30 +91,71 @@ var DocMediaInsert = common.Shortcut{ filePath := runtime.Str("file") mediaType := runtime.Str("type") caption := runtime.Str("caption") + selection := strings.TrimSpace(runtime.Str("selection-with-ellipsis")) + hasSelection := selection != "" fileViewType := fileViewMap[runtime.Str("file-view")] parentType := parentTypeForMediaType(mediaType) createBlockData := buildCreateBlockData(mediaType, 0, fileViewType) - createBlockData["index"] = "" + if hasSelection { + createBlockData["index"] = "" + } else { + createBlockData["index"] = "" + } batchUpdateData := buildBatchUpdateData("", mediaType, "", runtime.Str("align"), caption) d := common.NewDryRunAPI() + totalSteps := 4 + if docRef.Kind == "wiki" { + totalSteps++ + } + if hasSelection { + totalSteps++ + } + + positionLabel := map[bool]string{true: "before", false: "after"}[runtime.Bool("before")] + 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: " 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("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: " 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)). + Desc(fmt.Sprintf("[%d] Get document root block", stepBase)) + + if hasSelection { + mcpEndpoint := common.MCPEndpoint(runtime.Config.Brand) + mcpArgs := map[string]interface{}{ + "doc_id": documentID, + "selection_with_ellipsis": selection, + "limit": 1, + } + 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++ + } + + 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, runtime.FileIO(), filePath, parentType, stepBase+2) d.PATCH("/open-apis/docx/v1/documents/:document_id/blocks/batch_update"). @@ -144,13 +200,31 @@ 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 + selection := strings.TrimSpace(runtime.Str("selection-with-ellipsis")) + if selection != "" { + before := runtime.Bool("before") + // 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 + } + insertIndex = idx + 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 fmt.Fprintf(runtime.IO().ErrOut, "Creating block at index %d\n", insertIndex) createData, err := runtime.CallAPI("POST", @@ -224,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" @@ -332,19 +420,150 @@ 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 +} + +// 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) { + // 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": 2, + } + result, err := common.CallMCPTool(runtime, "locate-doc", args) + if err != nil { + return 0, err + } + + 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 (%s)", redactSelection(selection)), + "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") + 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) lookup. + rootSet := make(map[string]int, len(rootChildren)) + for i, c := range rootChildren { + if id, ok := c.(string); ok { + rootSet[id] = i + } + } + + // 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 = 32 + walkDepth := 0 + for depth := 0; depth < maxDepth; depth++ { + if visited[cur] { + break + } + 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 + } + 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 + } + block := common.GetMap(data, "block") + parent = common.GetString(block, "parent_id") + } + if parent == "" || parent == cur { + break + } + cur = parent + walkDepth++ + } + + return 0, output.ErrWithHint( + output.ExitValidation, + "block_not_reachable", + fmt.Sprintf("block matching selection (%s) is not reachable from document root", redactSelection(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 c13f7c64f..e5e9cd2fb 100644 --- a/shortcuts/doc/doc_media_insert_test.go +++ b/shortcuts/doc/doc_media_insert_test.go @@ -5,12 +5,15 @@ 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" ) @@ -222,7 +225,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) } @@ -232,6 +235,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)) + } +} + +// 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), + }, + }, + }, + } +} + +// 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, rootChildren []interface{}) *httpmock.Stub { + // 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": rootChildren, + }, + }, + }, + }) + // 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 — 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{}{ + "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) + // 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"}, + }, + }) + // 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{}{}}, + }) + return createStub +} + +// 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")) + createStub := registerInsertWithSelectionStubs(reg, "doxcnSEL", "blk_b", "doxcnSEL", + []interface{}{"blk_a", "blk_b", "blk_c"}) + + 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("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. 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")) + createStub := registerInsertWithSelectionStubs(reg, "doxcnSEL2", "blk_b", "doxcnSEL2", + []interface{}{"blk_a", "blk_b", "blk_c"}) + + 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("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 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 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, + 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"}, + }), + }) + // 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{}{ + "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_nested"}, + }, + }) + 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) + } + // 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 +// 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") + } + if !strings.Contains(err.Error(), "no_match") && !strings.Contains(err.Error(), "did not find") { + t.Fatalf("unexpected error: %v", err) + } +} + +// 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() + + 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"` + Body map[string]interface{} `json:"body"` + } `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 + } + } + if !foundMCP { + t.Fatalf("dry-run should include a locate-doc step, got: %+v", dry.API) + } + 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) { @@ -369,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") + } +}