Fix Issue #1010's Three Deferred Code-Review Findings

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.

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]]:

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."""

scripts/tests/test_spec_validate.py

@@ -5,6 +5,7 @@
 
 import sys
 import tempfile
+import time
 import unittest
 from pathlib import Path
 
@@ -188,6 +189,40 @@ 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_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.
@@ -206,6 +241,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), [])
 

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
@@ -624,18 +625,88 @@ 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_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)`. 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.
+
+    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 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
+    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 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_close.get(i)
+        if label_end is None:
+            i += 1
+            continue
+        label = text[i + 1 : label_end - 1]
+        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_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
+            continue
+        # 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):
     """Markdown links reduced to their text - `[text](url)` and `[text][ref]` become `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 +3469,41 @@ 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")
+        print(
+            "  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.
+    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" }'

spec/validate.py

@@ -42,10 +42,67 @@
 MARKDOWN_INLINE_LINK = re.compile(r"\]\((?P<target>[^)\s]+)")
 MARKDOWN_REFERENCE_LINK = re.compile(r"^\[[^]]+\]:\s*(?P<target>\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_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)`. 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.
+
+    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 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
+    return matches
+
+
+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.
+    """
+    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_close.get(i)
+        if label_end is None:
+            i += 1
+            continue
+        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 label_end in bracket_close:
+            return True
+        # 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
 
 
 def load(rel):
@@ -113,7 +170,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"]