t1345: Fix markdown-formatter MCP tool — 100% error rate#2443
Merged
alex-solovyev merged 4 commits intomainfrom Feb 27, 2026
Merged
t1345: Fix markdown-formatter MCP tool — 100% error rate#2443alex-solovyev merged 4 commits intomainfrom
alex-solovyev merged 4 commits intomainfrom
Conversation
…turn value bug) Two root causes: (1) MCP tool exposed lint/check/fix actions but bash script only handled format/advanced/cleanup — unknown actions hit default case exit 1; (2) fix_markdown_file() returned $changes_made (1=success) instead of 0, causing set -e to abort. MCP wrapper now uses Bun.spawn() to capture output on non-zero exit instead of throwing. Closes #2440
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
WalkthroughFixes for the markdown-formatter MCP: scripts now support additional actions (format/fix/lint/check/advanced/cleanup), fix routines return success correctly, and the TypeScript wrapper uses Bun.spawn to capture stdout/stderr even on non-zero exits; docs and a task brief were added/updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TS as markdown-formatter.ts
participant SH as markdown-formatter.sh
participant FS as Filesystem
User->>TS: invoke formatter (action)
TS->>SH: spawn process (stdin/stdout/stderr piped)
SH->>FS: read/write markdown files (format/fix/lint/check/cleanup)
FS-->>SH: file contents / write ack
SH-->>TS: stdout/stderr streamed
SH-->>TS: exit code
TS-->>User: aggregated output (includes stderr and exit code if non-zero)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Fri Feb 27 03:00:44 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/scripts/markdown-lint-fix.sh (1)
70-87:⚠️ Potential issue | 🔴 CriticalGenerated
.markdownlint.jsonis invalid JSON.Line 85 writes
return 0inside the heredoc payload, which corrupts the config file.Proposed fix
{ "default": true, "MD013": { "line_length": 120, "code_blocks": false, "tables": false }, "MD033": { "allowed_elements": ["br", "sub", "sup"] }, "MD041": false, "MD046": { "style": "fenced" } - return 0 } EOFAs per coding guidelines
.agents/scripts/*.sh: Automation scripts should prioritize reliability and robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/markdown-lint-fix.sh around lines 70 - 87, The generated heredoc written to the config_file in markdown-lint-fix.sh contains a stray "return 0" inside the JSON payload which makes .markdownlint.json invalid; remove the "return 0" from the heredoc so only valid JSON is written, and ensure the script still returns success after the EOF by placing any return/exit or status checks outside the heredoc (e.g., after the cat >>"$config_file" <<'EOF' ... EOF block), keeping the JSON keys shown (default, MD013, MD033, MD041, MD046) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/markdown-formatter.sh:
- Around line 101-105: The script counts unchanged files as modified because
fix_markdown_file always returns 0; modify either the fix_markdown_file function
to return non-zero when no edits were made (or return 0 only when a change
occurred), or change the loop to detect actual changes (e.g., compare original
vs formatted content, check a generated "changed" flag or use a
diff/exit-status) and only increment changed_files when that indicator is true;
update references in the loop that use changed_files and the function name
fix_markdown_file accordingly so the summary reflects real modifications.
- Around line 189-196: In the advanced-fixes block in markdown-formatter.sh,
treat the "no changes needed" branch as success: replace the else branch's
"return 1" with "return 0" so that cmp -s no-op cases don't cause failures under
set -e, and remove the unreachable trailing "return 0" that follows this
conditional; keep the existing mv "$temp_file" "$file" and print_success
behavior unchanged so only the exit code is adjusted.
- Around line 263-266: The script currently dereferences "$1" into local _arg1
and then uses "${1:-...}" while running under set -u, causing an unbound
variable error when no args are passed; update the positional-arg handling in
the top-level argument parsing (variables _arg1, command, target) to guard
against unset parameters by using parameter expansions with defaults (e.g.,
${1:-} and ${2:-.}) or an explicit check on $# before reading "$1"/"$2" so the
script safely works under set -u and falls back to the intended defaults.
In @.agents/scripts/markdown-lint-fix.sh:
- Around line 123-137: The script calls markdownlint --fix twice in each branch
which can exit early under set -e; update the branches handling "$target" (the
file branch and the directory branch) to call markdownlint --fix only once and
capture its exit status, then use that status to choose between print_success
and print_warning; specifically remove the unconditional markdownlint --fix
"$target" and markdownlint --fix "$target/**/*.md" calls and instead run
markdownlint --fix once (for the file: markdownlint --fix "$target"; for the
dir: markdownlint --fix "$target"/*.md or the glob you prefer), store the result
($? or use if command; e.g., if markdownlint --fix ...; then ...) and then call
print_success or print_warning accordingly while keeping print_info calls around
the operation.
- Around line 239-242: In main(), avoid an unbound variable error under set -u
by giving $1 a safe default when assigning _arg1: change the declaration local
_arg1="$1" to use parameter expansion with a default (e.g. local _arg1="${1-}"
or local _arg1="${1:-}") so the script won't fail when no arguments are
supplied; leave existing command="${1:-fix}" and target="${2:-.}" as-is.
In `@todo/tasks/t1345-brief.md`:
- Around line 17-24: Update the "Root Cause (two bugs)" heading to correctly
reflect the enumerated items by changing it to "Root Cause (three bugs)" (or
remove the numeric count and use "Root Cause") so the heading matches the list
items that follow; ensure the heading text exactly matches the intention used in
the block that lists the three root causes.
---
Outside diff comments:
In @.agents/scripts/markdown-lint-fix.sh:
- Around line 70-87: The generated heredoc written to the config_file in
markdown-lint-fix.sh contains a stray "return 0" inside the JSON payload which
makes .markdownlint.json invalid; remove the "return 0" from the heredoc so only
valid JSON is written, and ensure the script still returns success after the EOF
by placing any return/exit or status checks outside the heredoc (e.g., after the
cat >>"$config_file" <<'EOF' ... EOF block), keeping the JSON keys shown
(default, MD013, MD033, MD041, MD046) intact.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.agents/prompts/build.txt.agents/scripts/markdown-formatter.sh.agents/scripts/markdown-lint-fix.sh.opencode/tool/markdown-formatter.tstodo/tasks/t1345-brief.md
- Remove stray 'return 0' from heredoc corrupting .markdownlint.json
- Fix fix_markdown_file return value: use hash comparison to detect actual changes
- Fix 'return 1' in advanced-fixes no-op branch to 'return 0' (set -e safe)
- Guard $1 with ${1:-} in both scripts to prevent set -u unbound variable error
- Remove duplicate markdownlint --fix calls in file/directory branches
- Update t1345-brief.md heading to 'three bugs' matching enumerated list
…branches Three functions had dead 'return 0' after if/else blocks that already return in every branch: check_markdownlint(), install_markdownlint() in markdown-lint-fix.sh, and process_directory() in markdown-formatter.sh. Addresses remaining CodeRabbit dead-code findings on PR #2443.
Owner
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced Feb 27, 2026
…rmatter diagnosis
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Fri Feb 27 05:03:12 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.agents/scripts/markdown-formatter.sh (1)
338-340: Apply null-delimited traversal to both markdown processing scripts for robustness.The loop at line 338 uses line-delimited piping which is vulnerable to filenames with special characters. Switch to
-print0+read -r -d ''for consistency and maximum filename robustness across both.agents/scripts/markdown-formatter.sh:338and.agents/scripts/markdown-lint-fix.sh:250where this pattern appears.Suggested patch
- find "$target" -name "*.md" -type f | while read -r file; do + while IFS= read -r -d '' file; do apply_advanced_fixes "$file" - done + done < <(find "$target" -name "*.md" -type f -print0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/markdown-formatter.sh around lines 338 - 340, The find | while read loop that calls apply_advanced_fixes "$file" is fragile for filenames with spaces/newlines; replace the line-delimited pipeline with a null-delimited pattern using find ... -print0 and a while IFS= read -r -d '' file; do ... done loop (or feed the -print0 output into the while loop via process substitution) to robustly handle arbitrary filenames; apply the same change to the matching loop in the markdown-lint-fix script where the same find | while read pattern appears..agents/scripts/markdown-lint-fix.sh (1)
250-252: Harden directory iteration for edge-case filenames.Use null-delimited traversal (
-print0with process substitution) here to match the established pattern used throughout the codebase and handle filenames with special characters safely. This same unsafe pattern also appears inmarkdown-formatter.sh:338and should be refactored together.Suggested patch
- find "$target" -name "*.md" -type f | while read -r file; do + while IFS= read -r -d '' file; do apply_manual_fixes "$file" - done + done < <(find "$target" -name "*.md" -type f -print0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/markdown-lint-fix.sh around lines 250 - 252, The loop that iterates Markdown files uses plain newline-delimited find output which breaks on filenames with spaces/newlines; change the traversal to use null-delimited output from find (print0) and read with a null delimiter (e.g., IFS= and read -r -d '') via process substitution so apply_manual_fixes("$file") receives safe filenames; make the same change in markdown-formatter.sh where the same pattern appears. Ensure you keep using the existing target variable and the apply_manual_fixes function name so behavior is unchanged aside from robust filename handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/scripts/markdown-formatter.sh:
- Around line 338-340: The find | while read loop that calls
apply_advanced_fixes "$file" is fragile for filenames with spaces/newlines;
replace the line-delimited pipeline with a null-delimited pattern using find ...
-print0 and a while IFS= read -r -d '' file; do ... done loop (or feed the
-print0 output into the while loop via process substitution) to robustly handle
arbitrary filenames; apply the same change to the matching loop in the
markdown-lint-fix script where the same find | while read pattern appears.
In @.agents/scripts/markdown-lint-fix.sh:
- Around line 250-252: The loop that iterates Markdown files uses plain
newline-delimited find output which breaks on filenames with spaces/newlines;
change the traversal to use null-delimited output from find (print0) and read
with a null delimiter (e.g., IFS= and read -r -d '') via process substitution so
apply_manual_fixes("$file") receives safe filenames; make the same change in
markdown-formatter.sh where the same pattern appears. Ensure you keep using the
existing target variable and the apply_manual_fixes function name so behavior is
unchanged aside from robust filename handling.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.agents/prompts/build.txt.agents/scripts/markdown-formatter.sh.agents/scripts/markdown-lint-fix.shtodo/tasks/t1345-brief.md
✅ Files skipped from review due to trivial changes (1)
- todo/tasks/t1345-brief.md
Owner
|
Also resolves #2458 (t1347: markdown-formatter MCP tool has 81% error rate). |
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
lint,check,fixbut bash script only handledformat,advanced,cleanup— unknown actions returned exit 1fix_markdown_file()returned$changes_made(1 when successful) instead of 0, causingset -eto abort even on successChanges
.agents/scripts/markdown-formatter.shlint/check(dry-run mode) andfix(alias for format) actions; fixed return value bug.agents/scripts/markdown-lint-fix.shapply_manual_fixes().opencode/tool/markdown-formatter.tsBun.$(throws on non-zero) withBun.spawn()(captures output gracefully).agents/prompts/build.txttodo/tasks/t1345-brief.mdVerification
format,fix,lint,check,help) return exit 0lint/checkmode correctly reports issues without modifying filesCloses #2440
Summary by CodeRabbit
New Features
Bug Fixes
Documentation