fix(docs): validate --selection-by-title format early#256
Open
wwenrr wants to merge 2 commits intolarksuite:mainfrom
Open
fix(docs): validate --selection-by-title format early#256wwenrr wants to merge 2 commits intolarksuite:mainfrom
wwenrr wants to merge 2 commits intolarksuite:mainfrom
Conversation
|
|
github-actions
bot
added
domain/ccm
📝 WalkthroughWalkthroughDocsUpdate now validates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
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
Author
|
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
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
2 participants
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
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