fix(ci): tell an ARC runner kill apart from a real verify lane failure

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Its CI runs on a self-hosted ARC runner pool, and the verify job is the required check that aggregates every upstream lane into one merge gate
• When the ARC pool kills a runner mid-job, GitHub records that job's conclusion as failure, not cancelled — so verify reports "Upstream lane(s) reported failure", which reads as a defect in the author's diff
• verify already has correctly-worded "this is not your diff, re-run it" messaging on its cancelled branch, but that branch can never fire for a kill, so the one message written to prevent this misattribution is unreachable for the dominant case
• It needs addressing because agents and humans both burn time debugging a diff that was never at fault — #1403 sat red on three infrastructure kills while the bug it fixed stayed unshipped
• This pull request adds a classifier that goes back to the Actions API for the per-job detail the aggregator cannot see, and routes proven kills to their own annotation
• The benefit is that an infrastructure kill says so in plain language, while a genuine failure keeps its existing wording — and the merge gate itself is unchanged

Linked Issues or Issue Description

• Refs BLO-28999 — verify lane aggregation reports an ARC mid-job runner kill as "reported failure"
• Refs BLO-21662 (parent) — mid-job runner kills land on main CI as unrelated test failures
• Refs BLO-20867 — the earlier change that added the cancelled/skipped split this builds on

Searched open and merged PRs for verify / lane / runner / kill / ARC / flaky and for BLO-28999: no duplicate. The nearest neighbour is #1248 (classify external OOM kills), which is heartbeat-side, not CI-lane-side.

What Changed

• New scripts/classify-lane-failures.mjs — pure, dependency-free classifier plus a thin API-fetching CLI. Exports classifyJobFailure, classifyLaneFailures, jobBelongsToLane.
• .github/workflows/pr.yml — three steps added to verify (checkout, Node setup, classify) all gated on contains(needs.*.result, 'failure'); the aggregation step gains an infra_lanes bucket and a distinct annotation, and is now if: always().
• scripts/__tests__/pr-verify-lane-outcome.test.mjs — 10 new tests: both-fixture classification, annotation-independent detection, matrix-shard mapping, fail-safe on unmatched lane, index-pairing guard, and bash-level wording/exit assertions.
• scripts/__tests__/npmrc-devdeps-guard.test.mjs — its assertions were scoped to the whole verify job and pattern-matched the literal old branch, so they would have silently retargeted onto the new step. Now scoped to the aggregation step and strengthened: both destinations must exist and both must gate exit 1.

Detection uses two independent signals, either sufficient:

1. a failure-level annotation matching a runner-loss string (The operation was canceled., The runner has received a shutdown signal, lost communication with the server);
2. a failure conclusion with no step concluding failure.

Signal 2 alone is not enough, and the asymmetry is the subtle part: an empty failing-step set reliably means an abort, but a non-empty one means nothing — on runner loss the in-flight step can be marked failure with its paired Post <step> cancelled, so which steps appear is a timing artifact, not a property of the diff. Signal 1 is therefore checked independently, not as a tie-breaker. There is a test for exactly that shape.

Verification

Replayed the real killed run (32268626936, the worked example in BLO-28999) through the classifier:

infrastructure  Canary Dry Run               (failingSteps=0)
infrastructure  Build                        (failingSteps=0)
infrastructure  General tests (workspaces-b) (failingSteps=0)
reported        verify                       (failingSteps=1)

The three killed lanes classify as infrastructure; verify's own genuine step failure in that same run still classifies as reported. Genuine-failure controls (e2e job 96224412919, policy job 96228076444) carry Process completed with exit code 1. and a non-empty failing-step set. Test fixtures are these real payload shapes, not invented ones.

Tests — node --test across all six suites that read pr.yml: 42 passing.

Non-vacuity proven by negative control, not asserted:

• revert the classifier to conclusion-only → 3 tests go red;
• drop a lane from the aggregation array → the pairing guard goes red;
• make the failure branch silently drop a lane → the same guard goes red.

Seam check — with a stubbed node, confirmed the workflow passes failed lanes and their job names in lockstep (build → Build; general_tests, vendor_claude_k8s → General tests, Vendored claude_k8s adapter), and that node is not invoked at all on a green run or a cancelled/skipped-only run.

In CI on this PR: policy green, including the step Test verify job cancelled/skipped-vs-failed lane outcome. actionlint: 0 syntax-check findings. scripts/check-github-runner-labels.mjs: 25 workflows validated.

Risks

Low risk — this changes the explanation, not the gate. verify still exits non-zero for an infrastructure kill; nothing merges on the strength of one.

Every failure path degrades to the pre-existing wording rather than excusing a defect:

• the classify step is continue-on-error: true and the aggregation step is if: always(), so verify's verdict never depends on the classifier being available;
• a lane whose job cannot be matched stays a reported failure (fail-safe — we never excuse a failure without positive evidence it was infrastructural);
• a lanes/laneJobNames length mismatch throws rather than misreporting.

That last guard exists because I hit the bug: an earlier revision of the classify step appended to job_names for every lane but to failed only for failed ones, desynchronizing the arrays. Nothing errored — each lane simply matched no job and fell through to fail-safe, so detection was silently dead for every lane except the first. A unit test and a workflow-structure test now guard it.

Residual risks worth a reviewer's eye:

• Added latency on failing runs only. Checkout + Node setup + a few API calls, inside verify's timeout (raised 5→10m, with the classify step bounded at 3m). Green runs are unaffected. If a run has very many jobs, annotation fetches are one call per failing job.
• lane_job_names drift. If a lane's name: changes without updating the array, that lane stops being recognized. Guarded by a test that reads the declared name: out of pr.yml and compares.
• GITHUB_TOKEN scope. the workflow's existing token grants it — this was wrong, and Ally caught it. pr.yml declared no permissions: block at all, so under a restricted default-permissions preset every scope is none: the jobs call would 403 and the feature would have been inert from day one while looking identical to "no lanes were killed". Now declared explicitly at job level (contents/actions/checks: read), and a degraded classifier emits a warning annotation instead of failing silently.

Review round — Ally @ 1537f71c → 26f1c62a

Both Important findings addressed; both were about the classifier's inputs rather than its logic, and both shared a failure mode where the feature stops working while looking exactly like "no lanes were killed".

1. Missing token scopes (pr.yml:714). Confirmed exactly as reported: pr.yml had no permissions: block at workflow or job level, and this PR added its first GITHUB_TOKEN consumer. /actions/runs/{id}/jobs needs actions: read; /check-runs/{id}/annotations needs checks: read — a different scope. Declared at job level following this repo's own convention (master-health.yml, refresh-lockfile.yml), with contents: read included deliberately because a job-level block replaces rather than merges (the note already in docker.yml).
Also took the "consider" half: the classify step now captures the script's stderr and re-emits it as a ::warning title=verify: lane classifier degraded:: annotation, so a 403 can no longer masquerade as a clean result.

2. timeout-minutes expiry misclassified (classify-lane-failures.mjs:34). Correct, and worse than a tie: a timeout defeats both signals at once — GitHub cancels the job, records conclusion: failure, emits The operation was canceled. (signal 1) and leaves the in-flight step non-failure (signal 2). A hung or pathologically slow test from a diff would have been announced as "not a defect in this PR's diff — re-run the job", inverting this PR's purpose. Fixed as an override consulted before either signal, not a tie-breaker, precisely because signal 2 fires on an empty failing-step set alone.

All three Suggestions taken:

• main() now has coverage — the single-line stdout contract tail -n 1 depends on, the 403 degradation path (empty verdict and a stderr diagnostic), LANE_JOB_NAMES pairing, and the no-lanes case. Exercised by spawning the real script with a preloaded fetch stub, so argv parsing and pagination run for real.
• verify timeout-minutes 5 → 10, classify step bounded at 3. The failure path runs exactly when the pool is saturated; a verify timeout would lose the annotation entirely.
• Job pagination bounded at 20 pages rather than trusting a short final page.

Verification of this round: 38/38 green across both suites. Five mutations, each caught by a distinct test — remove the timeout override (2 red), remove the permissions block, drop checks: read alone, remove the stderr capture/warning, remove the classify step bound. All .github/workflows/*.yml parse and pr.yml still emits 12 jobs — checked because an unparseable workflow emits zero jobs and therefore fails nothing (BLO-23511).

Note on the two red lanes at the previous head: Typecheck + Release Registry and General tests (server 2/4) both failed with no failing step and ##[error]The runner has received a shutdown signal. — ARC mid-job kills, i.e. this PR's own subject matter, not defects in the diff. Merged master (a910162c) and pushed, which re-runs every lane.

Model Used

• Claude Opus 5 (claude-opus-5[1m]), 1M context, extended thinking, with tool use / code execution — running as the Paperclip Release Engineer agent.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, CI-only change
• I have updated relevant documentation to reflect my changes — the rationale lives in code comments next to the logic it explains
• I have considered and documented any risks above
• All Paperclip CI gates are green — in flight
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-28999
🔗 Paperclip issue: BLO-21662

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 1537f71

The design is sound and the fail-safe direction is right everywhere I checked: the gate is genuinely unchanged, and every degradation path lands on the pre-existing wording rather than excusing a defect. Two Important findings, both about the classifier's inputs rather than its logic — and both share a failure mode where the feature silently stops working while looking exactly like "no lanes were killed".

Critical Issues (0)

None. any_non_success=1 is set on the infra_lanes branch (pr.yml:898-901) and exit 1 is preserved, so no path here can make verify pass a run it previously failed.

Important Issues (2)

• [gstack/review] .github/workflows/pr.yml:714 — the GITHUB_TOKEN scope this step depends on is never declared, and a 403 is indistinguishable from "nothing was killed".
  pr.yml has no permissions: block at workflow or job level, and this PR adds its first GITHUB_TOKEN consumer. /actions/runs/{id}/jobs requires actions: read; /check-runs/{id}/annotations requires checks: read — a different scope. Under the restricted default-permissions preset both are none. The jobs call is not wrapped: githubJson throws (classify-lane-failures.mjs:127-137), main().catch emits the empty set (classify-lane-failures.mjs:198-204), and every killed lane keeps the old misattributing wording — which is the exact bug this PR exists to fix.
  Three things make that silent rather than loud: continue-on-error: true (pr.yml:712) suppresses the step failure, the diagnostic goes only to stderr, and no test asserts the classifier ever produced a verdict. The live path is also still unexercised — the PR run for this head is queued, and the classify step only fires when a lane fails, so CI green on this PR does not cover it.
  This repo's own convention is to be explicit: master-health.yml:58-61 declares actions: read with the comment "Required by the gate below to list this repository's workflow runs", and refresh-lockfile.yml:17-20 does the same at job level.

  • Add a job-level block to verify: contents: read, actions: read, checks: read. Include contents: read deliberately — docker.yml:329-334 already documents in this repo that a job-level block replaces rather than merges, and without it the new actions/checkout (pr.yml:693) loses repo access.
  • Consider making an auth/permission error distinguishable from a clean "no kills" result, so this cannot rot back to silence later.

• [native-codex] scripts/classify-lane-failures.mjs:34 — both detection signals also fire on a timeout-minutes expiry, which is not a pool kill.
  When a job exceeds its timeout-minutes, GitHub cancels it, records conclusion: failure, emits The operation was canceled., and leaves the in-flight step at a non-failure conclusion. That is precisely the shape signal 1 matches (:34) and the shape signal 2 matches (:58-59), so the two independent signals are not independent here — they agree on the wrong answer. Every lane in this workflow sets a timeout (general_tests 90m at pr.yml:522, typecheck_release_registry 40m at pr.yml:295, build 20m at pr.yml:913), so a hung, deadlocked, or pathologically slow test introduced by the diff would be announced as "KILLED MID-JOB by the CI runner pool… This is not a report of a defect in this PR's diff… Re-run the job" (pr.yml:898). For that defect class the change inverts its own purpose and turns a real red into an invitation to re-run indefinitely.
  Not hypothetical for this repo: pr.yml:28-35 records a degraded npm registry converting a fast red into a policy job timeout (BLO-28813).

  • Detect the timeout annotation (has exceeded the maximum execution time) and hard-return "reported" before either signal is consulted. Because signal 2 fires on an empty failing-step set alone, this has to be an override rather than a tie-breaker — same asymmetry argument the PR body already makes for signal 1.

Suggestions (3)

• [pr-review-toolkit/tests] scripts/classify-lane-failures.mjs:141 — main() has no automated coverage. All 10 new tests exercise the pure exports or the bash; none covers token/env validation, LANE_JOB_NAMES newline parsing, pagination, or the single-line stdout contract that tail -n 1 (pr.yml:780) relies on. The PR body describes a manual stubbed-node seam check — worth pinning as a test precisely because every failure on this path is silent by design.
• [gstack/review] .github/workflows/pr.yml:680 — verify keeps timeout-minutes: 5 while the failure path now adds checkout + Node 24 setup + up to N+1 API round trips, and that path runs exactly when the pool is saturated. If verify itself times out, the annotation is lost entirely. Consider the headroom reasoning already applied to policy (pr.yml:28-35), or bound the classify step with its own timeout-minutes.
• [native-codex] scripts/classify-lane-failures.mjs:163 — for (let page = 1; ; page += 1) terminates only on a short page. A page <= 20 cap makes it structurally bounded rather than dependent on API behaviour.

Strengths

• The fail-safe direction is correct at every layer — unmatched lane, length mismatch, thrown classifier, skipped step, unknown lane name in INFRA_LANES all degrade to the pre-existing failure wording. The gate is never loosened, and there is a test asserting exit 1 for the infra branch specifically.
• The signal-1-is-independent-not-a-tie-breaker insight is the subtle part, and it is both explained and tested (pr-verify-lane-outcome.test.mjs, the killedMidStep case). Easy to get wrong; you got it right and pinned it.
• Matrix lanes are only excused when every failing shard was killed — the conservative direction, with the mixed-shard test proving a real defect cannot hide behind a coincidental sibling kill.
• Noticing that the existing npmrc-devdeps-guard assertions would silently retarget onto the new step is the best thing in this diff. Scoping them to the aggregation step and strengthening them so both destinations must gate exit 1 turns a near-miss into a stronger guard than before.
• The lockstep pairing guard and the lane_job_names-vs-declared-name: test both target genuinely silent failure modes, and the readArray terminator correctly handles lane names containing parentheses.
• Toolchain choices are consistent with the rest of the file (actions/checkout@v6, actions/setup-node@v6, node-version: 24), and verify's job-level if: always() && !cancelled() (pr.yml:667) means the new step-level always() cannot fire on a cancelled run.

Recommended Action

1. No Critical issues — nothing blocks on merge safety; the gate is provably unchanged.
2. Address both Important issues this cycle. The permissions one is the higher priority: without it the feature may be inert from day one and would look identical to working. The timeout one can ship as a follow-up if you prefer, but it misattributes in the opposite direction, so it should not sit long.
3. Consider the Suggestions opportunistically — the main() coverage gap is the one that compounds, since it is what would catch a regression in either Important finding.

[allyblockcast]

@ally please re-review at head 26f1c62 — both Important findings from your 1537f71c review are addressed, and I want your eye on whether the fixes are the right shape.

Important 1 (token scopes, pr.yml:714) — confirmed exactly as you described: no permissions: block existed at workflow or job level. Added job-level contents: read / actions: read / checks: read, following master-health.yml and refresh-lockfile.yml, with contents: read deliberate per the replaces-not-merges note in docker.yml. I also took the "consider" half: the classify step now captures the script stderr and re-emits it as ::warning title=verify: lane classifier degraded::, so a 403 is no longer indistinguishable from a clean "nothing was killed".

Important 2 (timeout expiry, classify-lane-failures.mjs:34) — agreed, and I implemented it as a hard override consulted before either signal rather than a tie-breaker, for the reason you gave: signal 2 fires on an empty failing-step set alone, so a tie-breaker would still excuse a timeout. Please sanity-check the annotation string I matched (has exceeded the maximum execution time) — that is the one input I could not exercise against a real timed-out job in this repo, so if you know a counter-example shape it would be worth catching now.

All three suggestions taken: main() now has coverage (single-line stdout contract, 403 degradation, LANE_JOB_NAMES pairing, no-lanes case) via a preloaded fetch stub against the real script; verify timeout 5→10m with the classify step bounded at 3m; pagination capped at 20 pages.

Verification: 38/38 green. Five mutations each caught by a distinct test — removing the timeout override, removing the permissions block, dropping checks: read alone, removing the stderr capture/warning, removing the classify step bound. All workflows parse and pr.yml still emits 12 jobs.

Also merged master (a910162c); the branch was behind. The two red lanes at the previous head were themselves ARC mid-job kills (no failing step + The runner has received a shutdown signal.) — this PR's own subject matter, not diff defects.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 26f1c62

Both prior Important findings are fixed, and the shapes are right — the permissions block is scoped and documented, and the timeout override is an override rather than a tie-breaker, for the reason you gave. The stderr-capture addition is the best part of this round: it converts the classifier's silent-by-design degradation into something observable.

On your direct question — the annotation string is correct, but it is load-bearing in a way that is worth one more turn. has exceeded the maximum execution time is the right match for every timeout this workflow can actually produce: 12 of the 13 timeout-minutes in pr.yml are job-level, and the 13th is your new classify bound (pr.yml:740), which lives inside verify itself and is therefore never classified. So I found no counter-example shape in scope. The gap I did find is not the string — it is that the override only fires when there is an annotation set to read at all, and an empty one silently returns you to the pre-fix behaviour.

Prior Findings Dispositioned (2)

• prior:1537f71 important 1 — fixed — .github/workflows/pr.yml:694-704 — job-level permissions: block declares contents: read, actions: read, and checks: read, with contents: read explicitly justified against the replaces-not-merges note. The "consider" half is also taken: pr.yml:811-815 captures the script's stderr and re-emits it as ::warning title=verify: lane classifier degraded::, so a 403 is no longer indistinguishable from a clean result. pr-verify-lane-outcome.test.mjs:602 pins the scopes, including checks: read individually.
• prior:1537f71 important 2 — fixed — scripts/classify-lane-failures.mjs:79-82 — JOB_TIMEOUT_PATTERNS (:58) is consulted before both signals and hard-returns "reported", implemented as an override exactly as recommended. The test at pr-verify-lane-outcome.test.mjs:338 is stronger than asked for: it falsifies its own fixture (strip the timeout annotation and the same job must classify as infrastructure) and separately asserts the override precedes the step-shape signal. The residual condition below is a distinct defect, not this one unfixed.

Critical Issues (0)

None. any_non_success=1 is set on the infra_lanes branch and exit 1 is preserved (pr.yml:935-940), so no path here makes verify pass a run it previously failed. The gate is provably unchanged.

Important Issues (1)

• [native-codex] scripts/classify-lane-failures.mjs:220 — the timeout override is the only defense against misreporting a timeout-minutes expiry as a pool kill, and an empty annotation set disables it silently.
  classifyJobFailure can only reach the override through failureAnnotations. With an empty array, timedOut is false (:79-82), runnerLoss is false (:84-87), and signal 2 (:89-91) decides — which for a timed-out job is always infrastructure, because a job-level timeout leaves the in-flight step cancelled and the remainder skipped. Your own fixture proves this is not theoretical: TIMED_OUT_JOB carries no failure step, and pr-verify-lane-outcome.test.mjs:351-360 asserts that removing just the timeout annotation flips that exact job to infrastructure.
  Two routes reach an empty array, and neither is loud:
  • The per-job catch at :217-220 swallows any annotations error into []. Its comment frames falling back to "the step-shape check" as a safe degradation — true for a runner kill, but for a timeout the step-shape check returns the wrong answer, so this is the one place the fail-safe direction inverts. And this is reachable with the new permissions block: actions: read and checks: read are different scopes, exactly as :702-704 now documents, so the jobs call can succeed while /check-runs/{id}/annotations 403s, rate-limits, or 5xxs.
  • A 200 with [] — annotations not yet materialized for a job that just finished, and verify queries immediately after the lane fails. Downstream this is indistinguishable from the caught error.
    Neither route is covered, and the new ::warning cannot catch either: runClassifierMain's failStatus fails every fetch, so the jobs call throws first and never reaches the annotations loop, and the catch at :217 writes nothing to stderr, so pr.yml:814 sees an empty file and stays quiet. The net effect is the original misattribution — "KILLED MID-JOB … not a defect in this PR's diff … Re-run the job" on a genuinely hung test — restored through a narrower door, and most likely precisely when the pool is degraded and API calls are least reliable.
  • Distinguish "no annotation evidence" from "annotations say no timeout": set annotationsByJobId[job.id] = null on the catch and have classifyJobFailure return "reported" when annotations are unavailable. That keeps the fail-safe direction for the exact case the override exists to cover, at the cost of declining to excuse a kill you cannot prove — which is the tradeoff this script already makes everywhere else.
  • At minimum, have the catch write the failure to stderr so the degraded warning actually fires.
  • Add a jobs-200 + annotations-403 case over TIMED_OUT_JOB asserting reported. It fails today.

Suggestions (2)

• [pr-review-toolkit/tests] scripts/__tests__/npmrc-devdeps-guard.test.mjs:141 — assert.ok(aggregationBody, "verify must retain the lane-outcome aggregation step.") can never fail. When the step name is absent indexOf returns -1, and verifyBody.slice(-1) yields the last character of the file — a truthy one-character string (confirmed by running it). The regression is still caught one line later by assert.ok(laneNames !== null, …), so the guard fails closed; but it reports a missing lane_names array rather than the missing step, which is misleading for the scoping bug this assertion was added to catch. Capture the index and assert !== -1 before slicing.
• [gstack/review] scripts/classify-lane-failures.mjs:58 — worth a comment recording that JOB_TIMEOUT_PATTERNS is job-level-only. A step-level timeout-minutes on a lane renders in a different shape and would not match this pattern; it would leave the timed-out step at failure, so signal 2 correctly declines to excuse it, but signal 1 would still fire if the runner also emitted a cancellation annotation. No lane sets a step-level timeout today, so nothing is broken — but this PR introduces the file's first step-level bound, and a future one on a lane would land in the kill bucket without anything flagging it.

Strengths

• The override-not-tie-breaker reasoning is carried through correctly and pinned with a falsifying assertion. The test does not just check the happy path; it proves the fixture genuinely defeats both signals, so it cannot rot into passing by accident. That is the right way to test a defense whose failure mode is silence.
• The stderr capture is the highest-value addition in this round. Turning a silent degradation into a ::warning addresses the structural half of the earlier finding — the half that keeps the feature from rotting back to inert — not just the immediate scope gap.
• The permissions block is minimal and self-documenting. Job-level rather than workflow-level, so no other lane's behaviour changes; contents: read justified inline against the replaces-not-merges semantics; and checks: read annotated as a different scope from actions: read, which is the non-obvious part and is separately pinned by a test.
• Scoping the npmrc-devdeps-guard assertions to the aggregation step, then strengthening them, remains the best structural instinct in the diff — and the follow-through of asserting both destinations gate exit 1 means the split default branch cannot quietly excuse a lane.
• The timeout headroom is reasoned rather than rounded up: verify 5→10m with the classify step bounded tighter at 3m, so a hanging classifier cannot consume the budget of the annotation it exists to emit.
• The pagination cap comes with the reason it exists (:195-198) — structurally bounded rather than trusting the API to return a short final page.

Recommended Action

1. No Critical issues — the gate is unchanged and nothing blocks on merge safety.
2. Address the remaining Important finding this cycle. It is narrower than the one it replaces, but it fails in the same direction the PR exists to prevent, and the fix is small: make an unavailable annotation set a distinct state from an empty one.
3. Take the two Suggestions opportunistically — the dead assertion is a one-line change, and the job-level-only comment is cheap insurance against a future step timeout on a lane.

[allyblockcast]

Response to review at 26f1c62a — fixed in a29daa4

Thanks — the Important finding is correct and I've taken it, along with both Suggestions. Confirming your read on the annotation string too: it matches the only timeout shape this workflow produces, and the 13th timeout-minutes (the classify bound at pr.yml:740) is inside verify and never classified. So I've left the pattern alone and fixed the reachability gap instead.

Important — unavailable vs. empty annotations (fixed)

Your diagnosis is exact: the override was reachable only through failureAnnotations, so an empty set disarmed it, and signal 2 then answers infrastructure for every timed-out job. Three changes make absence-of-evidence a distinct state from evidence-of-absence:

• scripts/classify-lane-failures.mjs — the per-job catch records null instead of [], and writes the failure to stderr, so pr.yml:814 sees a non-empty file and the "lane classifier degraded" warning actually fires for this path.
• classifyLaneFailures — annotationsByJobId[job.id] ?? null, not ?? []. Worth calling out: coalescing at this layer would have silently re-disarmed the override one level below the fix, so there's a test pinning it.
• classifyJobFailure — returns "reported" for a non-array set, before either signal. Documented as the one place the fail-safe direction inverts, using your framing: safe to degrade to signal 2 for a kill, wrong for a timeout.

The cost is stated rather than hidden — a genuine kill whose annotations we can't read is now reported instead of excused. That's the trade the no-matching-job branch already makes, and there's an assertion pinning it so it can't be "fixed" later by accident.

Tests — all three fail if the classifier is reverted:

Test	Covers
classifier declines to excuse a job whose annotations could not be read	null vs [] on TIMED_OUT_JOB and KILLED_JOB
a lane whose job is missing from the annotations map is reported, not excused	the ?? null coalesce layer
main() reports a timed-out lane when only the annotations call is forbidden	jobs 200 + annotations 403, end to end

You were right that failStatus couldn't express the last one — it fails the jobs call first and never enters the annotations loop. Added an annotationsFailStatus knob to runClassifierMain for exactly that shape. And a paired falsification test (main() still excuses a provable kill when the annotations call succeeds) proves the fix didn't just switch detection off.

Verified locally: 40/40 pass; reverting the three classifier changes turns exactly the three new tests red and leaves the falsification test green.

One residual I deliberately did not close: 200 with []

I've fixed the catch route. I have not treated a successful 200-with-[] as unavailable, and I don't think it should be:

[] from a 200 is the only remaining way signal 2 ever fires. A killed job's annotations don't reliably match RUNNER_LOSS_PATTERNS, which is why the issue nominated the no-failing-step shape (BLO-28999, option 2) as the cheapest reliable discriminator in the first place. If empty-means-unavailable, signal 2 only ever fires when there are failure annotations matching neither pattern — which retires the PR's primary detector to close a race I can't demonstrate.

I also don't want to assert the race doesn't exist: the jobs API already reported conclusion: failure, so the job has completed, but whether annotation materialisation lags that is something I haven't measured. A bounded re-fetch on an empty set would narrow the window without changing semantics — happy to add it if you think it's worth the classify-step latency, but I'd rather not guess at a delay. Filing it as a follow-up note on the issue either way.

Suggestions (both taken)

• npmrc-devdeps-guard.test.mjs — you're right that the assertion was unfailable: indexOf → -1, slice(-1) → a truthy one-character string. Now captures the index and asserts !== -1 before slicing, so a missing aggregation step reports that rather than a missing lane_names array.
• JOB_TIMEOUT_PATTERNS — comment recording that it's job-level-only, why a step-level timeout renders differently (leaves the step at failure, so signal 2 declines to excuse it and no override is needed), that signal 1 could still fire on one, and that a future step-level bound on a lane would need its own pattern.

Gate unchanged

Unchanged, and worth restating since this round moves lanes between buckets: the change only moves lanes from the infrastructure wording to the ordinary failure wording. Both destinations set any_non_success=1 and reach exit 1 (pr.yml:935-940). Nothing here can make verify pass a run it previously failed.

No re-review request posted — the push to a29daa4 fires synchronize on a non-draft PR, so review re-triggers on its own. Note the branch is now behind master; I'll rebase or merge forward once this round settles rather than moving the head mid-review.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a29daa4

The remaining Important finding is fixed, and yes — the shape is right. You took all three parts of the recommendation rather than the cheapest one, and the ordering is what makes it work: null is rejected at classify-lane-failures.mjs:98, before failureAnnotations is even computed, so an unreadable set can never reach the timeout override or signal 2. Making it a guard rather than a branch inside the signal logic is the same structural instinct that made the timeout an override rather than a tie-breaker.

Two details worth calling out because they are the parts that usually get missed. First, :184 passes annotationsByJobId[job.id] ?? null — ?? [] there would have re-disarmed the override one layer below classifyJobFailure, defeating the whole fix, and the test at :407 pins exactly that. Second, the catch now writes to stderr (:261-263), which is what actually connects the new state to the ::warning at pr.yml:814; without it the degradation would have been detectable only by reading the classifier's source. The prior review noted the catch was silent and that the existing 403 test could not reach this path — annotationsFailStatus (test :808) fails only the annotations call and now covers it, with :832 as its falsification.

No new Critical or Important findings. The gate is provably unchanged: the infra_lanes branch sets any_non_success=1 (pr.yml:934-935) alongside the other three categories, and exit 1 at :939 is untouched, so no path here lets verify pass a run it previously failed.

Prior Findings Dispositioned (1)

• prior:26f1c62 important 1 — fixed — scripts/classify-lane-failures.mjs:98 — if (!Array.isArray(annotations)) return "reported" runs before failureAnnotations is built, so an unavailable set is now a distinct state from an empty one and cannot silently disarm the timeout override. Both routes named in the finding are closed: the per-job catch assigns null rather than [] (:257) and writes the cause to stderr (:261-263) so pr.yml:814 raises the degraded warning, and the ?? null at :184 keeps a missing map entry from being coalesced back to []. The named test that "fails today" now exists and passes as written (pr-verify-lane-outcome.test.mjs:808, jobs-200 + annotations-403 over TIMED_OUT_JOB asserting an empty verdict plus an audible stderr diagnostic), with three falsifying counterparts: :388 ([] on the same job must still read infrastructure), :401 (signal 2 stays live when genuinely queried and empty), and :832 (a provable kill is still named, so the fix cannot pass by disabling detection outright).

Critical Issues (0)

None.

Important Issues (0)

None.

Suggestions (3)

• [native-codex] scripts/classify-lane-failures.mjs:82 — the default parameter still points the permissive way. classifyJobFailure(job, annotations = []) means a one-argument call gets "queried successfully, nothing found" — the exact reading the JSDoc directly above (:72-76) now warns against, and for a timed-out job it returns infrastructure. No live caller is affected: :184 is the only production call site and it always passes explicitly, and all 11 test call sites pass two arguments, so the default is currently unreachable. That is precisely why it is worth changing now — an exported function whose unsafe state is the one you get by omission will eventually be called by omission. annotations = null costs nothing and makes the safe direction the default.
• [gstack/review] .github/workflows/pr.yml:1037 — the sibling required checks in this workflow are outside the classifier's reach, and this head demonstrates it. verify aggregates eight lanes, but e2e (:1037, needs: [policy]) and canary_dry_run (:978) are separately-required checks that verify does not needs, so the new wording can never apply to them. Live on a29daa49: job 96340464551 (e2e) concluded failure with no failing step — the in-flight Install dependencies is cancelled — and a failure annotation reading The operation was canceled., which is signal 1 and signal 2 firing together. It is the textbook shape this PR classifies, sitting red on the PR that introduces the classifier, still presenting as an unexplained defect in the diff. Out of scope for the title, and correctly so, but worth a follow-up issue rather than leaving it to be rediscovered.
• [pr-review-toolkit/tests] .github/workflows/pr.yml:755 — lane_names is now duplicated across two steps (:755 in the classify step, :862 in the aggregation step) with nothing pinning them equal. Each step's internal pairing is well covered — :91 validates the aggregation step's arrays (getVerifyLaneScript is correctly anchored to - name: Fail if any split verify lane failed, so the new step inserted above it did not silently retarget that test) and :550/:617 validate the classify step's three arrays against each lane job's declared name:. But no assertion compares the lane_names in one step against the other. Both drift directions are fail-safe, which is why this is a suggestion and not a finding: a lane added only to the aggregation step never reaches the script and keeps the ordinary wording, and one added only to the classify step is ignored by the aggregation loop (covered by :537). Still, drift would silently narrow detection coverage with every test green — a one-line set-equality assertion across the two steps closes it.

Strengths

• The fix rejects the bad state at the boundary instead of defending against it downstream. :98 sits ahead of failureAnnotations, so there is no ordering left for a future edit to get wrong — the override and both signals are structurally unreachable with unusable input. Compare the alternative of special-casing null inside the timeout check, which would have left signal 2 to be remembered separately.
• ?? null at :184 is the non-obvious half, and it is the half that was actually load-bearing. The guard in classifyJobFailure would have been inert if the caller kept coalescing to [], and the comment there says exactly why. The test at :407 covers the layer rather than the function, which is the right granularity for a defect that lived in the seam between them.
• Every new assertion ships with its own falsification. :388, :401 and :832 each prove the fixture still exercises the distinction, so none of these tests can rot into passing by accident — the correct pattern for a defense whose failure mode is silence, and consistently applied now across the timeout override, the null guard, and the 403 path.
• The stderr diagnostic names the job, not just the error. annotations unavailable for job ${job.id} (${job.name}) means the ::warning identifies which lane lost its evidence, so a systematically degraded scope reads differently from a single transient 403.
• The dead assertion was fixed by capturing and checking the index, not by deleting it. npmrc-devdeps-guard.test.mjs now asserts aggregationStart !== -1 before slicing and keeps the message that names the missing step, so the guard reports the right thing rather than being quietly dropped.
• The job-level-only scope of JOB_TIMEOUT_PATTERNS is now recorded where it will be read (:59-66), including why a step-level timeout needs its own pattern and why the classify bound this PR adds does not — the follow-up you cannot get from the code alone.

Recommended Action

1. No Critical issues — the gate is unchanged and nothing blocks merge safety.
2. No Important issues. annotations = null (first suggestion) is the only one I would fold into this PR; it is a one-token change that removes the last place the unsafe reading is the default.
3. The e2e/canary_dry_run coverage gap is worth its own issue while the evidence is fresh on this head — the shape is already confirmed against a real job.

[allyblockcast]

Thanks — took both foldable suggestions, and filed the third. Pushed as b33a6c30.

1. annotations = [] default → null (classify-lane-failures.mjs:82) — agreed, and for the reason you gave rather than a hypothetical one: the default contradicted the JSDoc immediately above it, and for a timed-out job a one-argument call would take signal 2 and answer infrastructure — the exact misattribution the guard was added to prevent. Confirmed your reachability claim before changing it: :191 is the only production call site and passes explicitly, and every test call site passes two arguments, so this is a no-op for live callers today. That is the argument for fixing it now rather than against.

Pinned with a test that the omitted argument reads as "never queried", plus its own falsification against an explicit []. Falsified: reverting the default to [] reddens it —

✖ classifyJobFailure defaults to the safe state when annotations are omitted
  AssertionError: omitting annotations must read as 'never queried', not as 'queried and empty'

2. e2e / canary_dry_run outside the classifier's reach → filed as BLO-29277, out of scope here as you said. I verified your evidence rather than taking it on trust — GET /actions/jobs/96340464551 on this head returns conclusion: failure with no failing step (Install dependencies cancelled, everything after skipped), so signal 1 and signal 2 both fire. It is in the issue as a ready-made regression fixture. I also confirmed verify's needs: is exactly the eight lanes, so neither check can reach the new wording. The issue notes that adding them to needs: is the wrong remedy — it would change the gate's dependency graph and serialize merges behind e2e — and flags BLO-22445 for reconciliation rather than duplication.

3. lane_names duplicated across the two steps — folded in too. You rated it a suggestion because both drift directions are fail-safe, which is right, but the failure mode is silent narrowing with every test green, and that is the shape this whole PR exists to eliminate. Cheap enough to just close.

One deliberate deviation: I asserted set equality, not order equality. Each step pairs its own arrays by index and is separately covered for that (:91, :550/:617), so the two steps do not have to agree on order to both be correct — requiring it would impose a constraint correctness does not need. Membership is the real invariant. Falsified: injecting a lane into the classify step only reddens it —

✖ both verify steps list the same lanes
  AssertionError: the classify and aggregation steps must cover the same lanes — a lane
  present in only one of them silently loses runner-kill detection while both steps keep working

Full suite green at b33a6c30: 42/42 in pr-verify-lane-outcome.test.mjs, and 66/66 across every suite that reads pr.yml (merge-group-concurrency, npmrc-devdeps-guard, playwright-install-deps-retry, ci-cache-routing, pr-ci-shard-folding, check-github-runner-labels) — the classify step's insertion did not retarget any of them.

No gate change in this commit: it touches the classifier's default parameter and two tests. infra_lanes still sets any_non_success=1 and exit 1 is untouched.

Not re-requesting review with a marker — this PR is not a draft, so the synchronize push already re-armed the automatic reviewer wake (and the review workflow is in-flight on b33a6c30). A marker here would just stack a second request on the one already open.

[allyblockcast]

Empirical validation against a real 7-job kill event (from #1446 / BLO-27676)

This morning Blockcast/paperclip run 32339000633 (PR #1446, head fd48fbf8) lost 7 jobs in one run to an ARC eviction storm — root cause was node reboots of the runner hosts (Rebooted k8s-paperclip-9 06:51:49Z, k8s-paperclip-2 06:52:49Z; both blockcast.net/arc-runner-host=true, both carrying 17 runner pods). Zero were real test failures.

I ran your classifyJobFailure logic at a29daa49 against all seven by hand. It gets 6/7 right, including the two cases I'd have expected a simpler classifier to miss. Scorecard:

job	conclusion	failure annotations	failing step?	verdict	✓
General tests (server 1/4)	failure	The operation was canceled.	no	signal 1 → infrastructure	✓
General tests (server 3/4)	failure	The operation was canceled.	no	signal 1 → infrastructure	✓
e2e	failure	The operation was canceled.	no	signal 1 → infrastructure	✓
k8s-ro seed transport cold start	failure	The operation was canceled. + Process completed with exit code 130	yes (step 9)	signal 1 → infrastructure	✓
Canary Dry Run	failure	The self-hosted runner lost communication with the server.	no (all null)	signal 1 → infrastructure	✓
General tests (workspaces-a)	failure	none at all	no	signal 2 → infrastructure	✓
General tests (server 4/4)	cancelled	none at all	yes (Setup Node.js)	line 83 early-return → reported	✗

Two things worth banking from the ✓ rows: workspaces-a produced zero annotations, so signal 1 alone would have missed it and signal 2 carried it — your argument for keeping the two signals independent is empirically load-bearing, not theoretical. And k8s-ro is the case where signal 2 correctly declines (real failing step) and signal 1 has to do the work.

The one miss, and why the obvious fix makes it worse

General tests (server 4/4) (job 96334715199) contradicts the premise stated at the top of the file:

When the ARC pool kills a runner mid-job, GitHub records the job's conclusion as failure — NOT cancelled

That job's log contains, at line 230:

2026-08-20T06:21:39.6703972Z ##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.

with Setup Node.js → failure and every test step skipped. It is unambiguously an ARC kill, and GitHub recorded it as cancelled. So kills can land as cancelled.

The practical consequence today is mild — I want to be precise rather than alarming. pr.yml:793 routes a cancelled lane result to BLO-20867's cancelled_lanes wording, so it is not laundered into "your diff is broken"; it just doesn't get the new infrastructure wording. In this specific run it didn't even change the lane verdict: general_tests still went failure on shards 1/4 and 3/4, both classified infrastructure, so allKilled held and the lane was excused correctly.

But the comment is the risk, not the code. It states the failure-not-cancelled behaviour as an invariant, and the natural fix a future reader derives from it — "relax line 83 to also accept cancelled" — would produce a wrong answer on this exact job:

• annotations: [] → timedOut false, runnerLoss false
• hasFailingStep: true (Setup Node.js)
• → returns reported — a genuine ARC kill announced as a real lane failure

This job is a double blind spot: it defeats signal 1 and signal 2 simultaneously, which no failure-conclusion job in the corpus does. Suggest correcting the comment to say kills are usually failure and can be cancelled (with this job as the citation), and noting that cancelled is deliberately left to BLO-20867's path rather than routed here — so the next person doesn't "fix" it into a misclassification. A test case pinned on this shape would make that intent executable.

One caveat on signal 1's evidence source

Annotations are not a reliable mirror of ##[error] log lines. Job 96334715199 has the runner-loss string in its log and no annotation whatsoever; workspaces-a likewise had none. Signal 1 can only see what GitHub chose to promote to an annotation, which is exactly why the ?? null / "evidence of absence is not usable as a negative result" reasoning in classifyLaneFailures matters. Worth a line in the header so nobody later "simplifies" signal 1 into a log grep or assumes an empty annotation set means a clean job.

Raw evidence for all seven jobs, plus the k8s node-reboot correlation, is on BLO-27676. Happy to hand over the job IDs as fixtures if you want them as test data — they're a ready-made labelled corpus, including the awkward one.

— Staff Engineer (pre-landing review; not an Ally review request)