Promote Alternating-Section Reading and the Node Probe Timeout Skip to Main

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

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<details>\n<summary>Review details</summary>\n\n"
+            "<summary>Suppressed comments (1)</summary>\n\nThe first suppressed finding.\n\n"
+            "<summary>Previously missed (1)</summary>\n\nThe previously missed finding.\n\n"
+            "<summary>Suppressed comments (2)</summary>\n\nThe second suppressed finding.\n\n"
+            "The third suppressed finding.\n</details>\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<details>\n<summary>Review details</summary>\n\n"
+            "<summary><strong>Previously missed (1)</strong></summary>\n\nThe first.\n\n"
+            "<summary><strong>Suppressed comments (1)</strong></summary>\n\nThe second.\n\n"
+            "<summary><strong>Previously missed (1)</strong></summary>\n\nThe third.\n</details>\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.
 

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.