feat(doc): warn before overwrite when document contains whiteboard or file blocks#825
feat(doc): warn before overwrite when document contains whiteboard or file blocks#825herbertliu wants to merge 3 commits into
Conversation
… file blocks Before executing an overwrite in v1 mode, pre-fetch the current document and scan the Markdown for <whiteboard> and <file> resource blocks. If any are found, print a warning to stderr listing the counts and suggesting the user take a backup with `docs +fetch` first. Overwrite replaces the entire document and cannot reconstruct these blocks from Markdown; previously the data was lost with no indication to the caller. The check is best-effort: a failed pre-fetch silently skips the guard rather than blocking the overwrite.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a best-effort pre-flight check for ChangesOverwrite Safety Guard
Sequence DiagramsequenceDiagram
participant User
participant executeUpdateV1
participant warnOverwriteResourceBlocks
participant MCP as fetch-doc_MCP_tool
participant checkOverwriteResourceBlocks
User->>executeUpdateV1: run update --mode overwrite
executeUpdateV1->>warnOverwriteResourceBlocks: call to pre-flight check
warnOverwriteResourceBlocks->>MCP: fetch-doc
MCP-->>warnOverwriteResourceBlocks: current document markdown
warnOverwriteResourceBlocks->>checkOverwriteResourceBlocks: scan markdown
checkOverwriteResourceBlocks-->>warnOverwriteResourceBlocks: warning text (if any)
warnOverwriteResourceBlocks-->>executeUpdateV1: warning string
executeUpdateV1->>executeUpdateV1: print warning to stderr
executeUpdateV1->>MCP: update-doc (proceed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
domain/ccm
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #825 +/- ##
==========================================
+ Coverage 65.67% 65.92% +0.25%
==========================================
Files 513 518 +5
Lines 47655 48872 +1217
==========================================
+ Hits 31297 32220 +923
- Misses 13652 13885 +233
- Partials 2706 2767 +61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@977e057d6ff641ef85de6c5198ea6f4e2bdb9ee2🧩 Skill updatenpx skills add larksuite/cli#feat/docs-overwrite-resource-block-guard -y -g |
…pty-md guard in warnOverwriteResourceBlocks
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Thanks for the PR! The best-effort warning design is sensible. I have a few concerns — the substring matching issue (#1) could lead to false positives and is worth addressing; the rest are suggestions.
| // warning string listing the counts if any are found, empty string otherwise. | ||
| func checkOverwriteResourceBlocks(markdown string) string { | ||
| var found []string | ||
| if n := strings.Count(markdown, "<whiteboard"); n > 0 { |
There was a problem hiding this comment.
Substring matching is too coarse — risk of false positives.
strings.Count(markdown, "<whiteboard") and strings.Count(markdown, "<file") can produce false matches:
- Text content: If the document contains prose mentioning whiteboards (e.g. a code example
<whiteboard token="..."/>), it will be counted as a whiteboard block. <fileprefix is overly broad: It could match<file-view>,<file-name>, or other tags that are not file attachment blocks.
Suggested fix — use a more precise regex:
var (
whiteboardRe = regexp.MustCompile(`<whiteboard[\s/>]`)
fileRe = regexp.MustCompile(`<file[\s/>]`)
)Or better yet, check block types directly via the document blocks API (/blocks) instead of scanning the Markdown representation.
There was a problem hiding this comment.
Fixed in 977e057. Replaced strings.Count with a regex <(whiteboard|file)[\s/>] that requires the tag name to be followed by whitespace, >, or / — so prose mentioning "whiteboard" or tags like <file-view> no longer match.
| }) | ||
| } | ||
|
|
||
| func TestValidateSelectionByTitleV1(t *testing.T) { |
There was a problem hiding this comment.
Unrelated test added in this PR.
TestValidateSelectionByTitleV1 tests validateSelectionByTitleV1, which is not part of this PR's changes. Unrelated tests should go in a separate PR to keep the diff focused and the review scoped.
There was a problem hiding this comment.
Acknowledged. The test was added to improve patch coverage for the PR (codecov/patch was at 58.82%, below the 60% threshold) rather than because it is logically related to this change. Happy to move it to a separate follow-up PR if that is preferred, but since the function being tested (validateSelectionByTitleV1) lives in the same file and had zero test coverage, it felt like a reasonable addition in-place.
| // Markdown. Pre-fetch the current content and warn when such blocks | ||
| // are present so the caller can take a backup before proceeding. | ||
| if runtime.Str("mode") == "overwrite" { | ||
| if w := warnOverwriteResourceBlocks(runtime); w != "" { |
There was a problem hiding this comment.
Extra latency from pre-fetch on every overwrite.
Every --mode overwrite call now incurs an additional fetch-doc MCP round-trip, even when the document has no resource blocks. This adds network latency to the critical path.
Consider:
- Documenting the performance impact in the PR description or code comments
- Making the guard opt-in via a flag (e.g.
--warn-resource-loss) so callers who don't need the warning aren't penalized
There was a problem hiding this comment.
Documented in 977e057. Added a comment above warnOverwriteResourceBlocks explaining that every --mode overwrite call incurs one extra fetch-doc round-trip, that the cost is intentional, and why the trade-off is acceptable (best-effort, silent on failure, bounded latency).
| // attachment blocks that would be permanently deleted by an overwrite. Returns | ||
| // an empty string (no warning) when the document is clean or the fetch fails | ||
| // (we never block the overwrite on a best-effort check). | ||
| func warnOverwriteResourceBlocks(runtime *common.RuntimeContext) string { |
There was a problem hiding this comment.
warnOverwriteResourceBlocks lacks test coverage.
checkOverwriteResourceBlocks has thorough unit tests, but warnOverwriteResourceBlocks (the MCP integration layer) has none. While MCP calls are hard to mock, it would be helpful to either:
- Add a comment explaining why it's not tested (best-effort, depends on external MCP call)
- Provide a mock-based test for the happy path and the silent-failure-on-error path
There was a problem hiding this comment.
Documented in 977e057. Added a comment to warnOverwriteResourceBlocks explaining that it is not unit-tested because it depends on an external MCP call, and that the pure detection logic is fully covered by checkOverwriteResourceBlocks tests.
| func warnOverwriteResourceBlocks(runtime *common.RuntimeContext) string { | ||
| args := map[string]interface{}{ | ||
| "doc_id": runtime.Str("doc"), | ||
| "skip_task_detail": true, |
There was a problem hiding this comment.
skip_task_detail: true lacks a comment.
The purpose of skip_task_detail is not self-evident. Please add a brief comment explaining why it's set (presumably to reduce response payload size and speed up the pre-fetch).
There was a problem hiding this comment.
Fixed in 977e057. Added an inline comment: // skip_task_detail reduces response payload by omitting per-block task metadata, making the pre-fetch faster and cheaper.
…e comments, document skip_task_detail purpose
2 participants
Summary
Add a best-effort pre-flight guard for the v1
overwritemode that warns the user when the current document contains whiteboard or file attachment blocks that cannot be reconstructed from Markdown.Changes
shortcuts/doc/docs_update.go: InexecuteUpdateV1, pre-fetch the document when--mode overwriteand callwarnOverwriteResourceBlocks/checkOverwriteResourceBlocksto detect resource blocksshortcuts/doc/docs_update_test.go: AddTestCheckOverwriteResourceBlockswith table-driven cases covering single/multiple whiteboard blocks, file attachments, and combined casesBehaviour
When
--mode overwriteis used and the current document contains<whiteboard>or<file>blocks:The guard is best-effort: if the pre-fetch fails (network error, permission, etc.) the warning is silently skipped and the overwrite proceeds normally. The overwrite is never blocked — this is a warning only.
Test Plan
go test ./shortcuts/doc/...— all passgo vet ./...— cleangofmt -l .— cleanSummary by CodeRabbit
New Features
Tests