fix: normalize document write inputs#2003
Closed
fangshuyu-768 wants to merge 3 commits into
Closed
Conversation
github-actions
Bot
added
domain/ccm
📝 WalkthroughWalkthroughThe PR normalizes XML titles during document creation by giving the CLI title precedence, strengthens v2 update validation for multiline XML patterns and invalid block IDs, and adds unit, contract, and CLI dry-run coverage. ChangesDocument v2 input handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant stripTopLevelXMLTitles
participant CreateRequest
CLI->>stripTopLevelXMLTitles: XML content and CLI title
stripTopLevelXMLTitles->>CLI: Content without top-level title elements
CLI->>CreateRequest: Escaped CLI title plus normalized content
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@15263efe3045463181fa2f197fc4d263b43fb6ff🧩 Skill updatenpx skills add larksuite/cli#codex/fix-docs-write-preflight -y -g |
fangshuyu-768
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/doc/docs_create_v2.go`:
- Around line 170-177: The title-stripping reconstruction in
shortcuts/doc/docs_create_v2.go#L170-L177 must return the built string without
strings.TrimSpace, preserving surrounding whitespace exactly; update
shortcuts/doc/docs_create_test.go#L34-L36 to expect "\n<p>body</p>\n" after
removing both title elements, and add an adjacent-text case if needed to verify
whitespace preservation.
In `@tests/cli_e2e/docs/docs_update_dryrun_test.go`:
- Around line 229-249: Isolate configuration in both dry-run tests,
TestDocs_CreateTitleDryRunNormalizesXMLTitle and the adjacent test covering
lines 251-284, by setting LARKSUITE_CLI_CONFIG_DIR to a unique t.TempDir() value
alongside the existing fake credential environment setup. Ensure each test uses
its own temporary configuration directory.
🪄 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: 7880facd-d2da-4317-971c-6eb8c3400b31
📒 Files selected for processing (5)
shortcuts/doc/doc_errors_test.goshortcuts/doc/docs_create_test.goshortcuts/doc/docs_create_v2.goshortcuts/doc/docs_update_v2.gotests/cli_e2e/docs/docs_update_dryrun_test.go
💤 Files with no reviewable changes (1)
- shortcuts/doc/doc_errors_test.go
Comment on lines
+170
to
+177
| var result strings.Builder | ||
| cursor := int64(0) | ||
| for _, item := range ranges { | ||
| result.WriteString(content[int(cursor):int(item.start)]) | ||
| cursor = item.end | ||
| } | ||
| result.WriteString(content[int(cursor):]) | ||
| return strings.TrimSpace(result.String()) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve non-title content exactly when stripping titles.
strings.TrimSpace removes bytes outside the matched <title> ranges, including meaningful leading/trailing text-node whitespace. Remove the trim and update the expectation to retain the surrounding newlines; add an adjacent-text case if needed.
shortcuts/doc/docs_create_v2.go#L170-L177: return the reconstructed string withoutstrings.TrimSpace.shortcuts/doc/docs_create_test.go#L34-L36: expect"\n<p>body</p>\n"after removing both title elements.
As per coding guidelines, “When transcribing input or transforming requests, preserve values faithfully.”
Proposed fix
- return strings.TrimSpace(result.String())
+ return result.String()- want: "<p>body</p>",
+ want: "\n<p>body</p>\n",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var result strings.Builder | |
| cursor := int64(0) | |
| for _, item := range ranges { | |
| result.WriteString(content[int(cursor):int(item.start)]) | |
| cursor = item.end | |
| } | |
| result.WriteString(content[int(cursor):]) | |
| return strings.TrimSpace(result.String()) | |
| var result strings.Builder | |
| cursor := int64(0) | |
| for _, item := range ranges { | |
| result.WriteString(content[int(cursor):int(item.start)]) | |
| cursor = item.end | |
| } | |
| result.WriteString(content[int(cursor):]) | |
| return result.String() |
📍 Affects 2 files
shortcuts/doc/docs_create_v2.go#L170-L177(this comment)shortcuts/doc/docs_create_test.go#L34-L36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/doc/docs_create_v2.go` around lines 170 - 177, The title-stripping
reconstruction in shortcuts/doc/docs_create_v2.go#L170-L177 must return the
built string without strings.TrimSpace, preserving surrounding whitespace
exactly; update shortcuts/doc/docs_create_test.go#L34-L36 to expect
"\n<p>body</p>\n" after removing both title elements, and add an adjacent-text
case if needed to verify whitespace preservation.
Source: Coding guidelines
Comment on lines
+229
to
+249
| func TestDocs_CreateTitleDryRunNormalizesXMLTitle(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_APP_ID", "app") | ||
| t.Setenv("LARKSUITE_CLI_APP_SECRET", "secret") | ||
| t.Setenv("LARKSUITE_CLI_BRAND", "feishu") | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| t.Cleanup(cancel) | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{ | ||
| "docs", "+create", | ||
| "--title", "Flag title", | ||
| "--content", "<title>Content title</title><p>body</p>", | ||
| "--dry-run", | ||
| }, | ||
| DefaultAs: "bot", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| require.Equal(t, "<title>Flag title</title>\n<p>body</p>", clie2e.DryRunGet(result.Stdout, "api.0.body.content").String()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Isolate CLI configuration state in the new dry-run tests.
Both tests set fake credentials but inherit LARKSUITE_CLI_CONFIG_DIR, making results depend on the developer or CI machine’s existing configuration. Add t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to each test.
Proposed fix
func TestDocs_CreateTitleDryRunNormalizesXMLTitle(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
t.Setenv("LARKSUITE_CLI_APP_ID", "app")
t.Setenv("LARKSUITE_CLI_APP_SECRET", "secret")
@@
func TestDocs_DryRunRejectsUnsafeWriteInputs(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
t.Setenv("LARKSUITE_CLI_APP_ID", "app")
t.Setenv("LARKSUITE_CLI_APP_SECRET", "secret")As per coding guidelines, **/*_test.go tests must isolate configuration with LARKSUITE_CLI_CONFIG_DIR and t.TempDir().
Also applies to: 251-284
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli_e2e/docs/docs_update_dryrun_test.go` around lines 229 - 249,
Isolate configuration in both dry-run tests,
TestDocs_CreateTitleDryRunNormalizesXMLTitle and the adjacent test covering
lines 251-284, by setting LARKSUITE_CLI_CONFIG_DIR to a unique t.TempDir() value
alongside the existing fake credential environment setup. Ensure each test uses
its own temporary configuration directory.
Source: Coding guidelines
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
1 participant
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
Normalize or reject deterministic warning-producing Docs write inputs while preserving existing command semantics. The CLI keeps the established
--title-wins contract, removes redundant XML title elements locally, and rejects inputs that cannot be executed unambiguously.Changes
<title>elements when--titleis supplied, then prepend the escaped flag value so existing--title-wins behavior remains compatible without duplicate-title warnings.str_replacepatterns; safe idempotent replacements remain allowed and are handled by the existing service contract.block_deleterequests.Test Plan
go test ./shortcuts/docLARK_CLI_BIN=... go test ./tests/cli_e2e/docsmake fmt-checkgo vet ./...go mod tidyproduces no changesgolangci-lint ... --new-from-rev=origin/mainreports 0 issuesmake unit-testcompletes cleanly in this local environment; unrelated tests that create temporary Git repositories intermittently fail duringt.TempDircleanup with.git: directory not emptyRelated Issues
Summary by CodeRabbit
<title>elements from--contentare removed so the CLI--titletakes precedence.str_replacepatterns when--doc-format=xml.block_deletehandling by rejecting missing/empty and duplicate--block-idvalues, and normalizing whitespace in batch IDs.--title,--pattern, and--block-idbehavior in XML vs Markdown modes.