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. 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.""" diff --git a/scripts/tests/test_spec_validate.py b/scripts/tests/test_spec_validate.py index 44da6a01..6b9d2137 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 @@ -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), []) diff --git a/spec/audit.py b/spec/audit.py index f15ca708..c696dac4 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 @@ -624,10 +625,71 @@ 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): @@ -635,7 +697,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 +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" }' diff --git a/spec/validate.py b/spec/validate.py index 112ef7c8..078ee2b8 100755 --- a/spec/validate.py +++ b/spec/validate.py @@ -42,10 +42,67 @@ 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_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"]