From 2f92c8754a1fcbf60f47e75345f5f55284964cb9 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Thu, 30 Jul 2026 12:38:58 -0700 Subject: [PATCH 1/2] feat(iterate-pr): mark handled top-level feedback with a reaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A top-level review comment has no thread to resolve, so the skill had to dedupe by scanning existing comments for a reference marker citing the same author and snippet. That is fragile, and it failed in practice — on #75 and again on #77 the summary comment kept reporting as unaddressed after it had been answered, leaving a permanent needs_attention to reason about by hand every pass. A hooray reaction on the original is a machine-readable acknowledgement. fetch_pr_feedback now reads reactions.hooray, marks the item acknowledged, and buckets it as resolved, so a re-run reports zero instead of re-surfacing it. Verified on #77: needs_attention went 2 to 0 with no other change. The reaction step is documented against the PR-scoped comments endpoint specifically. The repo-wide repos/{owner}/{repo}/issues/comments returns every comment in the repository, and selecting from it will eventually react on another PR's comment — I did exactly that here and got the right answer by luck. Items now carry comment_id so the id comes from the feedback data rather than from matching body text. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q --- .agents/skills/iterate-pr/SKILL.md | 21 ++++++++++++++++--- .../iterate-pr/scripts/fetch_pr_feedback.py | 17 +++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index a2abc362..6f0f9308 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.agents/skills/iterate-pr/SKILL.md @@ -57,7 +57,7 @@ Returns JSON with feedback categorized as: - `medium` - Should address (`m:`, standard feedback) - `low` - Optional (`l:`, nit, style, suggestion) - `bot` - Informational automated comments (Codecov, Dependabot, etc.) -- `resolved` - Already resolved threads +- `resolved` - Already resolved threads, and top-level comments carrying our 🎉 acknowledgement Review bot feedback (from Sentry, Warden, Copilot, Cursor, Bugbot, CodeQL, etc.) appears in `high`/`medium`/`low` with `review_bot: true` — it is NOT placed in the `bot` bucket. @@ -182,7 +182,22 @@ To close out several threads in one pass, use `scripts/resolve_pr_threads.py THR **Top-level comments** (items WITHOUT a `thread_id` — `review_summary` items and top-level PR/issue comments, e.g. a review bot like Claude that posts its findings as one top-level comment): -There is no thread to reply into, so post a **new top-level comment** with `gh pr comment --body "..."`. A PR can carry several independent top-level comments, so a bare reply is ambiguous — **open every top-level reply with a reference marker** identifying the comment you are addressing. Cite the author and the opening of the original, and link it when the item includes a `url`: +There is no thread to reply into, so do two things: post a **new top-level comment**, then **add a 🎉 reaction to the original**. The reaction is the machine-readable record that this item is handled — `fetch_pr_feedback.py` reads it and buckets the comment as `resolved`, so a re-run stops reporting it as needing attention. Without it, every later pass re-surfaces the same comment and you have to reason about whether you already dealt with it. + +```bash +gh pr comment --body "..." + +# React on the ORIGINAL comment. Use the PR-scoped endpoint — the repo-wide +# `repos/{owner}/{repo}/issues/comments` returns every comment in the repo, and +# picking from it will eventually react on the wrong PR. +gh api "repos/{owner}/{repo}/issues//comments" \ + --jq '.[] | select(.id == ) | .id' +gh api -X POST "repos/{owner}/{repo}/issues/comments//reactions" -f content=hooray +``` + +The feedback script reports `comment_id` on each top-level item, so take the id from there rather than searching by body text. + +A PR can carry several independent top-level comments, so a bare reply is ambiguous — **open every top-level reply with a reference marker** identifying the comment you are addressing. Cite the author and the opening of the original, and link it when the item includes a `url`: ``` > **Re:** @ — "…" @@ -197,7 +212,7 @@ There is no thread to reply into, so post a **new top-level comment** with `gh p - 1-2 sentences: what was changed, why it's not an issue, or acknowledgment of declined items. - End every reply with `\n\n*— AI Coding Agent*`. -- Before replying, dedupe against re-loops: for inline threads, check whether the thread already has a reply ending in `*- AI Coding Agent*` / `*— AI Coding Agent*`; for top-level comments, scan existing top-level comments for one whose **reference marker** already cites this author + snippet (the signature alone is not enough — distinct top-level items would otherwise collide). +- Before replying, dedupe against re-loops: for inline threads, check whether the thread already has a reply ending in `*- AI Coding Agent*` / `*— AI Coding Agent*`. For top-level comments the 🎉 reaction handles this — an item carrying `acknowledged: true` (bucketed as `resolved`) has already been answered, so skip it rather than replying twice. - If the `gh`/GraphQL call fails, log and continue — do not block the workflow. ### 4. Check CI Status diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.py b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.py index 69aafb8e..85d9f9f1 100644 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.py +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.py @@ -301,6 +301,8 @@ def extract_feedback_item( review_bot: bool = False, self_review: bool = False, thread_id: str | None = None, + comment_id: int | None = None, + acknowledged: bool = False, ) -> dict[str, Any]: """Create a standardized feedback item.""" # Truncate long bodies for summary @@ -329,6 +331,10 @@ def extract_feedback_item( item["self_review"] = True if thread_id: item["thread_id"] = thread_id + if comment_id is not None: + item["comment_id"] = comment_id + if acknowledged: + item["acknowledged"] = True return item @@ -462,12 +468,23 @@ def main(): if not body or len(body.strip()) < 3: continue + # A 🎉 reaction is our machine-readable "this was handled" marker for + # top-level comments, which have no thread to resolve. Set it after + # replying; see the skill's "Replying to Comments" section. + acknowledged = comment.get("reactions", {}).get("hooray", 0) > 0 + item = extract_feedback_item( body=body, author=author, url=comment.get("html_url"), + comment_id=comment.get("id"), + acknowledged=acknowledged, ) + if acknowledged: + feedback["resolved"].append(item) + continue + if is_review_bot(author): category = categorize_comment(comment, body) item["review_bot"] = True From 45f5345c3bd902a53bfd286b361ee03d11318861 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Thu, 30 Jul 2026 15:02:38 -0700 Subject: [PATCH 2/2] fix(iterate-pr): scope the acknowledgement to our own reaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the raw count trusts anyone: a maintainer reacting 🎉 to a review comment for unrelated reasons would bucket it resolved and drop real feedback silently — the one direction this must never fail in. Now confirmed against the authenticated user, and fails closed: if the viewer cannot be identified or the lookup errors, the item resurfaces. Answering twice beats dropping something. The per-comment reactions lookup is skipped when the count is zero, so the common case costs no extra call. Two more from the same review. The 🎉 dedupe only ever worked for issue comments, but the docs implied it covered review_summary items too — GitHub exposes no reactions endpoint for a review body, so those have no reaction target and keep the reference-marker scan. And the documented lookup query lacked --paginate, which defaults to 30 per page and would silently miss a comment on a later page. Verifying this caught a bug in the fix itself: the viewer lookup used `gh api user --jq .login`, but run_gh json.loads() its stdout and --jq emits a bare unquoted string. It threw, failed closed, and every item resurfaced — correct behavior from a broken lookup, which is the good kind of failure but still wrong. Reads the login off the object now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q --- .agents/skills/iterate-pr/SKILL.md | 21 +++--- .../iterate-pr/scripts/fetch_pr_feedback.py | 67 +++++++++++++++++-- 2 files changed, 75 insertions(+), 13 deletions(-) diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index 6f0f9308..6d8cadd4 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.agents/skills/iterate-pr/SKILL.md @@ -57,7 +57,7 @@ Returns JSON with feedback categorized as: - `medium` - Should address (`m:`, standard feedback) - `low` - Optional (`l:`, nit, style, suggestion) - `bot` - Informational automated comments (Codecov, Dependabot, etc.) -- `resolved` - Already resolved threads, and top-level comments carrying our 🎉 acknowledgement +- `resolved` - Already resolved threads, and top-level comments carrying our own 🎉 acknowledgement Review bot feedback (from Sentry, Warden, Copilot, Cursor, Bugbot, CodeQL, etc.) appears in `high`/`medium`/`low` with `review_bot: true` — it is NOT placed in the `bot` bucket. @@ -182,20 +182,23 @@ To close out several threads in one pass, use `scripts/resolve_pr_threads.py THR **Top-level comments** (items WITHOUT a `thread_id` — `review_summary` items and top-level PR/issue comments, e.g. a review bot like Claude that posts its findings as one top-level comment): -There is no thread to reply into, so do two things: post a **new top-level comment**, then **add a 🎉 reaction to the original**. The reaction is the machine-readable record that this item is handled — `fetch_pr_feedback.py` reads it and buckets the comment as `resolved`, so a re-run stops reporting it as needing attention. Without it, every later pass re-surfaces the same comment and you have to reason about whether you already dealt with it. +There is no thread to reply into, so post a **new top-level comment** — and when the item carries a `comment_id`, also **add a 🎉 reaction to the original**. The reaction is the machine-readable record that the item is handled: `fetch_pr_feedback.py` checks whether _we_ reacted and buckets the comment as `resolved`, so a re-run stops reporting it. Without it, every later pass re-surfaces the same comment and you have to reason about whether you already dealt with it. ```bash gh pr comment --body "..." +gh api -X POST "repos/{owner}/{repo}/issues/comments//reactions" -f content=hooray +``` + +Take the id from the item's `comment_id` rather than searching by body text. If you do need to look it up, use the **PR-scoped** endpoint _with_ `--paginate` — the repo-wide `repos/{owner}/{repo}/issues/comments` returns every comment in the repository, and the PR-scoped one defaults to 30 per page, so an unpaginated search silently misses comments on later pages: -# React on the ORIGINAL comment. Use the PR-scoped endpoint — the repo-wide -# `repos/{owner}/{repo}/issues/comments` returns every comment in the repo, and -# picking from it will eventually react on the wrong PR. -gh api "repos/{owner}/{repo}/issues//comments" \ +```bash +gh api "repos/{owner}/{repo}/issues//comments" --paginate \ --jq '.[] | select(.id == ) | .id' -gh api -X POST "repos/{owner}/{repo}/issues/comments//reactions" -f content=hooray ``` -The feedback script reports `comment_id` on each top-level item, so take the id from there rather than searching by body text. +**The reaction is scoped to us.** The script does not trust the raw reaction count — anyone can react 🎉 to a comment for unrelated reasons, and treating that as handled would silently drop real feedback. It confirms the reaction belongs to the authenticated user, and fails closed: if the viewer cannot be identified, the item resurfaces. Answering twice beats dropping something. + +**Items without a `comment_id` cannot be reacted to.** GitHub exposes no reactions endpoint for a pull-request review body, so `review_summary` items — the text of a submitted review, as opposed to an ordinary PR conversation comment — have no reaction target. For those, dedupe by scanning existing top-level comments for one whose **reference marker** already cites this author and snippet. In practice most review bots post their findings as an ordinary comment, which does carry a `comment_id`. A PR can carry several independent top-level comments, so a bare reply is ambiguous — **open every top-level reply with a reference marker** identifying the comment you are addressing. Cite the author and the opening of the original, and link it when the item includes a `url`: @@ -212,7 +215,7 @@ A PR can carry several independent top-level comments, so a bare reply is ambigu - 1-2 sentences: what was changed, why it's not an issue, or acknowledgment of declined items. - End every reply with `\n\n*— AI Coding Agent*`. -- Before replying, dedupe against re-loops: for inline threads, check whether the thread already has a reply ending in `*- AI Coding Agent*` / `*— AI Coding Agent*`. For top-level comments the 🎉 reaction handles this — an item carrying `acknowledged: true` (bucketed as `resolved`) has already been answered, so skip it rather than replying twice. +- Before replying, dedupe against re-loops: for inline threads, check whether the thread already has a reply ending in `*- AI Coding Agent*` / `*— AI Coding Agent*`. For a top-level comment with a `comment_id`, the 🎉 reaction handles it — an item carrying `acknowledged: true` (bucketed as `resolved`) has already been answered, so skip it. For a `review_summary` with no `comment_id`, fall back to the reference-marker scan. - If the `gh`/GraphQL call fails, log and continue — do not block the workflow. ### 4. Check CI Status diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.py b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.py index 85d9f9f1..004e63c2 100644 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.py +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.py @@ -147,6 +147,58 @@ def get_review_comments(owner: str, repo: str, pr_number: int) -> list[dict[str, return result if isinstance(result, list) else [] +_VIEWER_LOGIN: str | None = None +_VIEWER_LOOKUP_FAILED = False + + +def get_viewer_login() -> str | None: + """Login of the authenticated gh user, looked up once per run.""" + global _VIEWER_LOGIN, _VIEWER_LOOKUP_FAILED + if _VIEWER_LOGIN is None and not _VIEWER_LOOKUP_FAILED: + # No --jq here: run_gh json.loads() its stdout, and --jq emits a bare + # unquoted string that is not valid JSON. + result = run_gh(["api", "user"]) + login = result.get("login") if isinstance(result, dict) else None + if login: + _VIEWER_LOGIN = login + else: + _VIEWER_LOOKUP_FAILED = True + return _VIEWER_LOGIN + + +def has_our_acknowledgement( + owner: str, repo: str, comment_id: int, hooray_count: int +) -> bool: + """Whether *we* left the acknowledgement reaction on this comment. + + The count on the comment payload is every user's reaction, so a maintainer + celebrating a review would otherwise mark it handled and silently drop real + feedback. Only our own reaction counts. The per-comment lookup is skipped + entirely when the count is zero, so the common case costs nothing. + + Fails closed: if the viewer cannot be identified or the lookup errors, the + item is treated as unacknowledged and resurfaces, which is the safe + direction — answering twice beats dropping feedback. + """ + if hooray_count <= 0: + return False + viewer = get_viewer_login() + if not viewer: + return False + reactions = run_gh([ + "api", + f"repos/{owner}/{repo}/issues/comments/{comment_id}/reactions", + "--paginate", + ]) + if not isinstance(reactions, list): + return False + return any( + r.get("content") == "hooray" + and (r.get("user") or {}).get("login") == viewer + for r in reactions + ) + + def get_issue_comments(owner: str, repo: str, pr_number: int) -> list[dict[str, Any]]: """Get PR conversation comments (includes bot comments).""" result = run_gh([ @@ -468,10 +520,17 @@ def main(): if not body or len(body.strip()) < 3: continue - # A 🎉 reaction is our machine-readable "this was handled" marker for - # top-level comments, which have no thread to resolve. Set it after - # replying; see the skill's "Replying to Comments" section. - acknowledged = comment.get("reactions", {}).get("hooray", 0) > 0 + # Our own 🎉 reaction is the machine-readable "this was handled" marker + # for top-level comments, which have no thread to resolve. Scoped to the + # authenticated user: an unrelated 🎉 from anyone else must not silence + # real feedback. Set it after replying; see the skill's "Replying to + # Comments" section. + acknowledged = has_our_acknowledgement( + owner, + repo, + comment.get("id", 0), + comment.get("reactions", {}).get("hooray", 0), + ) item = extract_feedback_item( body=body,