MM-68425 Harden markdown bracket parsing#26
Conversation
|
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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughInline link parsing in ChangesInline parsing and test update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/index.js (2)
610-611: 💤 Low valueThe
argvparameter is unused.The
mainfunction signature acceptsargvbut line 611 callsparseArg()with no arguments.parseArginternally usesprocess.argv.slice(2)regardless of what's passed.This makes the function signature misleading and prevents testing
mainwith custom arguments.♻️ Either remove the unused parameter or wire it through
Option A: Remove unused parameter
-function main(argv, callback) { +function main(callback) { var opt = parseArg();Option B: Pass argv to parseArg (requires updating parseArg too)
function main(argv, callback) { - var opt = parseArg(); + var opt = parseArg(argv);And update
parseArg:function parseArg(argv) { - var argv = process.argv.slice(2) + argv = argv || process.argv.slice(2);🤖 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 `@test/index.js` around lines 610 - 611, The main function currently declares an unused argv parameter; either remove argv from main's signature to avoid the misleading parameter, or wire it through by changing main to pass argv into parseArg and then update parseArg to accept an argv parameter and use that (falling back to process.argv.slice(2) when argv is undefined) so tests can supply custom args; locate the functions main and parseArg in the file and apply one of these two changes consistently.
199-214: 💤 Low valueCallback invocation has inconsistent argument count.
The
callbackis invoked with different signatures depending on the code path:
- Line 203:
callback(ok)— single argument- Line 210:
callback(!err, err)— two argumentsWhile this works (the CLI callback handles
errbeingundefined), it's inconsistent and could confuse future maintainers or callers expecting a stable signature.♻️ Suggested fix for consistent callback signature
function runDefaultTests(options, callback) { var ok = runTests(options); if (!ok || options.filter) { - if (callback) callback(ok); + if (callback) callback(ok, null); return ok; } if (!callback) return ok; bracketParsing(function(err) { callback(!err, err); }); return null; }🤖 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 `@test/index.js` around lines 199 - 214, The callback in runDefaultTests has inconsistent signatures (callback(ok) vs callback(!err, err)); make them uniform by always calling the callback with (err, result) or (error, ok) consistently: treat the first parameter as an error and second as the boolean result. Update the branch that currently calls callback(ok) to call callback(null, ok) (or callback(undefined, ok)), and ensure the bracketParsing branch calls callback(err || null, !err) so both paths use the same (error, result) signature; adjust any callers if they expect the old order.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@test/index.js`:
- Around line 610-611: The main function currently declares an unused argv
parameter; either remove argv from main's signature to avoid the misleading
parameter, or wire it through by changing main to pass argv into parseArg and
then update parseArg to accept an argv parameter and use that (falling back to
process.argv.slice(2) when argv is undefined) so tests can supply custom args;
locate the functions main and parseArg in the file and apply one of these two
changes consistently.
- Around line 199-214: The callback in runDefaultTests has inconsistent
signatures (callback(ok) vs callback(!err, err)); make them uniform by always
calling the callback with (err, result) or (error, ok) consistently: treat the
first parameter as an error and second as the boolean result. Update the branch
that currently calls callback(ok) to call callback(null, ok) (or
callback(undefined, ok)), and ensure the bracketParsing branch calls
callback(err || null, !err) so both paths use the same (error, result)
signature; adjust any callers if they expect the old order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64d0c2e6-c826-4e09-b260-6043d5f82798
📒 Files selected for processing (3)
lib/marked.jstest/bracket_parsing.jstest/index.js
Co-authored-by: Cursor <cursoragent@cursor.com>
BenCookie95
force-pushed
the
mm-68425-bracket-parsing
branch
from
6532ed7 to
94021d2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/marked.js (1)
521-521: ⚡ Quick winAdd regression fixtures for the new
_insidebranches.This hardening looks good, but this regex is easy to regress. Please add targeted cases for escaped brackets, escaped backslashes, and backtick spans inside link labels to lock expected parsing behaviour.
🤖 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 `@lib/marked.js` at line 521, The new inline._inside regex introduced to tighten parsing has extra branches that are easy to regress; add focused regression fixtures that assert correct parsing of link labels containing escaped brackets (e.g., "\[" and "\]"), escaped backslashes (e.g., "\\\\" sequences) and backtick spans inside labels (e.g., "`code`" within "[...]" labels). Create tests that feed markdown snippets into the parser and assert the produced token stream or rendered HTML matches the expected output for each case, referencing the inline._inside behavior and ensuring the three scenarios (escaped brackets, escaped backslashes, backtick spans) are covered as separate assertions so future changes to inline._inside will be caught.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@lib/marked.js`:
- Line 521: The new inline._inside regex introduced to tighten parsing has extra
branches that are easy to regress; add focused regression fixtures that assert
correct parsing of link labels containing escaped brackets (e.g., "\[" and
"\]"), escaped backslashes (e.g., "\\\\" sequences) and backtick spans inside
labels (e.g., "`code`" within "[...]" labels). Create tests that feed markdown
snippets into the parser and assert the produced token stream or rendered HTML
matches the expected output for each case, referencing the inline._inside
behavior and ensuring the three scenarios (escaped brackets, escaped
backslashes, backtick spans) are covered as separate assertions so future
changes to inline._inside will be caught.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@lib/marked.js`:
- Around line 498-499: The composite bracket-matching regex is broken because
inline._inside currently only treats single-backtick code spans as opaque, so
update inline._inside to support variable-length backtick fences the same way
inline.code does (i.e., capture the opening backtick sequence and use a
backreference to match the identical closing fence, preventing premature ]
matches) or else extract the bracket scanning out of the composite regex and
perform incremental scanning that skips any code span using inline.code logic;
update the patterns referenced (inline._inside and inline.code) and add the
regression test from the comment demonstrating `[``a]b``](...)` to ensure
multi-backtick code spans do not terminate bracket parsing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@hmhealey @edgarbellot mattermost/mattermost#36710 has the commit if you want to conduct some testing |
hmhealey
left a comment
hmhealey
left a comment
There was a problem hiding this comment.
Could you also add a test case for this in test/tests/catastrophic.text? There's no expected output for that one. The tester just makes sure it completes quickly
| inline._inside = /(?:\[(?:\\.|[^\[\]\\])*\]|\\.|`+[^`]*?`+(?!`)|[^\[\]\\`])*/; | ||
| inline._label = /(?:\\.|[^\[\]\\])*/; | ||
| inline._href = /(?:[^()]|\([^()]*\)|\((?:[^()]*\([^()]*\))+[^()]*\))*/; | ||
| inline.link = replace(inline.link) | ||
| ('inside', inline._inside) |
There was a problem hiding this comment.
| inline._inside = /(?:\[(?:\\.|[^\[\]\\])*\]|\\.|`+[^`]*?`+(?!`)|[^\[\]\\`])*/; | |
| inline._label = /(?:\\.|[^\[\]\\])*/; | |
| inline._href = /(?:[^()]|\([^()]*\)|\((?:[^()]*\([^()]*\))+[^()]*\))*/; | |
| inline.link = replace(inline.link) | |
| ('inside', inline._inside) | |
| const _inlineLabel = /(?:\[(?:\\.|[^\[\]\\])*\]|\\.|`+[^`]*?`+(?!`)|[^\[\]\\`])*/; | |
| const _blockLabel = /(?:\\.|[^\[\]\\])*/; | |
| inline._href = /(?:[^()]|\([^()]*\)|\((?:[^()]*\([^()]*\))+[^()]*\))*/; | |
| inline.link = replace(inline.link) | |
| ('inside', _inlineLabel) |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Summary
Ticket Link
https://mattermost.atlassian.net/browse/MM-68425
Test Plan