From 62f6a2a712113b0bc95ed3633ae596d0520811ae Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 08:33:11 -0700 Subject: [PATCH 1/2] Pair a Review Findings Entry With Its Thread Past the Line Suffix in pr_review.py `entry_paths` now drops a trailing `:LINE` or `:START-END` from a path a list entry names, so `src/a.py:340` pairs with a thread on `src/a.py`. Kept, every entry in that shape read as unthreaded, and the digest reported each one under FINDINGS WITH NO THREAD. Fixes #3176 Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 15 +++++++++++++-- tests/test_pr_review.py | 9 ++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 4e3ef75d..ae5d9cea 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -2876,12 +2876,23 @@ def labelled_entries(body: str, label: re.Pattern[str]) -> list[str]: return entries +LINE_SUFFIX = re.compile(r":[0-9]+(?:-[0-9]+)?$") + + def entry_paths(entry: str) -> list[str]: - """The paths one list entry names, read from its own code spans.""" + """The paths one list entry names, read from its own code spans. + + A span written as `FILE:LINE` or `FILE:START-END` names the file, so the suffix is dropped. + Kept, `src/a.py:340` never named a thread on `src/a.py`, and every entry read as unthreaded. + """ return [ named for span in CODE_SPAN.finditer(entry) - if (named := bare_path(span.group()[len(span.group(1)) : -len(span.group(1))]).strip()) + if ( + named := LINE_SUFFIX.sub( + "", bare_path(span.group()[len(span.group(1)) : -len(span.group(1))]).strip() + ) + ) ] diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index f21cc7fb..f76ab2fb 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5664,9 +5664,12 @@ def test_an_entry_on_a_file_no_thread_is_on_is_counted_where_the_totals_balance( ) def test_every_entry_on_a_threaded_file_counts_none(self) -> None: - """A trailing part, a zero-width space after a slash, and a directory each name a file.""" + """A trailing part, a zero-width space after a slash, a directory, and a path carrying a + line or a line range each name a file. + """ for listed in ( PROSE_LISTED, + "- [P1] `lib/reader.py:340` drops an entry.\n- [P2] `writer.py:12-18` is stale.\n", "- `lib/\u200breader.py` and `lib/writer.py`: one issue each (2 votes each).\n", "- `lib/`: moderate retry issue (2 votes).\n", "- A retry issue\n in `lib/reader.py` (2 votes).\n", @@ -5680,6 +5683,10 @@ def test_a_second_entry_on_a_file_one_thread_is_on_is_counted(self) -> None: out = self.digest(with_prose(PROSE_LISTED + "- `lib/reader.py`: a stale name (1 vote).")) self.assertIn("FINDINGS WITH NO THREAD (1)", out) + def test_an_entry_carrying_a_line_on_a_file_no_thread_is_on_is_counted(self) -> None: + out = self.digest(with_prose(PROSE_LISTED + "- `lib/parser.py:7`: validation issue.")) + self.assertIn("FINDINGS WITH NO THREAD (1)", out) + def test_a_bold_label_is_read(self) -> None: body = with_prose("- `lib/parser.py`: validation issue (1 vote).").replace( "Review findings:", "**Review findings:**" From 7e79e169307c7b4543d69e45a13d044e9f59a9d3 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 08:36:19 -0700 Subject: [PATCH 2/2] Pin the Line Suffix in the Narrative Reader and Drop a Test That Cannot Fail The narrative reader shares `entry_paths`, so a test now pins that an `Open findings remain:` entry naming `pkg/guard.py:12` pairs with the thread on that file. The prose test for an unthreaded file carrying a line passed with or without the fix and is removed. The `entry_paths` docstring states the pairing rather than overstating the old defect. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 2 +- tests/test_pr_review.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index ae5d9cea..604913c9 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -2883,7 +2883,7 @@ def entry_paths(entry: str) -> list[str]: """The paths one list entry names, read from its own code spans. A span written as `FILE:LINE` or `FILE:START-END` names the file, so the suffix is dropped. - Kept, `src/a.py:340` never named a thread on `src/a.py`, and every entry read as unthreaded. + A thread carries the path alone, which is what the entry's path is paired against. """ return [ named diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index f76ab2fb..8bb0fda9 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5683,10 +5683,6 @@ def test_a_second_entry_on_a_file_one_thread_is_on_is_counted(self) -> None: out = self.digest(with_prose(PROSE_LISTED + "- `lib/reader.py`: a stale name (1 vote).")) self.assertIn("FINDINGS WITH NO THREAD (1)", out) - def test_an_entry_carrying_a_line_on_a_file_no_thread_is_on_is_counted(self) -> None: - out = self.digest(with_prose(PROSE_LISTED + "- `lib/parser.py:7`: validation issue.")) - self.assertIn("FINDINGS WITH NO THREAD (1)", out) - def test_a_bold_label_is_read(self) -> None: body = with_prose("- `lib/parser.py`: validation issue (1 vote).").replace( "Review findings:", "**Review findings:**" @@ -5870,6 +5866,10 @@ def test_entries_restating_threads_count_none(self) -> None: self.assertEqual(0, self.count(with_narrative(listed))) self.assertNotIn("FINDINGS WITH NO THREAD", self.digest(with_narrative(listed))) + def test_an_entry_naming_its_file_with_a_line_pairs_with_the_thread_on_it(self) -> None: + listed = "- `pkg/guard.py:12`: a failed read skips the check (**critical**, 1 vote).\n" + self.assertEqual(0, self.count(with_narrative(listed))) + def test_an_entry_stating_no_severity_this_reads_is_counted(self) -> None: for listed in ( "- `pkg/guard.py`: a failed read skips the check (1 vote).\n",