Skip to content

skills: reviewing-pull-requests can post its own review and trigger CI (v1.8.0) - #12

Open
oksuzian wants to merge 2 commits into
mainfrom
skill-autopost-optin
Open

skills: reviewing-pull-requests can post its own review and trigger CI (v1.8.0)#12
oksuzian wants to merge 2 commits into
mainfrom
skill-autopost-optin

Conversation

@oksuzian

@oksuzian oksuzian commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Lets reviewing-pull-requests finish the job — post the review to the PR — instead of stopping at a staged draft that needs a second command. Opt-in, off by default.

Why opt-in

Publishing writes to a colleague's PR under the invoking user's name. That is not something a skill update should switch on for someone, so nothing changes for anyone who does not ask for it: the skill drafts, reports the path, and /post-pr-review publishes, exactly as today.

Auto-post activates only when PR_REVIEW_AUTOPOST is set in the environment, or when the request itself says to post — an explicit "review and post", or a scheduled sweep whose prompt says so.

What is in it

  • reviewing-pull-requests 1.6.1 → 1.7.0 — new "Publishing the Review" section: the opt-in check, four fail-closed gates (naming, staleness, decision mapping, duplicate), the gh pr review invocation, and a "When NOT to post" list. The body is posted verbatim and never rewritten at post time.
  • post-pr-review 1.0.1 → 1.1.0 — cross-references the new section; still the normal publishing path for anyone who has not opted in.

The naming gate is worth a look even if you never enable auto-post

Draft filenames are now repo-qualified (<repo>_pr<N>_review.md) and the draft's first line must name Mu2e/<repo>#<N>. PR numbers collide across repos — Offline#7 and ArtAnalysis#7 both want pr7_review.md — and the naming gate is what stops a review landing on the wrong PR. This bit me in practice before the gate existed.

Validation

Exercised across four review rounds on Mu2e/EventNtuple#381 today (🔴 → 🔴 → 🟡 → 🟢) plus ArtAnalysis#7, Offline#1917/#1918/#1919, MLTrain#8 and an EventDisplay release tag. The gates fired as intended: the staleness gate caught a head that moved mid-review, and the "no PR to post to" case correctly left the EventDisplay tag review staged.

Not validated: the PR_REVIEW_AUTOPOST-unset path has not been run end to end, since my environment sets it.


Added: FNALbuild CI trigger (reviewing-pull-requests 1.7.0 → 1.8.0)

Second commit, same theme — which GitHub writes this skill is allowed to make. A review that turns on "does it build" is worthless without a build at the head being reviewed, so the skill may now ask for one:

gh pr comment <N> --repo Mu2e/<repo> --body "@FNALbuild run build test"

That is the only additional write authorized. It explicitly does not extend to gh pr edit, gh pr merge, gh pr close, git push, or any other comment.

Scoped to Offline and Production. FNALbuild watches neither of the other seven skill-scope repos — ~1580 comments on Offline and ~449 on Production, against zero on EventNtuple, EventDisplay, DQM, Tutorial, PassN, RefAna and ArtAnalysis. Posting the phrase elsewhere is a no-op comment on someone's PR, so the section says to check the repo first.

The guards matter more than the trigger, since build slots are shared collaboration-wide. Do not fire when a run is already queued at the current head, when the head is already green, or when the failure is genuinely caused by the diff — re-running reproduces it, so report the failing target as a finding instead. An invoking instruction that forbids state-changing commands (a scheduled sweep, a read-only brief) wins over the section; the review then records needs a CI run at <sha8> and leaves it to a human.

Validated by the guard firing correctly on Mu2e/Offline#1918: head had moved to faffc80f, but a build was already in flight there, so the correct action was to not trigger.

🤖 Generated with Claude Code

oksuzian and others added 2 commits August 7, 2026 18:14
reviewing-pull-requests gains a "Publishing the Review" section so a
review can end with the review actually on the PR, rather than at a
staged draft that needs a second command.

Auto-post is opt-in and OFF by default. Publishing writes to a
colleague's PR under the invoking user's name, which is not a behaviour
a skill update should switch on for someone. It activates only when
PR_REVIEW_AUTOPOST is set, or when the request itself says to post (an
explicit "review and post", or a scheduled sweep whose prompt says so).
Without it the skill behaves exactly as before: draft, report the path,
publish via /post-pr-review.

Posting is fail-closed behind four gates -- naming, staleness, decision
mapping, duplicate -- and the body is posted verbatim, never rewritten
at post time.

The naming gate also now requires repo-qualified draft filenames
(<repo>_pr<N>_review.md) and a Mu2e/<repo>#<N> header. PR numbers
collide across repos: Offline#7 and ArtAnalysis#7 both want
pr7_review.md, and the naming gate is what stops a review landing on
the wrong PR.

post-pr-review is updated to cross-reference the new section and to
state that it remains the normal publishing path for anyone who has not
opted in.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….8.0)

A review that turns on "does it build" needs a build at the head being
reviewed. Adds a "Triggering a CI Build" section authorizing exactly one
additional write beyond the review itself:

    gh pr comment <N> --repo Mu2e/<repo> --body "@FNALbuild run build test"

Scoped to Offline and Production only. FNALbuild watches neither of the
other seven skill-scope repos -- ~1580 comments on Offline and ~449 on
Production against zero on EventNtuple, EventDisplay, DQM, Tutorial,
PassN, RefAna and ArtAnalysis -- so the phrase elsewhere is a no-op
comment on someone's PR rather than a trigger.

Guards against wasting a shared build slot: do not fire when a run is
already queued at the current head, when the head is already green, or
when the failure is genuinely caused by the diff (re-running reproduces
it; report the failing target as a finding instead). An invoking
instruction that forbids state-changing commands -- a scheduled sweep,
a read-only brief -- wins over the section, in which case the review
records "needs a CI run at <sha8>" for a human to act on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@oksuzian oksuzian changed the title skills: opt-in auto-post for PR reviews (rpr v1.7.0, ppr v1.1.0) skills: reviewing-pull-requests can post its own review and trigger CI (v1.8.0) Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant