fix(docs): validate --selection-by-title format early#256
Conversation
|
|
github-actions
Bot
added
domain/ccm
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 |
Greptile SummaryThis PR adds early client-side validation for the Key changes:
Confidence Score: 5/5Safe to merge — logic is correct, tests are comprehensive, and both prior review concerns have been resolved. No P0 or P1 findings remain. The two issues flagged in previous review threads (multi-line check ordering and the missing multi-line-heading test case) are both addressed in the current HEAD. The validation logic is simple and well-covered by tests. No files require special attention. Important Files Changed
B`), and multi-line plain-text cases; the previously-missing multi-line heading test is now present. |Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["validateSelectionByTitle(title)"] --> B{title == ""}
B -- yes --> C[return nil ✓]
B -- no --> D["trimmed = TrimSpace(title)"]
D --> E{"contains \n or \r?"}
E -- yes --> F["error: must be a single heading line"]
E -- no --> G{"HasPrefix(trimmed, '#')"}
G -- yes --> H[return nil ✓]
G -- no --> I["error: must include markdown heading prefix '#'"]
Reviews (2): Last reviewed commit: "fix(docs): reject multiline selection-by..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/docs_update.go`:
- Around line 168-173: The current logic returns early for any trimmed string
starting with "#" (trimmed variable) before checking for newlines, allowing
multiline headings like "## Section\nNext" to pass; change the order or add a
newline check before the prefix acceptance so strings containing "\n" or "\r"
trigger common.FlagErrorf("--selection-by-title must be a single heading line
(for example: '## Section')") even if they start with "#". Ensure the multiline
guard (strings.Contains(trimmed, "\n") || strings.Contains(trimmed, "\r")) runs
prior to the if strings.HasPrefix(trimmed, "#") return nil branch.
🪄 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: 93550292-bb6d-47a0-9c95-a404218a42e1
📒 Files selected for processing (2)
shortcuts/doc/docs_update.goshortcuts/doc/docs_update_test.go
|
Addressed review feedback ✅ I fixed the validation-order bug in
Added test coverage:
Verification: docker run --rm -v /tmp/larksuite-cli:/src -w /src golang:1.24 bash -lc "export PATH=/usr/local/go/bin:$PATH; go test ./shortcuts/doc -run 'TestValidateSelectionByTitle|TestIsWhiteboardCreateMarkdown|TestNormalizeDocsUpdateResult'"Result: pass ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/doc/docs_update_test.go (1)
88-92: Consider adding edge case tests for robustness.The current tests cover the main scenarios well. For additional confidence, consider adding tests for:
- Single
#heading:"# Title"- Leading/trailing whitespace:
" ## Section "- Windows line endings:
"## A\r\n## B"📝 Additional test cases
t.Run("heading style title passes", func(t *testing.T) { if err := validateSelectionByTitle("## 第二章"); err != nil { t.Fatalf("expected nil error, got %v", err) } }) + + t.Run("single # heading passes", func(t *testing.T) { + if err := validateSelectionByTitle("# Title"); err != nil { + t.Fatalf("expected nil error, got %v", err) + } + }) + + t.Run("heading with surrounding whitespace passes", func(t *testing.T) { + if err := validateSelectionByTitle(" ## Section "); err != nil { + t.Fatalf("expected nil error, got %v", err) + } + }) + + t.Run("windows line endings fail", func(t *testing.T) { + err := validateSelectionByTitle("## A\r\n## B") + if err == nil { + t.Fatalf("expected validation error") + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_update_test.go` around lines 88 - 92, Add edge-case table-driven tests for validateSelectionByTitle: include t.Run cases for single-level heading "# Title", headings with leading/trailing whitespace like " ## Section ", and Windows CRLF input "## A\r\n## B" to ensure trimming and CRLF handling; locate the existing test block that contains t.Run("heading style title passes", func(t *testing.T) { ... }) and add these additional t.Run subtests asserting nil error (or expected behavior) for each input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/doc/docs_update_test.go`:
- Around line 88-92: Add edge-case table-driven tests for
validateSelectionByTitle: include t.Run cases for single-level heading "#
Title", headings with leading/trailing whitespace like " ## Section ", and
Windows CRLF input "## A\r\n## B" to ensure trimming and CRLF handling; locate
the existing test block that contains t.Run("heading style title passes", func(t
*testing.T) { ... }) and add these additional t.Run subtests asserting nil error
(or expected behavior) for each input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09644839-3178-4aa3-9e60-4717982abd83
📒 Files selected for processing (2)
shortcuts/doc/docs_update.goshortcuts/doc/docs_update_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@66d3688ab6622953e972f44bacb012ccd956a577🧩 Skill updatenpx skills add wwenrr/cli#fix/issue-93-selection-by-title-validation -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
=======================================
Coverage ? 61.05%
=======================================
Files ? 400
Lines ? 34130
Branches ? 0
=======================================
Hits ? 20838
Misses ? 11368
Partials ? 1924 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fangshuyu-768
force-pushed
the
fix/issue-93-selection-by-title-validation
branch
from
e206d9f to
4bfe54c
Compare
codecov/patch was at 27.27% because the PR added three lines to the
Validate closure (the `if err := validateSelectionByTitle(selTitle); err
!= nil { return err }` block) but nothing in the test file exercised
that closure — only the helper function was tested directly.
TestDocsUpdateValidate now builds a bare RuntimeContext via
common.TestNewRuntimeContext, sets the relevant flags on a cobra
command, and calls DocsUpdate.Validate(ctx, rt) across five cases:
1. Heading-style selection-by-title passes — covers the happy path
through the new call site and the final `return nil`.
2. Plain-text title is rejected with heading-prefix guidance —
covers the new error branch.
3. Multi-line title is rejected as not a single heading line —
covers the other error branch inside the helper.
4. Invalid --mode is still rejected first — proves the new check
doesn't swallow pre-existing validation.
5. Conflicting --selection-with-ellipsis + --selection-by-title is
rejected at the mutual-exclusion check — same ordering contract.
Coverage profile confirms the three added production lines
(docs_update.go L65-67) are now hit: condition 3x, error branch 2x,
happy path via the closure's return nil 1x.
fangshuyu-768
force-pushed
the
fix/issue-93-selection-by-title-validation
branch
from
4bfe54c to
66d3688
Compare
Summary
docs +update --selection-by-titleso invalid plain-text inputs are rejected before sending MCP requests## Section) for--selection-by-titleWhy
Issue #93 reports that users pass plain titles (like
第二章) and receive opaque MCP validation failures. This change fails fast in CLI with actionable guidance, reducing trial-and-error and noisy server calls.Thinking path
Changes
shortcuts/doc/docs_update.govalidateSelectionByTitleduring flag validation#,##, etc.) => passshortcuts/doc/docs_update_test.goTestValidateSelectionByTitlefor pass/fail casesVerification
Executed in Docker (host has no Go toolchain):
docker run --rm -v /tmp/larksuite-cli:/src -w /src golang:1.24 bash -lc "export PATH=/usr/local/go/bin:$PATH; go test ./shortcuts/doc -run 'TestValidateSelectionByTitle|TestIsWhiteboardCreateMarkdown|TestNormalizeDocsUpdateResult'"Result:
ok github.com/larksuite/cli/shortcuts/docAlso ran broad suite check:
docker run --rm -v /tmp/larksuite-cli:/src -w /src golang:1.24 bash -lc "export PATH=/usr/local/go/bin:$PATH; go test ./..."Observed unrelated pre-existing failures in
internal/registryand e2e integration tests requiring external binaries/env; changed package tests pass.Closes #93
Summary by CodeRabbit
Bug Fixes
Tests