Skip to content

Commit 9057299

Browse files
feat(doc): add --selection-with-ellipsis position flag to +media-insert (larksuite#335)
* feat(doc): add --after-keyword/--before-keyword flags to +media-insert 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 * test(doc): add fetchAllBlocks pagination and keyword dry-run coverage - 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% * 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. * fix(doc): show <locate_index> 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: "<locate_index>" instead of "<children_len>" to accurately reflect that the insertion position is computed from MCP locate-doc, not appended to end. * 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. * fix(doc): harden +media-insert selection UX on top of larksuite#335 (larksuite#577) Follow-up to larksuite#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 <shuyufang768@outlook.com>
1 parent 9e891b7 commit 9057299

2 files changed

Lines changed: 848 additions & 14 deletions

File tree

shortcuts/doc/doc_media_insert.go

Lines changed: 232 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"context"
88
"fmt"
99
"path/filepath"
10+
"strings"
1011

1112
"github.com/larksuite/cli/extension/fileio"
1213
"github.com/larksuite/cli/internal/output"
@@ -35,7 +36,7 @@ var fileViewMap = map[string]int{
3536
var DocMediaInsert = common.Shortcut{
3637
Service: "docs",
3738
Command: "+media-insert",
38-
Description: "Insert a local image or file at the end of a Lark document (4-step orchestration + auto-rollback)",
39+
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",
3940
Risk: "write",
4041
Scopes: []string{"docs:document.media:upload", "docx:document:write_only", "docx:document:readonly"},
4142
AuthTypes: []string{"user", "bot"},
@@ -45,6 +46,8 @@ var DocMediaInsert = common.Shortcut{
4546
{Name: "type", Default: "image", Desc: "type: image | file"},
4647
{Name: "align", Desc: "alignment: left | center | right"},
4748
{Name: "caption", Desc: "image caption text"},
49+
{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"},
50+
{Name: "before", Type: "bool", Desc: "insert before the matched block instead of after (requires --selection-with-ellipsis)"},
4851
{Name: "file-view", Desc: "file block rendering: card (default) | preview | inline; only applies when --type=file. preview renders audio/video as an inline player"},
4952
},
5053
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error {
@@ -55,6 +58,18 @@ var DocMediaInsert = common.Shortcut{
5558
if docRef.Kind == "doc" {
5659
return output.ErrValidation("docs +media-insert only supports docx documents; use a docx token/URL or a wiki URL that resolves to docx")
5760
}
61+
rawSelection := runtime.Str("selection-with-ellipsis")
62+
trimmedSelection := strings.TrimSpace(rawSelection)
63+
// Explicitly reject a flag that was supplied but blank: runtime.Str cannot
64+
// distinguish "omitted" from "provided as empty/whitespace", and a silent
65+
// trim-to-empty would make +media-insert fall back to append-mode and
66+
// write at the wrong location.
67+
if rawSelection != "" && trimmedSelection == "" {
68+
return output.ErrValidation("--selection-with-ellipsis must not be blank or whitespace-only")
69+
}
70+
if runtime.Bool("before") && trimmedSelection == "" {
71+
return output.ErrValidation("--before requires --selection-with-ellipsis")
72+
}
5873
if view := runtime.Str("file-view"); view != "" {
5974
if _, ok := fileViewMap[view]; !ok {
6075
return output.ErrValidation("invalid --file-view value %q, expected one of: card | preview | inline", view)
@@ -76,30 +91,71 @@ var DocMediaInsert = common.Shortcut{
7691
filePath := runtime.Str("file")
7792
mediaType := runtime.Str("type")
7893
caption := runtime.Str("caption")
94+
selection := strings.TrimSpace(runtime.Str("selection-with-ellipsis"))
95+
hasSelection := selection != ""
7996
fileViewType := fileViewMap[runtime.Str("file-view")]
8097

8198
parentType := parentTypeForMediaType(mediaType)
8299
createBlockData := buildCreateBlockData(mediaType, 0, fileViewType)
83-
createBlockData["index"] = "<children_len>"
100+
if hasSelection {
101+
createBlockData["index"] = "<locate_index>"
102+
} else {
103+
createBlockData["index"] = "<children_len>"
104+
}
84105
batchUpdateData := buildBatchUpdateData("<new_block_id>", mediaType, "<file_token>", runtime.Str("align"), caption)
85106

86107
d := common.NewDryRunAPI()
108+
totalSteps := 4
109+
if docRef.Kind == "wiki" {
110+
totalSteps++
111+
}
112+
if hasSelection {
113+
totalSteps++
114+
}
115+
116+
positionLabel := map[bool]string{true: "before", false: "after"}[runtime.Bool("before")]
117+
87118
if docRef.Kind == "wiki" {
88119
documentID = "<resolved_docx_token>"
89120
stepBase = 2
90-
d.Desc("5-step orchestration: resolve wiki → query root → create block → upload file → bind to block (auto-rollback on failure)").
121+
d.Desc(fmt.Sprintf("%d-step orchestration: resolve wiki → query root →%s create block → upload file → bind to block (auto-rollback on failure)",
122+
totalSteps, map[bool]string{true: " locate-doc →", false: ""}[hasSelection])).
91123
GET("/open-apis/wiki/v2/spaces/get_node").
92124
Desc("[1] Resolve wiki node to docx document").
93125
Params(map[string]interface{}{"token": docRef.Token})
94126
} else {
95-
d.Desc("4-step orchestration: query root → create block → upload file → bind to block (auto-rollback on failure)")
127+
d.Desc(fmt.Sprintf("%d-step orchestration: query root →%s create block → upload file → bind to block (auto-rollback on failure)",
128+
totalSteps, map[bool]string{true: " locate-doc →", false: ""}[hasSelection]))
96129
}
97130

98131
d.
99132
GET("/open-apis/docx/v1/documents/:document_id/blocks/:document_id").
100-
Desc(fmt.Sprintf("[%d] Get document root block", stepBase)).
133+
Desc(fmt.Sprintf("[%d] Get document root block", stepBase))
134+
135+
if hasSelection {
136+
mcpEndpoint := common.MCPEndpoint(runtime.Config.Brand)
137+
mcpArgs := map[string]interface{}{
138+
"doc_id": documentID,
139+
"selection_with_ellipsis": selection,
140+
"limit": 1,
141+
}
142+
d.POST(mcpEndpoint).
143+
Desc(fmt.Sprintf("[%d] MCP locate-doc: find block matching selection (%s)", stepBase+1, positionLabel)).
144+
Body(map[string]interface{}{
145+
"method": "tools/call",
146+
"params": map[string]interface{}{
147+
"name": "locate-doc",
148+
"arguments": mcpArgs,
149+
},
150+
}).
151+
Set("mcp_tool", "locate-doc").
152+
Set("args", mcpArgs)
153+
stepBase++
154+
}
155+
156+
d.
101157
POST("/open-apis/docx/v1/documents/:document_id/blocks/:document_id/children").
102-
Desc(fmt.Sprintf("[%d] Create empty block at document end", stepBase+1)).
158+
Desc(fmt.Sprintf("[%d] Create empty block at target position", stepBase+1)).
103159
Body(createBlockData)
104160
appendDocMediaInsertUploadDryRun(d, runtime.FileIO(), filePath, parentType, stepBase+2)
105161
d.PATCH("/open-apis/docx/v1/documents/:document_id/blocks/batch_update").
@@ -144,13 +200,31 @@ var DocMediaInsert = common.Shortcut{
144200
return err
145201
}
146202

147-
parentBlockID, insertIndex, err := extractAppendTarget(rootData, documentID)
203+
parentBlockID, insertIndex, rootChildren, err := extractAppendTarget(rootData, documentID)
148204
if err != nil {
149205
return err
150206
}
151207
fmt.Fprintf(runtime.IO().ErrOut, "Root block ready: %s (%d children)\n", parentBlockID, insertIndex)
152208

153-
// Step 2: Create an empty block at the end of the document
209+
selection := strings.TrimSpace(runtime.Str("selection-with-ellipsis"))
210+
if selection != "" {
211+
before := runtime.Bool("before")
212+
// Redact the selection when logging — it is copied verbatim from
213+
// document content and may contain confidential text.
214+
fmt.Fprintf(runtime.IO().ErrOut, "Locating block matching selection (%s)\n", redactSelection(selection))
215+
idx, err := locateInsertIndex(runtime, documentID, selection, rootChildren, before)
216+
if err != nil {
217+
return err
218+
}
219+
insertIndex = idx
220+
posLabel := "after"
221+
if before {
222+
posLabel = "before"
223+
}
224+
fmt.Fprintf(runtime.IO().ErrOut, "locate-doc matched: inserting %s at index %d\n", posLabel, insertIndex)
225+
}
226+
227+
// Step 2: Create an empty block at the target position
154228
fmt.Fprintf(runtime.IO().ErrOut, "Creating block at index %d\n", insertIndex)
155229

156230
createData, err := runtime.CallAPI("POST",
@@ -224,6 +298,20 @@ func blockTypeForMediaType(mediaType string) int {
224298
return 27
225299
}
226300

301+
// redactSelection summarizes --selection-with-ellipsis values for logging and
302+
// error messages without echoing raw document text. Returns the rune count and,
303+
// for longer strings, a short prefix so operators can still identify which
304+
// selection failed without leaking confidential content into terminals or CI
305+
// logs.
306+
func redactSelection(s string) string {
307+
const prefixRunes = 8
308+
runes := []rune(s)
309+
if len(runes) <= prefixRunes {
310+
return fmt.Sprintf("%d chars", len(runes))
311+
}
312+
return fmt.Sprintf("%q… %d chars total", string(runes[:prefixRunes]), len(runes))
313+
}
314+
227315
func parentTypeForMediaType(mediaType string) string {
228316
if mediaType == "file" {
229317
return "docx_file"
@@ -332,19 +420,150 @@ func buildBatchUpdateData(blockID, mediaType, fileToken, alignStr, caption strin
332420
}
333421
}
334422

335-
func extractAppendTarget(rootData map[string]interface{}, fallbackBlockID string) (string, int, error) {
423+
func extractAppendTarget(rootData map[string]interface{}, fallbackBlockID string) (parentBlockID string, insertIndex int, children []interface{}, err error) {
336424
block, _ := rootData["block"].(map[string]interface{})
337425
if len(block) == 0 {
338-
return "", 0, output.Errorf(output.ExitAPI, "api_error", "failed to query document root block")
426+
return "", 0, nil, output.Errorf(output.ExitAPI, "api_error", "failed to query document root block")
339427
}
340428

341-
parentBlockID := fallbackBlockID
429+
parentBlockID = fallbackBlockID
342430
if blockID, _ := block["block_id"].(string); blockID != "" {
343431
parentBlockID = blockID
344432
}
345433

346-
children, _ := block["children"].([]interface{})
347-
return parentBlockID, len(children), nil
434+
children, _ = block["children"].([]interface{})
435+
return parentBlockID, len(children), children, nil
436+
}
437+
438+
// locateInsertIndex uses the MCP locate-doc tool to find the root-level index
439+
// at which to insert relative to the block matching selection. It walks the
440+
// parent_id chain (using single-block GET calls when needed) to resolve nested
441+
// blocks to their top-level ancestor in rootChildren.
442+
func locateInsertIndex(runtime *common.RuntimeContext, documentID string, selection string, rootChildren []interface{}, before bool) (int, error) {
443+
// Ask for 2 matches so we can warn when the selection is ambiguous. locate-doc
444+
// orders matches by document position, so matches[0] is still deterministic.
445+
args := map[string]interface{}{
446+
"doc_id": documentID,
447+
"selection_with_ellipsis": selection,
448+
"limit": 2,
449+
}
450+
result, err := common.CallMCPTool(runtime, "locate-doc", args)
451+
if err != nil {
452+
return 0, err
453+
}
454+
455+
matches := common.GetSlice(result, "matches")
456+
if len(matches) == 0 {
457+
return 0, output.ErrWithHint(
458+
output.ExitValidation,
459+
"no_match",
460+
fmt.Sprintf("locate-doc did not find any block matching selection (%s)", redactSelection(selection)),
461+
"check spelling or use 'start...end' syntax to narrow the selection",
462+
)
463+
}
464+
if len(matches) > 1 {
465+
// Silently picking the first match surprises users whose selection appears
466+
// in more than one block (e.g. the same phrase in a title and a paragraph).
467+
// Surface that another match exists and point at the 'start...end' disambiguator.
468+
fmt.Fprintf(runtime.IO().ErrOut,
469+
"warning: selection (%s) matched more than one block; inserting relative to the first. "+
470+
"Pass --selection-with-ellipsis 'start...end' to narrow.\n",
471+
redactSelection(selection))
472+
}
473+
474+
matchMap, _ := matches[0].(map[string]interface{})
475+
anchorBlockID := common.GetString(matchMap, "anchor_block_id")
476+
if anchorBlockID == "" {
477+
// Fall back to first block entry if anchor_block_id is absent.
478+
blocks := common.GetSlice(matchMap, "blocks")
479+
if len(blocks) > 0 {
480+
if b, ok := blocks[0].(map[string]interface{}); ok {
481+
anchorBlockID = common.GetString(b, "block_id")
482+
}
483+
}
484+
}
485+
if anchorBlockID == "" {
486+
return 0, output.Errorf(output.ExitAPI, "api_error", "locate-doc response missing anchor_block_id")
487+
}
488+
parentBlockID := common.GetString(matchMap, "parent_block_id")
489+
490+
// Build root children set for O(1) lookup.
491+
rootSet := make(map[string]int, len(rootChildren))
492+
for i, c := range rootChildren {
493+
if id, ok := c.(string); ok {
494+
rootSet[id] = i
495+
}
496+
}
497+
498+
// Walk up the parent chain to the top-level ancestor in rootChildren. This
499+
// is serial by nature: each level's parent_id is only known after the
500+
// previous level's GET /blocks/{id} response arrives, so the calls cannot
501+
// be batched or parallelised.
502+
//
503+
// visited is the real cycle guard — it stops an A→B→A parent-id loop (seen
504+
// on malformed API responses) after one lap. maxDepth is belt-and-suspenders
505+
// in case both visited tracking and parent_id sanity simultaneously break;
506+
// 32 comfortably exceeds the deepest real docx nesting (~6–8 levels for
507+
// quote/callout/list combinations) without letting a bug run unbounded.
508+
cur := anchorBlockID
509+
nextParent := parentBlockID
510+
visited := map[string]bool{}
511+
const maxDepth = 32
512+
walkDepth := 0
513+
for depth := 0; depth < maxDepth; depth++ {
514+
if visited[cur] {
515+
break
516+
}
517+
visited[cur] = true
518+
519+
if idx, ok := rootSet[cur]; ok {
520+
if walkDepth > 0 {
521+
// The anchor was nested inside a callout / table cell / list and
522+
// got resolved to its top-level ancestor. Surface this so users
523+
// don't misread "insert before 'X'" as "insert right next to X"
524+
// when X is buried several levels deep.
525+
posLabel := "after"
526+
if before {
527+
posLabel = "before"
528+
}
529+
fmt.Fprintf(runtime.IO().ErrOut,
530+
"note: selection (%s) was nested %d level(s) deep; inserting %s its top-level ancestor at index %d\n",
531+
redactSelection(selection), walkDepth, posLabel, idx)
532+
}
533+
if before {
534+
return idx, nil
535+
}
536+
return idx + 1, nil
537+
}
538+
539+
// Advance: use the parent hint we already have, or fetch from API.
540+
parent := nextParent
541+
nextParent = "" // clear hint after first use
542+
if parent == "" || parent == cur {
543+
// Need to fetch this block to find its parent.
544+
data, err := runtime.CallAPI("GET",
545+
fmt.Sprintf("/open-apis/docx/v1/documents/%s/blocks/%s",
546+
validate.EncodePathSegment(documentID), validate.EncodePathSegment(cur)),
547+
nil, nil)
548+
if err != nil {
549+
return 0, err
550+
}
551+
block := common.GetMap(data, "block")
552+
parent = common.GetString(block, "parent_id")
553+
}
554+
if parent == "" || parent == cur {
555+
break
556+
}
557+
cur = parent
558+
walkDepth++
559+
}
560+
561+
return 0, output.ErrWithHint(
562+
output.ExitValidation,
563+
"block_not_reachable",
564+
fmt.Sprintf("block matching selection (%s) is not reachable from document root", redactSelection(selection)),
565+
"try a top-level heading or paragraph as the selection",
566+
)
348567
}
349568

350569
func extractCreatedBlockTargets(createData map[string]interface{}, mediaType string) (blockID, uploadParentNode, replaceBlockID string) {

0 commit comments

Comments
 (0)