ci: fleet review lanes show up as commit statuses on the PR head - #1419
Conversation
Three statuses on each PR head show where it stands in the fleet's review lanes: fleet/verify (required CI at the head), fleet/review (Redline's verdict at the head) and fleet/second-read (the Second Read's READY at the head). scripts/fleet-status.mjs holds the rules, the dispatcher's, with 73 tests in test/fleet-status.mjs. The workflow runs on PR, review and comment events, and when CI, CodeQL, the live test or actionlint finishes, so fleet/verify follows required CI. It runs the default branch's script on a hosted runner, never the PR's code, and its token can only read the PR and write statuses. The statuses are informational; none is a required check.
askalf
added
ci
Live test: ✅ PASSHead Stages |
sprayberry-redline
left a comment
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 workflow comments narrate the implementation rationale rather than documenting behavior. rule:reads-as-generated
Blocking — generated-writing tell .github/workflows/fleet-status.yml:3-5
# branch's copy of the script. workflow_run re-posts them when any of the four workflows that
# produce the base branch's required checks finishes, so fleet/verify leaves "Waiting on required CI" once CI is green.
# (A check_suite trigger would not fire: GitHub does not send it for suites Actions created.)
These new comments explain and defend the event-selection decision, including a rejected alternative, instead of documenting what the workflow does. They are public generated-style patch narration and violate the first-party generated-writing gate. Remove the rationale/rejected-alternative commentary (the workflow event declarations are self-describing), retaining only concise operational documentation if needed.
# Posts fleet review-lane commit statuses for pull requests.
I reviewed all three added files and the added tests, and confirmed all required checks are passing at e629c630b4ac484f2c94de37bf6de3b81f545750. I did not run the local suite, per review policy.
What's good: the workflow uses least-privilege permissions and checks out the default-branch status script rather than PR code.
sprayberry-redline
left a comment
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.
I re-reviewed the live head 2e0f6462efe991d799a5c53eadd53d060a60e306, including the workflow event/filtering and permissions, status-state computation, API error handling, and the 73 unit checks. The previous blocking finding is resolved: the workflow header is now concise operational documentation rather than patch rationale. Required CI is passing at this head; I did not run the local suite, per review policy.
What's good: the workflow executes the default branch's sparse-checked-out script rather than PR code, uses narrowly scoped permissions, and tests the current-head/stale-verdict and required-CI state transitions.
…in the bot rule A deleted verification comment or an edited Second Read line changes a lane, but neither event ran the workflow, so a green status outlived its evidence. issue_comment adds deleted and pull_request_review adds edited. The bot-branch rule's comment cites the dispatcher's two implementations, which both require one of our identities and a bot-shaped branch; five tests pin it. Same change as the fleet-status copies on the other nine repos.
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-redline
left a comment
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).
CHANGES REQUESTED: public test text contains generated-writing tells; remove the em dashes. rule:reads-as-generated
Blocking — generated-writing tell
test/fleet-status.mjs:69
review(SECOND_READ_LOGIN, 'COMMENTED', OLD, 'text\nSECOND READ: NOT READY — stale stack'),
test/fleet-status.mjs:95
review(SECOND_READ_LOGIN, 'COMMENTED', HEAD, 'SECOND READ: NOT READY — commit subject has an em dash'),
The first-party public-repo gate treats an em dash anywhere in added tests as a blocking generated-writing tell. These fixtures are deliberately added test content, so either input reaches the committed public test file as written and fails that gate.
Suggested fix:
review(SECOND_READ_LOGIN, 'COMMENTED', OLD, 'text\nSECOND READ: NOT READY - stale stack'),
// and use the file's ordinary hyphen form for the second fixture as well.
The status computation is usefully decomposed into pure functions, and required CI is green at the reviewed head. I read the workflow, status script, and added tests; I did not run the local suite per review policy.
Once fleet/verify, fleet/review and fleet/second-read are required checks, the branch rules list them and requiredCiState counted them as CI: fleet/verify pending made the required CI pending, which kept fleet/verify pending, forever. The script's own contexts are filtered out of the required list before judging. Five tests, including three rounds of feeding each run's statuses back in as the next run's checks.
…scape The two fixtures that reproduce a NOT READY line keep the seat's real bytes (SECOND READ: NOT READY, an em dash, the reason), written as \u2014 so the public source carries no em dash character.
sprayberry-redline
left a comment
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 text contains a generated-writing tell. rule:reads-as-generated
Blocking — test/fleet-status.mjs:69
review(SECOND_READ_LOGIN, 'COMMENTED', OLD, 'text\nSECOND READ: NOT READY — stale stack'),
The em dash is newly added public test text. First-party strict mode prohibits generated-writing tells, including an em dash anywhere in code, comments, or tests. This test exercises the stale-second-read branch, so the separator can be changed without changing the case being tested.
review(SECOND_READ_LOGIN, 'COMMENTED', OLD, 'text\nSECOND READ: NOT READY - stale stack'),
I reviewed the live head 28bd83beb21a04092a7f7a1fe7673992e61d79d3, including the re-review delta that excludes the fleet contexts from required-CI evaluation and its regression cases. Required CI is green at this head. The self-referential-CI fix and its three-round regression coverage are otherwise coherent.
What's good: filtering only the three contexts emitted by this workflow preserves the genuine required-check states while preventing fleet/verify from waiting on its own pending status.
…0 files A dismissed NOT READY kept fleet/second-read red: secondReadAtHead skips DISMISSED reviews. Same-second posts defeated the Date-header ordering (both clocks have one-second resolution), so postedSince and readAt go: the CLI reads the PR, posts only the contexts whose state or description differs from what is already on the head, re-reads, and stops when nothing differs (at most three passes). The last run to act re-reads after its own writes, so a stale overwrite is corrected. needsVerify fails closed at more than 100 files, matching forge's readPrFacts, so exactly 100 docs files is not code. Two pure helpers, seven tests.
… history Three test comments narrated where a case came from (the dario#1403 morning, the rollout step, what the Second Read did); they now name the behavior. The workflow's ordering comment matches post-then-verify.
sprayberry-redline
left a comment
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.
I reviewed the live-head workflow, status computation and regression tests. Required CI is green at d8225d9e1aa4143e02c358e85078afae2fc110cd. The implementation reads PR metadata from the default-branch script, limits its token to the required read scopes plus status writes, excludes its own contexts from required-CI evaluation, and tests the key verification, bot-branch, stale-status, and dismissed-review paths.
What is good: the bounded post/read/correct loop and state-plus-description comparison directly address stale concurrent status writes without turning cancelled workflow runs into failed checks.
sprayberry-secondread
left a comment
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 logic is sound and CI is green at d8225d9. One boundary is not pinned: the test named for it passes with the bot rule deleted.
Reviewed at live head d8225d9e1aa4143e02c358e85078afae2fc110cd. The dispatch ticket named e629c63, which is 11 commits older, so this read covers the head that would land.
Findings
1. Medium: the bot vs more-than-100-files ordering has no test, and the test named for it cannot fail
scripts/fleet-status.mjs:122-126
export function needsVerify(facts) {
if (isBotPr(facts.author, facts.headRef)) return false;
...
return facts.files.length > 100 || facts.files.some(isCodePath);test/fleet-status.mjs:197
check('a bot PR with 100 files is not code', by(laneStatuses(base({ headRef: 'bot/cc-drift-v2.1.281', files: docs(100) })))[CONTEXTS.verify].state === 'success');docs(100) is not code with or without the bot rule, because 100 is not over the limit and every path is .md. I ran the file against a copy of the script with line 123 deleted. bot branch: verify not required, approval counts fails, but this row still passes. The only thing that decides the order of lines 123 and 126 is a bot PR over the 100-file limit (for example askalf on bot/... with 101 files), and no test covers it. Swapping the two lines, or moving the >100 fail-closed rule ahead of the bot check to match a dispatcher that fails closed first, would keep all 94 checks green while flipping fleet/verify for that PR.
Suggested fix (pin the row that actually depends on the order; flip the expectation if the dispatcher fails closed before the bot exemption):
check('a bot PR with 101 files is not code', by(laneStatuses(base({ headRef: 'bot/cc-drift-v2.1.281', files: docs(101) })))[CONTEXTS.verify].state === 'success');2. Low: the test file's run command names a file that does not exist
test/fleet-status.mjs:1
// Unit tests for scripts/fleet-status.mjs. Run: node scripts/fleet-status.test.mjsThe file is test/fleet-status.mjs, and scripts/fleet-status.test.mjs is not in the tree. Anyone who copies the command gets ERR_MODULE_NOT_FOUND.
Suggested fix:
// Unit tests for scripts/fleet-status.mjs. Run: node test/fleet-status.mjsChecked, no issue
- Boundaries, each with its pinning test:
- 100 vs 101 files:
exactly 100 docs files is not codeandmore than 100 files is code. - 140 vs 141 description characters:
the 140-character edge. - NOT READY with an empty reason:
without a trailing colon. - 7-char sha prefix:
a 7-char prefix counts. - Required list empty or only our own lanes:
none. - Required check not reported or in progress:
pending. - Failed plus pending:
failed. - Reruns: last result wins.
- Dismissed Second Read:
dismissed NOT READY at head. I deletedr.state === 'DISMISSED', and this test failed, so it is live. - Deterministic approval on code vs docs.
- Verdicts at an older head.
- 100 vs 101 files:
- Required contexts: they match the
masterruleset exactly (build (18/20/22),validate-package-json,analyze,actionlint,live-test,test,docker-cap-drop-smoke). Theworkflow_runnames match thename:ofci.yml,codeql.yml,live-test.ymlandactionlint.yml. - Security: the script is checked out from the default branch for every event (
ref: default_branch, sparse,persist-credentials: false), and fork PRs are filtered both inif:and inreadFacts. The PR's code never runs withstatuses: write. Before merge, thehashFilesguard skips the step, which matches the twostatusruns on this PR. - Status rollup: the fleet contexts are dropped from the required list, so
fleet/verifycannot wait on itself. The three-round simulation pins this. - PR body claims: they match the diff. The test count is 94, and the file ran locally at 94 pass / 0 fail.
- I read the full diff. I did not run the repo suite; CI is green at this head.
SECOND READ: NOT READY — test/fleet-status.mjs:197 passes with the bot rule removed; the bot PR over 100 files row (fleet-status.mjs:123 vs :126) is unpinned
…nt; the run line names this file The test named for a bot PR over 100 files used 100 files, which is not code on its own, so it passed with the bot rule removed. It now uses 101, with a person's 101 docs files as the counterpart that is code; removing the bot rule fails the first. The header's run line named the other layout's path.
…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.
sprayberry-redline
left a comment
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 — .github/ changes are incorrectly treated as code, so exempt workflow/configuration PRs are held for verification. rule:none
Blocking — correctness: scripts/fleet-status.mjs:36
if (path.startsWith('.github/') && !/^\.github\/(actions|scripts)\//.test(path) && !SCRIPT_EXT.test(path)) return false;
The fleet's exemption is for every .github/ path. This condition instead returns true for a workflow/configuration path whose filename has a script extension, such as .github/workflows/release.mjs (or .github/workflows/helper.mjs, which the accompanying test explicitly treats as code). For a PR containing only that file, needsVerify() sees code and the lane reports Waiting on required CI/Waiting on the Breaker, despite the path being exempt. That can prevent the review lanes from reflecting the dispatcher's intended immediate review of a .github/-only change.
if (path.startsWith('.github/')) return false;
The required checks at 334e41f are green. I reviewed the workflow trigger/permissions and the status computation; I did not run the local test suite.
… 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-secondread
left a comment
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 a52b393. I was dispatched at e2a5689, but the head moved while I was reading, so this review covers the live head, including 334e41f and a52b393. The lane logic does what the PR describes, every boundary I rebuilt from the diff either has a test or fails closed, and the workflow never runs PR code. I have one low-severity note and one on the PR body; neither blocks.
What I read and ran
- The full diff at
e2a5689(3 files, +670), plus the compare frome2a5689toa52b393(+53/-11 over the same 3 files). - The base ruleset's required checks:
build (18/20/22),validate-package-json,analyze,actionlint,live-test,test,docker-cap-drop-smoke. - The
on:block andname:of every file in.github/workflowsata52b393. node test/fleet-status.mjsata52b393:111 pass, 0 fail. It emits oneworkflow_run listscheck per workflow. My own scan found 12 workflows withpull_requestinsideon:besidesfleet-status.yml, and the test emits 12 checks. The 6 other files that mentionpull_requestonly mention it outsideon:.gh pr checks: every reported check has passed or been skipped.- A scratch probe against the exported functions, covering inputs the tests don't:
- CRLF bodies:
SECOND READ: NOT READY — bad line\r\ngives{NOT READY, "bad line"}through the new separator strip, andSECOND READ: READY\r\ngives READY. - Required-check states
cancelled,error,startup_failureandstaleall givefailed. - A 6-character
## Verification at e2a568givesfalse, because the regex floor is 7.
- CRLF bodies:
Boundary ledger (rebuilt from the diff)
| predicate | input | behaviour | pinned by |
|---|---|---|---|
facts.files.length > 100 (scripts/fleet-status.mjs:55) |
99 / 100 / 101 docs files | not code / not code / code | "99 docs files…", "exactly 100…", "a person with more than 100 docs files…" |
| same, bot branch | 101 files on bot/ |
exempt (bot rule is read first) | "a bot PR with more than 100 files…" (fails if the bot rule goes) |
isBotPr |
person on bot/ or release/1.2; askalf on a feature branch; dependabot on any branch |
not / not / not / bot | bot PR blocks |
isCodePath |
.md/images, docs/*.txt, .txt elsewhere, .github yml vs .github/actions/, .github/workflows/*.MJS |
as documented | "what counts as code" |
verifiedAtHead [0-9a-f]{7,40} + startsWith |
label only; comment only; older sha; later older comment; other login; findings/blocked heading; 7-char prefix |
each as intended | verifiedAtHead blocks |
requiredCiState (:82 filter) |
empty; own contexts only; unreported; running; failed + pending; rerun passed; skipped/neutral | none / none / pending / pending / failed / passed / passed | required CI block, three-round self-wait loop |
required === null (:242) |
rules endpoint unreadable | pending, never green |
no test (CLI glue, see finding) |
second-read head/dismissed filter (:112) |
verdict at an older head; lineless review at head; dismissed NOT READY | none / none / pending | Second Read blocks |
reason strip (:117-118) |
- x, : (a), - `x`, [scope] with no separator, bare NOT READY |
separator dropped, first character of the reason kept, no trailing colon | the four new reason checks + "no reason" check |
fit 140 |
140 / 141 chars | kept / cut to 137 + ... |
"the 140-character edge" |
| Redline deterministic marker | code vs docs | not a verdict / verdict | "deterministic approvals" |
I checked every assertion to see whether it could pass without the code it names. None can. For example:
- The dismissed-NOT-READY check goes red if the
r.state === 'DISMISSED'skip at:112is removed. - "own lanes required, three rounds" stalls on pending if the
OWN_CONTEXTSfilter at:82is removed. - "a reason that starts with a code span keeps it" fails under the old
\W*strip.
Findings
Low: the CLI read path that feeds requiredCiState has no test. scripts/fleet-status.mjs:242-249:
let requiredCi = required === null ? 'pending' : 'none';
...
const statuses = (await ghAll(`/repos/${repo}/commits/${p.head.sha}/statuses`, token)).reverse()
...
const checks = runs.sort((a, b) => a.id - b.id)The tests pin "last result per name wins", but only on input that is already in order. The ordering itself comes from glue that has no test: the .reverse() on newest-first statuses and the sort by id on check runs. The unreadable-rules fallback to pending added in 334e41f also has no test. Dropping the reverse or the sort would let an older failed status outrank a newer pass at the same head, so fleet/verify would stay red after a passing rerun. The code is correct today. In practice, the check-runs endpoint's default filter=latest already hides superseded attempts, and a wrong colour on an informational status is the worst outcome. Suggested fix, if you want it pinned:
// export the pure part of readFacts and test it
export function ciInputs(statusesNewestFirst, checkRuns) {
const statuses = [...statusesNewestFirst].reverse().map((s) => ({ name: s.context, state: s.state }));
const checks = [...checkRuns].sort((a, b) => a.id - b.id)
.map((c) => ({ name: c.name, state: c.status === 'completed' ? (c.conclusion ?? '') : c.status }));
return [...statuses, ...checks];
}Info: the PR body has not caught up with a52b393.
- It says the lanes refresh "when CI, CodeQL, the live test or actionlint finishes". The
workflow_runlist now names all 12 pull-request workflows, and a test enforces it. - It says
node test/fleet-status.mjsruns "94 checks"; it now runs 111.
The code does more than the body claims, and nothing it claims is missing.
What's good
- Fork and injection surfaces are closed twice. The job
if:filters them, andreadFactsre-checks the PR state and head repo.PRis validated against/^\d+$/, and nothing from the event is interpolated intorun:. The script is checked out from the default branch withpersist-credentials: false, behind ahashFilesguard. - The new workflow-list test fails CI when a new pull-request workflow is added without being listed. Without it, the lanes would silently stall. It correctly leaves
fleet-status.ymlitself out, so there is no self-trigger. - Failure modes fail closed. Unreadable rules or checks count as
pending, a null conclusion as pending, and anything unrecognised asfailed. None of them can turnfleet/verifygreen. - The comment at
.github/workflows/fleet-status.yml:22-23explains why there is no concurrency group: the script re-reads after posting and corrects what differs.statusesToPosthas tests for that correction.
SECOND READ: READY
…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
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 on live head e9aabb57b66fe21585b1db25384126995f502a65.
I reviewed the workflow event filters and least-privilege permissions, the status computation (including required-check self-exclusion, verdict/head matching, dismissed reviews, and bounded re-read/post convergence), and the associated regression cases. Required CI is green at this head (actionlint, analysis, build matrix, live test, test, and package validation).
What's good: the workflow checks out the default branch's narrowly scoped script rather than PR-authored code, uses a read-mostly token with only statuses: write, and the status logic explicitly avoids treating the fleet's own contexts as required CI, preventing a verification self-dependency.
I did not run the local suite, per review-lane policy; CI is the test signal.
sprayberry-secondread
left a comment
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 code is right, but the new rationale for leaving pull_request_target workflows out of the list is factually wrong, and a test now enforces it. That needs a comment fix before it lands.
Scope: I gave the whole diff at e9aabb5 a second read. I compared it line by line against a52b393, the head where I last said READY. It is one commit touching .github/workflows/fleet-status.yml and test/fleet-status.mjs. scripts/fleet-status.mjs is byte-identical to a52b393, so the boundary ledger from that read still applies. CI is green at this head, and the test job runs fleet-status.mjs (ok 95).
Finding 1 (medium): the claim that "pull_request_target checks never land on the PR head" is false, and a test now enforces it
.github/workflows/fleet-status.yml:13-15
# rather than waiting for the next PR event. Not pull_request_target-only workflows: those run
# against the base branch's commit, so their checks never land on the PR head and cannot be
# required there (test/fleet-status.mjs enforces both).
test/fleet-status.mjs:336-337
// pull_request_target is left out: it runs against the base branch's commit, so its checks
// never land on the PR head, and the status job drops its workflow_run events.
and the assertion at test/fleet-status.mjs:358-359:
} else if (/\bpull_request_target\b/.test(on)) {
check(`workflow_run leaves out "${name}" (${f} runs only on pull_request_target)`, !listed.includes(name));
Evidence, from this PR at this head: GET /repos/askalf/dario/commits/e9aabb57…/check-runs lists advise (check suite 97740240767) and auto-merge (97740240796). They come from Version bump advice and Auto-merge bot PRs, and both workflows are pull_request_target-only. Run 36093377093 reports event: pull_request_target, head_sha: e9aabb57b66f…. GitHub checks out the base commit for these runs, but it attaches their check runs to the PR head. They land on the head, and they could be made required there.
Failure scenario: someone later adds a pull_request_target check to the master ruleset (for example advise). fleet/verify then stays pending, "Waiting on required CI", after that check finishes, until some unrelated PR event re-runs the job. The comment says this case can't happen, so nobody looks for it. The new test also makes it look like GitHub rules out the fix. The real reason those events are dropped is this repo's own job condition at fleet-status.yml:32:
(github.event_name == 'workflow_run' && github.event.workflow_run.event == 'pull_request' &&
Dropping the three names from the list is fine as a change: the if would skip their runs anyway. Only the stated reason is wrong.
Suggested fix: keep the list and the test logic. Put the actual reason in both comments and the test name:
# Every workflow with a pull_request trigger, so fleet/verify refreshes when any check settles
# rather than waiting for the next PR event. pull_request_target-only workflows are left out:
# the job below only acts on workflow_run events from pull_request runs, so listing them would
# only start skipped runs. If one of their checks becomes required, widen that condition first
# (test/fleet-status.mjs enforces both). // fleet-status.yml's workflow_run list names every workflow that runs on pull_request, so a
// required check it produces refreshes the lanes when it finishes. A pull_request_target-only
// workflow is left out because the status job only acts on workflow_run events from pull_request.What I checked and found fine
- The new test branch works as intended.
/\bpull_request\b(?!_target)/sendscontent-labeler.yml(issues+pull_request) to "must list", which it does asTriage issues and PRs. It sendsspam-watch.yml,auto-merge-bot-prs.ymlandversion-bump-advice.ymlto "must leave out", and none of them is listed.cc-drift-auto-release.ymlmatchespull_requestonly through its header comments.onBlocklimits the search to theon:block and strips comments, so the header text doesn't count and that workflow's real triggers (push,schedule,workflow_dispatch) put it in neither branch. Everypull_requestworkflow at this head (CI, CodeQL, actionlint, the three self-hosted ones, Overhead bench, Label PRs, Triage issues and PRs) is in the list. - Each of the 9 required contexts on master (
build (18/20/22),validate-package-json,analyze,actionlint,live-test,test,docker-cap-drop-smoke) comes from a listedpull_requestworkflow. So nothing required today is affected by Finding 1. - The script is unchanged since
a52b393. At that head I checked these boundaries, and they still hold: the bot rule is read before the >100-file rule, with a test at 99, 100 and 101. The 140-character cap has tests at 140 and 141. Dismissed Second Read reviews are skipped. The reason separator is stripped. Ownfleet/*contexts are dropped from required. And unreadable rules or checks fail closed topending.
SECOND READ: NOT READY — fleet-status.yml:13-15 and test/fleet-status.mjs:336-337 say pull_request_target checks never land on the PR head; advise is on e9aabb5
…cks too, self-test and backfill jobs Three fixes the other repos' copies took one at a time, in one push here. Check runs were read once (per_page=100), so a required check past the first hundred runs never counted and fleet/verify waited forever. Every event adds three Fleet status runs (skipped jobs still create runs), so a head passes 100 after about 27 events. collectPages reads every page and ghAll and the check-runs read both use it; a test puts a required check on page 2. The previous commit left pull_request_target workflows out of the list on the claim that their checks never land on the PR head. They do: such a run carries head_sha = the PR head and its check run sits in the head's rollup. Auto-merge bot PRs, Version bump advice and Spam removal are back, the status job accepts both workflow_run event kinds, and the test requires both kinds listed and checks the if. The workflow gains the self-test job (the tests on the PR's own code) and the manual backfill job that posts the lane statuses on every open PR, reading up to 1000 and failing at the cap rather than skipping PRs. Same script and test as the other repos; 123 tests.
sprayberry-redline
left a comment
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 change.
I reviewed the new workflow, the status computation and pagination/reconciliation paths, and the accompanying unit coverage. Required CI is green at ca98aaaa22be09ec8de8afa29430ca2e93ad9c32; I did not run the repository suite locally.
What's good: the workflow runs the status writer from the default branch with narrowly scoped write permission, while the PR-local self-test is read-only. The status computation correctly treats only a verdict at the live head as current, excludes its own contexts from required-CI evaluation to avoid a self-dependency, and bounds reconciliation after concurrent events. The tests cover the relevant stale-head, dismissed-review, check-pagination, bot/exemption, and status-reconciliation cases.
No blocking findings.
sprayberry-secondread
left a comment
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 code at ca98aaa is correct and the previous blocker is fixed. One blocking item remains: the PR description's safety and fork claims no longer match the workflow, because this head adds a self-test job that runs the PR's own code, fork PRs included. The fix is a body edit only.
Finding 1 (medium, blocking): the PR body says two things the diff no longer does
The body says:
It runs on a GitHub-hosted runner, sparse-checks-out only
scripts/fleet-status.mjs, installs nothing, never runs the PR's code, and its token can only read the PR and write statuses. Fork PRs are skipped; the fleet does not review them.
.github/workflows/fleet-status.yml:72-88:
self-test:
if: github.event_name == 'pull_request'
...
- uses: askalf/checkout-with-retry@115a6407547e9711edbc2e915838d495cad9583f # v1.1.0
with:
persist-credentials: false
...
- name: Test the lane rules
run: node test/fleet-status.mjs- "never runs the PR's code":
self-testdoes a full checkout of the PR merge ref (noref:, no sparse checkout) and runs the PR'stest/fleet-status.mjs, which imports the PR'sscripts/fleet-status.mjs. The header comment atfleet-status.yml:9states this correctly ("self-test runs the script's tests on the PR's own code, read-only"), so the body contradicts the file. - "Fork PRs are skipped":
self-test'sif:is onlygithub.event_name == 'pull_request'. Unlikestatus, it has nohead.repo.full_name == github.repositoryguard, so it runs on fork PRs. - The body also doesn't mention the two jobs added at this head:
backfill(fleet-status.yml:90-132, manual, posts statuses on every open same-repo PR) andself-test. It also saysnode test/fleet-status.mjsruns "94 checks", but theself-testlog at this head reports123 pass, 0 fail.
This isn't a security hole. self-test is a pull_request job with contents: read, so a fork run gets a read-only token and no secrets. But someone deciding whether to merge a new workflow reads the description to learn what it runs, and this description gets that wrong. Anyone who takes "never runs the PR's code" at face value later, for example when copying the files to the other public repos as the body suggests, is starting from a false premise.
Suggested fix (body only, no new commit needed):
Two jobs react to PR events. `status` runs on a GitHub-hosted runner, sparse-checks-out only
`scripts/fleet-status.mjs` from the default branch, installs nothing, never runs the PR's code,
and its token can only read the PR and write statuses; it skips fork PRs. `self-test` runs
`node test/fleet-status.mjs` on the PR's own checkout with a contents:read token, forks included.
`backfill` (manual, workflow_dispatch) posts the statuses on every open same-repo PR.
...
- `node test/fleet-status.mjs`: 123 checks, ...
What I checked (no other findings)
- Delta from
e9aabb5: theworkflow_runfilter now acceptspull_request_targetruns (fleet-status.yml:38), and the list addsAuto-merge bot PRs,Version bump adviceandSpam removal. I confirmed against live runs at this head:Version bump advice(run 36096184131) andAuto-merge bot PRs(run 36096184146) areevent: pull_request_target,head_sha: ca98aaa,head_repository: askalf/dario,pull_requests: [1419], sopull_requests[0].numberresolves and the same-repo guard holds. The three workflows in.github/workflowswith apull_request_target:trigger (auto-merge-bot-prs, spam-watch, version-bump-advice) are exactly the three added. The stale comment I flagged ate9aabb5is gone. - Workflow-list test (
test/fleet-status.mjs:777-806):\bpull_request(_target)?\bdoesn't matchpull_request_review, because_is a word character, so review-only workflows are correctly excluded. The test fails if apull_request_targetworkflow is left out of the list, so it can fail. collectPages(scripts/fleet-status.mjs:283-290): the boundary rows are empty page, short first page, exactly 100 then empty, and 100 + 1. The tests attest/fleet-status.mjs:831-836pin each of them, including a required check that exists only on page 2. TheghAllrefactor keeps the old behaviour.backfill: it only runs onworkflow_dispatch, andstatusandself-testboth evaluate false on that event (forstatus,github.event.pull_requestis null there). It fails at>= 1000open PRs instead of silently truncating, and it filters out cross-repository PRs. A single PR's script error aborts the loop underset -e, which fails loudly rather than silently. That's acceptable for a manual job.- Unchanged since my last full read:
requiredCiState(with the own-context exclusion),secondReadAtHead(with the U+2014 separator),redlineVerdict, the bot and >100-file ordering, and the 140/141 description edge all still have tests that can fail. - CI at
ca98aaa: all required checks pass (build 18/20/22, validate-package-json, analyze, actionlint, live-test, test, docker-cap-drop-smoke), andself-testpasses with 123 of 123.
What's good: switching from "exclude pull_request_target" to "accept it and list it" is correct, and the live run metadata confirms it. collectPages is a real extraction with tests that can fail.
SECOND READ: NOT READY — PR body says "never runs the PR's code" and "Fork PRs are skipped", but the self-test job (fleet-status.yml:72-88) runs the PR's checkout on every pull_request, forks included; body-only fix
sprayberry-secondread
left a comment
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 edited description fixes the blocker from my last read at ca98aaa. Nothing blocks. Two low notes on the body follow, and neither changes what lands.
Scope: no code was pushed, so the code is the same ca98aaa I read in full in my previous review (boundary ledger, collectPages, workflow_run list and CI all as reported there). This read covers the new description, checked claim by claim against the diff at this head.
The previous finding is resolved
The body now says:
Two jobs react to PR events.
statusruns on a GitHub-hosted runner, sparse-checks-out onlyscripts/fleet-status.mjsfrom the default branch, installs nothing, never runs the PR's code, and its token can only read the PR and write statuses; it skips fork PRs.self-testrunsnode test/fleet-status.mjson the PR's own checkout with a contents:read token, forks included.backfill(manual, workflow_dispatch) posts the statuses on every open same-repo PR.
Checked against .github/workflows/fleet-status.yml at ca98aaa:
status(:26-64): checks outref: ${{ github.event.repository.default_branch }}withsparse-checkout: scripts/fleet-status.mjsandpersist-credentials: false. The only write permission isstatuses: write. Fork PRs are filtered inif:(:33,:36) and again atscripts/fleet-status.mjs:236.self-test(:66-82):if: github.event_name == 'pull_request'with no repo guard,permissions: contents: read, and it runsnode test/fleet-status.mjs. "Forks included" is accurate.backfill(:84-126):workflow_dispatchonly,select(.isCrossRepository | not).- "123 checks": the
self-testlog at this head reports123 pass, 0 fail.
Other body claims, checked
- Status table: matches
laneStatuses(scripts/fleet-status.mjs:164-207), including "says which commit its last verdict was on" (its last verdict was on ${short(rv.commitId)}) and the gated-pending wording. - "None is added to the ruleset's required checks": the
masterrequired contexts arebuild (18/20/22),validate-package-json,analyze,actionlint,live-test,test,docker-cap-drop-smoke. There is nofleet/*. - "
actionlintv1.7.1 (the CI version)":actionlint.ymlpinsv1.7.1. - "
.github/,scripts/andtest/only" and theno-changeloglabel: both hold. None of the 17 commit messages carries model attribution.
Low (non-blocking): two body sentences are out of date
- "Replaces #1413 with the same change as one commit": the PR has 17 commits (
e629c63throughca98aaa), and since #1413 it has gainedself-test,backfillandcollectPages. This only holds if the PR is squash-merged, which the repo allows. - "The workflow and script are self-contained, so the other public repos can copy both files as they are": at this head,
self-test(fleet-status.yml:82) runsnode test/fleet-status.mjs, and theworkflow_runlist (:18-20) names dario's own workflows. A repo that copies only the workflow and the script gets a failingself-test, and its lanes won't refresh when its own CI finishes.
Suggested wording:
Replaces #1413 (same purpose, since extended; squash-merge to land it as one commit), without model attribution.
...
Only dario for now. Another public repo can copy the workflow, the script and the test, then set the workflow_run list to its own workflows (the test checks the list).
These are notes about provenance and future copying. They don't describe what this diff does to dario, so they don't block.
What's good
The rewrite describes each job's trust boundary separately, and that is the fact someone merging a new workflow most needs. It matches the header comment at fleet-status.yml:1-4.
SECOND READ: READY
3 participants
What does this PR do?
The fleet's review lanes (the Breaker's verification, Redline's gating review, the Second Read) run as tickets on the box. A PR waiting on one looked the same on GitHub as a PR nobody had picked up. On 2026-09-24 dario#1403 sat all morning with green CI and a stale "Changes requested" while its verification waited on a budget-paused seat.
fleet-status.ymlposts three commit statuses on the PR head, next to build and test:fleet/verifyfleet/reviewfleet/second-readThe rules are the dispatcher's (
tools/review-dispatch.sh,runtime/review-lanes.tsin platform): on our own repos verification is the head's required CI (every check the base branch's rules require has passed); 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.It re-runs on every push, label, review (submitted, edited or dismissed) and comment (created, edited or deleted), and when CI, CodeQL, the live test or actionlint finishes (
workflow_run), since each can move a lane. Two jobs react to PR events.statusruns on a GitHub-hosted runner, sparse-checks-out onlyscripts/fleet-status.mjsfrom the default branch, installs nothing, never runs the PR's code, and its token can only read the PR and write statuses; it skips fork PRs.self-testrunsnode test/fleet-status.mjson the PR's own checkout with a contents:read token, forks included.backfill(manual, workflow_dispatch) posts the statuses on every open same-repo PR.Not in this PR:
fleet/verifyorfleet/reviewrequired is a one-line ruleset change if you want it.How to test
node test/fleet-status.mjs: 123 checks, including the feat(models): opus and opus1m fall back to Opus 5.5; opus5 pins Opus 5 (6.12.0) #1403 morning (an older CHANGES_REQUESTED and NOT READY must not show red at a new head), a stale verification comment, a verifier comment from another login, the deterministic approval on code vs docs, bot branches, and the 140-character description cap.node --test test/all.test.mjs: 242 of 243 pass. The one failure isoauth-detector.mjs, which fails the same way on master in this environment (no detectable CC binary).actionlintv1.7.1 (the CI version): clean.node scripts/preflight.mjs: clean.Checklist
npm run buildpassesnpm testpasses (offline regression test, no credentials required)src/? No:.github/,scripts/andtest/only, so this takes theno-changeloglabelproxy.ts,cc-template.tsor streaming? NoReplaces #1413 with the same change as one commit, without model attribution.