From d893b8cb31a9b31904a743b1790ddb9dcc4a7b0c Mon Sep 17 00:00:00 2001 From: Yuri Oksuzian Date: Sat, 1 Aug 2026 12:19:20 -0500 Subject: [PATCH 1/5] reviewing-pull-requests: add From Review to Fix section (v1.3.0) New section covering the review-to-fix handoff: implement the full prescription of the finding (replace != add), state deliberate omissions in the PR body, don't treat an established idiom as a keep-reason, and keep minimal-diff about unrelated changes only. Motivated by Offline #1914: the #1908 review prescribed replacing the configure_file staging with an install(DIRECTORY data) rule, but the hotfix PR only added the install rule -- the reviewer had to request the removal the review had already called for. Co-Authored-By: Claude Fable 5 --- skills/reviewing-pull-requests/SKILL.md | 31 +++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/skills/reviewing-pull-requests/SKILL.md b/skills/reviewing-pull-requests/SKILL.md index 2a9567d..6ad94be 100644 --- a/skills/reviewing-pull-requests/SKILL.md +++ b/skills/reviewing-pull-requests/SKILL.md @@ -3,8 +3,8 @@ name: reviewing-pull-requests description: Perform effective code review for Mu2e pull requests. Use when reviewing PRs, assessing risk, checking cross-repo impacts, validating tests/builds, and producing actionable reviewer feedback with severity and evidence. compatibility: Requires git access, Mu2e offline context, and ability to run targeted checks when needed metadata: - version: "1.2.0" - last-updated: "2026-03-06" + version: "1.3.0" + last-updated: "2026-08-01" --- # Reviewing Pull Requests @@ -295,3 +295,30 @@ For PRs touching `.fcl` composition, include checks that: - dotted epilog overrides resolve as expected, - include resolution via `FHICL_FILE_PATH` is valid, - `fhicl-dump -a` provenance confirms final values. + +--- + +## From Review to Fix + +When authoring a patch that implements a finding from one of your own +reviews (yours or a colleague's): + +1. **Re-read the finding's full prescription before writing the patch.** + Implement all of it. A review that says "replace X with Y" is not + implemented by "add Y" β€” the removal of X was part of the finding. +2. **State deliberate omissions in the PR body.** If you intentionally + narrow the fix (hotfix urgency, risk control), write "deliberately + not touching X because ..." so the reviewer sees a decision, not an + oversight. Silent deltas between the review and the patch cost a + review round-trip at best. +3. **"Established idiom" is not a keep-reason.** Discovering that a + questionable line follows a repo-wide idiom explains its origin, not + its necessity. Check whether sibling packages pair the idiom with the + thing your fix adds; if none do, the idiom line is redundant in your + patch and should go (case study: `configure_file(${CURRENT_BINARY_DIR})` + staging removed alongside `install(DIRECTORY data ...)` in Offline + PR #1914 β€” the reviewer had to request what the original review + already prescribed). +4. **Minimal diff means no unrelated changes** β€” it does not mean + dropping in-scope parts of the prescription that touch adjacent + lines. From 78142bcdb3ea7c3a6a4babe07b6942699da2f3d9 Mon Sep 17 00:00:00 2001 From: Yuri Oksuzian Date: Sat, 1 Aug 2026 12:24:07 -0500 Subject: [PATCH 2/5] reviewing-pull-requests: colored-circle severity markers (v1.4.0) Map the S0-S3 scale onto GitHub-native colored circles for at-a-glance scanning: red S0, orange S1, yellow S2, white S3, green for verified-correct material and the approve decision. Findings lead with circle + tag; the Decision line carries red/yellow/green. Co-Authored-By: Claude Fable 5 --- skills/reviewing-pull-requests/SKILL.md | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/skills/reviewing-pull-requests/SKILL.md b/skills/reviewing-pull-requests/SKILL.md index 6ad94be..39e585c 100644 --- a/skills/reviewing-pull-requests/SKILL.md +++ b/skills/reviewing-pull-requests/SKILL.md @@ -3,7 +3,7 @@ name: reviewing-pull-requests description: Perform effective code review for Mu2e pull requests. Use when reviewing PRs, assessing risk, checking cross-repo impacts, validating tests/builds, and producing actionable reviewer feedback with severity and evidence. compatibility: Requires git access, Mu2e offline context, and ability to run targeted checks when needed metadata: - version: "1.3.0" + version: "1.4.0" last-updated: "2026-08-01" --- @@ -192,10 +192,19 @@ Reviewer check: ## Severity Levels -- **S0 Blocker**: incorrect behavior, data corruption, crash, invalid configuration, or missing required cross-repo change. -- **S1 Major**: high-likelihood bug/regression or incomplete validation for risky change. -- **S2 Minor**: maintainability/readability issue with low immediate risk. -- **S3 Nit**: style/format/comment-only suggestion. +Pair each severity tag with a colored circle so findings scan at a glance +(GitHub renders these natively): + +- πŸ”΄ **S0 Blocker**: incorrect behavior, data corruption, crash, invalid configuration, or missing required cross-repo change. +- 🟠 **S1 Major**: high-likelihood bug/regression or incomplete validation for risky change. +- 🟑 **S2 Minor**: maintainability/readability issue with low immediate risk. +- βšͺ **S3 Nit**: style/format/comment-only suggestion. +- 🟒 marks verified-correct material and the approve decision β€” use it on + "checked, no action needed" items so green sections read as cleared, + not skipped. + +Lead every finding title with its circle + tag (`🟠 [S1] ...`) and the +Decision line with πŸ”΄ (request changes), 🟑 (comment), or 🟒 (approve). Only raise severity when evidence supports it. @@ -231,18 +240,18 @@ Use these concise prompts: ### PR Review Summary **Decision** -- +- <🟒 approve | πŸ”΄ request changes | 🟑 comment only> **Scope understood** - <1-3 bullets> **Findings** -1. [S0|S1|S2|S3] +1. <πŸ”΄|🟠|🟑|βšͺ> [S0|S1|S2|S3] <title> - Evidence: <file/behavior/command> - Impact: <why it matters> - Suggested fix: <concrete change> -2. [Sx] ... +2. <circle> [Sx] ... **Validation check** - Build/tests run: <yes/no + commands> From e24754668d4af038bc163be4b090aecfbce3659f Mon Sep 17 00:00:00 2001 From: Yuri Oksuzian <oksuzian@gmail.com> Date: Sat, 1 Aug 2026 12:44:08 -0500 Subject: [PATCH 3/5] reviewing-pull-requests: re-reviews and carry-forward (v1.5.0) When a prior review exists (staged, posted, or another reviewer's), every prior finding must be accounted for in the new review: FIXED (verified at the same evidence bar, moved to green as 'was Sx'), UNADDRESSED (carried forward at original severity, leading the list for red/orange), PARTIAL (remaining part named), or WITHDRAWN (new evidence stated). Prior findings never silently vanish; untouched files keep their findings open by definition. Motivated by the Offline #1908 sequence: the carried-forward S1 (missing CMake data install) survived two re-review rounds unaddressed and then materialized as the spack ceSimReco failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --- skills/reviewing-pull-requests/SKILL.md | 32 ++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/skills/reviewing-pull-requests/SKILL.md b/skills/reviewing-pull-requests/SKILL.md index 39e585c..c33c099 100644 --- a/skills/reviewing-pull-requests/SKILL.md +++ b/skills/reviewing-pull-requests/SKILL.md @@ -3,7 +3,7 @@ name: reviewing-pull-requests description: Perform effective code review for Mu2e pull requests. Use when reviewing PRs, assessing risk, checking cross-repo impacts, validating tests/builds, and producing actionable reviewer feedback with severity and evidence. compatibility: Requires git access, Mu2e offline context, and ability to run targeted checks when needed metadata: - version: "1.4.0" + version: "1.5.0" last-updated: "2026-08-01" --- @@ -307,6 +307,36 @@ For PRs touching `.fcl` composition, include checks that: --- +## Re-Reviews and Carry-Forward + +When a review of this PR already exists β€” a staged file in +`~/pr_reviews/pr<N>_review.md`, a posted review, or another reviewer's +change requests β€” load it BEFORE reviewing, and account for every prior +finding in the new review. Prior findings never silently vanish. + +Classify each prior finding against the new head: + +- **FIXED** β€” verify the fix at the same evidence bar as a new finding + (do not trust the commit message; an author's "fixed" commit can + implement half the prescription β€” see From Review to Fix). Then move + it to the verified section as `🟒 [was Sx] ... β€” FIXED in <sha>, + verified`. +- **UNADDRESSED** β€” carry forward at the original severity, marked + "carried over". Unresolved πŸ”΄/🟠 lead the findings list of the new + review. +- **PARTIAL** β€” state exactly which part remains open; keep the + original severity unless the remaining part is genuinely lower. +- **WITHDRAWN** β€” only with new evidence; state explicitly what changed + the assessment. + +Scope of the re-review pass: diff `<previously-reviewed-head>..<new +head>` β€” but re-verify any prior finding whose evidence the delta +touches, and any prior finding in files the delta did NOT touch stays +open by definition. Include other reviewers' requested changes in the +accounting (addressed or not), not just your own. + +--- + ## From Review to Fix When authoring a patch that implements a finding from one of your own From 655a1fc16028995db964a428b111ef83f55c16d0 Mon Sep 17 00:00:00 2001 From: Yuri Oksuzian <oksuzian@gmail.com> Date: Sat, 1 Aug 2026 20:52:08 -0500 Subject: [PATCH 4/5] reviewing-pull-requests: simplification/efficiency review dimension (v1.6.0) New review priority 6: report (not apply) simplify/optimize opportunities -- dead or write-only state, duplication with an existing single home, work at the wrong cadence, complexity without payoff. Severity capped at S2/S3 and explicitly never gates approval, per the existing 'avoid requiring unrelated cleanup' rule. Grounded in practice: the write-only mapPhysVol_ set rebuilt every subrun (Offline #1908) and the cluster.diskID() simplification that also removed a failure mode (Offline #1911). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --- skills/reviewing-pull-requests/SKILL.md | 30 ++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/skills/reviewing-pull-requests/SKILL.md b/skills/reviewing-pull-requests/SKILL.md index c33c099..3d74250 100644 --- a/skills/reviewing-pull-requests/SKILL.md +++ b/skills/reviewing-pull-requests/SKILL.md @@ -3,7 +3,7 @@ name: reviewing-pull-requests description: Perform effective code review for Mu2e pull requests. Use when reviewing PRs, assessing risk, checking cross-repo impacts, validating tests/builds, and producing actionable reviewer feedback with severity and evidence. compatibility: Requires git access, Mu2e offline context, and ability to run targeted checks when needed metadata: - version: "1.5.0" + version: "1.6.0" last-updated: "2026-08-01" --- @@ -90,6 +90,34 @@ using Parameters = art::EDProducer::Table<Config>; - Is naming clear and consistent with nearby code? - Are assumptions documented where non-obvious? +### 6) Simplification and efficiency (never gates approval) + +Review the changed code through a simplify/optimize lens and report β€” +do not apply β€” opportunities as findings: + +- **Dead or write-only state**: members, config knobs, or containers + written but never read; per-event or per-subrun work whose product + nothing consumes. +- **Duplication with an existing single home**: logic re-implemented + where a shared helper, accessor, or prolog table already exists β€” + grep for the existing home before suggesting a new one. +- **Work at the wrong cadence**: per-event computation of + subrun/job-constant values; repeated lookups hoistable out of hot + loops. +- **Complexity without payoff**: a simpler idiom with identical + behavior β€” especially one that derives a value from an invariant + instead of reconstructing it through a fragile chain. + +Severity cap: 🟑 [S2] when the complexity hides risk or real cost, +βšͺ [S3] otherwise. These findings NEVER gate the decision β€” "avoid +requiring unrelated cleanup for approval" applies in full; a review +that is otherwise 🟒 approve stays 🟒. + +Examples from practice: a write-only `std::set` rebuilt every subrun +after its only reader was removed (Offline #1908); replacing a fragile +positional cluster↔MC pairing with `cluster.diskID()` β€” simpler AND +removes the failure mode (Offline #1911). + --- ## Rules From fe5725545968bb6ee886d5620d259c93f5477b40 Mon Sep 17 00:00:00 2001 From: Yuri Oksuzian <oksuzian@gmail.com> Date: Sat, 1 Aug 2026 20:56:57 -0500 Subject: [PATCH 5/5] reviewing-pull-requests: GitHub is the canonical prior-review source (v1.6.1) Addresses review feedback: the carry-forward contract now reads prior findings from the reviews/comments posted on the PR itself (GitHub-to-GitHub, works for any reviewer on any machine). The local ~/pr_reviews draft is demoted to a personal scratch convention, consulted only for drafted-but-unposted reviews, and its location is configurable via PR_REVIEW_DIR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --- skills/reviewing-pull-requests/SKILL.md | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/skills/reviewing-pull-requests/SKILL.md b/skills/reviewing-pull-requests/SKILL.md index 3d74250..494635d 100644 --- a/skills/reviewing-pull-requests/SKILL.md +++ b/skills/reviewing-pull-requests/SKILL.md @@ -3,7 +3,7 @@ name: reviewing-pull-requests description: Perform effective code review for Mu2e pull requests. Use when reviewing PRs, assessing risk, checking cross-repo impacts, validating tests/builds, and producing actionable reviewer feedback with severity and evidence. compatibility: Requires git access, Mu2e offline context, and ability to run targeted checks when needed metadata: - version: "1.6.0" + version: "1.6.1" last-updated: "2026-08-01" --- @@ -337,10 +337,21 @@ For PRs touching `.fcl` composition, include checks that: ## Re-Reviews and Carry-Forward -When a review of this PR already exists β€” a staged file in -`~/pr_reviews/pr<N>_review.md`, a posted review, or another reviewer's -change requests β€” load it BEFORE reviewing, and account for every prior -finding in the new review. Prior findings never silently vanish. +When a review of this PR already exists, load it BEFORE reviewing and +account for every prior finding in the new review. Prior findings never +silently vanish. + +Sources, in order of authority: + +1. **Reviews and comments posted on the PR itself** β€” the canonical + record. This keeps the loop GitHub-to-GitHub: it works for any + reviewer on any machine, and it covers other reviewers' change + requests, not just your own. +2. **A locally staged draft** β€” consulted only for a review drafted but + not yet posted. The draft location is a personal scratch convention, + not part of the shared workflow: default `~/pr_reviews/pr<N>_review.md`, + overridable with `$PR_REVIEW_DIR`. Once a review is posted, the PR + carries the record and the local draft is disposable. Classify each prior finding against the new head: