feat(doc): add --selection-with-ellipsis position flag to +media-insert#335
Open
herbertliu wants to merge 5 commits intomainfrom
Open
feat(doc): add --selection-with-ellipsis position flag to +media-insert#335herbertliu wants to merge 5 commits intomainfrom
herbertliu wants to merge 5 commits intomainfrom
Conversation
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 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- 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% Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
📝 WalkthroughWalkthroughAdded CLI flags Changes
Sequence DiagramsequenceDiagram
participant CLI as Client CLI
participant Cmd as DocMediaInsert Command
participant MCP as MCP (locate-doc)
participant API as Document API / Store
CLI->>Cmd: run with --selection-with-ellipsis [--before]
Cmd->>Cmd: validate flags
Cmd->>MCP: locate-doc(selection)
MCP-->>Cmd: matches + anchor_block_id + parent hints
Cmd->>API: walk parent chain / GET blocks as needed
API-->>Cmd: parent/ancestor info up to root
Cmd->>Cmd: compute insertIndex (before/after)
Cmd->>API: create placeholder block at insertIndex
API-->>CLI: confirm planned/created insertion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
domain/ccm
Greptile SummaryThis PR replaces the old Confidence Score: 5/5Safe to merge; core logic is correct with proper cycle detection, rollback, and dry-run step accounting. No P0 or P1 issues found. The single P2 finding (integration tests not asserting on the computed insert index) is a test-quality suggestion that does not affect production behavior. The locateInsertIndex traversal, visited-set cycle guard, and rollback all look correct. shortcuts/doc/doc_media_insert_test.go — the create-block stub should match on the expected index to catch future regressions.
|
| Filename | Overview |
|---|---|
| shortcuts/doc/doc_media_insert.go | New locateInsertIndex function and updated DryRun/Execute paths for --selection-with-ellipsis; logic is sound but integration tests don't verify the computed index value sent to the create-block API. |
| shortcuts/doc/doc_media_insert_test.go | Good coverage of after/before/nested/no-match/dry-run paths, but the create-block stub accepts any body so index correctness isn't verified by the integration tests. |
Sequence Diagram
sequenceDiagram
participant CLI
participant DocxAPI as Docx API
participant MCPAPI as MCP locate-doc
participant DriveAPI as Drive API
CLI->>DocxAPI: GET /documents/{id}/blocks/{id} (root block)
DocxAPI-->>CLI: root children list
alt --selection-with-ellipsis provided
CLI->>MCPAPI: POST /mcp tools/call locate-doc
MCPAPI-->>CLI: {anchor_block_id, parent_block_id}
CLI->>CLI: Walk parent chain (parent_block_id hint or GET block)
CLI->>CLI: Resolve root-level insert index (before ? idx : idx+1)
else default append
CLI->>CLI: insertIndex = len(rootChildren)
end
CLI->>DocxAPI: POST /documents/{id}/blocks/{id}/children (index=N)
DocxAPI-->>CLI: {block_id}
CLI->>DriveAPI: POST /medias/upload_all (or multipart)
DriveAPI-->>CLI: {file_token}
CLI->>DocxAPI: PATCH /documents/{id}/blocks/batch_update
DocxAPI-->>CLI: ok
note over CLI,DocxAPI: On upload/bind failure: DELETE /children/batch_delete (rollback)
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@4ccb830be3292996afa9c5ac348a3c9b2402f9eb🧩 Skill updatenpx skills add larksuite/cli#feat/media-insert-keyword-position -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
shortcuts/doc/doc_media_insert_test.go (1)
112-124: Assert the actual root-child IDs and order.The new
childrenreturn value is used to map keyword hits back to a root-level insertion index. Checking onlylen(children)will miss regressions that reorder or replace the IDs.Suggested patch
blockID, index, children, err := extractAppendTarget(rootData, "fallback") @@ - if len(children) != 3 { - t.Fatalf("extractAppendTarget() children len = %d, want 3", len(children)) + wantChildren := []interface{}{"c1", "c2", "c3"} + if !reflect.DeepEqual(children, wantChildren) { + t.Fatalf("extractAppendTarget() children = %#v, want %#v", children, wantChildren) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert_test.go` around lines 112 - 124, The test currently only checks len(children) which misses regressions in order or identity; update the test around the call to extractAppendTarget(rootData, "fallback") to assert that children equals the expected slice of root child IDs in the correct order (e.g., compare children to the explicit expected []string{"child1","child2","child3"} using reflect.DeepEqual or your test helper) and fail the test with a clear message if they differ; keep the existing checks for blockID and index and replace the len(children) assertion with this full equality check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/doc_media_insert.go`:
- Around line 377-410: fetchAllBlocks currently can silently return a partial
result when the API still indicates more pages (has_more==true) or when
has_more==true but no page_token is provided; update fetchAllBlocks to detect
these cases and return an explicit error instead of a partial slice: inside the
loop in fetchAllBlocks, if has_more is true and page == maxPages-1 (i.e. we've
hit the maxPages limit) return an error indicating pagination truncated;
likewise if has_more is true but page_token is empty return an error indicating
inconsistent pagination (has_more without page_token); reference the
variables/functions fetchAllBlocks, maxPages, pageToken and has_more when
implementing the checks and return a descriptive error rather than breaking and
returning partial all.
- Around line 64-80: The dry-run output still shows index: "<children_len>" even
when keyword insertion is used; update the dry-run create block payload to
reflect the computed insertion index when hasKeyword is true. In the block
around afterKeyword/beforeKeyword/hasKeyword and createBlockData (built via
buildCreateBlockData) replace or override createBlockData["index"] with a
descriptive placeholder that represents the computed position (e.g.,
"<insertion_index_from_search>") when hasKeyword is true, and ensure any related
batchUpdateData or totalSteps logic (the DryRunAPI usage and totalSteps
increment) remains consistent so the dry-run plan accurately describes
keyword-mode insertion.
In `@shortcuts/doc/doc_media_test.go`:
- Around line 445-455: The test's dry-run check falsely matches document root
block fetches because it only checks for substring "/blocks" in step.URL; update
the loop in the test (the foundListBlocks logic that iterates dry.API and
inspects step.URL) to assert the exact list-all-blocks endpoint (e.g., equality
to "/blocks" or the full expected path) or alternatively check step.Description
(or another explicit field) for the "List all blocks" label to reliably detect
the extra list-all-blocks step instead of any URL containing "/blocks".
- Around line 347-405: Change the first
/open-apis/docx/v1/documents/doxcnFB/blocks stub to return "has_more": true and
only non-keyword items, then add a second GET stub for the same URL that
emulates page 2 (e.g., using a page token) containing the keyword block so
pagination must be followed; update the create-child POST stub registered for
"/open-apis/docx/v1/documents/doxcnFB/blocks/doxcnFB/children" to validate the
incoming request body (assert the "index" field or placement in the JSON matches
the computed insert index) instead of accepting any body; apply the same changes
to the analogous stubs referenced around lines 411-420 so the test actually
verifies page-token handling and that Execute inserts at the expected index.
- Around line 330-331: Before calling cmdutil.TestFactory in
TestFetchAllBlocksPaginationViaExecute, isolate the CLI config by setting the
LARKSUITE_CLI_CONFIG_DIR env var to a test temp directory; add
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of
TestFetchAllBlocksPaginationViaExecute (before invoking cmdutil.TestFactory) so
the test uses an isolated config directory and won’t share state with other
tests.
---
Nitpick comments:
In `@shortcuts/doc/doc_media_insert_test.go`:
- Around line 112-124: The test currently only checks len(children) which misses
regressions in order or identity; update the test around the call to
extractAppendTarget(rootData, "fallback") to assert that children equals the
expected slice of root child IDs in the correct order (e.g., compare children to
the explicit expected []string{"child1","child2","child3"} using
reflect.DeepEqual or your test helper) and fail the test with a clear message if
they differ; keep the existing checks for blockID and index and replace the
len(children) assertion with this full equality check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0c5801d-351b-4e8b-8a79-34e0b5cc29ec
📒 Files selected for processing (3)
shortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_insert_test.goshortcuts/doc/doc_media_test.go
shortcuts/doc/doc_media_test.go
Outdated
Comment on lines
+330
to
+331
| func TestFetchAllBlocksPaginationViaExecute(t *testing.T) { | ||
| f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("fetch-blocks-app")) |
There was a problem hiding this comment.
Isolate config state before creating the test factory.
This case uses cmdutil.TestFactory(...) without setting LARKSUITE_CLI_CONFIG_DIR, so it can still share config state with other tests.
Suggested patch
func TestFetchAllBlocksPaginationViaExecute(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
f, _, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("fetch-blocks-app"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/doc/doc_media_test.go` around lines 330 - 331, Before calling
cmdutil.TestFactory in TestFetchAllBlocksPaginationViaExecute, isolate the CLI
config by setting the LARKSUITE_CLI_CONFIG_DIR env var to a test temp directory;
add t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of
TestFetchAllBlocksPaginationViaExecute (before invoking cmdutil.TestFactory) so
the test uses an isolated config directory and won’t share state with other
tests.
Collaborator
Author
There was a problem hiding this comment.
TestFetchAllBlocksPaginationViaExecute has been removed in commit c505fdb. This comment is no longer applicable.
There was a problem hiding this comment.
@herbertliu, thanks for the heads-up! Since TestFetchAllBlocksPaginationViaExecute has been removed in commit c505fdb, this comment is indeed no longer applicable. I'll disregard it.
🧠 Learnings used
Learnt from: CR
Repo: larksuite/cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-07T07:21:36.531Z
Learning: Applies to **/*_test.go : Isolate config state in Go tests by using `t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())`
Learnt from: caojie0621
Repo: larksuite/cli PR: 343
File: shortcuts/sheets/sheet_write_image_test.go:43-218
Timestamp: 2026-04-08T13:43:02.920Z
Learning: In larksuite/cli tests, only set `LARKSUITE_CLI_CONFIG_DIR` (e.g., via `t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())`) when the test exercises the real `NewDefault()` factory path that reads from the filesystem. Shortcut tests that build the CLI via `cmdutil.TestFactory(t, config)` should not set this env var because `TestFactory` provides an in-memory config closure (`func() (*core.CliConfig, error) { return config, nil }`) and does not access the filesystem.
Learnt from: niuchong0523
Repo: larksuite/cli PR: 280
File: internal/registry/registry_test.go:12-16
Timestamp: 2026-04-07T09:18:52.227Z
Learning: In the larksuite/cli repository, `ensureFreshRegistry(t *testing.T)` in `internal/registry/registry_test.go` isolates registry state by calling `t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())` to prevent machine-local cache reads, `t.Setenv("LARKSUITE_CLI_REMOTE_META", "off")` to disable remote fetches, then `resetInit()` and `Init()` to reinitialize from a clean state.
Learnt from: CR
Repo: larksuite/cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-07T07:21:36.531Z
Learning: Applies to **/*_test.go : Use `cmdutil.TestFactory(t, config)` for creating test factories in Go tests
Learnt from: caojie0621
Repo: larksuite/cli PR: 343
File: shortcuts/sheets/sheet_write_image_test.go:261-263
Timestamp: 2026-04-08T13:54:11.437Z
Learning: In larksuite/cli Go tests (files matching *_test.go), when creating or populating test fixtures under t.TempDir(), use the standard library filesystem APIs: os.Mkdir, os.Create, os.CreateTemp, and os.WriteFile. Treat the vfs.* abstraction as production-only infrastructure: do not use vfs.* for test fixture setup inside *_test.go files, since it would make the tests circular (vfs is intended to be mockable/mocked by tests rather than used to create the fixtures themselves).
Replace fetchAllBlocks + keyword scan with MCP locate-doc tool, consistent with DriveAddComment. Flags changed from --after-keyword / --before-keyword to --selection-with-ellipsis + --before. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
herbertliu
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/doc/doc_media_insert.go (1)
64-69:⚠️ Potential issue | 🟡 MinorSelection-mode dry-run still advertises append-to-end.
Once
--selection-with-ellipsisis set, the plan resolves a target position, but the create-block payload still showsindex: "<children_len>". That keeps the dry-run misleading for the new flow. Switch it to a target-position placeholder in selection mode.Suggested patch
createBlockData := buildCreateBlockData(mediaType, 0) createBlockData["index"] = "<children_len>" + if hasSelection { + createBlockData["index"] = "<target_index>" + }Also applies to: 77-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert.go` around lines 64 - 69, The dry-run always sets createBlockData["index"] to "<children_len>" even when selection mode is active; update the logic that builds the create-block payload so when selection := strings.TrimSpace(runtime.Str("selection-with-ellipsis")) yields hasSelection (selection != ""), replace the index placeholder with a target-position placeholder (e.g. "<target-position>") instead of "<children_len>". Adjust both the initial creation path around parentTypeForMediaType(mediaType) / buildCreateBlockData(mediaType, 0) and the duplicate handling later in the same function (the code covering the 77-123 region) so any createBlockData["index"] assignment uses the selection-mode placeholder when hasSelection is true.
🧹 Nitpick comments (2)
shortcuts/doc/doc_media_insert.go (1)
435-468: Avoid hard-coding an 8-level ancestry limit here.
visitedalready prevents parent-chain loops.const maxDepth = 8adds an undocumented client-side nesting cap, so a reachable match deeper than that will be reported asblock_not_reachable. Prefer terminating onparent == ""/parent == curinstead of a fixed depth ceiling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert.go` around lines 435 - 468, Remove the artificial const maxDepth = 8 and the depth-limited for loop; instead iterate until you hit the root or a visited node so only the visited map prevents infinite loops. Replace the for depth := 0; depth < maxDepth; depth++ { ... } with an unconditional loop (for { ... }) that keeps the existing visited[cur] check, the rootSet lookup (returning idx or idx+1), the parent hint handling via nextParent, the runtime.CallAPI fetch using documentID and cur to get parent_id, and the break conditions parent == "" or parent == cur; ensure no other implicit depth cap remains.shortcuts/doc/doc_media_insert_test.go (1)
153-205: These tests still don't prove the computed insertion position.The new behavior lives in the MCP request args and the
indexsent to the create-block call, but the stubs accept any request body and the assertions only check success / presence of alocate-docstep. A regression that always appends or flips--beforewould still pass. Please assert the outgoingselection_with_ellipsisand create-blockindexfor each scenario.Also applies to: 208-321, 367-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert_test.go` around lines 153 - 205, The tests currently register permissive stubs in registerInsertWithSelectionStubs (and the similar stubs around the other ranges) which accept any request bodies, so they don't verify the computed insertion position; update those stubs to assert the outgoing MCP POST contains the expected selection_with_ellipsis value and that the create-block POST to "/open-apis/docx/v1/documents/{docID}/blocks/{docID}/children" includes the correct "index" in its request body. Concretely, modify registerInsertWithSelectionStubs (and the corresponding stub registrations in the 208-321 and 367-410 blocks) to set an expected request body or a request matcher on the MCP stub that checks selection_with_ellipsis equals the scenario value, and on the create-block stub to validate the "index" field (failing the test if mismatched); keep existing response bodies but add these request assertions so regressions flipping/always-appending the index will fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/doc/doc_media_insert.go`:
- Around line 64-69: The dry-run always sets createBlockData["index"] to
"<children_len>" even when selection mode is active; update the logic that
builds the create-block payload so when selection :=
strings.TrimSpace(runtime.Str("selection-with-ellipsis")) yields hasSelection
(selection != ""), replace the index placeholder with a target-position
placeholder (e.g. "<target-position>") instead of "<children_len>". Adjust both
the initial creation path around parentTypeForMediaType(mediaType) /
buildCreateBlockData(mediaType, 0) and the duplicate handling later in the same
function (the code covering the 77-123 region) so any createBlockData["index"]
assignment uses the selection-mode placeholder when hasSelection is true.
---
Nitpick comments:
In `@shortcuts/doc/doc_media_insert_test.go`:
- Around line 153-205: The tests currently register permissive stubs in
registerInsertWithSelectionStubs (and the similar stubs around the other ranges)
which accept any request bodies, so they don't verify the computed insertion
position; update those stubs to assert the outgoing MCP POST contains the
expected selection_with_ellipsis value and that the create-block POST to
"/open-apis/docx/v1/documents/{docID}/blocks/{docID}/children" includes the
correct "index" in its request body. Concretely, modify
registerInsertWithSelectionStubs (and the corresponding stub registrations in
the 208-321 and 367-410 blocks) to set an expected request body or a request
matcher on the MCP stub that checks selection_with_ellipsis equals the scenario
value, and on the create-block stub to validate the "index" field (failing the
test if mismatched); keep existing response bodies but add these request
assertions so regressions flipping/always-appending the index will fail.
In `@shortcuts/doc/doc_media_insert.go`:
- Around line 435-468: Remove the artificial const maxDepth = 8 and the
depth-limited for loop; instead iterate until you hit the root or a visited node
so only the visited map prevents infinite loops. Replace the for depth := 0;
depth < maxDepth; depth++ { ... } with an unconditional loop (for { ... }) that
keeps the existing visited[cur] check, the rootSet lookup (returning idx or
idx+1), the parent hint handling via nextParent, the runtime.CallAPI fetch using
documentID and cur to get parent_id, and the break conditions parent == "" or
parent == cur; ensure no other implicit depth cap remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 433c3a8d-8d5f-45a9-9df9-90756c31b610
📒 Files selected for processing (2)
shortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_insert_test.go
…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. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/doc_media_insert.go`:
- Line 185: The stderr logging prints the raw variable selection (selection) via
fmt.Fprintf(runtime.IO().ErrOut, ...) which can leak document content; update
all occurrences (the fmt.Fprintf calls around "Locating block matching
selection" and the error paths around lines ~402-407 and ~474-479) to sanitize
the value before logging—e.g., replace full selection with a redacted/trimmed
representation (mask middle with "…", show only safe prefix/suffix or a length
and hash), and use that sanitized string in the fmt.Fprintf/error messages
instead of the raw selection variable.
- Around line 48-50: Add validation in the Validate function to reject blank or
whitespace-only values for the --selection-with-ellipsis flag so it cannot
silently fall back to append-mode; specifically, call
runtime.Str("selection-with-ellipsis") (RuntimeContext.Str /
RuntimeContext.Validate) and trim it (strings.TrimSpace) and return an error if
the result is empty, regardless of runtime.Bool("before") — this mirrors the
existing before-mode check around the if using runtime.Bool("before") but
enforces it unconditionally in Validate to catch cases where the flag was
provided but blank.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db0c7312-a58c-47ae-ba53-7100d2510fae
📒 Files selected for processing (2)
shortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_insert_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/doc/doc_media_insert_test.go
Comment on lines
+48
to
+50
| if runtime.Bool("before") && strings.TrimSpace(runtime.Str("selection-with-ellipsis")) == "" { | ||
| return output.ErrValidation("--before requires --selection-with-ellipsis") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n shortcuts/doc/doc_media_insert.go | head -200Repository: larksuite/cli
Length of output: 8838
🏁 Script executed:
cat -n shortcuts/doc/doc_media_insert.go | sed -n '175,220p'Repository: larksuite/cli
Length of output: 2234
🏁 Script executed:
find . -name "*.go" -type f -exec grep -l "RuntimeContext" {} \; | head -5Repository: larksuite/cli
Length of output: 7126
🏁 Script executed:
find shortcuts/common -name "*.go" -type f | head -10Repository: larksuite/cli
Length of output: 381
🏁 Script executed:
grep -n "type RuntimeContext" shortcuts/common/common.goRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
rg "type RuntimeContext" --type goRepository: larksuite/cli
Length of output: 112
🏁 Script executed:
cat -n shortcuts/common/runner.go | head -150Repository: larksuite/cli
Length of output: 5978
🏁 Script executed:
cat -n shortcuts/common/runner.go | sed -n '109,135p'Repository: larksuite/cli
Length of output: 908
🏁 Script executed:
rg "selection-with-ellipsis" --type go -B 2 -A 2 | head -40Repository: larksuite/cli
Length of output: 3302
🏁 Script executed:
rg "GetString\|GetBool" shortcuts/common/runner.go -A 2 -B 2Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
rg "Flag" shortcuts/common/common.go | head -20Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
rg "AddCommand\|PersistentFlags\|Flags\(\)" shortcuts/common/runner.go | head -30Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
cat -n shortcuts/common/common.go | head -100Repository: larksuite/cli
Length of output: 3681
🏁 Script executed:
rg "AddCommand\|AddStringFlag\|RegisterFlagCompletionFunc" shortcuts/common/ -A 3 | head -40Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
rg "AddCommand\|.Flags\(\)" shortcuts/base/ -l | head -5Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
rg "mountDeclarative\|RegisterFlags" shortcuts/ -l | head -5Repository: larksuite/cli
Length of output: 39
Validate --selection-with-ellipsis to prevent silent fallback to append-mode.
Line 48 only validates --selection-with-ellipsis when --before is set. In the default after-mode, whitespace-only values are silently trimmed to empty on lines 64-65 and 182-183, causing the command to fall back to appending at document end instead of failing. This turns a malformed positioning argument into an invisible write at the wrong location.
Since RuntimeContext.Str() delegates to Cobra's GetString() and cannot distinguish "flag omitted" from "flag provided but blank", add explicit validation in the Validate function to reject blank or whitespace-only --selection-with-ellipsis values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/doc/doc_media_insert.go` around lines 48 - 50, Add validation in
the Validate function to reject blank or whitespace-only values for the
--selection-with-ellipsis flag so it cannot silently fall back to append-mode;
specifically, call runtime.Str("selection-with-ellipsis") (RuntimeContext.Str /
RuntimeContext.Validate) and trim it (strings.TrimSpace) and return an error if
the result is empty, regardless of runtime.Bool("before") — this mirrors the
existing before-mode check around the if using runtime.Bool("before") but
enforces it unconditionally in Validate to catch cases where the flag was
provided but blank.
| 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) |
There was a problem hiding this comment.
Avoid logging raw document text from --selection-with-ellipsis.
The new stderr message on Line 185 and both error paths now echo the selection verbatim. Because this flag is copied from document content, failed runs will leak raw doc text into terminal and CI logs.
🔐 Minimal redaction patch
- fmt.Fprintf(runtime.IO().ErrOut, "Locating block matching selection: %q\n", selection)
+ fmt.Fprintf(runtime.IO().ErrOut, "Locating block matching selection\n")
...
- fmt.Sprintf("locate-doc did not find any block matching selection %q", selection),
+ "locate-doc did not find any block matching selection",
...
- fmt.Sprintf("block matching selection %q is not reachable from document root", selection),
+ "block matching selection is not reachable from document root",Also applies to: 402-407, 474-479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/doc/doc_media_insert.go` at line 185, The stderr logging prints the
raw variable selection (selection) via fmt.Fprintf(runtime.IO().ErrOut, ...)
which can leak document content; update all occurrences (the fmt.Fprintf calls
around "Locating block matching selection" and the error paths around lines
~402-407 and ~474-479) to sanitize the value before logging—e.g., replace full
selection with a redacted/trimmed representation (mask middle with "…", show
only safe prefix/suffix or a length and hash), and use that sanitized string in
the fmt.Fprintf/error messages instead of the raw selection variable.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
docs +media-insertcurrently only supports appending media to the end of a document. This PR adds two new flags —--selection-with-ellipsisand--before— that allow inserting an image or file at a position relative to a block identified by a text selection, using the MCPlocate-doctool (the same approach used bydrive +add-comment).This is more efficient than the earlier approach of fetching all document blocks:
locate-docreturns the matching block directly without paginated enumeration.Changes
shortcuts/doc/doc_media_insert.go:--after-keyword/--before-keywordwith--selection-with-ellipsis(plain text orstart...endsyntax) +--beforebool flagfetchAllBlocks,extractBlockPlainText,findInsertIndexByKeywordlocateInsertIndex(): calls MCPlocate-doc, usesparent_block_idhint to walk to the root-level ancestor (zero extra API calls in the common case; falls back to single-block GET for deeper nesting, max 8 levels)DryRunto show the MCPlocate-docstep when--selection-with-ellipsisis setMCPEndpointcall insideif hasSelectionto avoid nil-config panic in testsshortcuts/doc/doc_media_insert_test.go:extractBlockPlainText,findInsertIndexByKeyword)Execute+ httpmock: after-mode, before-mode, nested block (parent_block_id walking), no-match errorlocate-docstep appears when--selection-with-ellipsisis setshortcuts/doc/doc_media_test.go:TestFetchAllBlocksPaginationViaExecuteandTestDocMediaInsertDryRunWithAfterKeyword(deleted functions / renamed flags)Test Plan
go test ./shortcuts/doc/...— all tests passgo vet ./...— no issuesgofmt -l .— no unformatted fileslocateInsertIndexcoverage: 68.9% (above 60% threshold)Related Issues
Summary by CodeRabbit
New Features
Tests