From f8fbbce5c5fcceac168e8b8f372df97814ef7f97 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 25 Sep 2026 22:12:42 -0700 Subject: [PATCH 1/2] Fold Typographic Punctuation in pr_review.py reply --match A --match string copied verbatim from a rendered finding fails to select the thread it was copied from when the finding's typographic quotes, dashes, or ellipsis don't survive the copy in ASCII, and NO_MATCH reads identically to the thread being missing or already resolved. matching_threads() now folds both the pattern and each candidate body through an ASCII table before the substring compare, and NO_MATCH now states how many unresolved threads exist so a zero and a several no longer read the same. A parameterized test checks each of the issue's seven characters independently of the fold table itself, so dropping one from the table still fails the test that covers it. Closes on promotion: #1299 Co-Authored-By: Claude Sonnet 5 --- scripts/pr_review.py | 26 ++++++++++++--- scripts/tests/test_pr_review.py | 56 +++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index d01d40de..7f165f73 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -3092,19 +3092,35 @@ def describe(thread: dict) -> str: ) +_TYPOGRAPHIC_FOLD = str.maketrans( + { + "\u2018": "'", # left single quotation mark + "\u2019": "'", # right single quotation mark + "\u201c": '"', # left double quotation mark + "\u201d": '"', # right double quotation mark + "\u2013": "-", # en dash + "\u2014": "-", # em dash + "\u2026": "...", # horizontal ellipsis + } +) + + def matching_threads(threads: list[dict], match: str, path: str | None) -> list[dict]: """Threads whose finding text contains `match`, narrowed by `path` where one is given. Matched on the finding's own words rather than on a line number, because a fix push moves the line and every lookup keyed to one then misses: replies posted against nothing while the resolves still succeeded, so the threads closed carrying no answer. Case-insensitive, since - the text is quoted back out of a digest by a reader rather than compared by a machine. + the text is quoted back out of a digest by a reader rather than compared by a machine. Both + sides are folded through `_TYPOGRAPHIC_FOLD` first, since the pattern is a substring copied + from a rendered finding whose typographic quotes, dashes, or ellipsis may not survive that + copy in ASCII. """ - needle = match.lower() + needle = match.translate(_TYPOGRAPHIC_FOLD).lower() return [ t for t in threads - if needle in (first_comment(t).get("body") or "").lower() + if needle in (first_comment(t).get("body") or "").translate(_TYPOGRAPHIC_FOLD).lower() and (path is None or t.get("path") == path) ] @@ -3149,7 +3165,8 @@ def reply_to_thread( Every refusal below is a stop rather than a fallback. There is no id to guess at, no second-best thread to settle for, and no resolve on a reply that did not land, because each of those closes a finding while leaving it unanswered, which is the state a reviewer reads as - addressed. + addressed. A no-match names the unresolved count, since zero and several otherwise read the + same without the reader counting the lines the refusal prints below it. """ ok, why = in_scope(owner) if not ok: @@ -3163,6 +3180,7 @@ def reply_to_thread( f"status=NO_MATCH nothing was written: no unresolved thread on {owner}/{repo} " f"#{num} carries {match!r}" + (f" at {path}" if path else "") + + f", of {len(threads)} unresolved thread(s) total" + ". Widen the words or drop --path rather than reaching for an id, since the " "thread may also be resolved already, which reads the same from here." ) diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 299e4544..6593d36f 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -5220,6 +5220,62 @@ def test_the_match_reads_the_finding_text_rather_than_a_line_number(self) -> Non self.assertEqual(0, self.run_reply("--resolve")) self.assertIn("REPLIED_AND_RESOLVED", self.out.getvalue()) + def test_an_ascii_pattern_selects_a_body_written_with_typographic_punctuation(self) -> None: + """A `--match` string copied from a rendered finding is ASCII, and the body may not be.""" + body = ( + "The helper wasn\u2019t clear on \u201cwidening the words\u201d \u2014 it just " + "refuses\u2026" + ) + self.wire(page([rthread("t1", body=body)])) + self.assertEqual( + 0, + self.run_reply( + "--resolve", "--match", 'wasn\'t clear on "widening the words" - it just refuses' + ), + ) + self.assertIn("REPLIED_AND_RESOLVED", self.out.getvalue()) + + def test_a_typographic_pattern_selects_a_body_written_in_ascii(self) -> None: + """The fold runs both ways: a pasted pattern can carry the typographic form too.""" + self.wire(page([rthread("t1", body='The helper says "widen the words" - try again.')])) + self.assertEqual( + 0, + self.run_reply("--resolve", "--match", "\u201cwiden the words\u201d \u2014 try again"), + ) + self.assertIn("REPLIED_AND_RESOLVED", self.out.getvalue()) + + def test_every_documented_character_selects_across_the_ascii_boundary(self) -> None: + """Each of the issue's seven characters folds on its own, an expectation independent of + `_TYPOGRAPHIC_FOLD` itself, so dropping one from that table still fails this.""" + expected_folds = { + 0x2018: "'", # left single quotation mark + 0x2019: "'", # right single quotation mark + 0x201C: '"', # left double quotation mark + 0x201D: '"', # right double quotation mark + 0x2013: "-", # en dash + 0x2014: "-", # em dash + 0x2026: "...", # horizontal ellipsis + } + for code_point, ascii_form in expected_folds.items(): + char = chr(code_point) + with self.subTest(char=repr(char)): + self.wire(page([rthread("t1", body=f"The finding reads foo{char}bar plainly.")])) + self.assertEqual(0, self.run_reply("--resolve", "--match", f"foo{ascii_form}bar")) + self.assertIn("REPLIED_AND_RESOLVED", self.out.getvalue()) + + def test_no_match_names_how_many_unresolved_threads_there_are(self) -> None: + """Zero and several unresolved threads otherwise read the same, with nothing to tell them apart.""" + self.wire( + page( + [ + rthread("t1", body="An unrelated finding about naming."), + rthread("t2", body="A second, also unrelated finding."), + ] + ) + ) + self.assertEqual(60, self.run_reply("--resolve")) + self.assertIn("of 2 unresolved thread(s) total", self.out.getvalue()) + class TestReplyConfirmsBeforeResolving(ReplyCase): def test_a_reply_returning_no_url_leaves_the_thread_open(self) -> None: From 781876222a34d56773b984911b540719533e27c4 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 25 Sep 2026 22:23:05 -0700 Subject: [PATCH 2/2] Reset the captured stdout buffer between subTest iterations test_every_documented_character_selects_across_the_ascii_boundary captures stdout once in setUp and ran all seven fold checks against that same buffer, so a later iteration's assertIn found an earlier iteration's leftover REPLIED_AND_RESOLVED output rather than its own. Clearing the buffer at the start of each subTest makes the assertion specific to its own iteration. Co-Authored-By: Claude Sonnet 5 --- scripts/tests/test_pr_review.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 6593d36f..eaf42575 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -5259,6 +5259,8 @@ def test_every_documented_character_selects_across_the_ascii_boundary(self) -> N for code_point, ascii_form in expected_folds.items(): char = chr(code_point) with self.subTest(char=repr(char)): + self.out.seek(0) + self.out.truncate(0) self.wire(page([rthread("t1", body=f"The finding reads foo{char}bar plainly.")])) self.assertEqual(0, self.run_reply("--resolve", "--match", f"foo{ascii_form}bar")) self.assertIn("REPLIED_AND_RESOLVED", self.out.getvalue())