skills: reviewing-pull-requests can post its own review and trigger CI (v1.8.0) - #12
Open
oksuzian wants to merge 2 commits into
Open
skills: reviewing-pull-requests can post its own review and trigger CI (v1.8.0)#12oksuzian wants to merge 2 commits into
oksuzian wants to merge 2 commits into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lets
reviewing-pull-requestsfinish 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-reviewpublishes, exactly as today.Auto-post activates only when
PR_REVIEW_AUTOPOSTis 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-requests1.6.1 → 1.7.0 — new "Publishing the Review" section: the opt-in check, four fail-closed gates (naming, staleness, decision mapping, duplicate), thegh pr reviewinvocation, and a "When NOT to post" list. The body is posted verbatim and never rewritten at post time.post-pr-review1.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 nameMu2e/<repo>#<N>. PR numbers collide across repos —Offline#7andArtAnalysis#7both wantpr7_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-requests1.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:
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
OfflineandProduction. 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