From 9351de51ed7481f97c1538242a15bef8b2fa9c48 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 16:57:43 -0700 Subject: [PATCH 1/2] Tell a Timed-Out Node Probe From an Engine Disagreement in test_spec_validate (#3192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary For #3182. The corpus test in `test_spec_validate` runs `node` under a 60 second timeout. A cold start on a slow hosted Windows runner raised `TimeoutExpired` and failed the suite as though the schema patterns disagreed. - A `TimeoutExpired` from the probe now skips the test with that reason, the same way a host without `node` already skips. - Every other failure is unchanged: a disagreement still fails `assertEqual`, and a pattern `new RegExp` rejects still fails through `check=True`. - Runaway backtracking can't hide behind the skip. Python's `re.search` evaluates every pattern over the same corpus before `node` starts, so the run hangs there instead. ## Verification - A forced `TimeoutExpired` from the `node` call: the test reports `skipped=1`, with no failures or errors. - `python3 -m unittest tests.test_spec_validate` passes with `node` present. - ruff, mypy, the prose gate, and the eol gate are clean. - A local strict review found nothing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- tests/test_spec_validate.py | 45 ++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/tests/test_spec_validate.py b/tests/test_spec_validate.py index 96627095..c725d697 100755 --- a/tests/test_spec_validate.py +++ b/tests/test_spec_validate.py @@ -1481,6 +1481,9 @@ def test_both_engines_agree_on_every_pattern_over_a_corpus(self) -> None: Node is what an editor's JSON language service uses, so it is the engine the schema actually meets. A host without node skips this rather than failing, since the shape check above still binds everywhere. + + A probe that times out skips too, since a cold start on a slow runner is no verdict on the patterns. A + pattern that backtracks without end cannot hide behind that skip, since Python matches the corpus first. """ node = shutil.which("node") if node is None: @@ -1539,16 +1542,42 @@ def test_both_engines_agree_on_every_pattern_over_a_corpus(self) -> None: "console.log(JSON.stringify(Object.fromEntries(" "pats.map(p => [p, cases.map(c => new RegExp(p).test(c))]))));" ) - result = subprocess.run( - [node, "-e", probe, json.dumps([patterns, corpus])], - capture_output=True, - text=True, - encoding="utf-8", - timeout=60, - check=True, - ) + try: + result = subprocess.run( + [node, "-e", probe, json.dumps([patterns, corpus])], + capture_output=True, + text=True, + encoding="utf-8", + timeout=60, + check=True, + ) + except subprocess.TimeoutExpired: + self.skipTest("node timed out, which is no verdict on the patterns") self.assertEqual(json.loads(result.stdout), want) + def test_a_timed_out_corpus_probe_skips_rather_than_failing(self) -> None: + """A real timeout takes a minute, so the probe is made to raise one rather than to be slow. + + Node refusing a pattern exits non-zero, and that still fails, so a widened skip is caught too. + """ + + def probe(side_effect: BaseException) -> unittest.TestResult: + result = unittest.TestResult() + with ( + unittest.mock.patch.object(shutil, "which", return_value="node"), + unittest.mock.patch.object(subprocess, "run", side_effect=side_effect), + ): + type(self)("test_both_engines_agree_on_every_pattern_over_a_corpus").run(result) + return result + + timed_out = probe(subprocess.TimeoutExpired("node", 60)) + self.assertEqual([], timed_out.failures + timed_out.errors) + self.assertEqual(1, len(timed_out.skipped)) + self.assertIn("node timed out", timed_out.skipped[0][1]) + refused = probe(subprocess.CalledProcessError(1, "node")) + self.assertEqual([], refused.skipped) + self.assertEqual(1, len(refused.errors)) + def test_the_description_pattern_never_refuses_what_the_validator_accepts(self) -> None: """The schema's own statement rather than a copy, so the direction is what has to hold. From c41e5c079e6a9a468e1aa23e48711bb28c41a2bb Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 17:00:30 -0700 Subject: [PATCH 2/2] Read Every Section of a Kind When Sections Alternate in One Wrapper in pr_review.py (#3194) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary For #3180. `marker_blocks` followed its chain of section bounds only while each bound matched its own marker. So one wrapper holding a suppressed section, then a previously-missed one, then a second suppressed one left the third section unread. The digest counted one suppressed finding of three under `shapes=ok`. - The walk now visits every bound `sibling_heading` returns. It opens a block only where the marker matches and skips a sibling of the other kind to that sibling's own bound. Prose past a bound is still never scanned, since only bound lines are visited. - `outside_diff_blocks` is unchanged in behavior, because CodeRabbit's marker matches no section bound. ## Verification - The new test `test_sections_alternating_in_one_wrapper_are_each_read` builds the issue's body from vetted summary spellings. It asserts two suppressed blocks, `suppressed=3`, `shapes=ok`, and each finding printed once. It also covers the swapped kinds, where it asserts no unrecognized shape. - With `develop`'s `pr_review.py`, the test fails at `2 != 1`. With the fix it passes. - The full `tests.test_pr_review` suite passes, and ruff, mypy, the prose gate, and the eol gate are clean. - The local strict review took two rounds. Round 1 found that the test's docstring claimed `shapes=ok` for a body whose Markdown heading is unvetted, and fcbc3dee fixes that. It also found a pre-existing gap, filed as #3193: the vetted-but-unread check covers previously-missed summaries only. Round 2 found nothing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- scripts/pr_review.py | 7 ++++--- tests/test_pr_review.py | 42 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index e8dd3fb3..e0256edd 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -4534,7 +4534,7 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F """Return the review body's sections whose own heading matches `marker`. Each is sliced from its heading to the next sibling heading or the end of its own region. - Past a region's first section, only the sibling heading it ends at can open another. + Past a region's first section, only a sibling heading some section ends at can open another. Shared by `suppressed_blocks`, `previously_missed_blocks`, and `outside_diff_blocks`. `strip_blockquote` affects heading detection only, never the returned content. A summary split @@ -4565,9 +4565,10 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F bare = COUNT.search(line) and not OPENING_COUNT.match(line) if marker.search(line) and (HEADING.match(line) or bare) and not entry_heading(line): start = i - while start < len(scan_lines) and marker.search(scan_lines[start]): + while start < len(scan_lines): end = sibling_heading(scan_lines, start) - blocks.append("\n".join(raw_lines[start:end])) + if marker.search(scan_lines[start]): + blocks.append("\n".join(raw_lines[start:end])) start = end break return blocks diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 1382e7e6..1f3a4206 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1603,6 +1603,48 @@ def test_a_section_ends_where_its_sibling_s_heading_starts(self) -> None: self.assertEqual(1, out.count("The suppressed finding.")) self.assertEqual(1, out.count("The previously missed finding.")) + def test_sections_alternating_in_one_wrapper_are_each_read(self) -> None: + """The chain of bounds runs through a section of the other kind rather than stopping there. + + Stopped at the first sibling of the other kind, the second suppressed section reached no + block. The digest counted one suppressed finding of three, under `shapes=ok`. + """ + shared = ( + f"{OVERVIEW}\n
\nReview details\n\n" + "Suppressed comments (1)\n\nThe first suppressed finding.\n\n" + "Previously missed (1)\n\nThe previously missed finding.\n\n" + "Suppressed comments (2)\n\nThe second suppressed finding.\n\n" + "The third suppressed finding.\n
\n" + ) + suppressed = pr_review.suppressed_blocks(shared) + self.assertEqual(2, len(suppressed)) + self.assertIn("The first suppressed finding.", suppressed[0]) + self.assertIn("The second suppressed finding.", suppressed[1]) + self.assertIn("The third suppressed finding.", suppressed[1]) + self.assertNotIn("The previously missed finding.", "".join(suppressed)) + missed = pr_review.previously_missed_blocks(shared) + self.assertEqual(1, len(missed)) + self.assertNotIn("suppressed finding.", missed[0]) + self.answer(payload([review(body=shared)])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("suppressed=3", out) + self.assertIn("shapes=ok", out) + for finding in ( + "first suppressed", + "previously missed", + "second suppressed", + "third suppressed", + ): + self.assertEqual(1, out.count(f"The {finding} finding.")) + swapped = ( + f"{OVERVIEW}\n
\nReview details\n\n" + "Previously missed (1)\n\nThe first.\n\n" + "Suppressed comments (1)\n\nThe second.\n\n" + "Previously missed (1)\n\nThe third.\n
\n" + ) + self.assertEqual(2, len(pr_review.previously_missed_blocks(swapped))) + self.assertEqual([], pr_review.unrecognized_in(swapped)) + def test_a_finding_entry_is_no_section_whatever_its_title_says(self) -> None: """An entry's title is free text, and either section's marker words can appear in it.