feat(ci): give the review agent this project's toolchain - #196
Merged
Conversation
The retry wrapper added for #125/#164 was written as if a retry would land on a different mirror. It does not. The runner points sources.list at mirror+file:/etc/apt/apt-mirrors.txt, and apt-transport-mirror(1) is explicit on two points: failover to another mirror happens only when a fetch *fails*, and mirrors are tried in ascending `priority:` order. #164's failure mode was a mirror that answered and then served cmake at ~26 KB/s. That is never a failure, so failover never fired, and all three attempts went back to the same priority:1 host. The per-attempt timeout turned one slow mirror into three slow mirrors. Rotate the priority order between attempts so the next one genuinely starts elsewhere. Rewriting priorities is the operative part: line order in the file is not what apt honours. Parsing is tab-field aware. The format is URI, TAB, then metadata, so flattening the line to a string would join the URI and its metadata with a space, which apt then reads as part of the URI. Only the priority token is replaced; arch:/codename:/component: are carried through, since dropping them would widen which files a partial mirror is asked to serve. Best-effort by design: a missing or unwritable mirrorlist (any non-runner environment) leaves retry behaving exactly as before. Verified against a real mirrorlist: rotation walks one host per call and returns to the original head after N calls; comments survive; a URL containing a literal "priority:9" path segment is untouched; extra metadata stays tab-separated; single-mirror and comments-only files do not lose content; an unreadable list is refused rather than emptied. Confirmed separately that all three mirrors in the image serve the full noble archive, so a rotation cannot land on a partial one.
The reviewer runs on a bare ubuntu-latest. On PR #194 it reported exactly three things it could not do: no ruff, no golangci-lint, and a JDK that rejects our release 25. Those are the checks most likely to catch a real defect, so a reviewer restricted to reading code is a real loss of review power, not a cosmetic gap. Upstream PR #30 (agentic-workflow-template) added a `setup_script` input for exactly this. Wire it into pr-review and llmdoc-updater, both pointing at one script. Phases are ordered cheapest first and each is individually time-boxed, so a pathological phase costs only its own capability instead of everything after it: JDK 25 from the runner toolcache (JAVA_HOME_25_X64), no download Go newest toolcache 1.26.x, read from pine-go/go.mod Python tools ruff/pytest in a venv outside the repo golangci-lint v2.12.2, checksum-verified pine-cpp last and largest budget; apt is the one step that has ever failed CI on infrastructure rather than on code Versions are pinned to what CI actually resolved on PR #194's run rather than to what the config appears to say. Two things the hook's contract requires, both verified rather than assumed: The worktree must stay clean, or the job fails. Everything installs to RUNNER_TEMP except the C++ build, which uses pine-cpp/build-tests to match scripts/cpp-test.sh so `make cpp-test` reuses the cache; .gitignore's `pine-cpp/build*/` covers it and CMake's FetchContent cache beneath it. A full local build produces no worktree change. The assertion was mutation-tested: a script that touches one stray file fails with exit 1. Nonzero exit is non-fatal, so the script names the unavailable phases — that message is the agent's only signal about which capability is missing. Two defects here were found only by running it, not by reading it: `timeout` is a separate binary and cannot invoke a shell function, so every phase exited 127 while reporting all capabilities unavailable; and deriving the repo root from BASH_SOURCE pointed at .trusted-base, the sparse checkout holding only this script, so no phase could find pine-go/go.mod. The hook cds to the repo first, so the cwd is correct and is now asserted against three marker files. apt is capped at 300s as a whole rather than per attempt: three 300s attempts across update and install is ~1900s worst case, which would consume the entire 600s phase before cmake ran. Locally measured 9s to configure (including ~70 MB of rapidjson + doctest clones) and 35s to build the test target at -j4. Note this PR's own review does not benefit: pr-review reads the script from the base commit, where it does not exist yet. That path degrades to a warning, verified against the real hook. The gain starts next PR.
…t contract Three doc changes, one of them a correction rather than an addition. ci-quality-baseline.md said the retry wrapper self-heals because "mirror rotation usually lands a healthy endpoint". That was never true. The sentence stated an expectation as if it were a measurement, and nothing could observe that it was not happening — a slow-mirror day failing CI looks exactly like confirmation. Replaced with what apt actually does, plus the discipline: any comment claiming a mechanism self-heals has to name the specific behaviour it self-heals through. Filed as its own family member alongside #193's "an attribute nobody can see will rot". Same shape, different failure: #193's property was never written down; this one was, and what got written was a wish. Documented the setup_script extension point with the three things that must be known before touching it, all found by running rather than reading: repo root must come from $PWD (the hook executes from a sparse .trusted-base holding only that script), `timeout` cannot invoke a shell function, and a dirty worktree fails the job outright. Added a doc-gaps entry for the missing offline contract test, since this script's failure mode is specifically hard to notice: it is designed to report missing capabilities non-fatally, so a broken script and an honest report of a constrained runner produce identical output.
Nine findings from an independent review of this branch. The substantive one is that the rotation repeated the exact mistake it was written to fix. The function comment correctly said line order is not what apt honours, `priority:` is — and then shifted by line order anyway. For a list written `A priority:3 / B priority:1 / C priority:2` apt prefers B both before and after a rotation, so the rotation does nothing in precisely the case it exists for. Every test case I had was shaped like the runner's real mirrorlist, whose line order happens to equal its priority order, which makes the two implementations indistinguishable. Same trap as #183's "fixture happens to already be sorted", fourth appearance. Now sorts by effective priority (missing priority last, as apt does) and shifts that sequence. Verified the preferred mirror actually advances: B->C->A->B, and C->A->B->C for a list mixing explicit and absent priority. Also from the review: - Treat rc=137 as a timeout, not a failure. `--kill-after` escalating to KILL yields 137, not 124, and cmake's compiler fan-out can trigger it. Reporting it as "failed" relabels "out of budget" as "script is broken", collapsing the one distinction this setup exists to keep visible. - Track an overall deadline in the script. Phase budgets sum to 990s against a 780s hook cap, so hitting the total would kill the process group before the "unavailable: X, Y" summary runs — and the guide calls that summary the agent's only signal about missing capabilities. Phases are now trimmed or skipped so the summary is reached on every path. - Replace the mirrorlist atomically (cp to .new + mv) and keep a one-time .orig backup; `cp` onto the live file truncates first, so a mid-write failure left apt with no usable list. - Guard on "has at least one mirror line" rather than "file is non-empty": a comments-only list satisfied `-s` while leaving zero mirrors. - Assert cmake with `command -v` instead of printing `--version` with no failure check; `set -e` is off, so the assertion had no teeth. - Accept `go 1.26` without a patch level in go.mod. - Correct three comments that did not match behaviour: comments and metadata keep their content but not their position/separators, and the manual verification recipe was only half-present in the guide. While fixing the deadline parser I introduced and caught a second bug: `*s)` ordered before the non-numeric guard turned "bogus" into "bogu" and fed it to arithmetic. Now matches `*[0-9]m` / `*[0-9]s` explicitly.
…tion at one Independent review of the previous commit found the fix for one finding had undone the benefit of another. `cp` onto an existing file preserves the target's mode; `cp` to a .new path plus `mv` does not — the new file inherits mktemp's 0600 and mv moves that inode into place. This script runs unprivileged and writes via sudo, so the next rotation failed the `[[ -r ]]` test at the top of the function and took the "not rotatable" path. Rotation degraded from "every retry moves mirror" to "moves once", which is exactly the property the previous commit set out to establish, and it made the "3 attempts keeps two rotations" comment false. My tests missed it because each one called rotate_mirrorlist exactly once, and a single call cannot observe that the first call changed the conditions for later ones. Now preserves the mode explicitly via `install -m`; verified that three consecutive rotations keep mode 644 while the preferred mirror advances B->C->A. Also from the review: - Validate SETUP_HOOK_TIMEOUT as a whole string. Matching a trailing unit first let "13m30s" and "1e3s" through to arithmetic, which aborted the script under `set -u` before any phase ran, and "0.5m" produced a negative deadline that skipped every phase — output indistinguishable from an honestly constrained runner. An unusable budget now says so explicitly. - Correct the rc=137 attribution. I had written that cmake's process fan-out produces it; measured, that yields 124. 137 means TERM was ignored or something external SIGKILLed us, usually the OOM killer — which is a broken environment, not a spent budget. It now gets its own branch that does not claim to distinguish the two. The lesson is that "verified by measurement" has to name the behaviour measured: I had measured that 137 exists, not what causes it, then documented the latter. - Test-then-copy instead of `cp -n`, whose behaviour coreutils 9.4 warns is non-portable; remove a stray .new if the replacement fails; correct the guard comment now that awk rejects a mirror-less list first. - Stop repeating the phase-budget totals in two llmdoc files; the script's own phase calls are the single copy, per the no-hardcoded-quantities rule.
Third review round, third defect in the same ten-line parser — each one surfacing after I thought it had converged. `+([0-9])` accepts leading zeros and bash arithmetic reads those as octal. So `010m` quietly meant 480s instead of 600s, and `0900s` aborted with "value too great for base", taking the script down before any phase ran with no capability summary printed at all — the single outcome this deadline machinery exists to prevent. All three branches now force base 10. The useful generalization is that "is it digits" and "how is it read" are separate questions: passing `[[ $x == +([0-9]) ]]` says nothing about what `$(( x ))` will produce. What finally converged this function was not enumerating more bad inputs but changing the kind of check — validate the whole string, then pin the radix. Also from the review: - llmdoc/index.md still carried the rc=137 attribution the previous commit overturned, and still hardcoded the phase-budget totals the previous commit deliberately de-duplicated. Two of three copies had been updated; this is the repo's own "every copy of a claim needs cleaning up" rule. - The reflection presented the superseded parser as current state. - Corrected a comment that claimed `timeout` signals the whole process group. Measured: a TERM-ignoring grandchild survives while timeout still returns 124, so 124 means the direct child was signalled, nothing more. - Reworded the mirror-line guard: it is currently unreachable because awk rejects a mirror-less list first, so it is documented as a kept invariant rather than as a live defence against a case that cannot occur.
Round four. Zero blocking, and the two remaining code issues are both about claims outreaching what the code does. `10#` stops leading zeros being read as octal but does nothing about size: `999999999999999999m` multiplied by 60 wraps to a large *positive* integer, so the script concludes it has ample budget and trims nothing — the outcome the deadline exists to prevent. (Wrapping negative was already caught by the DEADLINE guard.) Digit count is now bounded before any arithmetic. The previous commit also wrote down "anything fed to bash arithmetic needs 10#" as a general rule without checking the other script in the same change. `ATTEMPTS=09` makes `[[ "$attempt" -lt "$ATTEMPTS" ]]` abort with "value too great for base", which silently disables the mirror rotation — so the commit that stated the rule broke it. Both values are now normalized. The generalization: when writing "every X must Y", grep every X in the change. Docs: the discipline about parsing externally-overridable numeric variables now lives in ci-quality-baseline.md, where index.md says it is. I made the same "already written into the guide" error twice in this task — the second time after the reflection had already recorded the first — so the rule now has a real home rather than another pointer.
…normalizing
Final full-range review: 0 blocking, 3 important. The notable one is that I
carried only half of a rule I had written one commit earlier.
setup.sh got both halves — validate the whole string, then bound the digit
count. ci-apt-install.sh got only `10#`. So `ATTEMPTS=9223372036854775808`
wrapped negative, `seq 1 -N` produced an empty sequence, and apt was never
invoked at all — whereas the un-normalized string had worked fine. Adding the
normalization made that input strictly worse. Both variables now bound the
digit count and fall back to the documented default when out of range, and
ATTEMPTS < 1 falls back too, since zero attempts is never what a caller means.
The rule I broke ("when you write 'every X must Y', grep every X in the
change") is one I had added to ci-quality-baseline.md in the commit
immediately before, and I broke it in that same commit. Second instance of
this shape in this task.
Corrected two attributions that measurement did not support:
- The ATTEMPTS=09 comment blamed `set -u`. Measured identical under `set +u`
and `set -eu`, and bash writes the arithmetic error to stderr every time, so
it is not silent either. The defect is real; the explanation was not.
- Comments and docs said pom.xml sets `maven.compiler.release`; it sets
`source`/`target`. Those are different javac settings, and the wording would
send the next reader grepping for a property that is not there. Fixed in all
three places.
Also, the capability summary now joins phase names with "; ". Phase names
contain spaces, so the space-joined form gave a reader no way to tell where
one item ended — and this line is the agent's only signal about what is
missing. `${arr[*]}` cannot do it (IFS contributes one character), so the
join is explicit. The no-budget warning now prints the computed deadline.
Replacement final full-range review: 0 blocking, 3 important. The arithmetic error is mine and it is the kind that reads as rigour. I computed apt's ~1900s worst case from the script's *default* parameters, then used that figure to argue 300s was ample for the parameters actually passed. ci-apt-install.sh runs its retry loop twice, once for update and once for install, so 3 attempts of 90s plus backoffs is 600s worst case against a 300s bound: update alone could exhaust the allowance and cmake would never run, exactly what the comment promised could not happen. Now 3 attempts of 60s under a 420s bound, which covers the full 420s worst case and still leaves 4.1x the measured cmake time inside the phase ceiling. - ATTEMPT_TIMEOUT=0 passed validation, and GNU timeout reads 0 as "no timeout", so the per-attempt kill — this wrapper's entire purpose — silently disappeared while the log still said it was applied. Rejected now, the same way ATTEMPTS already rejected a count below 1. - A mirror line separating URI from metadata with a space instead of a TAB was rewritten into a URI containing a space plus a duplicate priority field: worse than the input. Such a list is already invalid, so awk now refuses and leaves the original untouched. Verified a bare URI with no metadata, which is legal, still rotates. Corrected one more comment of mine that overstated. The review argued the "0900s takes the whole script down" claim was wrong because the DEADLINE guard catches it; measured, the guard does not: the `*s` branch does no arithmetic, so the raw string escapes to the caller, DEADLINE is never assigned and `set -u` aborts before the guard runs. 08m is the case the guard did catch. The comment now distinguishes the two instead of covering both with one story.
…d two terms Third final full-range review. Both findings are in the same mechanism, and together they explain each other. The per-attempt `timeout` had no `--kill-after`. Plain `timeout` sends TERM and then waits indefinitely, so a child that blocks TERM runs arbitrarily long while `timeout` still reports rc=124 — a bound that reads as enforced and is not. Measured: 12s elapsed under `timeout 3`. dpkg holding a lock is precisely the case this script anticipates. And the outer budget omitted two terms that run between attempts: the `dpkg --configure -a` repair, which had no timeout at all, and the mirrorlist rotation. Adding them up honestly gave 798s against a 600s phase ceiling — it did not fit. The fix is not to pick a bound and assert coverage but to shrink the parameters until the arithmetic closes: 3 attempts of 40s with the repair bounded at 30s is 508s worst case under a 510s bound, leaving 90s for a cmake step measured at ~44s. That number has now been wrong three times, each time looking like real arithmetic: computed from default parameters and applied to the passed ones, then missing terms, then finally complete and revealing it never fit. The comment now enumerates every term so the sum can be checked, and the guide carries the rule: a comment claiming a bound covers the worst case must be able to list each item being summed. Docs no longer restate the figures. Two minor fixes: `nproc` failing left `jobs` empty and `-j"$jobs"` expanded to a bare `-j`, the exact violation the adjacent comment cites; and a mirrorlist line with a TAB but a space inside the URI was still rewritten, so the whitespace check now covers the URI field however the line is separated.
Contributor
🔍 PR 审查
未发现需要修改的问题。已检查完整 diff 与当前工作树，并通过 Bash 语法检查、ShellCheck、 本 PR 的 base 中尚不存在 本次审查由 Codex 主链路 (gpt-5.6-sol) 完成。 |
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
1 participant
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.
Wires up the
setup_scriptextension point from upstreamagentic-workflow-templatePR #30, so the review agent can run this project's own checks instead of only reading code.Why
On PR #194 the reviewer reported exactly three things it could not do: no
ruff, nogolangci-lint, and a JDK older than the project's target release 25. Those are the checks most likely to catch a real defect, so a reviewer restricted to reading is a real loss of review power rather than a cosmetic gap..github/agentic/setup.shis wired into bothagentic-pr-review.ymlandagentic-llmdoc-updater.yml. Phases run cheapest-first and each is individually time-boxed, so a pathological phase costs only its own capability:JAVA_HOME_25_X64), no downloadpine-go/go.modruff/pytestin a venv outside the repoVersions are pinned to what CI actually resolved on PR #194's run, not to what the config appears to say.
Note this PR's own review does not benefit:
pr-reviewreads the script from the base commit, where it does not exist yet, so it degrades to a warning. Verified against the real upstream hook. The gain starts with the next PR.The bug this uncovered
scripts/ci-apt-install.shwas written for #125/#164 as if a retry would land on a different mirror. It does not. The runner pointssources.listatmirror+file:/etc/apt/apt-mirrors.txt, andapt-transport-mirror(1)is explicit: failover happens only when a fetch fails, and mirrors are tried in ascendingpriority:order. #164's failure was a mirror that answered and then served at ~26 KB/s — never a failure, so failover never fired and all three attempts went back to the samepriority:1host. The per-attempt timeout turned one slow mirror into three slow mirrors.Both the script comment and
ci-quality-baseline.mdasserted the self-healing that was not happening. Rotation now genuinely advances the preferred mirror, and the guide carries the rule: a comment claiming a mechanism self-heals has to name the specific behaviour it self-heals through.Review
Eight independent blind rounds (each a fresh session with no inherited context, in a fixed-commit snapshot clone under a
bwrap --unshare-netsandbox whose network denial was verified by probe), then two terminal evidence audits. Final round: 0 blocking / 0 important / 0 minor, APPROVE. Both terminal audits: PASS, no failure classifications.48 findings total (2 blocking / 18 important / 28 minor) — 47 fixed, 1 independently disproved with the code correctly left alone, 0 open, 0 vanished. The finding count was recounted by the terminal auditor from the reports themselves, which is how an error in my own ledger got corrected.
Two things worth stating rather than burying:
Both blocking findings were introduced by an earlier fix in this same task. The atomic-replace fix dropped the mirrorlist mode 644→600, which on a root-owned file made rotation fail its own readability check from the second call onward — capping rotation at one and silently undoing the fix before it. My tests missed it because each called the function exactly once, and a single call cannot observe that the first call changed the conditions for later ones.
One time-budget figure was wrong three times, each time looking like real arithmetic: computed from default parameters then applied to the passed ones; then missing the between-retry terms (including a
dpkg --configure -athat had no timeout at all); then, once summed completely, revealing it had never fit the phase ceiling. Resolved by shrinking parameters until the arithmetic closes (508s worst case under a 510s bound) and making the comment enumerate every summed term. Measured end-to-end afterwards with a TERM-ignoring apt stub: 71s actual against 71s predicted.Also fixed along the way: a per-attempt
timeoutwith no--kill-after(a child blocking TERM ran 12s under a 3s limit while still reporting rc=124 — a bound that read as enforced and was not), leading zeros parsed as octal, magnitude overflow wrapping a budget positive,ATTEMPT_TIMEOUT=0disabling the timeout entirely since GNUtimeoutreads 0 as "no timeout", and a bare-jwhennprocfails.The core rotation logic was never the problem: independently re-verified in four rounds, including 300-trial randomized differential tests against an independent reference implementation of apt's ordering rules, with no defect found in any round.
Verification
shellcheckclean on both scriptsscripts/check-metrics-help-parity.py: 0 mismatchesruff check apple/: passedgit diff --check: cleanrun-setup-hook.shfetched from the template repo, including the worktree-cleanliness assertion, which was mutation-verified (a script that touches one stray file fails the job with exit 1) and the script-absent-at-base path