fix(ci): retry pnpm setup so a slow npm registry stops ejecting PRs (BLO-28813) - #1410
Merged
Conversation
…BLO-28813) pnpm/action-setup@v6 cannot install a pinned pnpm in one step. It `npm ci`s a bootstrap pnpm pinned inside the action itself (v11.19.0), then unconditionally runs `pnpm self-update 9.15.4`. Because the action's bootstrap pin can never equal our packageManager pin, that self-update always runs, and pnpm 11's engine-identity check resolves `@pnpm/exe@9.15.4` plus every one of its per-platform optional dependencies against registry.npmjs.org and verifies the signatures fail-closed. A Linux runner fetches `@pnpm/win-arm64` to do it. So every job depended on a healthy registry before it could run a single test. On 2026-08-19 that killed three jobs in sixteen minutes with ERR_PNPM_PNPM_ENGINE_IDENTITY_UNVERIFIABLE, one of them a merge-queue candidate that got ejected as a result. v4 did not behave this way -- it ran `pnpm install pnpm@<target> --no-lockfile`, one call, no verifier -- so this is a regression the v6 upgrade brought in, not a new registry problem. Route the 18 v6 call sites through .github/actions/setup-pnpm, which retries once with jittered backoff and widens npm_config_fetch_timeout to 120s. 120s is not arbitrary: one of the failures logged a request that completed in 70,506ms and was aborted anyway under pnpm's 60s default. pnpm's own in-process retries are disabled there so the two retry layers cannot multiply into a worst case that outruns a job budget -- and because the policy job's budget was 5 minutes, below that worst case, it moves to 10. Deliberately NOT setting pmOnFail=ignore, which the error text suggests. That does not skip the fetch, it skips the version switch, leaving the job running pnpm 11 against a pnpm 9 lockfile. Failing closed is correct; the missing piece was tolerance around it. The three v4 sites are left alone on purpose. v4 never had this failure mode, so migrating them would hand them an exposure they do not currently have. Honest limit: one retry survives a blip, not a sustained outage. It turns the common case from "PR ejected from the merge queue" into "this job took another ninety seconds". scripts/__tests__/pnpm-setup-retry.test.mjs pins all of the above. Every assertion was mutation-tested: reintroducing a bare @v6 site, dropping the policy timeout back to 5, removing the checkout the wrapper depends on, duplicating the version pin, tolerating the retry too, restoring the 60s timeout, re-enabling in-process retries, and setting pmOnFail in either the action or a workflow each fail the specific test that claims to catch it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-28813 |
1 similar comment
|
🔗 Paperclip issue: BLO-28813 |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 721e313
Strong diagnosis and an unusually honest write-up. Two things worth resolving before this lands, both about the fix's own blast radius rather than the diagnosis.
I verified the mechanism rather than taking the description on trust: pnpm/action-setup@v6 src/install-pnpm/run.ts does await rm(dest, {recursive:true, force:true}) on entry (the "retry starts clean" claim holds), runCommand(cmd, args, {cwd: dest}) for self-update passes no env so it inherits process.env (the timeout knob does reach the failing call), and continue-on-error is a documented runs.steps[*] key for composite actions. I also ran node --test ./scripts/__tests__/pnpm-setup-retry.test.mjs against the files at this head: 7/7 pass. policy and Worktree install are already green on this head, so the local-action path resolves.
Critical Issues (0)
Important Issues (2)
-
[native-codex]
.github/actions/setup-pnpm/action.yml:93—npm_config_fetch_retries: "0"silently disables npm's retries for the bootstrapnpm ci, not just pnpm's.- The comment reasons only about pnpm ("pnpm's in-process retries are off on purpose... 2 in-process x 2 step attempts = 6 registry stalls"), but this is a plain
npm_config_*variable andrun.tsbuilds the bootstrap environment as{...process.env, [pathKey]: ...}— sonpm ci, which also pulls@pnpm/exefrom registry.npmjs.org, inherits it and drops fromfetch-retries: 2(3 attempts) to 1. Across the wrapper that is 2 total bootstrap attempts where master has 3. - The diagnosed failures were all at the
self-update/engine-identity stage, so this trades away resilience at a stage that was not failing. It is probably close to a wash once the 120s timeout is counted, but it is not currently a considered trade — it is a side effect. - The two layers are not separable via this knob, so the cheap fix is to keep the value and correct the comment to say both npm and pnpm retries are disabled and why that is acceptable. As written the comment will mislead whoever next tunes this.
- The comment reasons only about pnpm ("pnpm's in-process retries are off on purpose... 2 in-process x 2 step attempts = 6 registry stalls"), but this is a plain
-
[pr-review-toolkit: tests]
scripts/__tests__/pnpm-setup-retry.test.mjs:1— the retry branch has no execution coverage anywhere, and the suite reads as though it does.test("the wrapper retries once, and the retry is allowed to fail the job")asserts only that the YAML text containsid: first,if: steps.first.outcome == 'failure', andRANDOM. It cannot detect the failure mode that matters: ifsteps.<id>.outcomedoes not evaluate as expected for auses:step inside a composite action, the retry never fires and the whole mitigation is inert. GitHub documentscontinue-on-erroras a compositeruns.steps[*]key but does not documentsteps.*.outcomeavailability inside composite actions — the contexts reference only calls outsecretsas unavailable — so this rests on convention rather than a documented guarantee.- CI on this head cannot close the gap either: the first attempt succeeds, so the branch is never taken. The failure would surface for the first time during the next registry brownout, precisely when you are relying on it.
- Cheap way to close it: one
workflow_dispatchscratch job that forces attempt one to fail (for examplenpm_config_registry: http://127.0.0.1:1) and asserts thepnpm setup failed; retrying onceline appears and the job still ends green. Running it once by hand and linking the run in the PR would be enough; it does not need to stay in the matrix.
Suggestions (3)
- [gstack/review]
scripts/__tests__/pnpm-setup-retry.test.mjs:94— theif (!declared) continue;escape hatch rewards deleting a timeout over declaring a tight one. A future 3-minute job that adopts the wrapper either inflates to 10 or dropstimeout-minutesand silently inherits GitHub's 360m default, which is strictly worse merge-queue protection than the 5 it had — and the comment tells the author that is fine. Consider asserting that a wrapper job declares some budget, with the 10m floor applying above a threshold. - [gstack/review]
scripts/__tests__/pnpm-setup-retry.test.mjs:74—matchAll(/uses: pnpm\/action-setup@v(\d+)/g)only catches@vNtags. A SHA-pinneduses: pnpm/action-setup@<sha>, the direction repos usually drift toward for supply-chain reasons, bypasses the guard entirely. Matchingpnpm/action-setup@and allow-listing the three v4 sites by path would fail closed instead. - [native-codex]
.github/actions/setup-pnpm/action.yml:86— the evidence for 120s proves config plumbing, not that the failing code path honours it.pnpm config get fetch-timeoutreturning120000shows the value is read into config; it does not show the engine-identity verifier's@pnpm/exeresolution usesfetch-timeoutrather than its own deadline. Worth one direct check — drop it to something absurdly low and confirm the identity check fails faster — otherwise the retry may be doing all the work here.
Strengths
- Root cause is traced to the actual mechanism — the unconditional
self-updateand the fail-closed per-platform identity check — not to "the registry was flaky", and the v4 to v6 regression framing is correct. - The
pmOnFail=ignorerefusal is right, and encoding it as a test so nobody adds it later under outage pressure is the best part of this diff. - Dropping
version:in favour ofpackageManagercollapses 21 pins to one and removes a real drift surface;readTargetVersionthrows on disagreement, so there is no silent path. - Jitter with a stated reason (eight simultaneous
pr.ymljobs), the checkout-ordering test that guards the new implicit dependency onpackage.json, and the deliberate decision to leave the three v4 sites alone are all well argued. - "Honest limits" and the note that
actionlintdoes not validateuses: ./...are exactly the disclosures that make a CI change reviewable.
Recommended Action
- No Critical issues — nothing blocks on correctness of the happy path.
- Address the two Important items this cycle: correct or re-scope the
fetch-retriesknob, and exercise the retry branch once so the mitigation is known to fire. - Consider the Suggestions opportunistically; the SHA-pin gap in the guard is the one most likely to bite later.
…(BLO-28813) Addresses Ally's two Important findings at head 721e313, plus the two guard gaps she flagged as most likely to bite later. 1. `npm_config_fetch_retries: "0"` was justified in the comment as if it only affected pnpm. It does not — `npm_config_*` is npm's namespace and run.ts spawns the bootstrap `npm ci` with `{...process.env}`, so it disables npm's retries too, at a stage that was not failing. The value stays; the reasoning was the defect. The comment now states both stages are affected and shows the arithmetic: each stage trades 3 back-to-back attempts on one connection for 2 attempts on fresh processes 10-25s apart with double the deadline. Fewer raw attempts, better distributed, and bounded — leaving both layers on would be up to 6 registry stalls, which does not fit the 10-minute floor the guard test asserts. Also noted that this knob cannot separate the two stages, so they move together. 2. The retry branch had no execution coverage, and the suite read as though it did. GitHub documents `continue-on-error` as a composite `runs.steps[*]` key but does NOT document `steps.*.outcome` availability inside a composite. If it does not evaluate there, the `if:` is false, the retry is skipped, the first failure stays swallowed by `continue-on-error`, and the composite exits 0 WITH NO PNPM INSTALLED. Inert mitigation, silent until a brownout. `pnpm-setup-retry-proof.yml` closes it. The decisive job needs no proxy and no timing: run the wrapper against an unreachable registry, and assert the composite's outcome is `failure`. Only the retry step can fail this composite — the first attempt carries continue-on-error — so `failure` proves the retry ran, and anything else proves it was skipped. A second job drives a local registry that rejects the first burst and then redirects upstream, asserting attempt one was actually rejected (an unrejected run is reported as inconclusive, not a pass) and the pinned pnpm still lands. It runs on pushes touching the wrapper, not workflow_dispatch alone: dispatch requires the file on the default branch, so a dispatch-only proof is un-runnable on the PR that introduces it, and path-scoping re-proves it whenever the wrapper is edited instead of relying on memory. 3. The direct-call guard matched `@v(\d+)`, so a SHA pin — the direction repos drift for supply-chain reasons — bypassed it entirely and got reported as "no direct calls". Now matches `pnpm/action-setup@<anything>` and allow-lists exact (file, ref) pairs, so an unlisted file OR a listed file on a different ref both fail closed. 4. The timeout guard skipped jobs with no declared budget, which made deleting `timeout-minutes` the cheapest way to satisfy the 10m floor — and trading a tight budget for GitHub's 360m default is strictly worse merge-queue protection. Absence now has to be allow-listed. Only `release-penstock-scope.yml:publish` is listed; deliberately NOT given a timeout, because it is an approval-gated Rust/sccache publish and picking a ceiling for a release job unilaterally is the riskier change. Not done, and stated rather than quietly skipped: Ally's third Suggestion is right that `pnpm config get fetch-timeout` proves config plumbing, not that the engine-identity verifier honours it rather than its own deadline. That needs a separate measurement. Guard tests still 7/7, and the four new/changed assertions are mutation-tested: a SHA-pinned direct call, a v4 site bumped to v6, a deleted `timeout-minutes`, and a lowered one each fail the specific test that claims to catch them. The flaky-registry proxy was smoke-tested locally — 503 through the burst window, 302 to the real registry after, redirect resolving to live metadata. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Author
|
@allyblockcast — both Important findings addressed at Important 2: the retry branch is no longer unprovenYou were right that this was the load-bearing gap: New workflow
The backoff line proves
Attempt one was genuinely rejected, attempt two recovered with the pinned version. A run where nothing was rejected is reported as inconclusive, not a pass. It triggers on pushes touching the wrapper rather than Important 1: the comment was the defect, not the valueCorrect on all counts — Suggestions
|
13 tasks
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 505ab07
Both Important findings are resolved, and the second one is resolved the right way — by measurement, not by argument. Approving.
I did not take the linked run on trust. I pulled the raw logs for run 32211415212 and confirmed it is attached to this exact head with both jobs green, and the runner's own step trace is more decisive than the quoted stdout:
##[end-action id=pnpm.first;outcome=failure;conclusion=success;duration_ms=11027]
##[start-action display=Back off before retrying pnpm setup;id=pnpm.__run]
pnpm setup failed; retrying once in 19s (BLO-28813)
##[start-action display=Setup pnpm (retry);id=pnpm.retry]
##[end-action id=pnpm.retry;outcome=failure;conclusion=failure;duration_ms=5026]
outcome=failure;conclusion=success on first is continue-on-error swallowing the failure, and the two gated steps then start. That is direct evidence that steps.first.outcome evaluates inside a composite action — the undocumented behaviour the whole mitigation rests on. The recovery job shows the mirror image: first outcome=failure, 22s backoff, retry outcome=success, registry saw rejected=3 served=3, pnpm on PATH: 9.15.4 (pinned 9.15.4).
One thing worth saying plainly, because the checks page is actively misleading here: the wall of red on this head is not this PR. Every one of those jobs has conclusion: cancelled, not failure — PR run 32211417209 was superseded by 32212757768 on the same SHA. There are zero failure conclusions in that run, policy / Helm chart / Vendored claude_k8s adapter completed green, and every cancelled job recorded Setup pnpm -> success before it was killed. The wrapper resolves and installs on the real call sites. policy step 11 also ran the guard suite green, so the tightened matchers below are passing, not just written.
Prior Findings Dispositioned (2)
- prior:721e313 important 1 — fixed —
.github/actions/setup-pnpm/action.yml:87— the comment now names the mechanism rather than reasoning only about pnpm:npm_config_*is npm's namespace,run.tsspawns the bootstrap with{...process.env}, so both stages are affected. The arithmetic is spelled out per stage (3 back-to-back attempts on one connection → 2 attempts on fresh processes 10–25s apart at double the deadline), and line 100 records that the knob cannot separate the stages so they must move together. The value is retained, which is right — dropping it puts the worst case at 6 stalls × 120s = 12m, through the 10m floor the guard asserts. - prior:721e313 important 2 — fixed —
.github/workflows/pnpm-setup-retry-proof.yml:40—gating_actually_evaluatescloses it deterministically. Because attempt one carriescontinue-on-error, the composite can only reportfailureif the retry step actually ran, so assertingoutcome == 'failure'against a connection-refused registry isolates exactly the inert-mitigation failure mode, with no dependence on timing (the elapsed check is correctly demoted to a warning). Verified green at this head, and thepush-on-paths trigger is the right call — aworkflow_dispatch-only proof is un-runnable on the PR that introduces it.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [native-codex]
.github/workflows/pnpm-setup-retry-proof.yml:125—servedis computed and echoed but never asserted, so job two cannot distinguish "attempt one failed, attempt two recovered" from "attempt one rode out the burst and succeeded by itself".firstRequestAtis set by the very first request, sorejected >= 1is true by construction; the guard therefore only catches the zero-traffic case (a cached pnpm on an ARC runner), which is worth catching but is not the claim in the job name.BURST_MSis 8s while attempt one's duration is unbounded, so a loaded runner could serve its later requests. Assertingserved > 0alongside it, or a >=10s gap between the lastrejected:trueand the firstrejected:falsein the JSONL, would make the end-to-end claim self-verifying. Job one is unaffected and remains the decisive proof — this is corroborating evidence, which is why it is a suggestion. - [gstack/review]
scripts/__tests__/pnpm-setup-retry.test.mjs:43—readdir(...).filter((n) => n.endsWith(".yml"))makes every guard in this file blind to a.yamlworkflow, which GitHub accepts identically. A workflow added as.yamlwould silently escape the direct-call ban, the 10m floor, and the checkout-ordering check all at once. Latent today (all 25 workflows are.yml, andsetup-pnpmis the only composite action), but it is the same fail-open shape as the SHA-pin gap you just closed, and/\.ya?ml$/costs nothing.
Strengths
- The response to the retry-coverage finding is the standout: rather than asserting more YAML text, it isolates the one branch that could not be exercised and proves it against a registry that cannot answer. Structuring the assertion so that only the retry step can produce
failureis what makes it evidence instead of a smoke test. - Reporting a run where nothing was rejected as inconclusive rather than a pass is the detail that keeps this proof honest as it ages.
- The SHA-pin guard now keys on
(file, ref)pairs and fails closed on both axes — an unlisted file or a listed file on a different ref — which is strictly stronger than the version-shaped match I suggested. - Making absent
timeout-minutesrequire an explicit allow-list entry removes the perverse incentive exactly; declining to invent a ceiling for the approval-gated Rust/sccache publish, and saying why, is the right call rather than a gap. - Dropping
version:in favour ofpackageManagercollapses the pins to one source and, forrelease-verify.yml, means a verified ref is now exercised with the pnpm that ref actually pins. - Conceding the
fetch-timeoutpoint as unresolved instead of dressing up config plumbing as proof is worth more to the next reader than a third green check would have been.
Recommended Action
- No Critical or Important issues — this is good to land once the superseding PR run finishes green.
- Both suggestions are guard-hardening, not correctness; take them opportunistically.
- The
fetch-timeoutquestion stays open by agreement. Worth its own measurement — drop it to something absurdly low and confirm the engine-identity check fails faster — since if the verifier uses its own deadline, the retry is carrying this fix alone.
kkroo
added a commit
that referenced
this pull request
Aug 19, 2026
…s (BLO-28818) Addresses both Important findings from review of c624ba1. 1. A signal-killed server was misreported as alive. `child.exitCode` is null BOTH while a child runs and after it dies from a signal — Node records the signal in `child.signalCode` instead — so the `exitCode !== null` guard never fired for a SIGKILL. The probe then polled the full 120s and threw a message that hard-asserted "(process still alive)". That is not incidental to this PR. An OOM-kill of a server co-resident with embedded Postgres in a CI shard is a leading candidate for the very stall being investigated, and this PR's own reasoning ("a dead child throws a different error") used that guard to conclude the observed flake was a stall rather than a death. Under a signal kill the old code would not have thrown a different error either — so the message stated as fact something the code could not establish. Verified on this runtime: spawn("sleep",["30"]); kill("SIGKILL") -> exitCode: null signalCode: SIGKILL killed: true Both throw paths now report the observed pair, and the loop exits early on a signal kill instead of burning the remaining budget. 2. The orienting comment cited BLO-28813 — a real but unrelated issue (pnpm registry retries, PR #1410) — so a reader chasing the log-capture rationale landed somewhere actively misleading. The tense implied the fix landed elsewhere, when this diff IS the change. Also takes all three suggestions, each mutation-verified: - The tail budget now counts BYTES. `readFileSync(…, "utf8")` returns a string, so the old `slice(-n)` counted UTF-16 code units: the server logs through pino-pretty, whose glyphs ("◇ │ ✓ └") are 7 units but 15 bytes, so an "8000 byte" cap admitted multiples of that and the "last N of M bytes" figure was simply wrong. Reads a Buffer and slices bytes. - A zero budget is a floor, not "unlimited". `slice(-0)` is `slice(0)` — the whole string — so asking for nothing returned everything. - The per-file read-error branch is tested. The previous message claimed this was not portably constructible ("running as root, chmod 000 is still readable"); that was wrong. A subdirectory inside the log dir throws EISDIR from readFileSync on every platform and every uid — confirmed here as uid 1000 — and a rotated-log subdirectory is a plausible real layout. Mutation-verified (each reverted after): char-based slice -> caught by "budgets in BYTES"; clamp removed -> caught by "zero budget as a floor"; per-file guard removed -> caught by "unreadable entry". Tree green before and after. 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
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.
Thinking Path
Linked Issues or Issue Description
Path (B) — no GitHub issue; tracked on the Paperclip board as BLO-28813, and the problem is described in full above and below.
Symptom. Failed step is
Setup pnpm; everything downstream isskipped, so the job reports failure having run zero tests.32200464275k8s-ro seed transport cold start32200730405policy32201423561General tests (server 4/4)— ejected #1404 from the merge queueBlast radius. 21 call sites; 18 on v6 (
pr.yml×8,release-verify×4,release×3, plus 3 singles).pr.ymlfans out one per shard, so a PR run makes dozens of simultaneous chances to hit it.Not a duplicate. Searched all open+closed PRs for
pnpm/action-setup/setup-pnpm: the nearest are #1300 (postinstall symlink race), #1273 (override drift), and lockfile refreshes — different failure modes. No ROADMAP overlap; this is not core feature work.What Changed
.github/actions/setup-pnpmcomposite: attempt → jittered backoff → retry, with the retry deliberately not tolerated so a second failure is the job's failure.npm_config_fetch_timeoutraised to 120s. Not arbitrary: one failure loggedRequest took 70506ms— the request completed, then was aborted under pnpm's 60s default.version:input; the action readspackageManagerinstead, collapsing 21 duplicated pins to one (it hard-errors on disagreement, so there is no silent path).policyjob timeout 5 → 10, because the wrapper's worst case is ~5m and at 5 a degraded registry became a job timeout — same ejection, more runner burned.scripts/__tests__/pnpm-setup-retry.test.mjs(7 tests) pinning all of the above, wired into thepolicyjob.pnpm-setup-retry-proof.yml+flaky-registry-proxy.mjs: proves the retry actually fires, since normal CI never takes that branch.Verification
The retry branch is proven, not assumed — run 32211415212, both jobs green:
Only the retry step can fail the composite (attempt one carries
continue-on-error), sooutcome=failureproves the retry ran rather than being silently skipped — which matters becausesteps.*.outcomeinside a composite is undocumented.Guard tests mutation-verified. Each of these fails the specific test claiming to catch it, checked against a green tree every time: reintroduce a bare
@v6site; SHA-pin a direct call; bump a v4 site to v6; deletetimeout-minutes; lower it to 5; duplicate the version pin; tolerate the retry too; restore the 60s timeout; re-enable in-process retries; setpmOnFailin the action or in a workflow.node --test ./scripts/__tests__/pnpm-setup-retry.test.mjs→ 7/7. Sibling CI guards (ci-cache-routing,check-github-runner-labels,merge-group-concurrency) still pass.Risks
pmOnFail=ignorewas NOT taken, though the error text suggests it. It doesn't skip the fetch, it skips the version switch, leaving the job running pnpm 11 against a pnpm 9 lockfile in a repo that pinspackageManager, blocks lockfile edits in PRs, and monitors drift. A test blocks anyone adding it later under outage pressure.npm_config_fetch_retries: "0"is npm's namespace, so it also disables retries for the bootstrapnpm ci, not just the self-update. Each stage trades 3 back-to-back attempts on one connection for 2 on fresh processes 10–25s apart with double the deadline. The knob cannot separate the stages.pnpm config get fetch-timeoutproves the value reaches config, not that the engine-identity verifier honours it over its own deadline. That needs a separate measurement (raised by review).actionlintdoes not validateuses: ./...paths — verified by pointing a site at a nonexistent action and getting silence. Its clean report is not evidence here; the PR's own CI is, sincepr.ymlcarries 8 of the sites.Model Used
Claude Opus 5 — exact model ID
claude-opus-5[1m], 1M context window, extended thinking enabled, agentic tool use via Claude Code (shell, GitHub API, file edits). Review findings incorporated fromallyblockcast(pr-review-toolkit + gstack/review + native-codex lenses).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateaction.yml; no docs file covers pnpm setup