Skip to content
Open
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
12 changes: 12 additions & 0 deletions shortcuts/doc/docs_create_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import (
"context"
"fmt"
"strings"

"github.com/larksuite/cli/shortcuts/common"
Expand All @@ -27,6 +28,11 @@
if runtime.Str("parent-token") != "" && runtime.Str("parent-position") != "" {
return common.FlagErrorf("--parent-token and --parent-position are mutually exclusive")
}
if runtime.Str("doc-format") != "markdown" && runtime.Str("content") != "" {
if msg := CheckV2XMLBareAmpersand(runtime.Str("content")); msg != "" {
return common.FlagErrorf("%s", msg)

Check warning on line 33 in shortcuts/doc/docs_create_v2.go

View check run for this annotation

Codecov / codecov/patch

shortcuts/doc/docs_create_v2.go#L33

Added line #L33 was not covered by tests
}
}
return nil
}

Expand All @@ -45,6 +51,12 @@
func executeCreateV2(_ context.Context, runtime *common.RuntimeContext) error {
body := buildCreateBody(runtime)

if runtime.Str("doc-format") != "markdown" {
for _, w := range CheckV2XMLWarnings(runtime.Str("content")) {
fmt.Fprintf(runtime.IO().ErrOut, "warning: %s\n", w)

Check warning on line 56 in shortcuts/doc/docs_create_v2.go

View check run for this annotation

Codecov / codecov/patch

shortcuts/doc/docs_create_v2.go#L56

Added line #L56 was not covered by tests
}
}

data, err := doDocAPI(runtime, "POST", "/open-apis/docs_ai/v1/documents", body)
if err != nil {
return err
Expand Down
75 changes: 75 additions & 0 deletions shortcuts/doc/docs_update_check.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,3 +279,78 @@ func leadingRun(s string, c byte) string {
}
return s[:i]
}

// ── v2 XML content guards ──────────────────────────────────────────────────

// xmlEntityRe matches a valid XML entity reference: & < > '
// " &#N; or &#xH;. Used to skip over valid references when scanning for
// bare ampersands.
var xmlEntityRe = regexp.MustCompile(`&(amp|lt|gt|apos|quot|#\d+|#x[0-9a-fA-F]+);`)

// CheckV2XMLBareAmpersand returns a non-empty error message when content
// contains a bare & that would cause the v2 XML parser to reject the request.
// Only runs when --doc-format xml (the default). Callers in Validate should
// return this as a hard error.
//
// Go's regexp package does not support lookahead, so we detect bare ampersands
// by replacing all valid entity references with a placeholder and then
// checking whether any & remains.
func CheckV2XMLBareAmpersand(content string) string {
if content == "" || !strings.Contains(content, "&") {
return ""
}
// Replace every valid entity reference with a fixed placeholder so that
// the subsequent Contains check only fires on truly bare ampersands.
// ("ENTITY" is not the same length as each entity; byte offsets are not
// preserved, but that is fine — we only need a yes/no bare-& answer.)
stripped := xmlEntityRe.ReplaceAllString(content, "ENTITY")
if !strings.Contains(stripped, "&") {
return ""
}
return "content contains a bare & character that is not a valid XML entity reference; " +
"the v2 XML parser will reject the request. " +
"Escape it as &amp; (and < as &lt;, > as &gt; where needed)."
}

// quoteContainerTagRe matches the opening of a <quote-container> element
// (tag name followed by whitespace, >, or />) to avoid false positives on
// hypothetical attributes or element names that start with "quote-container".
var quoteContainerTagRe = regexp.MustCompile(`<quote-container(?:\s|>|/)`)

// columnIntWidthRe matches a <column … width="N" …> attribute where N is a
// plain integer (not a float). The pattern requires:
// - whitespace before "width" to exclude attributes like data-width
// - the value to be enclosed in quotes with digits immediately before the
// closing quote, so width="0.5" does NOT match (the dot prevents \d+
// from consuming the full value up to the quote).
var columnIntWidthRe = regexp.MustCompile(`<column\b[^>]*\swidth\s*=\s*["']\d+["']`)

Comment thread
coderabbitai[bot] marked this conversation as resolved.
// CheckV2XMLWarnings returns a list of non-fatal warnings for v2 XML content.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

columnIntWidthRe produces false positives on float values.

The regex <column\b[^>]*\swidth\s*=\s*['"]?\d+['"]? will match width="0.5" because \d+ greedily matches the 0 before the dot, producing a spurious warning for a valid float value.

Suggested fix: use FindStringSubmatch to capture the attribute value and then check in Go code whether it's a pure integer (no decimal point), e.g.:

var columnWidthRe = regexp.MustCompile(`<column\b[^>]*\swidth\s*=\s*(['"])([^'"]+)\1`)

// then in CheckV2XMLWarnings:
for _, m := range columnWidthRe.FindAllStringSubmatch(content, -1) {
    val := m[2]
    if _, err := strconv.Atoi(val); err == nil {
        // pure integer → warn
    }
}

Alternatively, if the regex approach is preferred, add a negative lookahead equivalent by excluding values containing a dot: \d+(?!\.\d) — but since Go's RE2 doesn't support lookahead, the code-based check is the way to go.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 259bc0e. Used ["']\d+["'] — requiring the value to be wrapped in quotes with digits immediately before the closing quote, so width="0.5" doesn't match (the dot prevents \d+ from consuming the full value up to the quote). Avoids backreferences (not supported in Go RE2). Added a test case covering width="0.5" → wantLen: 0.

// These describe constructs that are silently dropped or ignored by the v2 API
// but do not cause the request to fail. Callers should print these to stderr
// before executing the API call.
//
// Warnings emitted:
//
// 1. <quote-container> is not recognised by the v2 XML parser; the block is
// silently dropped. Use <blockquote> instead.
//
// 2. <column width="N"> with an integer value has no effect in v2. The
// correct attribute is width-ratio="0.N" (e.g. width-ratio="0.5").
func CheckV2XMLWarnings(content string) []string {
if content == "" {
return nil
}
var warnings []string
if quoteContainerTagRe.MatchString(content) {
warnings = append(warnings,
"<quote-container> is not supported in v2 XML and will be silently dropped; "+
"use <blockquote> instead.")
}
if columnIntWidthRe.MatchString(content) {
warnings = append(warnings,
"<column width=\"N\"> with an integer value has no effect in v2 XML; "+
"use width-ratio=\"0.5\" (float 0–1) to set column width.")
}
return warnings
}
118 changes: 118 additions & 0 deletions shortcuts/doc/docs_update_check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -373,3 +373,121 @@ func TestDocsUpdateWarningsEmpty(t *testing.T) {
t.Fatalf("expected no warnings, got: %v", warnings)
}
}

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

tests := []struct {
name string
content string
wantErr bool
}{
{name: "empty is fine", content: "", wantErr: false},
{name: "no ampersand", content: "<text>hello world</text>", wantErr: false},
{name: "amp entity is fine", content: "<text>a &amp; b</text>", wantErr: false},
{name: "lt entity is fine", content: "&lt;tag&gt;", wantErr: false},
{name: "gt entity is fine", content: "a &gt; b", wantErr: false},
{name: "apos entity is fine", content: "&apos;", wantErr: false},
{name: "quot entity is fine", content: "&quot;", wantErr: false},
{name: "decimal numeric ref is fine", content: "&#65;", wantErr: false},
{name: "hex numeric ref is fine", content: "&#x41;", wantErr: false},
{name: "bare ampersand flagged", content: "a & b", wantErr: true},
{name: "bare ampersand in tag flagged", content: `<text color="blue">R&D</text>`, wantErr: true},
{name: "unknown entity flagged", content: "&nbsp;", wantErr: true},
// mixed: valid entity alongside a bare & — the bare one must still be caught
{name: "valid entity mixed with bare ampersand flagged", content: "a &amp; b & c", wantErr: true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
got := CheckV2XMLBareAmpersand(tt.content)
if (got != "") != tt.wantErr {
t.Fatalf("CheckV2XMLBareAmpersand(%q) = %q, wantErr=%v", tt.content, got, tt.wantErr)
}
})
}
}

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

tests := []struct {
name string
content string
wantContains []string
wantLen int
}{
{name: "empty returns nil", content: "", wantLen: 0},
{name: "clean XML no warnings", content: "<blockquote><p>text</p></blockquote>", wantLen: 0},
{
name: "quote-container triggers warning",
content: `<quote-container><p>text</p></quote-container>`,
wantContains: []string{"quote-container", "blockquote"},
wantLen: 1,
},
{
name: "column integer width triggers warning",
content: `<grid><column width="50"><p>A</p></column></grid>`,
wantContains: []string{"width-ratio"},
wantLen: 1,
},
{
name: "column float width-ratio is fine",
content: `<grid><column width-ratio="0.5"><p>A</p></column></grid>`,
wantLen: 0,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Missing test case: mixed valid entities and bare ampersand.

There's no test for content that contains both valid entities and a bare &, e.g. "a &amp; b & c". This is a common real-world scenario and would help verify that the replace-then-check approach works correctly when valid and invalid references coexist.

Suggested addition:

{name: "mixed valid entity and bare ampersand flagged", content: "a &amp; b & c", wantErr: true},

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 259bc0e. Added test case {name: "valid entity mixed with bare ampersand flagged", content: "a &amp; b & c", wantErr: true}.

},
{
name: "both issues produce two warnings",
content: `<quote-container/><grid><column width="30"/></grid>`,
wantLen: 2,
},
// false-positive guards: names that start with "quote-container" but aren't the tag
{
name: "quote-containerized attribute prefix is not flagged",
content: `<block quote-containerized="true"/>`,
wantLen: 0,
},
// false-positive guard: data-width should not trigger column warning
{
name: "data-width attribute is not flagged",
content: `<column data-width="50"/>`,
wantLen: 0,
},
// single-quoted width should be caught
{
name: "column single-quoted integer width triggers warning",
content: `<grid><column width='30'/></grid>`,
wantLen: 1,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Missing test case: width with float value should NOT trigger warning.

This is the flip side of the columnIntWidthRe false-positive issue. Adding a test for <column width="0.5"> would both document the intended behavior and catch the regex bug (it currently produces a false positive because \d+ matches the 0 in 0.5).

Suggested addition:

{
    name:    "column float width value is fine",
    content: `<grid><column width="0.5"><p>A</p></column></grid>`,
    wantLen: 0,
},

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 259bc0e. Added test case {name: "column float width value is not flagged", content: ..., wantLen: 0}.

},
// width with spaces around = should be caught
{
name: "column width with spaces around equals triggers warning",
content: `<grid><column width = "40"/></grid>`,
wantLen: 1,
},
// float value must NOT trigger warning — width="0.5" is valid width-ratio syntax
{
name: "column float width value is not flagged",
content: `<grid><column width="0.5"><p>A</p></column></grid>`,
wantLen: 0,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
got := CheckV2XMLWarnings(tt.content)
if len(got) != tt.wantLen {
t.Fatalf("CheckV2XMLWarnings(%q) returned %d warnings, want %d: %v", tt.content, len(got), tt.wantLen, got)
}
combined := ""
for _, w := range got {
combined += w
}
for _, sub := range tt.wantContains {
if !strings.Contains(combined, sub) {
t.Errorf("expected warning to contain %q, got: %s", sub, combined)
}
}
})
}
}
11 changes: 11 additions & 0 deletions shortcuts/doc/docs_update_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@
return common.FlagErrorf("--command append requires --content")
}
}
if runtime.Str("doc-format") != "markdown" && content != "" {
if msg := CheckV2XMLBareAmpersand(content); msg != "" {
return common.FlagErrorf("%s", msg)

Check warning on line 108 in shortcuts/doc/docs_update_v2.go

View check run for this annotation

Codecov / codecov/patch

shortcuts/doc/docs_update_v2.go#L106-L108

Added lines #L106 - L108 were not covered by tests
}
}
return nil
}

Expand All @@ -124,6 +129,12 @@
apiPath := fmt.Sprintf("/open-apis/docs_ai/v1/documents/%s", ref.Token)
body := buildUpdateBody(runtime)

if runtime.Str("doc-format") != "markdown" {
for _, w := range CheckV2XMLWarnings(runtime.Str("content")) {
fmt.Fprintf(runtime.IO().ErrOut, "warning: %s\n", w)

Check warning on line 134 in shortcuts/doc/docs_update_v2.go

View check run for this annotation

Codecov / codecov/patch

shortcuts/doc/docs_update_v2.go#L132-L134

Added lines #L132 - L134 were not covered by tests
}
}

data, err := doDocAPI(runtime, "PUT", apiPath, body)
if err != nil {
return err
Expand Down
Loading