From c11fbeb7ef0fc368b2779eba559d4576901f7096 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 25 Aug 2026 15:27:45 -0700 Subject: [PATCH 1/5] Balance Bracket Nesting in Markdown Link Detection `[^\]]*`-based regexes in spec/validate.py and spec/audit.py stopped at the first `]`, so a link label carrying its own nested brackets (`[API [docs]](url)`) passed both the registry description gate and strip_md_links() undetected. Replaced with a balanced bracket/paren scanner in both files, kept in sync as before. Fixes one of the three findings tracked in #1010. --- scripts/tests/test_spec_validate.py | 18 +++++++ spec/audit.py | 80 ++++++++++++++++++++++++++--- spec/validate.py | 60 ++++++++++++++++++++-- 3 files changed, 147 insertions(+), 11 deletions(-) diff --git a/scripts/tests/test_spec_validate.py b/scripts/tests/test_spec_validate.py index 44da6a01..d8942302 100755 --- a/scripts/tests/test_spec_validate.py +++ b/scripts/tests/test_spec_validate.py @@ -188,6 +188,24 @@ def test_a_reference_style_markdown_link_is_rejected(self) -> None: ["Fixture: description carries Markdown links - keep it link-free plain text"], ) + def test_a_nested_bracket_link_label_is_rejected(self) -> None: + # Regresses a gap where `[^\]]*` stopped at the first `]` and missed a label with its own brackets. + self.assertEqual( + validate.description_errors( + "Fixture", "See [API [docs]](https://example.test) for more." + ), + ["Fixture: description carries Markdown links - keep it link-free plain text"], + ) + + def test_a_destination_with_two_parenthesized_groups_is_rejected(self) -> None: + # Regresses the matching gap on the destination side: more than one balanced `()` run after the link. + self.assertEqual( + validate.description_errors( + "Fixture", "See [docs](https://example.test/a_(b)_(c)) for more." + ), + ["Fixture: description carries Markdown links - keep it link-free plain text"], + ) + def test_leading_or_trailing_whitespace_is_rejected(self) -> None: # Not silently trimmed here, even though spec/audit.py and configure.sh both strip it defensively. # Rejecting it at the source keeps the registry's own text the exact canonical form every mirror carries. diff --git a/spec/audit.py b/spec/audit.py index f15ca708..dcf65379 100755 --- a/spec/audit.py +++ b/spec/audit.py @@ -624,10 +624,65 @@ def heading_texts(markdown): _HTML_COMMENT = re.compile(r"", re.DOTALL) -_MD_LINK_INLINE = re.compile( - r"\[([^\]]*)\]\((?:[^()]|\([^()]*\))*\)" -) # URL may hold one level of () -_MD_LINK_REF = re.compile(r"\[([^\]]*)\]\[[^\]]*\]") + + +def _bracket_span(text, open_pos, open_char, close_char): + """The index just past the char matching text[open_pos], counting only open_char/close_char nesting. + + A character class like `[^\\]]*` cannot count depth, so it stops at the first close and misses a link + label carrying its own nested brackets, e.g. `[API [docs]](url)`. This walks one bracket type at a time + instead, so it is called once for a `[]` run and once for a `()` run rather than mixed in a single pass. + Returns None if open_pos is out of range or the run never balances back to depth 0. + """ + if open_pos >= len(text) or text[open_pos] != open_char: + return None + depth = 0 + for i in range(open_pos, len(text)): + c = text[i] + if c == open_char: + depth += 1 + elif c == close_char: + depth -= 1 + if depth == 0: + return i + 1 + return None + + +def markdown_link_spans(text): + """Yield (start, end, label) for each `[label](dest)` or `[label][ref]` use, brackets/parens balanced. + + Kept in sync with spec/validate.py's DESCRIPTION_LINK_INLINE/DESCRIPTION_LINK_REF detection, which needs + the same balanced-nesting rule for the same reason. + """ + i, n = 0, len(text) + while i < n: + if text[i] != "[": + i += 1 + continue + label_end = _bracket_span(text, i, "[", "]") + if label_end is None: + i += 1 + continue + label = text[i + 1 : label_end - 1] + dest_end = ( + _bracket_span(text, label_end, "(", ")") + if label_end < n and text[label_end] == "(" + else None + ) + if dest_end is not None: + yield i, dest_end, label + i = dest_end + continue + ref_end = ( + _bracket_span(text, label_end, "[", "]") + if label_end < n and text[label_end] == "[" + else None + ) + if ref_end is not None: + yield i, ref_end, label + i = ref_end + continue + i = label_end def strip_md_links(text): @@ -635,7 +690,16 @@ def strip_md_links(text): The plain-text form GOVERNANCE.md "Repository Details" says the About description carries. """ - return _MD_LINK_REF.sub(r"\1", _MD_LINK_INLINE.sub(r"\1", text)) + spans = list(markdown_link_spans(text)) + if not spans: + return text + out, pos = [], 0 + for start, end, label in spans: + out.append(text[pos:start]) + out.append(label) + pos = end + out.append(text[pos:]) + return "".join(out) def title_and_intro(text): @@ -3398,14 +3462,18 @@ def _selftest(): ) # Description mirror: links reduce to their text, and a link-free line passes through unchanged. linked = "Utility to clean [media](https://x.example/Foo_(bar)) per the [spec][spec-ref]." + nested = "See [API [docs]](https://example.test/a_(b)_(c)) for details." if ( strip_md_links(linked) != "Utility to clean media per the spec." or strip_md_links("Plain intro line.") != "Plain intro line." + or strip_md_links(nested) != "See API [docs] for details." ): ok = False print(" FAIL description: strip_md_links behavior") else: - print(" ok description: Markdown links reduce to their text, plain text passes through") + print( + " ok description: Markdown links reduce to their text, plain text passes through, nested brackets/parens balance" + ) # cspell duplication: a workspace cSpell word list is detected, and a mere cspell.json mention is not. ws_dup = '{ "settings": { "cSpell.words": ["foo"] } }' ws_ok = '{ "settings": { "editor.rulers": [100] }, "note": "words live in cspell.json" }' diff --git a/spec/validate.py b/spec/validate.py index 112ef7c8..8881b205 100755 --- a/spec/validate.py +++ b/spec/validate.py @@ -42,10 +42,60 @@ MARKDOWN_INLINE_LINK = re.compile(r"\]\((?P[^)\s]+)") MARKDOWN_REFERENCE_LINK = re.compile(r"^\[[^]]+\]:\s*(?P\S+)", re.MULTILINE) TEMPLATE_REPOSITORY_URL = "https://github.com/ptr727/ProjectTemplate" -# A description-shaped link use, `[text](url)` or `[text][ref]`, kept in sync with spec/audit.py's strip_md_links(). -# The carried-link regexes above find a definition's target inside a whole document, not a use inside one short string. -DESCRIPTION_LINK_INLINE = re.compile(r"\[([^\]]*)\]\((?:[^()]|\([^()]*\))*\)") -DESCRIPTION_LINK_REF = re.compile(r"\[([^\]]*)\]\[[^\]]*\]") + + +def _bracket_span(text, open_pos, open_char, close_char): + """The index just past the char matching text[open_pos], counting only open_char/close_char nesting. + + A character class like `[^\\]]*` cannot count depth, so it stops at the first close and misses a link + label carrying its own nested brackets, e.g. `[API [docs]](url)`. This walks one bracket type at a time + instead, so it is called once for a `[]` run and once for a `()` run rather than mixed in a single pass. + Returns None if open_pos is out of range or the run never balances back to depth 0. + """ + if open_pos >= len(text) or text[open_pos] != open_char: + return None + depth = 0 + for i in range(open_pos, len(text)): + c = text[i] + if c == open_char: + depth += 1 + elif c == close_char: + depth -= 1 + if depth == 0: + return i + 1 + return None + + +def contains_description_markdown_link(text): + """Whether `text` carries a `[text](url)` or `[text][ref]` use, brackets/parens balanced. + + Kept in sync with spec/audit.py's markdown_link_spans(), which needs the same balanced-nesting rule for + the same reason: this is a description-shaped link use inside one short string, not the carried-link + regexes above, which find a definition's target inside a whole document. + """ + i, n = 0, len(text) + while i < n: + if text[i] != "[": + i += 1 + continue + label_end = _bracket_span(text, i, "[", "]") + if label_end is None: + i += 1 + continue + if ( + label_end < n + and text[label_end] == "(" + and _bracket_span(text, label_end, "(", ")") is not None + ): + return True + if ( + label_end < n + and text[label_end] == "[" + and _bracket_span(text, label_end, "[", "]") is not None + ): + return True + i = label_end + return False def load(rel): @@ -113,7 +163,7 @@ def description_errors(name, desc): return [ f"{name}: description must be plain single-line text with no leading or trailing whitespace" ] - if DESCRIPTION_LINK_INLINE.search(desc) or DESCRIPTION_LINK_REF.search(desc): + if contains_description_markdown_link(desc): return [f"{name}: description carries Markdown links - keep it link-free plain text"] if len(desc) > 100: return [f"{name}: description is {len(desc)} characters, over the 100-char limit"] From c9f7471f132e1399a1ed43de6f8b4fcc9cc37866 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 25 Aug 2026 15:28:01 -0700 Subject: [PATCH 2/5] Separate Persistence from PATH Availability in Pre-Commit README The snippet's README claimed uv tool install gives an unconditionally PATH-available command, contradicting the very next sentence, which already treats PATH availability as conditional on uv's tool bin directory. Claim only persistence here. Fixes one of the three findings tracked in #1010. --- catalog/snippets/pre-commit/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/catalog/snippets/pre-commit/README.md b/catalog/snippets/pre-commit/README.md index 10453ad2..771d7c70 100644 --- a/catalog/snippets/pre-commit/README.md +++ b/catalog/snippets/pre-commit/README.md @@ -20,8 +20,8 @@ the commit rather than silently skipping the gate. Install and enable with `uv tool install pre-commit` once, then `pre-commit install`. `pre-commit` itself is never added as a project dependency: the lint-only profile has no -project environment to add it to, and `uv tool install` gives a persistent, PATH-available -command independent of any project, the same footing `uvx` gives the tools the hooks run. +project environment to add it to, and `uv tool install` gives a persistent command independent +of any project. The hooks use `uvx` to run tools independently of the project. If `pre-commit install` reports the command not found right after installing it, `uv tool install`'s own bin directory is not yet on `PATH`: run `uv tool update-shell` and restart or re-source the shell, or add the directory `uv tool dir --bin` prints directly. From 532636218e81db03f53296a37c4789d6fcd9e6e0 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 25 Aug 2026 15:28:15 -0700 Subject: [PATCH 3/5] Widen the Bot-ID Lookback on a Comment-Only Narrow Window copilot_history() only widened past HISTORY_PRS when the narrow window came back fully empty. A narrow window carrying only a Copilot comment, no formal review, returned early instead, leaving copilot_bot_id() with nothing to read since only the review connection carries the id. A formal review just outside the narrow window went permanently unread. Widening is now keyed on copilot_bot_id(entries) rather than emptiness. Fixes one of the three findings tracked in #1010. --- scripts/pr_review.py | 33 ++++++++++++++++++++++----------- scripts/tests/test_pr_review.py | 23 +++++++++++++++++++++++ 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 2f63d189..3ca13f6d 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -522,19 +522,30 @@ def copilot_history(owner: str, repo: str) -> list[tuple[int, dict]]: """The reviewer's own reviews and comments across the repository's most recently updated pull requests, newest activity first regardless of which connection it came from. - Read at HISTORY_PRS first and, only where that comes back with nothing at all, read again at - the wider HISTORY_PRS_WIDE. A narrow window emptying out is the ordinary case, a repository - whose most recent activity genuinely carries none of the reviewer's, and costs nothing beyond - the one call either caller below was always going to make. It stops being ordinary once an - outage outlasts HISTORY_PRS pull requests: every one of them then carries the same silence, - the narrow window empties out too, and both callers would otherwise fall back to blind - polling for the rest of the outage with no way to tell that outage apart from a repository - that has simply never seen a Copilot review (#985, reproduced on ptr727/ProjectTemplate - PRs #981-984). The wider read is what tells the two apart, and it is tried only once the - narrow one is empty, so the ordinary case still costs one call rather than two. + Read at HISTORY_PRS first and, only where that comes back carrying no usable bot id, read + again at the wider HISTORY_PRS_WIDE. A narrow window emptying out is the ordinary case, a + repository whose most recent activity genuinely carries none of the reviewer's, and costs + nothing beyond the one call either caller below was always going to make. It stops being + ordinary once an outage outlasts HISTORY_PRS pull requests: every one of them then carries + the same silence, the narrow window empties out too, and both callers would otherwise fall + back to blind polling for the rest of the outage with no way to tell that outage apart from a + repository that has simply never seen a Copilot review (#985, reproduced on + ptr727/ProjectTemplate PRs #981-984). The wider read is what tells the two apart. + + Emptying out is not the only way the narrow window fails a bot-id lookup, though: it can + carry real activity and still have none, when every entry within it is a plain comment. A + formal review, `copilot_bot_id`'s only source for the id, can sit just outside the narrow + window while a newer comment sits inside it, and returning the narrow read as soon as it has + anything at all left that review permanently unread. Widening is keyed on + `copilot_bot_id(entries)` rather than on emptiness for exactly that case, so a comment-only + narrow window still triggers the wider read the same way an empty one does. The ordinary case + -- a narrow window already carrying a review -- still costs one call rather than two, since + that is the common shape a usable bot id already satisfies. """ entries = _copilot_history_window(owner, repo, HISTORY_PRS) - return entries if entries else _copilot_history_window(owner, repo, HISTORY_PRS_WIDE) + if entries and copilot_bot_id(entries) is not None: + return entries + return _copilot_history_window(owner, repo, HISTORY_PRS_WIDE) def _copilot_history_window(owner: str, repo: str, prs: int) -> list[tuple[int, dict]]: diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index b41ce072..44a760d2 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -2790,6 +2790,29 @@ def fake(_query: str, **variables: object) -> dict: pr_review.copilot_history("o", "r") self.assertEqual([pr_review.HISTORY_PRS], seen) + def test_a_comment_only_narrow_window_still_widens(self) -> None: + """A narrow window carrying activity is not itself a usable bot id: a comment carries + none, per `copilot_bot_id`, so this must widen the same as an empty window would rather + than returning a history no caller can read a bot id from.""" + self.answer(payload([])) + seen: list[object] = [] + narrow_node = { + "number": 970, + "reviews": {"nodes": []}, + "comments": {"nodes": [comment(at=LATE)]}, + } + + def fake(_query: str, **variables: object) -> dict: + seen.append(variables["prs"]) + if variables["prs"] == pr_review.HISTORY_PRS: + return {"repository": {"pullRequests": {"nodes": [narrow_node]}}} + return {"repository": {"pullRequests": {"nodes": [hist_review(900, QUOTA_REFUSED)]}}} + + self.enterContext(mock.patch.object(pr_review, "gh_graphql", side_effect=fake)) + history = pr_review.copilot_history("o", "r") + self.assertEqual([pr_review.HISTORY_PRS, pr_review.HISTORY_PRS_WIDE], seen) + self.assertEqual("BOT_1", pr_review.copilot_bot_id(history)) + def test_both_windows_empty_still_carries_no_signal_and_no_bot_id(self) -> None: """An outage wide enough to empty HISTORY_PRS_WIDE too is a real, if rarer, case: still no id to request with and no fabricated one, rather than a crash on the second call.""" From 93904d5a2deb5ee4747780b0d63c860981c5d01e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 25 Aug 2026 15:36:18 -0700 Subject: [PATCH 4/5] Match Brackets in One Linear Pass, Not a Rescan Per Open markdown_link_spans() and contains_description_markdown_link() called a fresh depth-counting scan from every unmatched '[', O(N^2) on a run of N of them. spec/audit.py applies this to a README tagline before any length limit, so one large or malformed README could stall the fleet audit. Replaced with a single stack-based pass per bracket type, producing the same open-to-close pairing in one scan instead of N. CodeRabbit finding on PR #1011, spec/audit.py:657-665 and spec/validate.py:76-84. --- scripts/tests/test_spec_validate.py | 8 ++++ spec/audit.py | 68 +++++++++++++++++------------ spec/validate.py | 49 ++++++++++----------- 3 files changed, 69 insertions(+), 56 deletions(-) diff --git a/scripts/tests/test_spec_validate.py b/scripts/tests/test_spec_validate.py index d8942302..3d5eefc7 100755 --- a/scripts/tests/test_spec_validate.py +++ b/scripts/tests/test_spec_validate.py @@ -5,6 +5,7 @@ import sys import tempfile +import time import unittest from pathlib import Path @@ -224,6 +225,13 @@ def test_an_embedded_newline_is_rejected(self) -> None: ], ) + def test_a_long_run_of_unmatched_brackets_stays_linear(self) -> None: + # A run of unmatched '[' used to re-scan the remaining text from every position. + # That was O(N^2) (#1011, CodeRabbit), and a slow run here means a regression back to it. + start = time.monotonic() + validate.contains_description_markdown_link("[" * 20000) + self.assertLess(time.monotonic() - start, 1.0) + def test_exactly_the_cap_is_clean(self) -> None: self.assertEqual(validate.description_errors("Fixture", "a" * 100), []) diff --git a/spec/audit.py b/spec/audit.py index dcf65379..2823c56b 100755 --- a/spec/audit.py +++ b/spec/audit.py @@ -36,6 +36,7 @@ import re import subprocess import sys +import time import urllib.error import urllib.request from datetime import UTC, datetime @@ -626,58 +627,52 @@ def heading_texts(markdown): _HTML_COMMENT = re.compile(r"", re.DOTALL) -def _bracket_span(text, open_pos, open_char, close_char): - """The index just past the char matching text[open_pos], counting only open_char/close_char nesting. +def _bracket_matches(text, open_char, close_char): + """Map from each open_char index in text to the index just past its balanced close_char. A character class like `[^\\]]*` cannot count depth, so it stops at the first close and misses a link - label carrying its own nested brackets, e.g. `[API [docs]](url)`. This walks one bracket type at a time - instead, so it is called once for a `[]` run and once for a `()` run rather than mixed in a single pass. - Returns None if open_pos is out of range or the run never balances back to depth 0. + label carrying its own nested brackets, e.g. `[API [docs]](url)`. Counting only open_char/close_char + nesting, ignoring the other bracket type, needs one such map per bracket type rather than one pass + mixing both. Built with a single left-to-right stack pass over the whole text rather than one depth- + counting scan per open position: re-scanning from every unmatched open is what made the previous + version O(N^2) on a run of N unmatched opens (#1011, CodeRabbit). A close pops the most recently pushed + open, the same pairing a fresh depth count from that open would find, so this is one pass, not N. + An open with no closing partner, or a close with nothing open, is left out of the map, same as before. """ - if open_pos >= len(text) or text[open_pos] != open_char: - return None - depth = 0 - for i in range(open_pos, len(text)): - c = text[i] + stack = [] + matches = {} + for i, c in enumerate(text): if c == open_char: - depth += 1 - elif c == close_char: - depth -= 1 - if depth == 0: - return i + 1 - return None + stack.append(i) + elif c == close_char and stack: + matches[stack.pop()] = i + 1 + return matches def markdown_link_spans(text): """Yield (start, end, label) for each `[label](dest)` or `[label][ref]` use, brackets/parens balanced. - Kept in sync with spec/validate.py's DESCRIPTION_LINK_INLINE/DESCRIPTION_LINK_REF detection, which needs - the same balanced-nesting rule for the same reason. + Kept in sync with spec/validate.py's contains_description_markdown_link(), which needs the same + balanced-nesting rule for the same reason. """ + bracket_close = _bracket_matches(text, "[", "]") + paren_close = _bracket_matches(text, "(", ")") i, n = 0, len(text) while i < n: if text[i] != "[": i += 1 continue - label_end = _bracket_span(text, i, "[", "]") + label_end = bracket_close.get(i) if label_end is None: i += 1 continue label = text[i + 1 : label_end - 1] - dest_end = ( - _bracket_span(text, label_end, "(", ")") - if label_end < n and text[label_end] == "(" - else None - ) + dest_end = paren_close.get(label_end) if label_end < n and text[label_end] == "(" else None if dest_end is not None: yield i, dest_end, label i = dest_end continue - ref_end = ( - _bracket_span(text, label_end, "[", "]") - if label_end < n and text[label_end] == "[" - else None - ) + ref_end = bracket_close.get(label_end) if label_end < n and text[label_end] == "[" else None if ref_end is not None: yield i, ref_end, label i = ref_end @@ -3474,6 +3469,21 @@ def _selftest(): print( " ok description: Markdown links reduce to their text, plain text passes through, nested brackets/parens balance" ) + # A run of unmatched '[' used to re-scan the remaining text from every position (#1011, CodeRabbit), + # O(N^2) on a README tagline read before any length limit. Linear now: a slow run means a regression. + pathological = "[" * 20000 + start = time.monotonic() + strip_md_links(pathological) + elapsed = time.monotonic() - start + if elapsed > 1.0: + ok = False + print( + f" FAIL description: strip_md_links took {elapsed:.2f}s on unmatched brackets, expected linear" + ) + else: + print( + f" ok description: strip_md_links stays linear on unmatched brackets ({elapsed:.3f}s for 20000)" + ) # cspell duplication: a workspace cSpell word list is detected, and a mere cspell.json mention is not. ws_dup = '{ "settings": { "cSpell.words": ["foo"] } }' ws_ok = '{ "settings": { "editor.rulers": [100] }, "note": "words live in cspell.json" }' diff --git a/spec/validate.py b/spec/validate.py index 8881b205..61b0e1bd 100755 --- a/spec/validate.py +++ b/spec/validate.py @@ -44,26 +44,27 @@ TEMPLATE_REPOSITORY_URL = "https://github.com/ptr727/ProjectTemplate" -def _bracket_span(text, open_pos, open_char, close_char): - """The index just past the char matching text[open_pos], counting only open_char/close_char nesting. +def _bracket_matches(text, open_char, close_char): + """Map from each open_char index in text to the index just past its balanced close_char. A character class like `[^\\]]*` cannot count depth, so it stops at the first close and misses a link - label carrying its own nested brackets, e.g. `[API [docs]](url)`. This walks one bracket type at a time - instead, so it is called once for a `[]` run and once for a `()` run rather than mixed in a single pass. - Returns None if open_pos is out of range or the run never balances back to depth 0. + label carrying its own nested brackets, e.g. `[API [docs]](url)`. Counting only open_char/close_char + nesting, ignoring the other bracket type, needs one such map per bracket type rather than one pass + mixing both. Built with a single left-to-right stack pass over the whole text rather than one depth- + counting scan per open position: re-scanning from every unmatched open is what made a prior version of + this walk O(N^2) on a run of N unmatched opens (#1011, CodeRabbit, on spec/audit.py's sibling + implementation). A close pops the most recently pushed open, the same pairing a fresh depth count from + that open would find, so this is one pass, not N. An open with no closing partner, or a close with + nothing open, is left out of the map, same as before. """ - if open_pos >= len(text) or text[open_pos] != open_char: - return None - depth = 0 - for i in range(open_pos, len(text)): - c = text[i] + stack = [] + matches = {} + for i, c in enumerate(text): if c == open_char: - depth += 1 - elif c == close_char: - depth -= 1 - if depth == 0: - return i + 1 - return None + stack.append(i) + elif c == close_char and stack: + matches[stack.pop()] = i + 1 + return matches def contains_description_markdown_link(text): @@ -73,26 +74,20 @@ def contains_description_markdown_link(text): the same reason: this is a description-shaped link use inside one short string, not the carried-link regexes above, which find a definition's target inside a whole document. """ + bracket_close = _bracket_matches(text, "[", "]") + paren_close = _bracket_matches(text, "(", ")") i, n = 0, len(text) while i < n: if text[i] != "[": i += 1 continue - label_end = _bracket_span(text, i, "[", "]") + label_end = bracket_close.get(i) if label_end is None: i += 1 continue - if ( - label_end < n - and text[label_end] == "(" - and _bracket_span(text, label_end, "(", ")") is not None - ): + if label_end < n and text[label_end] == "(" and label_end in paren_close: return True - if ( - label_end < n - and text[label_end] == "[" - and _bracket_span(text, label_end, "[", "]") is not None - ): + if label_end < n and text[label_end] == "[" and label_end in bracket_close: return True i = label_end return False From ed0fb6cde9bce2a3fde53b8f333dc9ca573c5410 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 25 Aug 2026 15:42:01 -0700 Subject: [PATCH 5/5] Retry Past a Non-Link Span and Skip Escaped Delimiters The balanced-bracket scanner had two correctness gaps of its own, introduced by the same rewrite that fixed the nested-label regex gap: On a span that matched brackets but was not itself followed by a destination or reference, both scanners jumped to the end of the whole span instead of retrying one character in, so a link nested inside a non-link bracket run, e.g. [[docs](url)], was skipped entirely. The bracket-matching stack counted a backslash-escaped delimiter (\[, \], \(, \)) as real nesting, corrupting the label of a link whose text legitimately contains an escaped bracket. Both fixed in both files, with regression tests in each. qodo findings on PR #1011, spec/validate.py:92 and spec/audit.py. --- scripts/tests/test_spec_validate.py | 16 ++++++++++++++++ spec/audit.py | 26 +++++++++++++++++++++++--- spec/validate.py | 16 ++++++++++++++-- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/scripts/tests/test_spec_validate.py b/scripts/tests/test_spec_validate.py index 3d5eefc7..6b9d2137 100755 --- a/scripts/tests/test_spec_validate.py +++ b/scripts/tests/test_spec_validate.py @@ -207,6 +207,22 @@ def test_a_destination_with_two_parenthesized_groups_is_rejected(self) -> None: ["Fixture: description carries Markdown links - keep it link-free plain text"], ) + def test_a_link_nested_inside_a_non_link_bracket_run_is_still_rejected(self) -> None: + # A failed outer span used to jump past the whole run instead of retrying one character in. + # That skipped the valid inner link in `[[docs](url)]` (#1011, qodo). + self.assertEqual( + validate.description_errors("Fixture", "See [[docs](url)] for more."), + ["Fixture: description carries Markdown links - keep it link-free plain text"], + ) + + def test_an_escaped_bracket_inside_a_label_does_not_corrupt_the_match(self) -> None: + # A backslash-escaped `\[` used to count as real nesting, corrupting the label match. + # It reads as a literal character instead (#1011, qodo). + self.assertEqual( + validate.description_errors("Fixture", r"See [API \[docs](url) for more."), + ["Fixture: description carries Markdown links - keep it link-free plain text"], + ) + def test_leading_or_trailing_whitespace_is_rejected(self) -> None: # Not silently trimmed here, even though spec/audit.py and configure.sh both strip it defensively. # Rejecting it at the source keeps the registry's own text the exact canonical form every mirror carries. diff --git a/spec/audit.py b/spec/audit.py index 2823c56b..c696dac4 100755 --- a/spec/audit.py +++ b/spec/audit.py @@ -638,11 +638,20 @@ def _bracket_matches(text, open_char, close_char): version O(N^2) on a run of N unmatched opens (#1011, CodeRabbit). A close pops the most recently pushed open, the same pairing a fresh depth count from that open would find, so this is one pass, not N. An open with no closing partner, or a close with nothing open, is left out of the map, same as before. + + A backslash-escaped delimiter (`\\[`, `\\]`, `\\(`, `\\)`) is skipped rather than pushed or popped, + matching Markdown's own escaping rule, so a literal bracket inside a label does not corrupt the nesting + count (#1011, qodo). """ stack = [] matches = {} + escaped = False for i, c in enumerate(text): - if c == open_char: + if escaped: + escaped = False + elif c == "\\": + escaped = True + elif c == open_char: stack.append(i) elif c == close_char and stack: matches[stack.pop()] = i + 1 @@ -677,7 +686,10 @@ def markdown_link_spans(text): yield i, ref_end, label i = ref_end continue - i = label_end + # This span is not itself a link. + # A nested bracket run starting inside it may still be one, e.g. `[[docs](url)]`. + # Retry one character in rather than skipping past the whole span (#1011, qodo). + i += 1 def strip_md_links(text): @@ -3458,16 +3470,24 @@ def _selftest(): # Description mirror: links reduce to their text, and a link-free line passes through unchanged. linked = "Utility to clean [media](https://x.example/Foo_(bar)) per the [spec][spec-ref]." nested = "See [API [docs]](https://example.test/a_(b)_(c)) for details." + # A failed outer span used to jump past the whole run instead of retrying one character in, + # so a link nested inside a non-link bracket run was skipped (#1011, qodo). + inner_link = "See [[docs](url)] for details." + # A backslash-escaped `\[` used to count as real nesting, corrupting the label match instead + # of being read as a literal character (#1011, qodo). + escaped_bracket = r"See [API \[docs](url) for details." if ( strip_md_links(linked) != "Utility to clean media per the spec." or strip_md_links("Plain intro line.") != "Plain intro line." or strip_md_links(nested) != "See API [docs] for details." + or strip_md_links(inner_link) != "See [docs] for details." + or strip_md_links(escaped_bracket) != r"See API \[docs for details." ): ok = False print(" FAIL description: strip_md_links behavior") else: print( - " ok description: Markdown links reduce to their text, plain text passes through, nested brackets/parens balance" + " ok description: Markdown links reduce to their text, plain text passes through, nested brackets/parens balance, nested and escaped labels handled" ) # A run of unmatched '[' used to re-scan the remaining text from every position (#1011, CodeRabbit), # O(N^2) on a README tagline read before any length limit. Linear now: a slow run means a regression. diff --git a/spec/validate.py b/spec/validate.py index 61b0e1bd..078ee2b8 100755 --- a/spec/validate.py +++ b/spec/validate.py @@ -56,11 +56,20 @@ def _bracket_matches(text, open_char, close_char): implementation). A close pops the most recently pushed open, the same pairing a fresh depth count from that open would find, so this is one pass, not N. An open with no closing partner, or a close with nothing open, is left out of the map, same as before. + + A backslash-escaped delimiter (`\\[`, `\\]`, `\\(`, `\\)`) is skipped rather than pushed or popped, + matching Markdown's own escaping rule, so a literal bracket inside a label does not corrupt the nesting + count (#1011, qodo). """ stack = [] matches = {} + escaped = False for i, c in enumerate(text): - if c == open_char: + if escaped: + escaped = False + elif c == "\\": + escaped = True + elif c == open_char: stack.append(i) elif c == close_char and stack: matches[stack.pop()] = i + 1 @@ -89,7 +98,10 @@ def contains_description_markdown_link(text): return True if label_end < n and text[label_end] == "[" and label_end in bracket_close: return True - i = label_end + # This span is not itself a link. + # A nested bracket run starting inside it may still be one, e.g. `[[docs](url)]`. + # Retry one character in rather than skipping past the whole span (#1011, qodo). + i += 1 return False