Harden the documentation link parser - #3102
Conversation
Chris0Jeky
added
docs
ChatGPT Codex Connector
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcf82d6110
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Fresh-context post-fix review at exact head 063e23b480a51e77bd83e78eaebbef948a529a41 against current main 8082a602db24fc3060a63b956c252d510d2faec8: no confirmed CRITICAL/HIGH merge blocker.
The five review defects are closed and exercised through the checker: footnotes, continuation-line destinations, quoted > attributes, HTML comments, and unmatched-backtick recovery. Independent verification passed 30/30 parser tests, the 705-file repository check with zero broken links, Windows case-exact tests, and git diff --check.
Additional non-blocking parser hardening findings (literal < before a tag, attribute-like text inside quoted values, unterminated comments, pathological delimiter performance, and nested reference definitions) are tracked in #3119 rather than expanding this already-bounded fix round. Hosted PostgreSQL failure remains a separate shared baseline gate and must be green before merge.
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Limited review of the shared path resolver, not approval of the whole PR.
This head retains two defects also present on main: inside.startsWith('..') rejects legitimate internal names such as ..notes.md, and the early existsSync return reports wrong-case links as missing on Linux. PR #3128 supplies a path-only correction and eight real-filesystem regressions. Six of the new cases fail on main before the fix; all eight pass afterward, along with the 16 existing fixture cases.
When integrating, retain this PR's parser/masking improvements and apply only #3128's path-resolution hunks plus its new tests. Do not replace this branch's checker with the main-based file.
The PR body still names dcf82d6 as its exact-head qualification, while this review is anchored to 063e23b. Refresh the evidence for the actual final head before claiming merge readiness.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14b21ee891
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Current checkpoint: #3102 remains parked and unmergeable.
A separately scoped GH-2762 follow-up should carry the remaining parser cases before any new merge qualification. |
|
Current-base requalification is complete and remains merge-blocked.
These are real red checks, not called flaky, and no baseline update or PR-local code change is justified. The PR remains parked pending ownership/resolution of the shared CI seams. |
|
@codex address that feedback Please implement and regression-test the three valid parser residuals already documented in review threads on the current PR branch:
Add focused Node regressions that fail on the current head and pass after the implementation. Also retain coverage proving empty inline labels, angle-bracket destinations, and file-only handling of explicit fragments so those already-supported cases are not misclassified as residuals. Keep the change limited to the documentation-link parser/tests; do not modify CI workflows or control-plane files. |
Summary
Testing
|
|
@codex please publish the completed parser residual fix to this PR's existing branch |
Summary
Testing
|
|
@codex publication cannot use your local Git credentials, so please reply here with the exact unified diff for local commit |
diff --git a/scripts/check-doc-links.mjs b/scripts/check-doc-links.mjs
index 6d1dbdc7b115b8bdce07c87263eee2473cd41af7..dc084d518c96b81b714430b2344e229230c0f74d 100644
--- a/scripts/check-doc-links.mjs
+++ b/scripts/check-doc-links.mjs
@@ -132,7 +132,6 @@ export function maskCodeWithDiagnostics(markdown) {
})
}
- maskHtmlComments(buffer, markdown)
const fenceMasked = buffer.join('')
let cursor = 0
while (cursor < fenceMasked.length) {
@@ -177,6 +176,8 @@ export function maskCodeWithDiagnostics(markdown) {
cursor = end
}
+ maskHtmlComments(buffer, buffer.join(''))
+
return { masked: buffer.join(''), diagnostics }
}
@@ -325,6 +326,60 @@ function parseReferenceDestination(text) {
return text.slice(start, cursor)
}
+function stripBlockContainerPrefix(line) {
+ let cursor = 0
+ while (cursor < line.length) {
+ const quote = /^[ \t]{0,3}>[ \t]?/.exec(line.slice(cursor))
+ if (quote) {
+ cursor += quote[0].length
+ continue
+ }
+
+ const listItem = /^[ \t]{0,3}(?:[-+*]|\d{1,9}[.)])[ \t]+/.exec(line.slice(cursor))
+ if (listItem) {
+ cursor += listItem[0].length
+ continue
+ }
+ break
+ }
+ return { text: line.slice(cursor), offset: cursor }
+}
+
+function findHtmlAttribute(tag, name, start) {
+ let cursor = start
+ while (cursor < tag.length) {
+ while (cursor < tag.length && /\s/.test(tag[cursor])) cursor += 1
+ if (cursor >= tag.length || tag[cursor] === '>' || tag[cursor] === '/') break
+
+ const nameStart = cursor
+ while (cursor < tag.length && !/[\s=/>]/.test(tag[cursor])) cursor += 1
+ const attributeName = tag.slice(nameStart, cursor)
+ while (cursor < tag.length && /\s/.test(tag[cursor])) cursor += 1
+
+ let value = ''
+ let valueStart = cursor
+ if (tag[cursor] === '=') {
+ cursor += 1
+ while (cursor < tag.length && /\s/.test(tag[cursor])) cursor += 1
+ const quote = tag[cursor] === '"' || tag[cursor] === "'" ? tag[cursor] : null
+ if (quote) {
+ cursor += 1
+ valueStart = cursor
+ while (cursor < tag.length && tag[cursor] !== quote) cursor += 1
+ value = tag.slice(valueStart, cursor)
+ if (tag[cursor] === quote) cursor += 1
+ } else {
+ valueStart = cursor
+ while (cursor < tag.length && !/[\s>]/.test(tag[cursor])) cursor += 1
+ value = tag.slice(valueStart, cursor)
+ }
+ }
+
+ if (attributeName.toLowerCase() === name) return { value, valueStart }
+ }
+ return null
+}
+
function extractLocalTargetsFromMasked(masked) {
const found = []
let sequence = 0
@@ -355,27 +410,45 @@ function extractLocalTargetsFromMasked(masked) {
// A reference-style link's path lives in its definition, so validating every
// local definition covers both links and images without resolving labels.
- const definitionPattern = /^[ \t]{0,3}\[(?!\^)[^\]\n]+\]:[ \t]*(.*)$/gm
- let definition
- while ((definition = definitionPattern.exec(masked)) !== null) {
+ const definitionPattern = /^[ \t]{0,3}\[(?!\^)[^\]\n]+\]:[ \t]*(.*)$/
+ let lineStart = 0
+ while (lineStart < masked.length) {
+ const lineEnd = masked.indexOf('\n', lineStart)
+ const end = lineEnd === -1 ? masked.length : lineEnd
+ const container = stripBlockContainerPrefix(masked.slice(lineStart, end))
+ const definition = definitionPattern.exec(container.text)
+ if (!definition) {
+ if (lineEnd === -1) break
+ lineStart = lineEnd + 1
+ continue
+ }
+
let destinationText = definition[1]
- let destinationIndex = definition.index
+ let destinationIndex = lineStart + container.offset + definition.index
if (destinationText.trim() === '') {
- const lineBreak = masked.indexOf('\n', definition.index)
+ const lineBreak = masked.indexOf('\n', lineStart)
if (lineBreak !== -1) {
const nextLineStart = lineBreak + 1
const nextLineEnd = masked.indexOf('\n', nextLineStart)
- const continuation = /^[ \t]*(\S.*)$/.exec(
- masked.slice(nextLineStart, nextLineEnd === -1 ? masked.length : nextLineEnd),
+ const nextLine = masked.slice(
+ nextLineStart,
+ nextLineEnd === -1 ? masked.length : nextLineEnd,
)
+ const nextContainer = stripBlockContainerPrefix(nextLine)
+ const continuation = /^[ \t]*(\S.*)$/.exec(nextContainer.text)
if (continuation) {
destinationText = continuation[1]
- destinationIndex = nextLineStart + continuation[0].indexOf(destinationText)
+ destinationIndex =
+ nextLineStart +
+ nextContainer.offset +
+ continuation[0].indexOf(destinationText)
}
}
}
const target = parseReferenceDestination(destinationText)
if (target !== null) push(target, destinationIndex)
+ if (lineEnd === -1) break
+ lineStart = lineEnd + 1
}
const findTagEnd = (start) => {
@@ -412,18 +485,12 @@ function extractLocalTargetsFromMasked(masked) {
}
const attributeName = opening[1].toLowerCase() === 'a' ? 'href' : 'src'
- const attributePattern = new RegExp(
- `\\s${attributeName}\\s*=\\s*(?:"([^"]*)"|'([^']*)'|([^\\s>]+))`,
- 'i',
- )
- const attribute = attributePattern.exec(tag)
+ const attribute = findHtmlAttribute(tag, attributeName, opening[0].length)
if (!attribute) {
cursor = end + 1
continue
}
- const target = attribute[1] ?? attribute[2] ?? attribute[3] ?? ''
- const valueOffset = attribute[0].indexOf(target)
- push(target, cursor + attribute.index + Math.max(0, valueOffset))
+ push(attribute.value, cursor + attribute.valueStart)
cursor = end + 1
}
diff --git a/scripts/check-doc-links.test.mjs b/scripts/check-doc-links.test.mjs
index 5ad2a52cfd3fd04f1c08ded20c64339fe0a1e11b..c8edf5bc0b1f49afedeb37ee5340ab66ecc21596 100644
--- a/scripts/check-doc-links.test.mjs
+++ b/scripts/check-doc-links.test.mjs
@@ -117,6 +117,23 @@ test('image links, angle-bracket targets and titles are all recognised', () => {
)
})
+test('empty inline labels, angle destinations and explicit fragments retain file targets', () => {
+ const markdown = [
+ '[](./empty-label.md)',
+ '[angle](<./angle destination.md>)',
+ '[fragment](./guide.md#details)',
+ ].join('\n')
+
+ assert.deepEqual(
+ extractLocalTargets(markdown).map(({ target, pathPart }) => ({ target, pathPart })),
+ [
+ { target: './empty-label.md', pathPart: './empty-label.md' },
+ { target: './angle destination.md', pathPart: './angle destination.md' },
+ { target: './guide.md#details', pathPart: './guide.md' },
+ ],
+ )
+})
+
test('reference-style definitions contribute their local destinations', () => {
const markdown = [
'[guide][guide-ref]',
@@ -167,6 +184,20 @@ test('HTML comments do not contribute local destinations', () => {
assert.deepEqual(extractLocalTargets(markdown), [])
})
+test('comment markers inside code do not mask later real links', () => {
+ const inline = ['`<!--`', '[real](inline.md)'].join('\n')
+ const fenced = ['```html', '<!--', '```', '[real](fenced.md)'].join('\n')
+
+ assert.deepEqual(
+ extractLocalTargets(inline).map(({ pathPart }) => pathPart),
+ ['inline.md'],
+ )
+ assert.deepEqual(
+ extractLocalTargets(fenced).map(({ pathPart }) => pathPart),
+ ['fenced.md'],
+ )
+})
+
test('HTML href and src attributes contribute local destinations', () => {
const markdown = [
'<a class="guide" href="./guide.md">Guide</a>',
@@ -180,6 +211,33 @@ test('HTML href and src attributes contribute local destinations', () => {
)
})
+test('HTML attribute-like text inside quoted values is ignored', () => {
+ const markdown = [
+ `<a title="fake href='./not-a-link.md'">No destination</a>`,
+ `<img alt='fake src="./not-an-image.svg"'>`,
+ ].join('\n')
+
+ assert.deepEqual(extractLocalTargets(markdown), [])
+})
+
+test('reference definitions inside nested block containers are recognised', () => {
+ const markdown = [
+ '> [quoted]: ./quoted.md',
+ '> - [unordered]: ./unordered.md',
+ '1. > [ordered]:',
+ ' > ./ordered.md',
+ ].join('\n')
+
+ assert.deepEqual(
+ extractLocalTargets(markdown).map(({ pathPart, line }) => ({ pathPart, line })),
+ [
+ { pathPart: './quoted.md', line: 1 },
+ { pathPart: './unordered.md', line: 2 },
+ { pathPart: './ordered.md', line: 4 },
+ ],
+ )
+})
+
test('a linked local image checks both the outer document and inner image', () => {
const targets = extractLocalTargets('[](docs/status.md)') |
1 participant
Outcome
Addresses the repository-actionable parser and masking residuals from GH-2762 without changing CI control-plane wiring or the working-tree scan policy.
The documentation checker now recognises additional local-link forms that the original single regular expression could silently miss:
<a href>and<img src>targetsIt also replaces the broad code-mask regular expressions with a stateful mask:
TDD and verification
The tests were committed before the implementation and add focused cases for every new form plus separate warning/link-failure behavior.
Hosted exact-head qualification for
dcf82d611065169b3f51e8d1b45edf84e22fafc2:34912930797: passed34912930572: passed34912940903reported only the repository's unrelated Backend Solution Regression and PostgreSQL Testcontainers failures; neither failing job exercises these two JavaScript filesThe implementation was also exercised in an isolated Node probe against the existing external-target, masking, title, missing-target, root-relative, percent-decoding, containment, case-exact, skip-list, and formatting behaviors, plus every new parser form and masking diagnostic.
Scope and residuals
scripts/check-doc-links.mjsandscripts/check-doc-links.test.mjs..github/**, Smart CI, required-context, branch-protection, or scheduled-workflow change.git ls-files; that portability/reproducibility choice remains open.Part of GH-2762.