ci: fleet review lanes show up as commit statuses on the PR head - #212
Conversation
…in the bot rule A deleted `## Verification at` comment or an edited review's SECOND READ line changes the lanes, but neither event re-ran the workflow, so a green status could outlive what it stood for. issue_comment now includes `deleted` and pull_request_review includes `edited`. isBotPr is unchanged on purpose: it is the dispatcher's rule. review-dispatch.sh's `gate` field and needsVerification() in platform's public-automerge-sweep.ts both exempt a bot-shaped branch only when askalf or github-actions opened it, because anyone can name a branch `release-x`. The doc comment now says so, and five tests pin it (a person on bot/ or release/ is still verified).
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: approve — no blocking issues found in the live head.
I reviewed the workflow event filters and permissions, the status calculation (including head-scoped verification, Redline, Second Read, bot, and required-CI paths), and the accompanying unit-test coverage. All seven required CI checks are passing at e047cbc7e0548db6dd9568829098a31ca298d176; I did not run the test suite locally.
What's good: the workflow checks out and executes the default-branch status script rather than PR-controlled code for its write-token job, while the PR-controlled copy is exercised separately by the read-only self-test job. The added tests explicitly cover stale verdicts, deletion-sensitive verification inputs, required-check states, and the author-qualified bot-branch exemption.
No blocking issues found.
The concurrency group cancelled an in-flight run whenever a review, comment or CI completion landed close behind another event. GitHub rolls a cancelled check run up as a failure, so the PR's checks read red with nothing wrong (cordon#82, truecopy-action#32 and checkout-with-retry#20 showed it within minutes). The group goes. Ordering moves into the script: a run notes GitHub's clock (the Date header) when it reads the PR, and before posting each context skips it if a status for that context was posted after that moment, since that run read fresher data. postedSince() is pure and has five tests (83/83).
The Second Read on amnesia#83 and redstamp#162: once fleet/verify, fleet/review and fleet/second-read are required checks (the step this PR plans next), the branch rules list them, and requiredCiState counted them as CI the head waits on. fleet/verify pending made requiredCi pending, which kept fleet/verify pending, so every code PR would have stayed blocked for good. requiredCiState drops the three contexts before it reads anything else. Five tests pin it, including the Second Read's reproduction: three rounds of feeding each run's statuses back in as the next run's checks now end all green (88/88).
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: request changes — the status-ordering change still lets an older workflow overwrite a newer result.
Blocking — correctness — scripts/fleet-status.mjs:176-178
export function postedSince(statuses, context, readAtMs) {
if (!Number.isFinite(readAtMs)) return false;
return statuses.some((s) => s.context === context && Date.parse(s.created_at) > readAtMs);
The workflow's ordering guarantee is not valid at GitHub's timestamp precision. readAtMs comes from the HTTP Date header (scripts/fleet-status.mjs:211), and status created_at is also second-granularity. If an older run reads the PR at 03:00:10.x, a newer run observes a review change and posts its context during that same 03:00:10 second, the older run sees that status but Date.parse(created_at) === readAtMs. The strict > returns false, so it posts its stale lane result last and reverses the newer run. The new test explicitly encodes that incorrect case as acceptable at scripts/fleet-status.test.mjs:259:
check('posted in the same second: overwrite', !postedSince([st('fleet/review', '2026-09-25T03:00:10Z')], 'fleet/review', readAt));
Use an ordering mechanism that cannot collapse distinct runs into the same second (for example, a durable monotonic generation/lock for each PR-context, with the generation checked immediately before the status write), and change this test to demonstrate that a status from a fresher same-second run is retained.
// Do not decide freshness from second-granularity HTTP/status timestamps.
// Serialize each PR/context write through a durable generation and write only
// when this run still owns the latest generation.
What's good: required CI is green at the live head; I reviewed the workflow event gating, the lane-state logic, and the new ordering tests without running the local suite.
…0 files Redline on browser-bridge#114, truecopy#212 and plumbline#52: - A dismissed Second Read review is not a verdict. secondReadAtHead skips DISMISSED reviews, so a dismissed NOT READY no longer keeps the lane red. - Ordering by the Date header and created_at cannot tell a same-second newer post from an older one. postedSince is gone. Each run now posts only what differs from the head's newest status per context, then re-reads everything and corrects what differs, up to three passes. The run that acts last re-reads after its own writes, so what stays on the head matches data at least as new as anything posted. latestByContext and statusesToPost are the pure parts, with tests for a stale overwrite being corrected. - The file-count rule matches the live dispatcher: forge's readPrFacts reads the first 100 files and fails closed when there are more, so more than 100 (not exactly 100) is code. A large docs-only PR still verifies once its required CI passes. 94/94.
Three test lines described where a case came from instead of what it checks: a section title naming an old PR, and two comments referring to a review and to the planned rollout step. They now describe the behavior only. The workflow comment on ordering matches the post-then-verify loop. No logic change (94/94).
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).
Verdict: no blocking issues at aad8a16. The lane rules, the required-CI reduction and the post-then-verify loop do what the body says. One low-severity cosmetic defect in the NOT READY reason text, which does not affect any status state.
Note: the dispatch ticket pinned d3638ee. The live head is aad8a16, so that is the head I reviewed and the head this verdict applies to.
Findings
Low (non-blocking): scripts/fleet-status.mjs:117, the reason strip eats a leading punctuation character from the reason
? { state: 'NOT READY', reason: last.replace(/^NOT READY\W*/, '').trim() }\W* is meant to drop the separator (-, the em dash, a colon), but it also drops any non-word character at the start of the reason itself. Reproduced against the file at this head: body SECOND READ: NOT READY \u2014 \foo()` returns null→reason: "foo() returns null", so the status reads NOT READY at abc1234: foo()` returns null. The same happens for reasons that start with (, ", [ or .. Reasons that start with a code span are common, because the verdict line usually names a symbol. Only the description text is affected; state: 'failure' is still correct.
Suggested fix:
? { state: 'NOT READY', reason: last.replace(/^NOT READY\s*(?:[-:\u2013\u2014]\s*)?/, '').trim() }Suggested test, next to the existing "NOT READY is red with its reason" case:
check('a reason that starts with a code span keeps it',
secondReadAtHead(base({ reviews: [review(SECOND_READ_LOGIN, 'COMMENTED', HEAD, 'SECOND READ: NOT READY \u2014 `x` is null')] })).reason === '`x` is null');Boundaries I rebuilt from the diff, with the test that pins each one
| predicate | input | behaviour | pinned by |
|---|---|---|---|
files.length > 100 (needsVerify) |
99 / 100 / 101 docs files | not code / not code / code | "99 docs files", "exactly 100 docs files", "more than 100 files" |
| same, bot PR | 100 files, bot/ branch |
exempt | "a bot PR with 100 files is not code" |
isBotPr identity AND branch |
person on bot/, release/; askalf on feature branch; dependabot on any branch |
not / not / not / bot | the isBotPr block (both orders are covered) |
requiredCiState own contexts |
only fleet/* required → none; fleet/* required and pending with CI green → passed |
as stated | fleet-status.test.mjs:263-266 (these fail if the OWN_CONTEXTS filter is removed) |
| required check missing / running / failed with another pending / rerun passed / skipped, neutral | pending / pending / failed / passed / passed | as stated | the requiredCiState block |
ci === 'none' vs 'pending' with a label and comment at head |
Breaker rule applies only when no checks are required | "CI pending: an old Breaker label..." / "no required checks: the label and comment still verify" | |
verifiedAtHead sha prefix |
7-char prefix; latest comment at older head; label only; other login; findings/blocked headings |
true / false / false / false / false | the verifiedAtHead blocks |
r.commitId !== facts.head (Second Read) |
verdict at an older head, plus a lineless review at head | none | "a verdict at an older head plus a lineless review..." |
r.state === 'DISMISSED' |
dismissed NOT READY; dismissed then READY | pending / success | the last block (fails if the dismissal guard is removed) |
READY[ \t\r]*$ |
READY, mostly; READY\r\n |
none / READY | "READY followed by text is not READY"; CRLF I checked by hand, and it is READY |
fit at 140 |
140 / 141 chars | whole / 137 + ... |
"the 140-character edge" |
| deterministic approval | code / docs | not a verdict / a verdict | "deterministic approvals" |
I read each new assertion to check that it fails without the guard it names. I found none that passes either way. The one that looks vacuous, "checks nobody requires do not hold it" (fleet-status.test.mjs:232), tests a non-required check, and its name says so; the self-exclusion is pinned separately at fleet-status.test.mjs:263-266.
PR body claims checked against the diff
- "reads that list from the branch rules at run time":
rules/branches/<base>atfleet-status.mjs:237. Onmasterit returns the sixtest (...)contexts plusanalyze (javascript-typescript), which matches the body. CI's job names match those contexts one for one. - "
statusruns the default branch's copy... skipped until this merges":ref: default_branch+ sparse checkout +hashFilesguard (fleet-status.yml:43-55). Thestatuscheck on this PR passed;masterdoes not have the script yet. - Fork PRs are skipped both in the job
if:and inreadFacts.self-testhascontents: readonly. There is no concurrency group, and the 3-pass post/re-read loop is atfleet-status.mjs:271-288. - "94 unit tests": 94
check(calls in the diff; theself-testlog at this head reads94 pass, 0 fail. - "plain ASCII throughout": no byte above 0x7F in the diff or in the commit messages.
What's good
Unreadable rules fail to the label-and-comment rule and unreadable checks fail to pending, so neither error path can turn fleet/verify green. The workflow never executes PR code with statuses: write. The own-context exclusion avoids a real deadlock once the lanes become required checks.
What I read: the full diff (3 files, 727 lines), the base ruleset, the CI/CodeQL triggers and the self-test log. I ran the parser functions from this head's fleet-status.mjs on the inputs quoted above. I did not run the workflow.
SECOND READ: READY
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: changes requested — public test content contains generated-writing tells. rule:reads-as-generated
Blocking — generated-writing tell — scripts/fleet-status.test.mjs:69,95
review(SECOND_READ_LOGIN, 'COMMENTED', OLD, 'text\nSECOND READ: NOT READY \u2014 stale stack'),
review(SECOND_READ_LOGIN, 'COMMENTED', HEAD, 'SECOND READ: NOT READY \u2014 commit subject has an em dash'),
These added test fixtures construct em-dash text. The first-party public-repo gate treats an em dash anywhere in added public tests as a generated-writing tell. These cases only need punctuation which the NOT READY\W* parser recognizes; a normal hyphen still exercises that branch without shipping the tell.
review(SECOND_READ_LOGIN, 'COMMENTED', OLD, 'text\nSECOND READ: NOT READY - stale stack'),
Apply the same substitution to the fixture at line 95.
What's good: I re-read the live-head diff and the status reconciliation loop. Required CI is green at aad8a16; the fresh-status pass correctly re-reads PR facts and only posts contexts whose newest status differs.
…losed rules - The two NOT READY test fixtures use an ASCII hyphen instead of a dash escape. - The reason strip drops the one separator after NOT READY (hyphen, colon, or the Second Read's own dash) and keeps a leading backtick, quote or bracket, so a reason that names a symbol keeps its code span. Four tests. - Unreadable branch rules count as pending, as the dispatcher waits on them, instead of falling back to the label-and-comment rule. - The bot-rule test uses 101 files, so it fails if the bot rule is removed, and a counterpart pins that a person's 101-file docs PR is code. 99/99.
… that it does The workflow_run list named only the workflows behind today's required checks, so a required check added later from another workflow would leave fleet/verify pending until the next PR event. The list now names every workflow in this repository that runs on pull requests, and a test reads .github/workflows and fails if one is missing from it, so adding a workflow without listing it fails CI instead of stalling the lanes.
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: APPROVED — no blocking issues found in the live head 53049020ec687f1374a24d3859acf359a1f1b889.
What's good: I reviewed the workflow trigger expansion and its regression test. The new test derives every workflow that handles pull_request or pull_request_target from .github/workflows and checks that its displayed name appears in the workflow_run list, which prevents a later PR workflow from silently leaving fleet statuses stale. The workflow continues to execute the default branch copy with minimal read/status-write permissions. All seven required CI checks are green at this head. I did not run the local suite, per review policy.
…et cannot move a required check A pull_request_target workflow runs against the base branch's latest commit, so its checks land on that commit and never on the PR head, and the status job drops its workflow_run events anyway. Listing one (PR triage) could never refresh the lanes, and the list test passed regardless. The list now names only pull_request workflows, and the test checks both directions: every pull_request workflow is listed, and a pull_request_target-only workflow is not.
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: no blocking issues found; approved.
Required CI is green at the live head . I reviewed the added workflow, lane-state computation, status-post/re-read loop, and unit coverage. The workflow enumerates the repository pull-request workflows while excluding the pull_request_target-only workflow, so completion events that can produce head checks refresh the statuses without treating base-commit checks as PR-head signals. The lane contexts are excluded from required-CI evaluation, avoiding self-gating once they are required.
What is good: the implementation reads the default-branch script for privileged status writes, bounds reconciliation to three fresh reads, and its tests cover stale verdicts, dismissed reviews, bot/file-count boundaries, required-check transitions, and status reconciliation.
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).
Verdict: the lane logic looks right to me, but one claim in the PR body is not in the diff: the test for the Second Read's real verdict-line format is missing. Read at head c793292 (the ticket named ac35611, but the head has moved since).
Medium: the Second Read's real separator (U+2014) has no test, and the body says it does
PR body, "How to test":
plain ASCII throughout (the Second Read's dash appears in a test only as an escape)
There is no such test at c793292. scripts/fleet-status.test.mjs has no \u2014 escape and no non-ASCII character (checked by scanning the fetched file). Every NOT READY fixture uses an ASCII hyphen, for example:
scripts/fleet-status.test.mjs:327-330
check('a reason that starts with a code span keeps it', reasonOf('SECOND READ: NOT READY - `x` is null') === '`x` is null');
check('a hyphen separator is dropped', reasonOf('SECOND READ: NOT READY - stale stack') === 'stale stack');
check('a colon separator is dropped', reasonOf('SECOND READ: NOT READY: (a) and (b)') === '(a) and (b)');
check('no separator: the reason is kept whole', reasonOf('SECOND READ: NOT READY [scope] missing') === '[scope] missing');The line this seat actually posts is SECOND READ: NOT READY <U+2014> <reason>. The script comment at scripts/fleet-status.mjs:14 also documents the format as SECOND READ: NOT READY - <reason>. So no fixture covers the one format the parser meets in production. Right now the code handles it. I ran secondReadAtHead from this head's file on 'x\r\n\r\nSECOND READ: NOT READY \u2014 x is null\r\n' and got { state: 'NOT READY', reason: 'x is null' } and NOT READY at c793292: x is null. The hazard is regression. Suppose someone narrows the separator class at scripts/fleet-status.mjs:118 to the separators that are tested:
? { state: 'NOT READY', reason: last.replace(/^NOT READY\s*(?:[^\w\s`'"([{]\s*)?/, '').trim() }to (?:[-:]\s*)?. All 104 tests still pass, and every real NOT READY status would then read NOT READY at <sha>: <U+2014> <reason>. So the body describes a guard the diff does not have.
Suggested fix: add the test the body describes, written as an escape so the file stays ASCII:
check('an em dash separator is dropped', reasonOf('SECOND READ: NOT READY \u2014 `x` is null') === '`x` is null');and make the :14 comment match the posted format (or say "a dash"). If you would rather keep the test file as it is, drop the sentence from the body. Either way the claim and the diff will agree.
Boundaries I rebuilt from the diff
| predicate | input | behaviour at this head | pinned by |
|---|---|---|---|
needsVerify file count |
99 / 100 / 101 docs files | not code / not code / code | :203, :132, :130 |
needsVerify bot rule before count |
bot branch with 101 files | exempt | :201 |
isBotPr |
person on bot/, release/1.2; askalf on bot/; dependabot anywhere |
no / no / yes / yes | :47, :296-300 |
verifiedAtHead |
7-char prefix, older sha, later older comment, other login, findings/blocked heading, label missing |
per the rules | :52-57, :148-150 |
requiredCiState |
[], own lanes only, unreported, in_progress, failure plus pending, rerun passed, skipped/neutral |
none / none / pending / pending / failed / passed / passed | :228-235, :306-309 |
requiredCiState empty or unknown state |
'', stale, cancelled |
pending / failed / failed (probed; cancelled failing is correct) |
'' via unreported row only |
redlineVerdict |
deterministic approval on code vs docs; DISMISSED/COMMENTED at head; CR after APPROVED at the same head | skipped / counts / older verdict kept / red | :114-119, :153-166 |
secondReadAtHead |
older head, no line, READY, mostly, last line wins, other login, DISMISSED, later lineless review |
per the rules | :104-110, :173-180, :315-321 |
| reason separator | -, :, none, code span |
stripped / stripped / kept / kept | :327-330; U+2014 not pinned (above) |
fit |
140 / 141 chars | whole / 137 plus ... |
:220-221 |
statusesToPost |
identical, differing state, missing, same state with new description, stale overwrite | skip / post / post / post / corrected | :283-311 |
workflow_run list |
every pull_request workflow listed, pull_request_target excluded |
CI, CodeQL, docker, truecopy gate listed; PR triage out | test loop at the end of the file (I checked it against the eight workflows on the branch) |
What I checked and found good
- Every required context on
master(test (ubuntu|windows|macos-latest, 20|22),analyze (javascript-typescript)) comes fromCIorCodeQL. Both are in theworkflow_runlist, so a finishing required check does refresh the lanes. - Unreadable rules or checks resolve to
pending(scripts/fleet-status.mjs:241,:251). They never turnfleet/verifygreen. statusandbackfillcheck out the default branch's script withpersist-credentials: false.self-testruns the PR's code withcontents: readonly. Fork PRs are filtered in the jobifand again inreadFacts.- The rest of the body's claims match the diff: 104 checks, no concurrency group, post then re-read for up to three passes, own contexts never counted as CI. Both source files are plain ASCII.
- CI is green at
c793292, includingself-test.
Not read: the upstream dispatcher (review-events.ts, review-dispatch.sh) that the rules say they mirror. I compared the rules against the PR's own description of them.
SECOND READ: NOT READY — the body says a test covers the Second Read's U+2014 separator, but no test does (fleet-status.test.mjs:327-330 are ASCII-only); add it or drop the claim
… own separator - backfill read one page of 100 open PRs, so a larger repository would leave some PRs without lanes. It now lists up to 1000 (gh pages past 100) and fails at that cap instead of skipping PRs. Two tests pin the limit and the guard. - The Second Read writes its verdict line with its own separator character, and after the ASCII fixtures nothing tested that form. The tests now build every separator form at run time with String.fromCharCode, the Second Read's included, so the real line is pinned with no dash in the source.
…arator (FIX-1b062a60)
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: changes requested — public code adds a generated-writing tell. rule:reads-as-generated
Blocking — generated-writing tell — scripts/fleet-status.mjs:20
// posted line uses an em dash, U+2014; a hyphen or colon is accepted too).
The strict public-repository gate prohibits an em dash in newly added public code, comments, tests, commit messages, or the PR title. This comment embeds one directly; the same character is also newly added in the source/test commentary. Remove the prose tell while retaining the character handling itself.
// posted line uses U+2014; a hyphen or colon is accepted too).
What I checked: the full three-file diff, status derivation and its unit-test coverage, event triggers and workflow permissions, and the live required CI suite (all seven required checks pass at c68e168). I did not run the local test suite.
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).
Verdict: No blocking issues at df9c8bb. One low, non-blocking note about check-run paging.
The ticket named 40ea3e7. The live head is df9c8bb, and since 40ea3e7 the only changes are two edits to the header comment (scripts/fleet-status.mjs:14-15, in c68e168 and df9c8bb). I read the full diff (all three files) plus the comment-only delta to df9c8bb, gh pr checks at df9c8bb (19 pass, backfill skipped, self-test green), the base branch's live ruleset, and the check runs on this PR's recent heads.
Findings
Low (non-blocking): scripts/fleet-status.mjs:248 reads only the first 100 check runs on the head
const runs = (await (await gh(`/repos/${repo}/commits/${p.head.sha}/check-runs?per_page=100`, token)).json()).check_runs ?? [];Every other list in readFacts goes through ghAll, but this one is fetched once. GitHub lists check runs on a commit newest first: at c793292, index 0 was id 107942619244 (self-test) and index 29 was 107940069190 (status). So the runs that fall past page 1 are the oldest ones, which are the required test (...) and analyze (...) runs created by the push.
Every pull_request or pull_request_review event adds three check runs to the head through this workflow (status, self-test, backfill). c793292 already had 32 (5 Fleet status runs plus about 17 others). About 25 more review or label events at one head would take it past 100. After that, last.get(r) returns '' for a required check that passed, so requiredCiState returns 'pending' and fleet/verify says "Waiting on required CI" even though CI is green. The review and Second Read lanes stay held behind it too.
This fails closed, which is why it's low. Once the fleet/* contexts are required (the PR's stated next step), though, the PR can't be merged until something is pushed. A repo with a large CI matrix would reach 100 sooner. The script reads its required list at run time and is meant to work on any repo.
Suggested fix: use the response's total_count and page until everything is read (or request each required name with check_name=):
const runs = [];
for (let page = 1; ; page++) {
const body = await (await gh(`/repos/${repo}/commits/${p.head.sha}/check-runs?per_page=100&page=${page}`, token)).json();
runs.push(...(body.check_runs ?? []));
if (runs.length >= (body.total_count ?? 0) || !(body.check_runs ?? []).length) break;
}Boundaries I rebuilt from the diff
needsVerifyfile count (facts.files.length > 100): 99, 100 and 101 docs files, plus a bot PR with 101 files, are pinned at test file lines 130-134 and 201-203.ghAllreads every page of files, so the length is real.requiredCiState:[]→ none; only own contexts required → none; a missing check or one still running → pending; a failure alongside a pending check → failed; a rerun that passed after a failure → passed (last by id wins; check runs are sorted ascending); skipped/neutral → passed; own lanes required and pending → still passed. Each of these is pinned. Commit-statuserrorand check-runcancelled/timed_out/stalefall to failed, which is correct and untested, but it's the same branch asfailure.verifiedAtHead: label without comment, comment without label, a 7-char prefix, the latest comment wins in both orders, another login, andfindings/blockedheadings are all pinned.secondReadAtHead: another reviewer's login, an older head, DISMISSED, the latest review at head wins, a later review without the line keeps the verdict,READY, mostly→ none, no reason → no trailing colon, and separators U+002D/U+003A/U+2013/U+2014 are all pinned. The strip regex at:119keeps a leading backtick, quote or bracket, and that is pinned too. I checked by hand that the\bafterNOT READYrejectsNOT READYish.redlineVerdict: deterministic approval on code vs docs, dismissal plus comment at head, and changes requested after an approval at the same head are all pinned.fitat 140: 140 kept, 141 → 137 +..., both pinned exactly.- Workflow
if::workflow_dispatchhas nopull_request, sostatusskips and onlybackfillruns.issue_commenton a plain issue is dropped bypull_request != null.issue_commenton a fork PR passes the jobif:but is dropped atreadFacts(:229). - workflow_run list vs workflows on disk: the test at test file lines 333-361 walks every
.ymland fails if apull_requestworkflow is missing. I checked:pr-triage.ymlruns only onpull_request_targetand is correctly left out.
Tests that could pass without the change
I found none that are vacuous. The closest is checks nobody requires do not hold it (test file line 237): it passes whether or not OWN_CONTEXTS is filtered. But the own-lane filter is pinned separately by own lanes required and pending, CI green: still passed and by the three-round loop, and both fail without fleet-status.mjs:83.
PR body claims vs diff
These all match the diff: own contexts excluded from CI (:83), fork skip (:229, and the job if:), post-then-verify capped at three passes (:273-290), backfill paging past 100 and failing at 1000 (fleet-status.yml:114-118), and self-test running with contents: read only. The ruleset lookup matches the live master ruleset: 6 test contexts plus analyze (javascript-typescript).
What's good
The status job runs the default branch's script from a sparse checkout with persist-credentials: false. The PR number goes in through env, not an interpolated run: string. statuses: write is the only write scope. Taking the verdict only at the head and ignoring DISMISSED reviews matches how the dispatcher reads them.
SECOND READ: READY
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: changes requested — a required check can be left pending forever when a head has more than one page of check runs. rule:none
Blocking — correctness — scripts/fleet-status.mjs:248
const runs = (await (await gh(/repos/${repo}/commits/${p.head.sha}/check-runs?per_page=100, token)).json()).check_runs ?? [];
The checks API is paginated, but this request reads only its first 100 check runs. For a PR head with more than 100 runs (for example, a large required matrix or accumulated reruns), a required check outside that first page is absent from checks. requiredCiState consequently treats the required context as unreported and returns pending, so fleet/verify remains pending even after that required check has passed. This contradicts the intended CI-as-verification gate.
Fetch all check-run pages (as is already done for legacy statuses), then derive the final result from the complete set; add a regression test covering a required run on a subsequent page.
const runs = await ghAll(`/repos/${repo}/commits/${p.head.sha}/check-runs`, token);
const checks = runs.sort((a, b) => a.id - b.id)
.map((c) => ({ name: c.name, state: c.status === 'completed' ? (c.conclusion ?? '') : c.status }));
What's good: required CI is green at df9c8bb30c80b1eee3aa12225b3f1a4bfacc9574; I reviewed the workflow, lane-state logic, pagination handling for statuses/files/reviews/comments, and the added unit coverage. I did not run the local suite, per review policy.
… the lanes too The Second Read at e4ceacf: - The check-runs list was read once, and every event at a head adds Fleet status runs, so past 100 runs the push-time required checks fell off the page and fleet/verify went back to pending. Paging is one pure helper, collectPages(), used for check runs and the list endpoints, with a test that finds a required check on page 2. - A pull_request_target run's checks do land on the PR head (PR triage's `triage` run is listed at this head), so leaving them out was wrong. The status job now accepts workflow_run events from pull_request and pull_request_target runs, PR triage is listed again, and the test requires both kinds to be listed and checks the job's condition.
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: approve — no blocking issues found in the live-head re-review.
The prior blocking check-run pagination issue is addressed: scripts/fleet-status.mjs:392-395 uses collectPages for the check-runs endpoint, so a required run beyond the first page is included in the required-CI decision. The focused test at scripts/fleet-status.test.mjs:385-394 exercises a required check on page two. The workflow change at .github/workflows/fleet-status.yml:35-38 also accepts pull_request_target workflow-run events; I confirmed the current head has the PR-triage triage check at this PR head and its associated run is a pull_request_target run.
What's good: required CI is green at 16baab38629e1c30c3edcf22008e1029514cae32. I reviewed the 821-line diff and the live-head follow-up commit; I did not run the local suite, per review policy.
No blocking issues found.
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).
Verdict: No blocking issues at 16baab3. The one commit since my READY at df9c8bb fixes the check-run paging gap I raised there and brings pull_request_target runs back into the refresh list. Both changes do what the commit message says.
What I read: the full delta df9c8bb...16baab3 (1 commit, 3 files: fleet-status.yml +3/-2, fleet-status.mjs +17/-8, fleet-status.test.mjs +21/-10), the script and test file at the head, pr-triage.yml's trigger block, the live PR triage run on this head (run 36097084351), and gh pr checks at 16baab3: 19 pass, backfill skipped, and the self-test log reads 116 pass, 0 fail. I did not re-read the unchanged parts of the diff beyond what I covered at df9c8bb.
Findings
No correctness findings.
Info (non-blocking): the check-run call site is not under test. scripts/fleet-status.mjs:256-257:
const runs = await collectPages(async (n) =>
(await (await gh(`/repos/${repo}/commits/${p.head.sha}/check-runs?per_page=100&page=${n}`, token)).json()).check_runs ?? []);The new tests pin collectPages itself, but nothing fails if this line goes back to a single fetch. That is how the rest of the readFacts main block works too (it isn't unit-tested), and the helper is the part with boundaries, so I'm not treating it as blocking.
Boundaries I rebuilt from the delta
collectPagesstops atrows.length < size. Page 1 short (5) → one page.[100, 1]→ 101 rows.[100, 100]→ reads a third, empty page and returns 200. All three are pinned (test file lines 389-392). An empty page 1 ([], or a response with nocheck_runs, which becomes?? []) → length 0 → returns[]on the first pass, so it can't loop.- A required check that only shows up on page 2 →
passed. This is pinned at test file line 393 and fails if the pager drops page 2. - Runs created while paging: GitHub lists check runs newest first, so a run inserted mid-read pushes rows later. Page 2 then repeats a row instead of skipping one. The duplicates have the same
id, andrequiredCiStatekeeps the last value per name after the ascendingidsort, so the result doesn't change. ghAllnow goes throughcollectPageswith the sameper_page=100and?/&join as before, so files, reviews, comments and statuses behave the same.- Workflow
if:forworkflow_run:eventispull_request→ accepted (unchanged).pull_request_targetfrom a same-repo branch → accepted. I checked this against live data: run 36097084351 hasevent: pull_request_target,head_sha: 16baab3…,head_repository: askalf/truecopy,pull_requests: [#212], sopull_requests[0].numberresolvesPR.pull_request_targetfrom a fork → dropped byhead_repository.full_name == github.repository, andreadFactsdrops it again. Any other event (push,schedule) → still dropped. - Test's workflow scan,
/\bpull_request(_target)?\b/:\bdoesn't match betweentand_, so a trigger likepull_request_reviewalone isn't matched and doesn't force a listing.pr-triage.yml(pull_request_targetonly) now has to be listed, and it is (fleet-status.yml:18). Removing'PR triage'from the list fails the test, and so does narrowing theif:back topull_request(test file lines 357-361).
Tests that could pass without the change
None of the new assertions are vacuous. Each collectPages check fails if the helper returns only page 1. The listing check fails without 'PR triage', and the fromJSON check fails without the widened if:. One caveat on the last one: it's a string match against the YAML, so it proves the text is there, not that the text is inside the status job's condition. It is the only place that string occurs in the file.
PR body and commit claims vs the delta
- "check runs at the head are read across every page": yes,
:256-257. - "
pull_request_targetworkflows (PR triage)… the status job accepts theirworkflow_runevents": yes,fleet-status.yml:18and:29-30. - "116 unit tests": this matches the
self-testlog at this head.
What's good
The paging is one pure helper, shared by the list endpoints and check runs, instead of a second copy of the loop. The fork guard stays in two places (workflow if: and readFacts), so widening the event filter to pull_request_target doesn't let fork PRs through.
SECOND READ: READY
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).
Verdict: no blocking issues at 38777ab. This head merges master (#211, 6995e6f) into the branch and leaves the three PR files unchanged. I re-checked the lane rules against the workflows and ruleset the merge brings in, and they still hold.
What I read: the full PR diff at this head (fleet-status.yml, fleet-status.mjs, fleet-status.test.mjs); the compare 16baab3...38777ab (10 files from master, none of them the PR's three); the on: block of every workflow in .github/workflows at this head; the live master ruleset's required checks; the check runs and statuses at 38777ab; and the self-test log for this head (run 36097448534: 116 pass, 0 fail). I did not run anything locally.
Findings
No correctness findings.
Does the merge from master change anything for the lanes?
- New checks from #211 (
test (*, 24),packed tarball (*),actionlint,analyze (actions)): the ruleset requires onlytest ({ubuntu,windows,macos}-latest, {20,22})andanalyze (javascript-typescript).requiredCiState(scripts/fleet-status.mjs:82-94) looks only at required names, so the new checks can't holdfleet/verifyor turn it red. All of them pass at this head anyway. - Workflow triggers: #211 adds no new
pull_request/pull_request_targetworkflow.marketplace-watch.ymlgains apushtrigger only. At this head the self-test's workflow scan listsCI,CodeQL,docker,PR triageandtruecopy gate, and all five are listed inworkflows: [CI, CodeQL, docker, 'truecopy gate', 'PR triage'](.github/workflows/fleet-status.yml:18). - New linters now cover this PR's workflow:
actionlintand CodeQLanalyze (actions)both pass at this head. The code-scanning query forrefs/pull/212/mergereturns no open alerts.
Boundaries I rebuilt, and what pins each
facts.files.length > 100(fleet-status.mjs:56): 99 and 100 docs files are not code, 101 is. A bot PR over 100 files is still exempt (fleet-status.test.mjs:131-135,:202-204). 0 files is not code: verify and second-read go green and review waits on Redline, and there is nothing to verify.requiredCiState: no required checks →none; one not reported yet ('') or running →pending; a failure alongside a pending check →failed; a failure then a passing rerun of the same name →passed;skipped/neutral→ passed; thefleet/*contexts are excluded even when they are required and pending. Each has its own check (fleet-status.test.mjs:229-238,:269-286). A completed check run with anullconclusion maps to'', so it stays pending and can't go green.verifiedAtHead: needs both the label and the comment; takes the latest verifier comment; a 7-char prefix counts;findings/blockedheadings don't count (fleet-status.test.mjs:52-58,:149-151). It is consulted only whenrequiredCi === 'none', and pending CI with a stale label stays pending (:249).secondReadAtHead: another login, another head, aDISMISSEDstate,READY, mostly, a later lineless review, and U+002D/003A/2013/2014 separators are each pinned (fleet-status.test.mjs:106-110,:174-187,:319-330,:370-374).NOT READYISHcan't match, because\bneeds a word boundary afterREADY. A CRLF body ends the.*capture before\r.fitat 140/141 characters:fleet-status.test.mjs:221-222.collectPages: pages of[5],[100, 1]and[100, 100], plus a required check that only appears on page 2 (fleet-status.test.mjs:390-393). An empty first page returns[]immediately, so it can't loop.- Workflow
if::workflow_dispatchhas nopull_request, sostatusis skipped and onlybackfillruns. Fork events are dropped byhead_repository/head.repoin the workflow and again inreadFacts(fleet-status.mjs:237).issue_commenton a plain issue is dropped byissue.pull_request != null.
Tests that could pass without the change
I found none. Each own-context check fails if the OWN_CONTEXTS filter at fleet-status.mjs:83 is removed. Each separator check fails if its character is dropped from the strip. The workflow-list checks fail if a listed workflow is removed.
PR body claims vs the diff at this head
- "the base branch requires
test (6 OS/Node combinations),analyze (javascript-typescript)": the ruleset lists exactly those 7 contexts, even though CI now runs 9 test combinations. - "116 unit tests": matches the
self-testlog at38777ab. - "runs the default branch's copy … never the PR's code":
statusandbackfillcheck outgithub.event.repository.default_branchwith a sparse checkout of the script andpersist-credentials: false. The step is gated onhashFiles, so it is skipped until this merges. - "No concurrency group … up to three passes":
fleet-status.mjs:282-299.
What's good
The only inputs that could be injected are numeric (PR comes from event number fields), and no ${{ }} reaches a run: script in the status job. Unreadable rules or checks fail closed to pending (fleet-status.mjs:243-261), so an API error can never turn fleet/verify green.
SECOND READ: READY
What does this PR do?
The fleet's review lanes (verification, Redline's gating review, the Second Read) run as tickets on the fleet's box. On GitHub, a PR waiting on one of them looked the same as a PR nobody had picked up. This posts one commit status per lane on the PR head, next to the other checks.
fleet/verifyfleet/reviewfleet/second-readHere the base branch requires
test (6 OS/Node combinations),analyze (javascript-typescript). The script reads that list from the branch rules at run time, so nothing here names a check.workflow_runre-runs it when anypull_requestorpull_request_targetworkflow finishes, and a test fails if one is missing from that list, so a required check added later from any workflow still refreshes the lanes. That includespull_request_targetworkflows (PR triage): their checks land on the PR head too, so the status job accepts theirworkflow_runevents.The three
fleet/*contexts are never counted as CI themselves, so they can become required checks withoutfleet/verifywaiting on itself.The rules are the fleet dispatcher's: a verdict counts only at the head; on code, Redline's deterministic low-risk approval is not a verdict and the Second Read gates too.
statusjob runs the default branch's copy ofscripts/fleet-status.mjs, never the PR's code. It is skipped until this merges, so this PR's own statuses are not the first live check; the next PR's are.checks: read,statuses: write, reads only otherwise). It does not use the fleet's GitHub quota.self-testruns the script's 116 unit tests on the PR's code, read-only.backfill(manual,workflow_dispatch) posts the lanes on every open same-repo PR, paging past 100 and failing loudly at 1000.Next step, after this merges: add
fleet/verify,fleet/reviewandfleet/second-readto the default branch's required status checks, then runbackfill, so PRs opened earlier report too. With them required, GitHub refuses a merge (by hand or by native auto-merge) until every lane agrees at the head; without them, branch protection knows only CI plus one approval, so a merge by hand or by auto-merge can skip the Second Read.How to test
node scripts/fleet-status.test.mjs: 116 pass, 0 fail.String.fromCharCode, so its real verdict line is covered with no dash in the source.