Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions shortcuts/doc/doc_errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,26 @@ func TestValidateUpdateV2Contract(t *testing.T) {
str: map[string]string{"doc": testDocxToken, "command": "str_replace"},
wantParam: "--pattern",
},
{
name: "XML str_replace rejects multiline pattern",
str: map[string]string{"doc": testDocxToken, "command": "str_replace", "doc-format": "xml", "pattern": "line one\nline two", "content": "replacement"},
wantParam: "--pattern",
},
{
name: "block_delete without block id",
str: map[string]string{"doc": testDocxToken, "command": "block_delete"},
wantParam: "--block-id",
},
{
name: "block_delete rejects empty ID",
str: map[string]string{"doc": testDocxToken, "command": "block_delete", "block-id": "blkA,,blkB"},
wantParam: "--block-id",
},
{
name: "block_delete rejects duplicate ID",
str: map[string]string{"doc": testDocxToken, "command": "block_delete", "block-id": "blkA, blkA"},
wantParam: "--block-id",
},
{
name: "block_insert_after without block id",
str: map[string]string{"doc": testDocxToken, "command": "block_insert_after"},
Expand Down
40 changes: 40 additions & 0 deletions shortcuts/doc/docs_create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,46 @@ import (

// ── V2 (OpenAPI) tests ──

func TestStripTopLevelXMLTitles(t *testing.T) {
t.Parallel()

tests := []struct {
name string
content string
want string
}{
{
name: "single title",
content: "<title>Content title</title><p>body</p>",
want: "<p>body</p>",
},
{
name: "multiple titles",
content: "<title>First</title>\n<p>body</p>\n<title>Second</title>",
want: "<p>body</p>",
},
{
name: "nested title is preserved",
content: "<callout><title>Nested</title></callout><p>body</p>",
want: "<callout><title>Nested</title></callout><p>body</p>",
},
{
name: "malformed XML is preserved",
content: "<title>Content title</title><p>A & B</p>",
want: "<title>Content title</title><p>A & B</p>",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
if got := stripTopLevelXMLTitles(tt.content); got != tt.want {
t.Fatalf("stripTopLevelXMLTitles() = %q, want %q", got, tt.want)
}
})
}
}

func TestDocsCreateV2BotAutoGrantSuccess(t *testing.T) {
t.Parallel()

Expand Down
63 changes: 62 additions & 1 deletion shortcuts/doc/docs_create_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"bytes"
"context"
"encoding/xml"
"errors"
"io"
"strings"

"github.com/larksuite/cli/errs"
Expand All @@ -16,7 +18,7 @@ import (
// v2CreateFlags returns the flag definitions for the v2 (OpenAPI) create path.
func v2CreateFlags() []common.Flag {
return []common.Flag{
{Name: "title", Desc: "document title; when provided, the CLI prepends it to --content as <title>...</title> so the title wins over later content titles"},
{Name: "title", Desc: "document title; the CLI prepends it to --content as <title>...</title>. In XML mode, top-level <title> elements in --content are removed so this flag wins without duplicate-title warnings"},
{Name: "content", Desc: "document body; XML by default or Markdown when --doc-format markdown. " + docsContentSkillHelp + "; use --help for the latest command flags", Input: []string{common.File, common.Stdin}},
{Name: "reference-map", Desc: docsReferenceMapFlagDesc, Input: []string{common.File, common.Stdin}},
{Name: "doc-format", Desc: "content format; xml is default and supports richer DocxXML blocks, markdown imports plain Markdown", Default: "xml", Enum: []string{"xml", "markdown"}},
Expand Down Expand Up @@ -108,6 +110,9 @@ func buildCreateContentWithBody(runtime *common.RuntimeContext, content string)
if title == "" {
return content
}
if runtime.Str("doc-format") == "xml" {
content = stripTopLevelXMLTitles(content)
}

titleTag := "<title>" + escapeDocTitleText(title) + "</title>"
if content == "" {
Expand All @@ -116,6 +121,62 @@ func buildCreateContentWithBody(runtime *common.RuntimeContext, content string)
return titleTag + "\n" + content
}

type docContentRange struct {
start int64
end int64
}

// stripTopLevelXMLTitles preserves the established --title-wins contract while
// avoiding duplicate-title warnings from XML content. If the fragment is not
// well-formed XML, it is left untouched for the service to diagnose.
func stripTopLevelXMLTitles(content string) string {
const wrapperStart = "<root>"
wrapped := wrapperStart + content + "</root>"
decoder := xml.NewDecoder(strings.NewReader(wrapped))
wrapperLen := int64(len(wrapperStart))
depth := 0
activeStart := int64(-1)
ranges := make([]docContentRange, 0, 1)

for {
tokenStart := decoder.InputOffset()
token, err := decoder.Token()
if errors.Is(err, io.EOF) {
break
}
if err != nil {
return content
}

switch value := token.(type) {
case xml.StartElement:
if depth == 1 && value.Name.Space == "" && value.Name.Local == "title" {
activeStart = tokenStart - wrapperLen
}
depth++
case xml.EndElement:
depth--
if activeStart >= 0 && depth == 1 && value.Name.Space == "" && value.Name.Local == "title" {
ranges = append(ranges, docContentRange{start: activeStart, end: decoder.InputOffset() - wrapperLen})
activeStart = -1
}
}
}

if len(ranges) == 0 {
return content
}

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())
Comment on lines +170 to +177

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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 without strings.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

}

func escapeDocTitleText(title string) string {
var buf bytes.Buffer
_ = xml.EscapeText(&buf, []byte(title))
Expand Down
36 changes: 34 additions & 2 deletions shortcuts/doc/docs_update_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ func v2UpdateFlags() []common.Flag {
{Name: "doc-format", Desc: "content format for --content; xml is default for precise rich edits, markdown for user-provided Markdown or plain append/overwrite", Default: "xml", Enum: []string{"xml", "markdown"}},
{Name: "content", Desc: "replacement or inserted content; XML by default or Markdown when --doc-format markdown; empty with str_replace deletes match. " + docsContentSkillHelp + "; use --help for the latest command flags", Input: []string{common.File, common.Stdin}},
{Name: "reference-map", Desc: docsUpdateReferenceMapFlagDesc, Input: []string{common.File, common.Stdin}},
{Name: "pattern", Desc: "str_replace match pattern; XML mode is inline text, Markdown mode can match multiline text"},
{Name: "block-id", Desc: "target block ID(s) for block operations (comma-separated for batch delete); -1 means document end where supported"},
{Name: "pattern", Desc: "str_replace match pattern; XML mode accepts inline text only, Markdown mode can match multiline text"},
{Name: "block-id", Desc: "target block ID(s) for block operations (comma-separated unique IDs for batch delete); -1 means document end where supported"},
{Name: "src-block-ids", Desc: "comma-separated source block ids for block_copy_insert_after and block_move_after"},
{Name: "revision-id", Desc: "base revision id; -1 means latest", Type: "int", Default: "-1"},
}
Expand Down Expand Up @@ -73,10 +73,16 @@ func validateUpdateV2(_ context.Context, runtime *common.RuntimeContext) error {
if pattern == "" {
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--command str_replace requires --pattern").WithParam("--pattern")
}
if runtime.Str("doc-format") == "xml" && strings.ContainsAny(pattern, "\r\n") {
return errs.NewValidationError(errs.SubtypeInvalidArgument, "XML str_replace --pattern must be inline and cannot contain line breaks; use --doc-format markdown or a block operation for multiline changes").WithParam("--pattern")
}
case "block_delete":
if blockID == "" {
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--command block_delete requires --block-id").WithParam("--block-id")
}
if err := validateBlockDeleteIDs(blockID); err != nil {
return err
}
case "block_insert_after":
if blockID == "" {
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--command block_insert_after requires --block-id").WithParam("--block-id")
Expand Down Expand Up @@ -124,6 +130,29 @@ func validateUpdateV2(_ context.Context, runtime *common.RuntimeContext) error {
return nil
}

func validateBlockDeleteIDs(raw string) error {
seen := make(map[string]struct{})
for _, part := range strings.Split(raw, ",") {
blockID := strings.TrimSpace(part)
if blockID == "" {
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--block-id contains an empty ID; provide a comma-separated list of non-empty block IDs").WithParam("--block-id")
}
if _, ok := seen[blockID]; ok {
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--block-id contains duplicate ID %q; each block may be deleted only once per request", blockID).WithParam("--block-id")
}
seen[blockID] = struct{}{}
}
return nil
}

func normalizeBlockDeleteIDs(raw string) string {
parts := strings.Split(raw, ",")
for i := range parts {
parts[i] = strings.TrimSpace(parts[i])
}
return strings.Join(parts, ",")
}

func dryRunUpdateV2(_ context.Context, runtime *common.RuntimeContext) *common.DryRunAPI {
// Validate has already accepted --doc; parseDocumentRef cannot fail here.
ref, _ := parseDocumentRef(runtime.Str("doc"))
Expand Down Expand Up @@ -199,6 +228,9 @@ func buildUpdateBodyBase(runtime *common.RuntimeContext) map[string]interface{}
body["pattern"] = v
}
if blockID != "" {
if cmd == "block_delete" {
blockID = normalizeBlockDeleteIDs(blockID)
}
body["block_id"] = blockID
}
if v := runtime.Str("src-block-ids"); v != "" {
Expand Down
60 changes: 59 additions & 1 deletion tests/cli_e2e/docs/docs_update_dryrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,11 @@ func TestDocs_DryRunDefaultsToV2OpenAPI(t *testing.T) {
"docs", "+update",
"--doc", "doxcnDryRunE2E",
"--command", "block_delete",
"--block-id", "blkA,blkB,blkC",
"--block-id", "blkA, blkB, blkC",
"--dry-run",
},
wantContains: []string{"/open-apis/docs_ai/v1/documents/doxcnDryRunE2E"},
wantBody: map[string]any{"block_id": "blkA,blkB,blkC"},
},
{
name: "history list",
Expand Down Expand Up @@ -225,3 +226,60 @@ func TestDocs_CreateTitleDryRunPrependsContent(t *testing.T) {
require.Equal(t, "markdown", clie2e.DryRunGet(out, "api.0.body.format").String(), "stdout:\n%s", out)
require.Equal(t, "<title>Dry Run &amp; Title</title>\n## Body", clie2e.DryRunGet(out, "api.0.body.content").String(), "stdout:\n%s", out)
}

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())
}
Comment on lines +229 to +249

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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


func TestDocs_DryRunRejectsUnsafeWriteInputs(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)

tests := []struct {
name string
args []string
want string
}{
{
name: "multiline XML str_replace",
args: []string{"docs", "+update", "--doc", "doxcnDryRunE2E", "--command", "str_replace", "--pattern", "line one\nline two", "--content", "replacement", "--dry-run"},
want: "must be inline",
},
{
name: "duplicate block delete ID",
args: []string{"docs", "+update", "--doc", "doxcnDryRunE2E", "--command", "block_delete", "--block-id", "blkA,blkA", "--dry-run"},
want: "duplicate ID",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result, err := clie2e.RunCmd(ctx, clie2e.Request{Args: tt.args, DefaultAs: "bot"})
require.NoError(t, err)
result.AssertExitCode(t, 2)
require.Contains(t, result.Stdout+"\n"+result.Stderr, tt.want)
})
}
}
Loading