feat: add broken-link drift checker - #61
Conversation
theDakshJaitly
left a comment
There was a problem hiding this comment.
Thanks for another checker, @advancedresearcharray — this one's noticeably more ambitious than #60, and it shows. The fenced-code and inline-code stripping is genuinely thoughtful (and you tested both), the file-relative + .mex/-prefix resolution mirrors path.ts correctly, and catching image links () as a bonus is a nice touch. Tests pass and the structure is clean.
I'm marking this request-changes for two things before merge — both straightforward:
1. Local links with a #fragment are falsely flagged as broken (see inline). A link like [install](./target.md#install) reports BROKEN_LINK even when target.md exists, because the #install fragment is never stripped before the existence check. I reproduced it against the checker. Linking to a heading in another doc is a common pattern, and at error severity (-10 each) those false positives add up. The fix is small — strip a trailing #... (and ideally ?...) in normalizeLinkTarget.
2. Rebase on main and recount the checkers. #60 just merged and touched the same README / CONTRIBUTING / CHANGELOG lines plus index.ts / reporter.ts / types.ts / checkers.test.ts, so this branch now conflicts on 6 files. After rebasing, main already lists ten checkers (it added tool-config-sync + todo-fixme), so with broken-link the count becomes eleven — the intro line should read "Eleven," and the table just needs the broken-link row added (the rest are already there). The CONTRIBUTING "Adding a drift checker" count moves to 11 too. (Tiny thing while you're in CHANGELOG: main uses ## [Unreleased] with brackets to match the ## [0.3.5] style — yours dropped the brackets.)
Non-blocking, optional follow-ups:
- Severity:
path.tsdowngrades towarningforpatterns/files and placeholder paths, reservingerrorfor real misses — worth considering the same here so a pattern doc demonstrating link syntax doesn't hard-error. - Reference-style links (
[a][ref]), paths with spaces,~~~fences, and double-backtick inline code aren't handled — all fine to leave; a one-line scope comment would be plenty.
Really solid work overall. Once the fragment fix and the rebase/recount are in, this is good to go. Thanks again for taking on the harder checkers.
| function normalizeLinkTarget(raw: string): string { | ||
| let target = raw.replace(/^<|>$/g, "").trim(); | ||
| const titleSplit = target.match(/^([^\s]+)(?:\s+["'].+["'])?$/); | ||
| if (titleSplit) target = titleSplit[1]; | ||
| return target; |
There was a problem hiding this comment.
This is where the #fragment false positive originates. For [install](./target.md#install), target stays ./target.md#install, which fails existsSync, so an existing file is reported broken (at error severity). Reproduced locally.
Suggested fix — drop the fragment/query before returning:
function normalizeLinkTarget(raw: string): string {
let target = raw.replace(/^<|>$/g, "").trim();
const titleSplit = target.match(/^([^\s]+)(?:\s+["'].+["'])?$/);
if (titleSplit) target = titleSplit[1];
// Drop in-page fragment / query so links to a heading in another file resolve
target = target.replace(/[#?].*$/, "");
return target;
}Bonus: a pure #section link then normalizes to "" and is already skipped by the !target guard up in the loop, so same-page anchors stay ignored without needing the startsWith("#") branch. Worth a test for [x](./target.md#heading) (target exists) -> expect no issue.
| if (!linkTargetExists(target, fileDir, projectRoot, scaffoldRoot)) { | ||
| issues.push({ | ||
| code: "BROKEN_LINK", | ||
| severity: "error", |
There was a problem hiding this comment.
Optional / non-blocking: path.ts (the closest analog) emits warning rather than error for patterns/ files and placeholder-y paths, keeping error for genuinely-missing real paths. Since a broken link is the same class of problem, mirroring that would stop example links in pattern docs from costing 10 points each.
|
Pushed fixes for the review in 73570d8: strip |
Scan scaffold markdown for local link targets that do not exist on disk. Skips external URLs, anchors, and links inside code spans. Closes #52 Co-authored-by: Cursor <cursoragent@cursor.com>
Addresses review on #61: normalize #/? suffixes before existsSync, downgrade severity in patterns/ like path checker, add tests. Co-authored-by: Cursor <cursoragent@cursor.com>
73570d8 to
d4d3d9b
Compare
|
Rebased onto |
theDakshJaitly
left a comment
There was a problem hiding this comment.
Re-reviewed after the rebase — all good now.
- Fragment fix is in (
normalizeLinkTargetstrips[#?].*$). I re-checked independently:./target.md#installand query strings resolve, and a genuinely-missing file with a fragment still flags, so the strip isn't over-broad. Good regression test for it too. - Rebased cleanly on
main; checker count reconciled to eleven across the README intro, the table, and CONTRIBUTING, and the CHANGELOG[Unreleased]bracket style is preserved. - You also picked up the optional severity nuance (
warninginpatterns/, mirroringpath.ts) with a test — appreciated.
typecheck / 175 tests / build all green locally. Approving.
|
Merged — and this is your first contribution to land in mex. Thank you for taking on two of the trickier good-first-issues back to back, and for the clean turnaround here: the rebase, the fragment fix, and picking up the severity nuance unprompted all made this easy to ship. Genuinely solid work. Hope to see more from you whenever you're up for it. |
Summary
Adds a broken-link drift checker for #52:
[text](path)links with local/relative targetshttp(s)://,mailto:,#anchors, and links inside fenced or inline codeTest plan
npm run typecheck && npm test && npm run buildCloses #52