Skip to content

Commit f70efb5

Browse files
will-lamertonclaude
andcommitted
feat(nc-review): mention the author, and track what a re-review fixed
Two changes. Mention the PR author, so the review reaches them instead of waiting to be noticed. Bots are skipped — dependabot and github-actions cannot read a review and an @-mention on every dependency bump is noise. Detection uses the API's is_bot rather than matching a "[bot]" suffix, which would miss accounts that do not follow the convention. Feed the previous verdict back in on /re-review. Without it the agent re-derives every finding from scratch and re-files ones the author has already fixed, which is worse than not re-reviewing at all: it tells someone their work did not land. The most recent nc-review comment is now included in the context, with instructions to sort each prior point into resolved / not resolved / cannot tell, and a new `addressed` array in the schema. Fixed points are rendered as an explicit "Addressed since the last review" block rather than silently dropped. Absence is ambiguous — from the author's side it reads as the bot forgetting rather than the work landing. The rubric also says plainly that re-raising a fixed point is the fastest way to make people stop reading the reviews, and that a fix can introduce a new problem which is a new finding like any other. Verified by extracting the real Post review step and running it against three cases: a re-review with three addressed items and one remaining finding, a bot author (no mention emitted), and a first review with no `addressed` field (no empty block). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PZY52ePXLjwG9TaQgq2cHT
1 parent 82079ba commit f70efb5

2 files changed

Lines changed: 92 additions & 1 deletion

File tree

.github/nc-review/rubric.md

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,28 @@ Derived mechanically from the findings — do not set it by feel:
238238
fixing is **not** clean, even if none of them block the merge — labelling it
239239
clean tells a maintainer to skim past findings you spent the run producing.
240240

241+
## If you have reviewed this pull request before
242+
243+
The context may contain your previous verdict. When it does, the author has
244+
probably been working from it, and the diff you are looking at is the current
245+
state — fixes included.
246+
247+
Go through what you said last time and place each point:
248+
249+
- **Resolved** → list it in `addressed`, phrased so the author recognises it
250+
("the dead `realRename` capture is gone"). Do **not** re-file it in
251+
`findings`.
252+
- **Not resolved** → file it in `findings` again, saying what is still
253+
outstanding rather than repeating your original wording word for word.
254+
- **Cannot tell** → say so in the summary. Do not guess in either direction.
255+
256+
Re-raising something the author has already fixed is the fastest way to make
257+
people stop reading you. Silently dropping it is nearly as bad — from the
258+
author's side that is indistinguishable from you forgetting. Say what landed.
259+
260+
Judge the current diff on its own merits as well: a fix can introduce a new
261+
problem, and that is a new finding like any other.
262+
241263
## Output
242264

243265
**The file is the entire deliverable.** Anything you write in chat is discarded
@@ -262,7 +284,8 @@ before or after, no markdown fences. Schema:
262284
"detail": "Specific and actionable. What is wrong, why it matters, and what would fix it."
263285
}
264286
],
265-
"duplicate_of": null
287+
"duplicate_of": null,
288+
"addressed": []
266289
}
267290
```
268291

@@ -284,3 +307,6 @@ before or after, no markdown fences. Schema:
284307
Lead with what is wrong, then why it matters, then what would fix it. Two or
285308
three sentences is usually right; go longer only when quoting code earns it.
286309
- `duplicate_of` — PR number as an integer, or `null`. Only when confident.
310+
- `addressed` — short strings, one per point from your previous review that is
311+
now resolved. Omit or leave empty on a first review. Never list something here
312+
and in `findings`.

.github/workflows/nc-review.yml

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,18 @@ jobs:
176176
| jq --argjson me "$PR" 'map(select(.number != $me))' \
177177
> /tmp/nc-review/open-prs.json
178178
179+
# The most recent previous nc-review verdict on this pull request, if
180+
# any. Without it a /re-review re-derives every finding from scratch
181+
# and re-files ones the author has already fixed, which is worse than
182+
# not re-reviewing at all — it tells someone their work did not land.
183+
gh api "repos/$REPO/issues/$PR/comments?per_page=100" --paginate \
184+
--jq '[.[] | select(.user.login == "github-actions[bot]")
185+
| select(.body | startswith("### nc-review"))] | last // empty' \
186+
> /tmp/nc-review/prev.json 2>/dev/null || : > /tmp/nc-review/prev.json
187+
HAS_PREV=0
188+
[ -s /tmp/nc-review/prev.json ] && HAS_PREV=1
189+
echo "previous review found: $HAS_PREV"
190+
179191
# Issues this PR claims to close, with their full bodies — the agent
180192
# cannot judge whether the issue was actually addressed without them.
181193
# Only GraphQL exposes closingIssuesReferences; `gh pr view --json`
@@ -246,6 +258,29 @@ jobs:
246258
/tmp/nc-review/issues.json
247259
fi
248260
echo
261+
if [ "$HAS_PREV" = "1" ]; then
262+
echo "## Your previous review of this pull request"
263+
echo
264+
echo "You have reviewed this pull request before. The diff below is the CURRENT"
265+
echo "state, which may already include fixes made in response to what you said."
266+
echo
267+
echo "For each point you raised last time, work out whether it is now resolved:"
268+
echo
269+
echo "- **Resolved** — list it in the \`addressed\` array. Do NOT put it in"
270+
echo " \`findings\` again. Re-raising a fixed point tells the author their work"
271+
echo " did not land, and is the fastest way to make people stop reading you."
272+
echo "- **Not resolved** — file it again in \`findings\`, and say briefly what is"
273+
echo " still outstanding rather than repeating the original wording verbatim."
274+
echo "- **Cannot tell** — say so in the summary. Do not guess in either direction."
275+
echo
276+
echo "Judge the current diff on its own merits too: a fix can introduce a new"
277+
echo "problem, and that is a new finding."
278+
echo
279+
echo "\`\`\`markdown"
280+
jq -r '.body' /tmp/nc-review/prev.json
281+
echo "\`\`\`"
282+
echo
283+
fi
249284
echo "## Other open pull requests (duplicate-detection corpus)"
250285
echo '```json'
251286
cat /tmp/nc-review/open-prs.json
@@ -440,6 +475,18 @@ jobs:
440475
fi
441476
442477
SUMMARY=$(jq -r '.summary // "No summary provided."' "$V")
478+
479+
# Mention the author so the review reaches them rather than waiting to
480+
# be noticed. Bots are skipped: dependabot and github-actions cannot
481+
# read a review, and @-mentioning them is noise on every dependency
482+
# bump. `is_bot` is authoritative here — matching on a "[bot]" suffix
483+
# would miss accounts that do not use the convention.
484+
AUTHOR=$(gh pr view "$PR" --repo "$REPO" --json author --jq '.author.login' 2>/dev/null || echo "")
485+
IS_BOT=$(gh pr view "$PR" --repo "$REPO" --json author --jq '.author.is_bot' 2>/dev/null || echo "true")
486+
MENTION=""
487+
if [ -n "$AUTHOR" ] && [ "$IS_BOT" != "true" ]; then
488+
MENTION="@${AUTHOR}"
489+
fi
443490
DUP=$(jq -r '.duplicate_of // empty' "$V")
444491
445492
N_BLOCK=$(jq '[.findings[]? | select(.severity == "blocking")] | length' "$V")
@@ -493,8 +540,26 @@ jobs:
493540
clean) echo "### nc-review: nothing to raise" ;;
494541
esac
495542
echo
543+
if [ -n "$MENTION" ]; then
544+
case "$VERDICT" in
545+
needs-work) echo "$MENTION — there is a blocking item below." ;;
546+
comments) echo "$MENTION — a few things worth a look, none blocking." ;;
547+
clean) echo "$MENTION — nothing to raise from the automated review." ;;
548+
esac
549+
echo
550+
fi
496551
echo "$SUMMARY"
497552
echo
553+
# Fixed points are stated explicitly rather than silently dropped.
554+
# On a re-review, "what did I actually fix" is the first thing the
555+
# author wants to know, and absence is ambiguous — it reads as the
556+
# bot forgetting rather than the work landing.
557+
if [ "$(jq '(.addressed // []) | length' "$V")" -gt 0 ]; then
558+
echo "**Addressed since the last review**"
559+
echo
560+
jq -r '(.addressed // [])[] | "- ✅ \(.)"' "$V"
561+
echo
562+
fi
498563
if [ -n "$DUP" ]; then
499564
echo "> **Possible duplicate of #${DUP}** — worth checking before going further."
500565
echo

0 commit comments

Comments
 (0)