Skip to content

feat(nc-review): mention the author, and track what a re-review fixed - #1217

Merged
will-lamerton merged 1 commit into
mainfrom
feat/nc-review-mention-author
Sep 6, 2026
Merged

feat(nc-review): mention the author, and track what a re-review fixed#1217
will-lamerton merged 1 commit into
mainfrom
feat/nc-review-mention-author

Conversation

@will-lamerton

Copy link
Copy Markdown
Member

Two changes.

1. Mention the PR author

So the review reaches them rather than waiting to be noticed.

Bots are skipped. dependabot and github-actions cannot read a review, and an @-mention on every dependency bump is pure noise. Detection uses the API's is_bot flag rather than matching a [bot] suffix, which would miss accounts that do not follow the convention.

2. Track what a /re-review actually fixed

Previously a re-review re-derived every finding from scratch, with no memory of what it had already said. That means re-filing points the author has already fixed — which is worse than not re-reviewing at all, because 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, plus a new addressed array in the schema.

Fixed points render explicitly rather than being silently dropped — absence is ambiguous, and from the author's side reads as the bot forgetting rather than the work landing:

nc-review: comments — 1 important

@awhite0030 — a few things worth a look, none blocking.

The dispatcher gap is fixed. One test concern remains.

Addressed since the last review

  • ✅ The dispatcher now has a skill branch, so subscriptions no longer fall through silently.
  • fix.patch and the two .orig files are gone from the diff.
  • ✅ A changeset was added.

🟠 important · tests · source/skills/registrar.spec.ts

Still no end-to-end test that a skill: subscription reaches the dispatcher.

The rubric also now says plainly that re-raising a fixed point is the fastest way to make people stop reading, and that a fix can introduce a new problem which is a new finding like any other.

Verified

Extracted the real Post review step and ran it against three cases:

  • re-review with three addressed items and one remaining finding → renders as above
  • bot author → no mention emitted
  • first review with no addressed field → no empty block

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
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

No changeset found

This PR does not add a changeset, so it will not appear in the changelog or trigger a release.

If the change is user-facing, add one:

pnpm changeset

Pick a bump (patch / minor / major) and write the changelog entry in our usual voice ("Added X... Thanks to @you. Closes #123."), then commit the generated .changeset/*.md file.

If this PR is docs-only or a chore that needs no release note, you can ignore this — or run pnpm changeset --empty to record that intentionally.

@github-actions github-actions Bot added the area:ci GitHub Actions and CI label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

nc-review: nothing to raise

The change adds an author mention to nc-review comments (skipping bots via the API's is_bot flag) and forwards the most recent nc-review comment to the model on a re-review, paired with a new addressed field in the verdict schema. The shell, jq, and Markdown pieces hang together: the filter select(.body | startswith("### nc-review")) matches the headings the bot itself writes, last // empty picks the right one, and the empty-file fallback (2>/dev/null || : > ... plus [ -s ... ]) keeps HAS_PREV honest when the API fails. The author-mention block defaults to skipping on error (IS_BOT=true fallback), which is the safe direction. No security regressions: the previous-review body is filtered to github-actions[bot] only, so it carries no attacker-controlled content into the prompt. Two minor inefficiencies are not worth raising — a second gh pr view for the author rather than reusing /tmp/nc-review/pr.json, and a re-review of a prior clean verdict starts fresh without an explicit note. Ready to merge.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:clean nc-review had nothing to raise label Sep 6, 2026
@will-lamerton
will-lamerton merged commit 08d61c8 into main Sep 6, 2026
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:clean nc-review had nothing to raise area:ci GitHub Actions and CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant