From 329f0381f2e0070e0ef29ecc27971b14c1f08acd Mon Sep 17 00:00:00 2001 From: oksuzian <48927306+oksuzian@users.noreply.github.com> Date: Fri, 14 Aug 2026 11:15:56 -0500 Subject: [PATCH] skills: read FNALbuild's table before verifying anything yourself mu2e/buildtest runs ~20 checks, most with no separate commit-status context. Reading the statuses API alone reports that a check does not exist when it ran and passed -- g4surfaceCheck and rootOverlaps are the cases that matter, since they cover geometry overlaps across the whole geometry. Adds a "What buildtest already covers" section listing what runs inside buildtest, and an Evidence Rules bullet telling the reviewer to find out what CI verified before verifying it themselves. Both say plainly not to hand-roll a substitute and not to ask an author to run a check CI ran. Prompted by a review that asked the author to run surfaceCheck.fcl when CI had already run it green, backed by a home-made scan that silently covered one of the four solid families the geometry loads. Co-Authored-By: Claude Opus 5 (1M context) --- skills/reviewing-pull-requests/SKILL.md | 42 +++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/skills/reviewing-pull-requests/SKILL.md b/skills/reviewing-pull-requests/SKILL.md index faf04e5..2c8a35c 100644 --- a/skills/reviewing-pull-requests/SKILL.md +++ b/skills/reviewing-pull-requests/SKILL.md @@ -153,6 +153,11 @@ Only raise severity when evidence supports it. them on merit. Length is not thoroughness. Watch especially for a verification pass that restates the finding it was asked to check instead of testing it — agreement that adds no evidence is not confirmation. +- **Find out what CI already verified before verifying anything yourself.** + See "What buildtest already covers" — most of those checks have no status + context, so it is easy to conclude a check does not exist when it ran and + passed. Do not hand-roll a substitute for one: it is slower, narrower, and + can be confidently wrong in ways you will not notice. --- @@ -749,6 +754,43 @@ This is the **only** state-changing command besides the review itself that this skill authorizes. It does not extend to `gh pr edit`, `gh pr merge`, `gh pr close`, `git push`, or any other comment. +### What buildtest already covers — read the table, not the statuses + +`mu2e/buildtest` is not one test, it is about twenty, and most have **no +separate commit-status context**. The statuses API returns roughly eleven +contexts; FNALbuild's result table lists the rest. Read the table. Reading +statuses alone will tell you a check does not exist when in fact it ran and +passed. + +Running inside `buildtest`, invisible to the statuses API: + +- **`g4surfaceCheck`** — `mu2e -c Offline/Mu2eG4/fcl/surfaceCheck.fcl`, the + Geant4 overlap check over the entire geometry. Graded on log content, not + exit code: it fails when `grep 'Checking overlaps for volume' | grep -v OK` + is non-empty, with a `LEGAL > 0` guard so a run that checked nothing cannot + pass vacuously. +- **`rootOverlaps`** — an independent overlap check via ROOT. +- `ceMix`, `ceSteps`, `ceDigi`, `muDauSteps`, `check_cmake`, `trigger`. +- the whitespace check, clang-tidy, and the FIXME/TODO count. + +**Do not re-derive what these already prove, and never ask an author to run +one of them.** A geometry PR with `g4surfaceCheck` and `rootOverlaps` green has +been checked across the whole geometry — dirt, detector and rotated volumes +included, at G4's own tolerances. Writing your own analysis of the same +question is slower, narrower, and wrong in ways that do not announce +themselves: a scan that silently drops whole classes of volume still reports +"no overlaps", which reads exactly like verification. This skill has already +shipped a 🟢 claiming "every active hall volume" from a scan that covered one +of the four families the geometry loads. + +If you run an independent check anyway, it is a cross-check of CI, never the +evidence. Say which it is, and do not let it upgrade a finding CI has settled. + +Job lists live in `Mu2e/codetools` +(`bin/github/jenkins_tests/mu2e-offline-build-test/job.sh` — `JOBNAMES` and +`ADDITIONAL_JOBNAMES`); the test vocabulary is `Mu2e/CI` +(`Mu2eCI/test_suites.py`). + ### Where it works **`Offline` and `Production` only.** FNALbuild watches neither