ci: fleet review lanes show up as commit statuses on the PR head - #32
Conversation
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 workflow, status calculation, or unit-test coverage reviewed.
What's good: the workflow executes the default-branch script with narrowly scoped read/status permissions; the status logic keeps verdicts SHA-scoped and distinguishes required-CI verification from Breaker verification. I also checked the required CI signal at 573c703: all seven required checks are passing. I did not run the local suite; CI is the test signal.
…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).
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).
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 change does what it says as merged. The follow-up the PR body prescribes (make the fleet/* contexts required) would stop fleet/verify from ever going green on a code PR, and after that every code PR is blocked for good.
Findings
1. High: required fleet/* contexts feed back into requiredCiState and deadlock fleet/verify
scripts/fleet-status.mjs:225-226 (CLI block, live head 874c199) builds the required list straight from the branch rules:
required = rules.filter((r) => r.type === 'required_status_checks')
.flatMap((r) => (r.parameters?.required_status_checks ?? []).map((c) => c.context));and then scripts/fleet-status.mjs:236 hands the PR head's commit statuses, which include the script's own fleet/* statuses, to:
requiredCi = requiredCiState(required, [...statuses, ...checks]);The PR body's Next step is to "add fleet/verify, fleet/review and fleet/second-read to the default branch's required status checks". Once that is done:
- On the first run at a new code head, none of the
fleet/*contexts has reported yet, sorequiredCiStatereturns'pending'(s === ''). laneStatusesthen postsfleet/verify= pending, andgatedholdsfleet/reviewandfleet/second-readat pending.- On every later run,
fleet/verifyis required andpending, which matchesCHECK_WAITING, sorequiredCistays'pending'.verifiedis false on every run.
I reproduced this against the script at both 573c703 and 874c199. Inputs: required = ['analyze (actions)', 'fleet/verify', 'fleet/review', 'fleet/second-read'], CI green, Redline APPROVED and SECOND READ: READY at head. Each run's output was fed back in as statuses:
run1 pending -> fleet/verify=pending fleet/review=pending fleet/second-read=pending
run2 pending -> fleet/verify=pending fleet/review=pending fleet/second-read=pending
run3 pending -> fleet/verify=pending fleet/review=pending fleet/second-read=pending
Because those contexts would be required, GitHub then refuses every merge of a code PR, including when all three lanes agree. Adding the verified label and the Verification comment does not help, because ci !== 'none' skips verifiedAtHead. The test checks nobody requires do not hold it (fleet-status.test.mjs) covers the case where fleet/verify is not required. Nothing covers the case this PR sets up next.
The same list-building code is in askalf/dario#1419 at 2e0f646 (the source this PR says it matches), so the fix probably needs to go there too.
Suggested fix:
required = rules.filter((r) => r.type === 'required_status_checks')
.flatMap((r) => (r.parameters?.required_status_checks ?? []).map((c) => c.context))
.filter((c) => !Object.values(CONTEXTS).includes(c));A pure-function version of the fix, if you prefer it in requiredCiState so it can be tested:
export function requiredCiState(required, checks) {
required = required.filter((r) => !Object.values(CONTEXTS).includes(r));
if (!required.length) return 'none';Suggested test:
check('the fleet lanes themselves are never required CI',
requiredCiState(['test', CONTEXTS.verify, CONTEXTS.review, CONTEXTS.secondRead],
[ok('test'), { name: CONTEXTS.verify, state: 'pending' }]) === 'passed');What I checked
- The head moved from
573c703(the head the ticket named) to874c199while I was reviewing, so this review covers the live head. I read the delta (4ff4c1a,874c199): it addseditedreviews anddeletedcomments as triggers, drops the concurrency group, and addspostedSinceso an older run skips a context a newer run has already posted. The ordering logic and its 5 new tests are sound. Its tie case (a status posted in the same second as the read gets overwritten) is deliberate and pinned by a test.isBotPris unchanged apart from its doc comment, and it gets 5 more tests. - Boundaries in the lane logic.
requiredCiState: empty required list givesnone; unreported, running and failed checks each give the documented result; the last result per name wins; skipped and neutral pass.verifiedAtHead: label without comment, comment without label, a 7-char prefix, an older head, a later older comment, another login, and thefindings/blockedheadings.secondReadAtHead: no line,READY, mostly, the last line in a body, the latest review at head, a later review without the line, an old-head verdict. The 140/141-character cut and the 99/100-file threshold. Each has a pinning test that fails when the predicate is flipped, and I found no vacuous assertion. - The deterministic-approval exclusion applies only on code, and a docs PR still shows Redline's verdict. That matches the body's table.
- Workflow:
permissions: {}at the top, with per-job grants matching the body. Thestatusandbackfilljobs sparse-check-out the default branch's script, so PR code is never run withstatuses: write.self-testhascontents: readonly. The fork filter is in both the jobifand the script.workflow_runlistsciandCodeQL, which are the workflows (ci.yml,codeql.yml) that produce the seven required contexts I read from the branch rules. - Status and check-run ordering:
/statusesis newest-first and gets.reverse()d; check-runs are sorted byid. In both cases "last wins" means newest wins. - I ran
scripts/fleet-status.test.mjsonce at each head: 73 pass at573c703, 83 pass at874c199, 0 fail. The body still says 73; at874c199the count is 83. CI (gh pr checks) was green at573c703.
What's good
The lane rules are pure and have thorough tests. Both statuses and the fork filter are handled read-only where they need to be. Running the default branch's script under statuses: write is the right trust boundary for this.
SECOND READ: NOT READY — fleet-status.mjs keeps its own fleet/* contexts in the required list, so once they are made required (the body's next step) fleet/verify stays pending on every code PR
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: approved — no blocking issues found at e1bfa3b5e736d6b959ace73e1c92fae5fd8de571.
What I checked: all seven required CI checks pass at the live head. I reviewed the workflow trust boundary and the status computation, including required-check filtering, status/check ordering, stale-run suppression, fork handling, verification/review gates, and the new regression tests. In particular, scripts/fleet-status.mjs:209-220 filters the three self-posted fleet/* contexts before evaluating required CI, and scripts/fleet-status.test.mjs:664-681 pins both pending self-statuses and three successive status-feedback rounds. This resolves the prior deadlock scenario without excluding actual required CI. I also reviewed the latest head delta and its commit messages; no generated-writing tell or security-sensitive privilege expansion was introduced. I did not run the local suite, per review policy; required CI is green.
What's good: .github/workflows/fleet-status.yml:43-66 checks out the default branch's status script for the job holding statuses: write, while the PR-code self-test remains read-only. That preserves the intended trust boundary.
…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 2a3ba22. The self-wait from my last read (at 874c199) is fixed, and a test pins the fix that fails without it.
The ticket named e1bfa3b. The live head is 2a3ba22, one commit later, and that commit changes only test names, test comments and one workflow comment. This review is at 2a3ba22.
Previous finding, re-checked
scripts/fleet-status.mjs:82
required = required.filter((r) => !OWN_CONTEXTS.has(r));The three fleet/* contexts are dropped before anything else is read. The tests that pin it would fail without the filter:
own lanes required and pending, CI green: still passed. Without the filter,fleet/verifyis PENDING, so the result ispending.only own lanes required: none. Without the filter, the result ispending.own lanes required, three rounds: all three green. This feeds each round's output back in as the next round's checks. Without the filter, round 0 has thefleet/*contexts unreported, so the result ispendingand stays pending, and the assertion fails.
Boundaries I rebuilt from the diff
| predicate | input | behaviour | pinned by |
|---|---|---|---|
facts.files.length > 100 |
99 / 100 / 101 docs files | not code / not code / code | 99 docs files, exactly 100 docs files, more than 100 files |
| same, with CI | 101 docs files, requiredCi: 'passed' |
verify green | more than 100 docs files still verifies once required CI passes |
requiredCiState |
[] / only own / own + CI green / own + CI running |
none / none / passed / pending | four checks in the own-lanes block |
requiredCiState last-wins |
fail then success for the same name | passed | a rerun that passed |
secondReadAtHead |
DISMISSED at head / lineless later review / older-head verdict | skipped / keeps verdict / none | three checks, one each |
fit |
140 / 141 chars | kept whole / cut to 137 + ... |
the 140-character edge |
statusesToPost |
identical / state differs / missing / description differs | skip / post / post / post | four checks in the post-then-verify block |
Every row I could reach has a test. I found no assertion that passes both with and without the code it is meant to cover.
Notes (non-blocking)
scripts/fleet-status.mjs:112skipsr.state === 'DISMISSED'for the Second Read. This seat only posts--commentreviews, and GitHub refuses to dismiss a COMMENTED review. So this branch guards a state the live data does not produce today. The skip is harmless and stays correct if that ever changes.scripts/fleet-status.mjs:246reads check-runs with a single?per_page=100and does not paginate. Past 100 runs, a required check could be missed. The code then treats it aspending, so it cannot turnfleet/verifygreen by mistake. This repo has about 13 runs per head.- The PR body says the base requires "the seven scan/verify fixtures,
analyze (actions)". The live ruleset onmasterlists six scan/verify fixtures plusanalyze (actions), seven in total. The script reads the list at run time, so this affects only the body's wording.
What I checked
- The full diff (3 files, +727).
gh pr checksat2a3ba22: all required checks andself-testpass.- 94
check(calls in the test file, which matches the body's 94. - No non-ASCII bytes anywhere in the diff.
scripts/fleet-status.mjshas the same blob (8b338ee) as at askalf/dario#1419's head. I could not fetch the test file there to compare it.- The trust boundary:
statusandbackfillcheck out and run only the default branch's script. The PR number reaches the script only throughenv, never interpolated intorun:. Fork PRs are filtered in both theif:andreadFacts.self-testruns withcontents: readonly.
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: request changes — a public test adds an em dash, a generated-writing tell that must be removed. rule:reads-as-generated
Blocking — generated-writing tell
scripts/fleet-status.test.mjs:70
review(SECOND_READ_LOGIN, 'COMMENTED', OLD, 'text\nSECOND READ: NOT READY \u2014 stale stack'),
This test input produces an em dash in the Second Read text. First-party strict mode treats an em dash in public code or tests as a blocking generated-writing tell. The parser only needs a NOT READY line to establish that an older-head verdict remains stale; a plain ASCII separator exercises that behavior without the tell.
review(SECOND_READ_LOGIN, 'COMMENTED', OLD, 'text\nSECOND READ: NOT READY - stale stack'),
What's good: required CI is green at 2a3ba22; I reviewed the workflow's event/permission boundaries, status reconciliation loop, required-check exclusion, and the unit coverage for lane state transitions.
I did not run the local test suite, per review policy; CI is the test signal.
…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.
…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: approve — no blocking issues found in the live head.
I reviewed the complete 773-line addition: the event and permission boundaries in .github/workflows/fleet-status.yml, the status/verification and review-state logic in scripts/fleet-status.mjs, and its unit coverage in scripts/fleet-status.test.mjs. In particular, I checked the required-CI gate, same-head verdict checks, dismissal handling, bot and documentation exemptions, status convergence loop, and the workflow list guard. All required CI checks are passing at c63cfad. I did not run the test suite locally, per review-environment policy; CI is the execution signal.
What's good: the workflow checks out the default branch’s status script with a minimally scoped token, avoids treating its own statuses as CI, and the unit tests cover the relevant stale-head and rerun-state transitions.
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: one reachable row is left untested. The Second Read's em dash is the only separator it actually posts, and after the ASCII-fixture change no test covers it. The PR body still says one does.
Read at c63cfad: the full delta since my last read at 2a3ba22 (9c40f53, 006ea23, c63cfad: scripts/fleet-status.mjs +8/-7, scripts/fleet-status.test.mjs +49/-4, .github/workflows/fleet-status.yml +1/-1). I also read laneStatuses, requiredCiState and the readFacts/post loop in context, and the on: blocks of all six workflows. CI is green at this head, and self-test reports 103 pass, 0 fail.
Medium: the em dash separator is a reachable row with no test, and the body says a test covers it
scripts/fleet-status.mjs:117-118
// Drop the one separator after NOT READY; keep a leading backtick, quote or bracket.
? { state: 'NOT READY', reason: last.replace(/^NOT READY\s*(?:[^\w\s`'"([{]\s*)?/, '').trim() }The Second Read posts its verdict in exactly one shape: SECOND READ: NOT READY + + U+2014 + <reason>. The commit message of 9c40f53 names that case ("hyphen, colon, or the Second Read's own dash"). But 9c40f53 also changed the two fixtures that held it ('...NOT READY \u2014 stale stack' and '...NOT READY \u2014 commit subject has an em dash') to ASCII hyphens, at scripts/fleet-status.test.mjs:73 and :99. The four new separator tests at scripts/fleet-status.test.mjs:327-330 cover hyphen, colon, code span after a hyphen, and no separator. The test file now contains no \u2014 and no non-ASCII character at all.
Failure scenario: someone narrows the class to the separators the tests show, say /^NOT READY\s*(?:[-:]\s*)?/. All 103 tests still pass. I ran both regexes on the real verdict shape:
"NOT READY \u2014 `x` is null" current => "`x` is null" narrowed => "\u2014 `x` is null"
With the narrowed regex, every real NOT READY status would read NOT READY at <sha>: — ..., and nothing would fail. The current code handles the case correctly; no test pins it.
The PR body says the opposite: "(the Second Read's dash appears in a test only as an escape)". That was true before 9c40f53 and is false at this head.
Suggested fix (the escape keeps the file ASCII, which is what the body describes):
check('the Second Read\'s own dash separator is dropped', reasonOf('SECOND READ: NOT READY \u2014 `x` is null') === '`x` is null');Checked, no finding
- Fail-closed rules (
scripts/fleet-status.mjs:236-252): rules unreadable →required = null→'pending'. Rules read with none required →'none'→ label-and-comment rule. Rules list onlyfleet/*contexts → fetched, thenrequiredCiStatefilters them out (:82) →'none', sofleet/verifynever waits on itself. A statuses or check-runs read failure →'pending'. None of these paths can turnfleet/verifygreen on a read error. - Bot rule vs file count (
scripts/fleet-status.test.mjs:201-203): the bot test now uses 101 files, so it fails if the bot rule is removed or reordered after the >100 rule. The 101-file person counterpart pins the other side. workflow_runlist test (scripts/fleet-status.test.mjs:333-361):\bpull_request\b(?!_target)does not matchpull_request_target(_is a word character).labels.ymlruns on path-filteredpull_requestand is listed, andpr-triage.ymlruns only onpull_request_targetand is left out; the CI log shows all four rows. The test can fail: droplabelsfrom.github/workflows/fleet-status.yml:18and its row fails. Thename:regex is anchored at column 0, so a job-levelname:cannot shadow the workflow name.- The script matches askalf/dario#1419's copy byte for byte at
e9aabb5, as the body says.
What's good: the rework makes every read failure pending, and the list test runs in both directions, so it cannot pass just because the list is short.
SECOND READ: NOT READY — no test pins the em dash separator, the only one the Second Read posts (fleet-status.test.mjs has no \u2014), and the body says one does
…n escape) The ASCII-fixture change left no test on the only separator the Second Read actually posts. Narrowing the strip class to [-:] now fails this case. Ticket FIX-6c0a53d9
… 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.
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 documentation-only .txt PR is classified as code and is incorrectly held for verification. rule:none
Blocking — scripts/fleet-status.mjs:163-166
if (/\.(md|svg|png|jpe?g|webp|gif)$/i.test(path)) return false;
if (/^docs\/.*\.txt$/i.test(path)) return false;
The first-party exemption covers documentation and assets including .txt files, not only .txt files below docs/. A same-repository PR that changes only a root NOTICE.txt (or changelog/release-notes.txt) reaches line 167 as a code path. needsVerify() then returns true, so fleet/verify remains “Waiting on the Breaker” instead of reporting the exempt success state. This makes a docs-only PR wait for a verification that should not be required.
if (/\.(md|txt|svg|png|jpe?g|webp|gif)$/i.test(path)) return false;
if (/^docs\/.*\.txt$/i.test(path)) return false;
Add a unit case for a .txt file outside docs/ so this classification remains aligned with the exemption rule.
I checked the live-head workflow, status computation, and associated unit-test changes. All required CI checks are passing at fcfaa5a.
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 2ad71ce. My last NOT READY (at c63cfad: no test covered the em dash separator) is fixed, and every claim in the body matches the code.
The ticket was cut at fcfaa5a, but the head moved to 2ad71ce (backfill paging) before I started. A verdict only counts at the head, so this read covers 2ad71ce: all three files in full (scripts/fleet-status.mjs, scripts/fleet-status.test.mjs, .github/workflows/fleet-status.yml), plus the c63cfad..2ad71ce delta and gh pr checks.
What I checked
Prior finding, now closed. scripts/fleet-status.test.mjs:364-375:
for (const code of [0x2d, 0x3a, 0x2013, 0x2014]) {
...
check(`separator U+${hex} is dropped`, reasonOf(`SECOND READ: NOT READY ${String.fromCharCode(code)} stale stack`) === 'stale stack');
}
const line = `SECOND READ: NOT READY ${String.fromCharCode(0x2014)} \`x\` is null`;Both new assertions can fail. If the separator strip in fleet-status.mjs (/^NOT READY\s*(?:[^\w\s'"([{]\s*)?/) were narrowed to ASCII [-:], the U+2014 case would return " stale stack". The end-to-end case goes through laneStatuses, so it pins the real verdict line (em dash, then a reason that opens with a code span) all the way to a red fleet/second-readdescription ending in ``xis null ``. CI's self-test log at this head showsseparator U+2014 is droppedok and110 pass, 0 fail`.
Backfill delta. .github/workflows/fleet-status.yml:113-119. I rebuilt the boundaries myself:
| input | behaviour | pinned by |
|---|---|---|
| 0 open PRs | prs is empty, the loop does nothing, exit 0 |
not tested, but trivially correct |
| 1 to 999 | gh pages on its own, and every same-repo PR is posted |
backfill reads more than one page (limit > 100) |
| exactly 1000 (the cap) | ::error:: + exit 1. This is conservative because it can't tell 1000 from more. |
backfill fails at its cap (-ge ${limit}) |
| fork PRs | dropped by `select(.isCrossRepository | not)`, and the script checks again |
The two workflow tests read the YAML's text, so they can fail: lowering --limit to 100 or removing the -ge/::error:: guard breaks them.
Other boundaries I rebuilt from the diff:
requiredCiState:[]givesnone.- A list of only its own
fleet/*contexts givesnone(lines 237 and 271). So the self-wait I raised at874c199stays fixed. - A required check that is missing, unreadable or still waiting gives
pending, and a non-pass conclusion givesfailed.
needsVerify:- At the 100/101 file edge, 101 is code no matter what the files are (lines 129-133 and 201-202).
- A bot branch opened by an outside author is still code.
secondReadAtHead:- A review at an old sha or a DISMISSED review doesn't count, and the last line at the head wins.
READY, mostlyis not a verdict.- CRLF bodies still match, because JS
$/.treat\ras a line end.
- Descriptions:
- Exactly 140 characters is kept.
- 141 is truncated to 140 (lines 144 and 214-220).
Body claims:
- The script, tests and workflow are plain ASCII. I checked this with a byte grep: no bytes ≥ 0x80.
workflow_runlistsci,CodeQLandlabels, which are exactly thepull_requestworkflows in.github/workflows/.PR triage(pull_request_target) is left out, and lines 352-361 enforce both.statuschecks out only the default branch'sscripts/fleet-status.mjsand is gated onhashFiles.- Permissions are exactly
contents/pull-requests/issues/checks: readandstatuses: write. - There is no concurrency group, and the re-read/correct loop is capped at three passes.
Not a finding, for whoever runs it: on a repo with hundreds of open PRs, backfill (about 8 API calls per PR per pass, timeout-minutes: 10) would hit the timeout or the GITHUB_TOKEN rate limit before it reached the 1000 cap. Both fail loudly under set -e, so no PR is skipped silently. At this repo's size it doesn't matter.
What's good: the lane rules are pure functions with the I/O kept separate. Everything that can't be read counts as pending and can never turn a lane green. The workflow-list test keeps the refresh triggers matching the repo's workflows as they change.
SECOND READ: READY
Dismissed for a re-read: this verdict came from Redline's pre-398 rule text (.txt exempt everywhere). Migration 398 (live since 04:54Z) states the dispatcher's rule: .txt is exempt only under docs/, which is what this script implements.
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 — required check-runs beyond the first API page are treated as missing, so the verify lane can remain pending forever after all required CI is green. rule:none
Blocking — correctness — scripts/fleet-status.mjs:247
const runs = (await (await gh(/repos/${repo}/commits/${p.head.sha}/check-runs?per_page=100, token)).json()).check_runs ?? [];
The endpoint is requested only once, unlike the statuses endpoint immediately above which uses ghAll. On a head with more than 100 check runs (for example matrix jobs, reruns, or check-producing integrations), a required check whose run falls on page two is absent from checks. requiredCiState() then receives no state for that required context and returns pending; fleet/verify consequently stays pending even though every required check passed. The new workflow is intended to make these lane statuses required, so that becomes a merge-blocking false pending state.
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 }));
Please add a regression case covering a required check returned only after the first page.
I checked the live-head diff and the required CI signal (all required checks are green). I did not run the local suite, per review-environment 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: approved — no blocking issues found in the live-head rework.
The replacement uses collectPages() for check runs at scripts/fleet-status.mjs:255-258, and the added tests exercise a required check returned on page two at scripts/fleet-status.test.mjs:374-383. I also checked the workflow change at .github/workflows/fleet-status.yml:19-20: PR triage is included and both pull-request event variants are admitted; its check run on this live head is attached to the PR head.
Required CI is green at ba0339182cefbf7b5ef8259cafbf650611baed7d. I reviewed the live-head diff and did not run the local suite, per review-environment policy.
What is good: the pagination helper is shared by the list endpoints and has direct boundary coverage for the first short page, a second page, and a full final page.
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: I found no blocking issues at ba03391. The one new commit fixes both points I raised at the previous head: check runs are now read across every page, and pull_request_target runs refresh the lanes too. Each fix has a test that can fail.
Scope of this read: I read the delta 2ad71ce..ba03391 (one commit, +41/-20 across the 3 files) in full, plus the surrounding readFacts() / requiredCiState() code in a read-only clone at ba03391. I read the rest of the PR at earlier heads, and it is unchanged since then. I did not run the tests locally. CI self-test at this head logs 115 pass, 0 fail, and every check passes (backfill is skipped, as it should be).
Boundaries rebuilt from the delta
collectPages(page, size = 100), scripts/fleet-status.mjs:145-151:
const rows = await page(n);
out.push(...rows);
if (rows.length < size) return out;| input | behaviour | pinned by |
|---|---|---|
| empty first page (0 rows) | returns [] after one call |
not pinned separately. It is the same < size branch as the 5-row case. |
| short first page (5) | one page, 5 rows | a short first page is the only page (test.mjs:392) |
exactly size (100) then more |
reads on | 101 rows over two pages are all read (test.mjs:390) |
| every page full (100, 100) | reads a third, empty page, then stops at 200 | a full last page reads one more, empty, page (test.mjs:391) |
| required check only on page 2 | requiredCiState sees it and returns passed |
a required check on page 2 counts (test.mjs:393). This fails if paging stops after page 1 (it would give pending). |
check-runs page with no check_runs key |
?? [] ends the loop |
.mjs:255-256. Not tested. An API error throws in gh() first (.mjs:216), and the catch makes it pending. |
The workflow condition is fleet-status.yml:29-32:
(github.event_name == 'workflow_run' &&
contains(fromJSON('["pull_request","pull_request_target"]'), github.event.workflow_run.event) &&- Did a
pull_request_targetrun land on the head? I checked the live run instead of trusting the comment. Run36100877419(PR triage) hasevent: pull_request_target,head_sha: ba03391...,head_repository: askalf/truecopy-actionandpull_requests: [{number: 32, head: ba03391...}]. It passes every clause of the condition, and itstriagecheck is listed on this head. This settles the reversal ofc63cfad: it is right. - Fork PRs: they are still dropped by the
head_repository.full_nameclause, and again byreadFacts()(.mjs:236). - Self-trigger loop:
fleet-status.ymlis not in its ownworkflow_runlist, and the statuses it posts do not raiseworkflow_run.scorecard.ymlnever runs on a PR, so leaving it out is correct.
Test assertions
test.mjs:355:if (!/\bpull_request(_target)?\b/.test(onBlock(y))) continue;does not matchpull_request_review, because\bfails betweentand_. So only real PR workflows are required in the list. Removing'PR triage'fromfleet-status.yml:18fails the per-workflow check forpr-triage.yml.test.mjs:360-361: deletingpull_request_targetfrom thefromJSONarray fails this check. It matches the text, not the parsedif:, which is the file's existing convention for workflow tests.
Claims in the body, checked against the diff
- "Check runs at the head are read across every page":
.mjs:255-256. True. - "
workflow_runre-runs it when anypull_requestorpull_request_targetworkflow finishes":.yml:18and.yml:30, enforced bytest.mjs:353-362. True. - "115 unit tests": the CI log says
115 pass, 0 fail. True. - "plain ASCII": a byte scan of all three files finds 0 bytes at or above 0x80. True.
Non-blocking note
The tests pin collectPages itself, not the fact that readFacts() calls it for check runs. If .mjs:255 went back to a single fetch, the tests would stay green. This matches the rest of the file, where the I/O block under import.meta.url has no tests and the rules are tested as pure functions. It is not a defect in this head.
What's good
Pulling the pager out into one pure helper, and building ghAll on it, removes the duplicate loop instead of adding a second one. The page-2 test uses the real requiredCiState, so it tests the actual failure (fleet/verify falling back to pending), not just a row count.
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 0248e36. I found one low, non-blocking inaccuracy in the PR body.
What changed since my last read (ba03391)
compare/ba03391...0248e36 has three commits: b2b1b9c (#31, random report delimiter), fdf9ee2 (#33, author-fixes label) and the merge 0248e36. None of them touches .github/workflows/fleet-status.yml, scripts/fleet-status.mjs or scripts/fleet-status.test.mjs, so this PR's own diff is byte-identical to the head I last read. I checked how the merged-in master changes interact with this PR:
ci.ymlstill hasname: ciand still runs onpull_request, so theworkflow_runlist stays complete. The new jobscan: a hostile manifest cannot forge the report outputhas a colon in its name, but nothing in the script parses check names..github/labels.jsongainsauthor-fixes.isCodePathtreats it as.githubconfig, and no lane reads that label.
Findings
Low (body only, non-blocking): the required-check count in the body is wrong at this head.
The body says: "Here the base branch requires the seven scan/verify fixtures, analyze (actions)."
GET /repos/askalf/truecopy-action/rules/branches/master lists six scan/verify contexts plus analyze (actions). After the master merge, ci.yml has seven scan/verify jobs, and the new scan: a hostile manifest cannot forge the report output is not required. The script reads the list at run time (scripts/fleet-status.mjs:382-384), so the code is right: fleet/verify will go green without waiting on the new job, and it will stay green if that job fails. Only the sentence is off. If the new job was meant to gate, that is a ruleset change outside this PR.
Suggested fix:
Here the base branch requires the six scan/verify fixtures and `analyze (actions)`.
Boundary pass (rebuilt from the diff)
| predicate | input | behaviour | pinned by |
|---|---|---|---|
fit (fleet-status.mjs:291) |
140 / 141 chars | kept whole / 137 + ... |
"exactly 140 characters is kept whole", "141 characters is cut..." |
needsVerify files.length > 100 (:191) |
99 / 100 / 101 docs files, bot PR with 101 files | not code / not code / code / exempt | "99 docs files", "exactly 100", "more than 100", "a bot PR with more than 100 files" |
requiredCiState own-context filter (:218) |
only fleet/* required; fleet/* required and pending | none / passed |
"only own lanes required: none", "own lanes required and pending, CI green", three-round loop (this fails without the filter) |
requiredCiState empty/unknown state (:224-226) |
not yet reported, in_progress, failure while another is pending, rerun that passed, skipped/neutral |
pending / pending / failed / passed / passed | the six requiredCiState checks |
rules or checks unreadable (:385, :396) |
HTTP error | pending, never green |
not unit-tested (CLI path); I read the catch arms, and both set a non-green state |
verifiedAtHead (:196-203) |
no label, older sha, 7-char prefix, other login, findings/blocked heading, latest-wins both ways |
as the rule says | the verifiedAtHead block (10 checks) |
secondReadAtHead (:248-255) |
other login, older head, DISMISSED, no line, READY, mostly, last line wins, later lineless review, NOT READY with no reason, separators - : U+2013 U+2014 |
none / none / skipped / none / none / last / kept / NOT READY at <h> / dropped |
the Second Read blocks, including the U+2014 line built with String.fromCharCode |
Redline deterministic approval (:237) |
code / docs | ignored / counts | "on code it is not Redline's verdict", "on docs it is" |
collectPages (:286) |
100+1, 100+100, 5 | 101 / 200 (reads one more empty page) / 5 | the paging block |
backfill cap (fleet-status.yml:121-125) |
1000 open PRs | ::error::, exit 1 |
"backfill fails at its cap" |
I didn't find a reachable row that behaves wrongly or has no test, apart from the CLI error arms noted above.
Tests that cannot fail
I read each assertion to see whether it could pass without the code it covers. "checks nobody requires do not hold it" would still pass without the own-context filter, because fleet/verify isn't in that required list. Its name says what it covers, and the three-round loop and "own lanes required and pending" tests do pin the filter. None of the other assertions I read would pass without its code.
Claims in the body
- "115 unit tests": the
self-testlog at this head shows115 pass, 0 fail. - Runs the default branch's script: yes,
ref: default_branchwith a sparse checkout, andhashFilesskips the step until the script exists there. - Fork PRs skipped: skipped in the job
ifand again inreadFacts(:372). - Check runs read across pages:
collectPagesoncheck-runs(:391-392). - No concurrency group; the script re-reads and corrects for up to three passes (
:417-434). pull_request_targetaccepted: yes (fleet-status.yml:36), and the test checks it.- Permissions:
checks: read,statuses: write, the rest read-only.
All of these hold, apart from the required-check count above.
What's good
The job runs only default-branch code under a scoped token, and PR is checked against ^\d+$ before it reaches any URL. A lane can't turn green on unreadable data, and fleet/verify can't end up waiting on its own status. CI is green at 0248e36.
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
the seven scan/verify fixtures,analyze (actions). 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 115 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: 115 pass, 0 fail.String.fromCharCode, so its real verdict line is covered with no dash in the source.