Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions scripts/check_pr_path_collisions.py
Original file line number Diff line number Diff line change
Expand Up @@ -525,10 +525,36 @@ def list_open_prs(repo: str, limit: int) -> list[PrRow]:


def find_marker(repo: str, number: int) -> tuple[str, str] | None:
"""(id, body) of the marker comment on PR ``number``, or None."""
comments = _gh_json(["pr", "view", str(number), "--repo", repo,
"--json", "comments"]) or {}
return find_marker_entry(comments.get("comments") or [])
"""(id, body) of the marker comment on PR ``number``, or None.

Reads the REST collection, NOT ``gh pr view --json comments`` (#14421).
The two routes disagree on what ``id`` means, and only one of them is
addressable by the writer:

========================================== ==========================
source ``id`` of a comment
========================================== ==========================
``gh pr view --json comments`` (GraphQL) ``IC_kwDOH2Odns8AAAAB...``
``gh api repos/O/R/issues/N/comments`` ``5535634631``
========================================== ==========================

``edit_comment`` spends that id on ``PATCH /repos/{repo}/issues/comments/
{id}``, a REST route that only accepts the database id. Fed a GraphQL
node id it answers ``404 Not Found`` -- deterministically, for every
update and every retract, while POST (which needs no id at all) keeps
succeeding. That asymmetry is the whole signature of #14421: run 364
reported ``post=13 update=0 retract=0``, 13/13 creations confirmed and
0/10 in-place writes, and the organ stayed mute ~39 h because it could
never refresh a marker it had already posted.

``--paginate`` matters: a PR whose marker sits past the first page of
comments would otherwise read as "no marker" and be re-POSTED as a
duplicate. ``--slurp`` wraps the pages in an outer list, flattened here.
"""
pages = _gh_json(["api", "--paginate", "--slurp",
f"repos/{repo}/issues/{number}/comments"]) or []
comments = [c for page in pages for c in page]
return find_marker_entry(comments)


def scan_markers(
Expand Down
43 changes: 43 additions & 0 deletions scripts/tests/test_check_pr_path_collisions.py
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,49 @@ def test_find_marker_entry_returns_id_and_body(self):
self.assertEqual(_mod.find_marker_entry([{"id": 55, "body": body}]), ("55", body))
self.assertIsNone(_mod.find_marker_entry([{"id": 1, "body": "plain"}]))

def test_find_marker_reads_the_rest_route_not_graphql(self):
"""The id must be spendable by the PATCH writer (#14421).

``find_marker_entry`` is pure and cannot tell a database id from a
GraphQL node id -- both are truthy strings, so every unit test above
passes under either source. The defect therefore lives entirely in
the *fetch*, which is why this test asserts on the argv rather than
on the returned tuple.

Measured 2026-09-04 on #14495: ``gh pr view --json comments`` renders
``IC_kwDOH2Odns8AAAABSfMUxw`` where the REST collection renders
``5535634631``; ``GET /repos/.../issues/comments/IC_kwDO...`` answers
``404 Not Found``. ``edit_comment`` spends this id on that very route,
so a GraphQL id makes every update and retract 404 while POST keeps
working -- ``post=13 update=0 retract=0`` in run 364.
"""
seen = {}

def _capture(argv, **kwargs):
argv = list(argv)
seen["argv"] = argv
page = [{"id": 5535634631,
"body": "x " + _mod.COMMENT_MARKER_START + " y"}]
return _subprocess.CompletedProcess(
argv, 0, stdout=json.dumps([page]), stderr="")

with mock.patch.object(_mod.subprocess, "run", side_effect=_capture):
entry = _mod.find_marker("owner/repo", 4242)

self.assertIsNotNone(entry)
self.assertEqual(entry[0], "5535634631")
# The REST collection, addressable by the PATCH sibling ...
self.assertIn("api", seen["argv"][:2])
self.assertIn("repos/owner/repo/issues/4242/comments", seen["argv"])
# ... paginated, so a marker past page 1 is not misread as absent ...
self.assertIn("--paginate", seen["argv"])
# ... and never the GraphQL projection, whose ids are not spendable.
self.assertNotIn("view", seen["argv"])
self.assertFalse(
[a for a in seen["argv"] if a == "comments"],
"'--json comments' returns GraphQL node ids the writer cannot use",
)


class TestGhRowExtraction(unittest.TestCase):
def test_from_gh_dict_reads_body_and_refs(self):
Expand Down
Loading