Skip to content

Keep address-review triage aligned with latest actionable feedback #2798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
justin808 merged 1 commit into from
Mar 21, 2026
Merged

Keep address-review triage aligned with latest actionable feedback #2798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions .agents/workflows/address-review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ Execution flow when terminal access is available:

4. Filter comments:
- Skip resolved threads.
- Skip replies where `in_reply_to_id` is set.
- Do not create standalone triage items from comments where `in_reply_to_id` is set, but use reply text as the latest thread context when it updates or narrows the unresolved concern.

@claude claude Bot Mar 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanics of "use reply text as the latest thread context" are left implicit. An agent executing this step may not know whether to:

  • Append the reply to the parent comment's body before triaging
  • Replace the parent's body with the reply
  • Just note the reply alongside the parent

A more concrete formulation might help:

Suggested change
- Do not create standalone triage items from comments where `in_reply_to_id` is set, but use reply text as the latest thread context when it updates or narrows the unresolved concern.
- Do not create standalone triage items from comments where `in_reply_to_id` is set; instead, prepend the most recent reply text to the parent comment's body before triaging, so the triage reflects the current state of the concern rather than only the original wording.

Also, the qualifier "when it updates or narrows the unresolved concern" leaves two edge cases unaddressed:

  1. A reply that withdraws the concern ("never mind, I see it now") — should the thread be auto-skipped?
  2. A reply that broadens the concern — the current wording would still fold it in, but it would be good to say so explicitly.

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fetch reply ordering before using “latest” thread context

This new rule relies on reply text to represent the current unresolved concern, but the workflow still fetches review comments without any created_at/updated_at field or other per-thread ordering metadata. In threads with multiple replies, the agent cannot tell whether it is looking at the reviewer’s latest narrowed concern or an older reply, so it can triage stale context instead of the thread’s actual remaining issue.

Useful? React with 👍 / 👎.

- Keep bot comments by default, but deduplicate duplicates and skip status-only bot posts.
- Focus on correctness bugs, regressions, security issues, missing tests that hide bugs, and clear adjacent-code inconsistencies.
- Skip style nits, speculative suggestions, documentation nits, changelog wording, duplicate comments, and "could consider" feedback unless I ask for polish work.
Expand All @@ -77,11 +77,12 @@ Execution flow when terminal access is available:
5. Triage every remaining comment:
- `MUST-FIX`: correctness bugs, regressions, security issues, missing tests that could hide a bug, and clear inconsistencies with adjacent code that would likely block merge.
- `DISCUSS`: reasonable scope-expanding suggestions, architectural opinions, and comments that need a decision.
- `SKIPPED`: style preferences, documentation nits, comment requests, test-shape preferences, speculative suggestions, changelog wording, duplicate comments, status posts, summaries, and factually incorrect suggestions.
- `SKIPPED`: style preferences, documentation nits, comment requests, test-shape preferences, speculative suggestions, changelog wording, duplicate comments, status posts, non-actionable summaries, and factually incorrect suggestions.
- Deduplicate overlapping comments before classifying them.
- Verify reviewer claims locally before calling something `MUST-FIX`.
- If a claim is wrong, classify it as `SKIPPED` and say why.
- Preserve comment IDs and thread IDs for later replies and thread resolution.
- Treat actionable review summary bodies as normal feedback to classify (`MUST-FIX`/`DISCUSS` as appropriate); skip only boilerplate or status-only summaries.

@claude claude Bot Mar 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new bullet was added after Preserve comment IDs and thread IDs for later replies and thread resolution. Functionally it fits, but it reads slightly out of sequence: the rule about what to classify (actionable summaries as MUST-FIX/DISCUSS) would be easier to follow if it appeared before the subject-format rule at the bottom. Minor ordering nit only.

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude superseded review summaries from MUST-FIX triage

Treating review summary bodies as normal actionable feedback becomes problematic because step 3 still pulls every non-empty body from /pulls/{PR_NUMBER}/reviews, including summaries from earlier review rounds. On a PR that had an early summary requesting changes and a later approval after fixes, this instruction will resurface the stale summary as a fresh MUST-FIX/DISCUSS item even though the workflow has no thread-resolution signal for review bodies. That makes the triage checklist regress to already-addressed feedback.

Useful? React with 👍 / 👎.

- Track only `MUST-FIX` items as your working checklist.
- Use one checklist entry per must-fix item or deduplicated issue.
- Use the subject format: `"{file}:{line} - {comment_summary} (@{username})"`.
Expand Down
5 changes: 3 additions & 2 deletions .claude/commands/address-review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ gh api graphql --paginate -f owner="${OWNER}" -f name="${NAME}" -F pr={PR_NUMBER
**Filtering comments:**

- Skip comments belonging to already-resolved threads (match via `thread_id` and `is_resolved` from the GraphQL response)
- Skip comments where `in_reply_to_id` is set (these are replies, not top-level comments)
- Do not create standalone triage items from comments where `in_reply_to_id` is set, but use reply text as the latest thread context when it updates or narrows the unresolved concern

@cursor cursor Bot Mar 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filtering section still skips all summaries before triage

Medium Severity

The filtering section (line 87) tells the AI to skip "summaries" during comment filtering, but the new triage rule (line 112) says to classify actionable summaries as normal feedback. This creates an internal contradiction — actionable summaries get discarded in filtering before reaching triage. The corresponding line in .agents/workflows/address-review.md says only "skip status-only bot posts" (no mention of summaries), so the two files are not aligned despite the PR's stated goal of mirroring behavior.

Additional Locations (1)
Fix in Cursor Fix in Web

- Do not skip bot-generated comments by default. Many actionable review comments in this repository come from bots.
- Deduplicate repeated bot comments and skip bot status posts, summaries, and acknowledgments that do not require a code or documentation change
- Treat as actionable by default only: correctness bugs, regressions, security issues, missing tests, and clear inconsistencies with adjacent code
Expand All @@ -101,14 +101,15 @@ Before creating any todos, classify every review comment into one of three categ

- `MUST-FIX`: correctness bugs, regressions, security issues, missing tests that could hide a real bug, and clear inconsistencies with adjacent code that would likely block merge
- `DISCUSS`: reasonable suggestions that expand scope, architectural opinions that are not clearly right or wrong, and comments where the reviewer claim may be correct but needs a user decision
- `SKIPPED`: style preferences, documentation nits, comment requests, test-shape preferences, speculative suggestions, changelog wording, duplicate comments, status posts, summaries, and factually incorrect suggestions
- `SKIPPED`: style preferences, documentation nits, comment requests, test-shape preferences, speculative suggestions, changelog wording, duplicate comments, status posts, non-actionable summaries, and factually incorrect suggestions

Triage rules:

- Deduplicate overlapping comments before classifying them. Keep one representative item for the underlying issue.
- Verify factual claims locally before classifying a comment as `MUST-FIX`.
- If a claim appears wrong, classify it as `SKIPPED` and note briefly why.
- Preserve the original review comment ID and thread ID when available so the command can reply to the correct place and resolve the correct thread later.
- Treat actionable review summary bodies as normal feedback to classify (`MUST-FIX`/`DISCUSS` as appropriate); skip only boilerplate or status-only summaries.

## Step 5: Create Todo List

Expand Down
Loading