ci: fleet review lanes show up as commit statuses on the PR head - #83
Conversation
Deploying amnesia-site with
|
| Latest commit: |
e2f7721
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://02b16db5.amnesia-site.pages.dev |
| Branch Preview URL: | https://ci-fleet-status.amnesia-site.pages.dev |
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 — deleting a verification comment can leave its success status green at the same head. rule:none
-
Blocking —
.github/workflows/fleet-status.yml:14:issue_commentsubscribes only totypes: [created, edited]. The lane state is recomputed from the current issue comments (the script fetches/issues/${pr}/commentsatscripts/fleet-status.mjs:200), but a deletion of the## Verification at <head>comment emits anissue_comment.deletedevent which this workflow does not handle. For a non-exempt PR on a branch with no required CI checks, start with theverifiedlabel and a valid verification comment sofleet/verifyis posted successful, then delete that comment. No workflow runs to recompute the lanes, so the previously successfulfleet/verifystatus remains on the head despite the underlying verification no longer satisfying the dispatcher rule; once these contexts are required, that stale success can permit merging without verification.issue_comment: types: [created, edited, deleted]
The required CI checks are green at 743e0f7. I reviewed the workflow event coverage and the status-state logic/tests; I did not run the local test suite per review policy.
…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: one blocking defect. The PR body's own next step (make the fleet/* contexts required) makes every code PR's lanes stay pending, which blocks every code merge.
Head note: I was dispatched at 743e0f7. The branch moved twice while I was reading:
93f5850:edited/deletedtriggers, the bot-rule comment,isBotPrtests.6e771d8: concurrency group replaced bypostedSince.
I read both deltas, so this review and its verdict are for 6e771d8. The finding below is in all three heads.
Findings
High: the script's own fleet/* contexts count as required CI once they are required
scripts/fleet-status.mjs:225-226
required = rules.filter((r) => r.type === 'required_status_checks')
.flatMap((r) => (r.parameters?.required_status_checks ?? []).map((c) => c.context));scripts/fleet-status.mjs:141
const verified = code && (ci === 'passed' || (ci === 'none' && verifiedAtHead(facts)));The PR body's Next step, after this merges says to add fleet/verify, fleet/review and fleet/second-read to the default branch's required status checks. After that, required contains the three contexts this script writes. Here is what happens on a code PR:
- On the first run no
fleet/*status exists at the head, solast.get('fleet/verify')is''andrequiredCiStatereturns'pending'. ci === 'pending', soverifiedis false. That makesgatedtrue, and all three lanes are posted aspending.- On the next run,
fleet/verifyisPENDINGat the head (andfleet/reviewandfleet/second-readtoo), which matchesCHECK_WAITING. SorequiredCiStatereturns'pending'again. Nothing can ever break the loop.
Because the rules list is non-empty, ci is never 'none', so the verified label and comment fallback can't break the loop either. I reproduced this at 6e771d8 by feeding laneStatuses its own output back through requiredCiState for three rounds. The input was a code PR with all four real CI checks green, the verified label with a ## Verification at <head> comment, Redline APPROVED at the head, and a Second Read READY at the head:
required = 4 contexts (today)
round 1..3 requiredCi=passed fleet/verify=success fleet/review=success fleet/second-read=success
required = 7 contexts (after the documented next step)
round 1..3 requiredCi=pending fleet/verify=pending fleet/review=pending fleet/second-read=pending
Once they are required, GitHub refuses every code merge because fleet/verify is never green. That includes merges by hand and by auto-merge. The backfill job would then post pending on every open PR. The existing test checks nobody requires do not hold it (scripts/fleet-status.test.mjs:227) covers a fleet/verify status that is present but not required. No test covers the case where it is required, which is exactly the configuration the body says comes next.
Suggested fix (exclude the lanes this script owns from the CI it waits on, plus a test that pins it):
const OWN = new Set(Object.values(CONTEXTS));
// in requiredCiState, before the loop:
required = required.filter((r) => !OWN.has(r));
if (!required.length) return 'none';check('our own lanes being required do not hold verification',
requiredCiState(['test', CONTEXTS.verify, CONTEXTS.review, CONTEXTS.secondRead], [ok('test')]) === 'passed');Filtering inside requiredCiState rather than at the CLI call site means the test drives the same code the workflow runs.
Info: the test count in the body is stale
The body says 73 pass, 0 fail and "the script's 73 unit tests". At 6e771d8, node scripts/fleet-status.test.mjs prints 83 pass, 0 fail, which matches the latest commit message.
What I checked
- Boundaries I rebuilt from the diff:
requiredCiStatewith emptyrequired→'none', pinned.- A required check not yet reported →
pending, pinned. - A required check failed while another is pending →
failed, pinned. - A rerun where the last result wins → pinned.
- Skipped and neutral count as passed → pinned.
fitat 140 and 141 characters → pinned.- 99 vs 100 files → pinned.
- A bot branch opened by a person → pinned.
- A verdict at an older head vs this head, for both Redline and the Second Read → pinned.
- A
NOT READYwith no reason and no trailing colon → pinned. - A deterministic approval on code vs docs → pinned.
postedSince→ pinned for all five of its cases: after the read, before it, in the same second, another context, and noDateheader.
- The unguarded row is the one above: the
fleet/*contexts inrequired. postedSinceordering: a stale overwrite would need the older run to read before an event and the newer run to post within the sameDatesecond as that read. Runner startup takes seconds, so the newer run'screated_atis always strictly later, and the same-second overwrite only happens between runs that read the same state. It holds.- New assertions: each one I read would fail against a plausible wrong implementation. I found none that holds either way.
- Workflow:
- The
statusjob checks out the default branch's script only (sparse,persist-credentials: false). It is skipped byhashFilesuntil merge, and the job token is least-privilege. - Fork PRs are excluded both in the job
ifand again in the script. workflow_runis limited toCI/CodeQL. Both produce the four required contexts; I confirmed this from the run names.
- The
- CI: I ran
self-testlocally at6e771d8and got 83/0. At93f5850, all required checks passed;kickfailed but is not required. - What's good: the rules are pure functions with the I/O confined to the CLI block, a verdict counts only at the head everywhere, and when the script can't read something it falls back to
pending/label rather than to green.
SECOND READ: NOT READY — requiring the fleet/* contexts (the body's next step) makes requiredCiState wait on the script's own statuses, so every code PR stays pending forever
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: changes requested — concurrent same-second runs can still let stale lane state overwrite fresher state. rule:none
-
Blocking correctness —
scripts/fleet-status.mjs:256:if (postedSince(posted, s.context, readAt)) { console.log(' (a newer run already posted this; skipped)'); continue; }delegates the ordering decision to a strict timestamp comparison.readAtcomes from GitHub's HTTPDateheader and statuscreated_atis also second-granularity;postedSince()only returns true whenDate.parse(s.created_at) > readAtMs. The added test codifies the unsafe equality case atscripts/fleet-status.test.mjs:259:check('posted in the same second: overwrite', !postedSince([st('fleet/review', '2026-09-25T03:00:10Z')], 'fleet/review', readAt));. If run A reads at 03:00:10.100, run B reads newer review data later in that same second and posts its lane status, then A reaches this check, both timestamps parse to 03:00:10 and A overwrites B's fresh status with its stale decision. That is precisely the concurrent ordering race this change is meant to prevent, and it can leave a green or red required lane inconsistent with the newest review state.// Treat an equal, server-second timestamp as newer too: this run cannot // establish that it read after the already-posted status. return statuses.some((s) => s.context === context && Date.parse(s.created_at) >= readAtMs);
Required CI is green at 0e1fca7. I re-read the workflow event handling, required-check exclusion, status ordering path, and the accompanying unit tests. I did not run the local suite per review policy.
…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 adb8b0b. The lane rules match what the PR body says, the new tests fail when the code they cover is broken, and every claim in the body checks out against the diff. There is one low-severity note on the post-then-verify loop.
Scope: the dispatch ticket named 0e1fca7, but the head has since moved to adb8b0b9 (two commits: the post-then-verify loop, dismissed Second Read reviews, the >100-file rule, and test renames). I reviewed the live head. I read the full diff (3 files, +727), the base branch's required-check rules, ci.yml's triggers, and the check runs at the head. I did not run the tests; CI's self-test passed at this head.
Findings
Low: scripts/fleet-status.mjs:401-418: on pass 3 the loop posts statuses and never re-reads them
for (let pass = 1; pass <= 3; pass++) {
const read = await readFacts();
...
const todo = statusesToPost(want, have);
if (!todo.length) break;
...
for (const s of todo) {
await gh(`/repos/${repo}/statuses/${read.facts.head}`, token, {The comment at 397-399 says "the run that acts last re-reads after its own writes". That holds for passes 1 and 2, but not for pass 3. Pass 3 reads the facts, posts what differs, and the loop exits without checking again.
Failure scenario: two runs overlap on a busy PR, and run A needs a correction on every pass. After A's pass-3 read, Redline posts CHANGES_REQUESTED at the same head. Run B reads that and posts fleet/review failure. Then A's pass-3 POST lands with its older data (success) and A exits. The head now shows green over a red verdict. Nothing corrects it until the next PR event, and there may be no next event.
This needs three consecutive diffs in one run, so it is unlikely. The body's "(up to three passes)" and the comment's "the next event covers anything after that" already describe the limit. The statuses are also informational in this PR. So this is not blocking, but it matters once fleet/* become required checks, because a stale success could then let a merge through.
Suggested fix: after the last pass, only verify and log. Never end the loop on a write.
for (let pass = 1; pass <= 4; pass++) {
const read = await readFacts();
if (!read) break;
const want = laneStatuses(read.facts);
if (pass === 1) for (const s of want) console.log(`${s.context.padEnd(18)} ${s.state.padEnd(8)} ${s.description}`);
if (dryRun) break;
const have = latestByContext(await ghAll(`/repos/${repo}/commits/${read.facts.head}/statuses`, token));
const todo = statusesToPost(want, have);
if (!todo.length) break;
if (pass === 4) { console.log(`still differs after 3 passes: ${todo.map((s) => s.context).join(', ')}`); process.exitCode = 1; break; }
...A failed status job at least makes the unsettled state visible on the PR.
Boundaries I rebuilt from the diff
| Predicate | Input | Behaviour | Pinned by |
|---|---|---|---|
needsVerify files.length > 100 |
99 / 100 / 101 docs files | not code / not code / code | "99 docs files are not code", "exactly 100 docs files is not code", "more than 100 files is code whatever they are" |
| same, required CI passed | 101 docs files | verify green | "more than 100 docs files still verifies once required CI passes" |
isBotPr |
person on bot/, askalf on feat/, dependabot on any branch |
not bot / not bot / bot | the isBotPr checks |
verifiedAtHead !facts.head / no label / older sha / latest comment / other login / findings/blocked heading |
each | false | the verifiedAtHead block |
requiredCiState empty required / only fleet/* required |
[], own |
none |
"no required checks -> none", "only own lanes required: none" |
| same, unreported / running / failed / rerun passed / skipped+neutral | each | pending / pending / failed / passed / passed | the "required CI is the verification" block |
fleet/* pending while required |
own lanes PENDING, CI green | passed (does not wait on itself) | "own lanes required and pending, CI green: still passed" and the three-round loop |
redlineVerdict deterministic approval |
code / docs | ignored / counts | "on code it is not Redline's verdict" / "on docs it is" |
| same, DISMISSED or COMMENTED at head after an older APPROVED | pending, names the old sha | "a dismissal and a comment at head leave..." | |
secondReadAtHead commitId != head / dismissed / no line / READY, mostly / last line in the body / later review with no line |
each | none / none / none / none / last wins / keeps the verdict | the "Second Read, review by review" block and the dismissed block |
| NOT READY with empty reason | SECOND READ: NOT READY |
NOT READY at 4753643, no trailing colon |
"NOT READY with no reason is red without a trailing colon" |
fit at 140 / 141 chars |
kept whole / cut to 137 + ... |
"the 140-character edge" | |
statusesToPost identical / new state / missing / same state, new description |
skip / post / post / post | the post-then-verify block |
I found no reachable row with wrong behaviour and no reachable row left untested, apart from the pass-3 loop exit above, which is CLI I/O with no unit seam.
Assertions that could not fail: I checked each of the 94 check() calls against the branch it targets. The ones most likely to be vacuous do in fact fail if the code they cover is broken. "review pending even with an approval at head" fails without the gated branch. "the latest comment wins" fails with an any-comment rule. "own lanes required and pending, CI green: still passed" fails without the OWN_CONTEXTS filter. "a later READY still counts after a dismissed NOT READY" fails if dismissed reviews are not skipped.
PR body claims, checked
- Required checks:
rules/branches/mainreturns exactlyValidate HTML and Scripts,Analyze (actions),Analyze (javascript-typescript)andScorecard (file-based checks). The first and last come fromci.ymland the Analyze pair from CodeQL, soworkflow_run: [CI, CodeQL]covers every producer. - "runs the default branch's copy of
scripts/fleet-status.mjs": the sparse checkout pinsref: default_branch, andhashFilesskips the step until merge. Thestatuscheck passed in 7s at this head, which fits a skipped step. Onpull_request*events the workflow YAML itself is still the PR's copy. That is expected and harmless, since only same-repo authors reach this job. - Permissions, fork skip in both the
ifand the script, no concurrency group,self-testread-only, manualbackfill: all as the body states. - "94 unit tests": there are 94
check(calls. - "plain ASCII throughout": the diff contains 0 bytes >= 0x80.
- "same files as askalf/dario#1419":
scripts/fleet-status.mjshas the same blob (8b338ee). The test file differs only in location (test/fleet-status.mjsthere) and its import path.
What's good
The script is pure functions over fetched facts, and the CLI is a thin shell around them, so nearly every rule can be unit-tested. Unreadable rules and unreadable checks both fail closed: neither can turn fleet/verify green. Filtering the fleet's own contexts out of the required list prevents the self-wait deadlock ahead of the planned "make them required" step.
Operator note for that step, outside this diff: any workflow run with statuses: write can post a fleet/* context, including a modified workflow on a same-repo PR. Requiring these contexts gates merges by convention among trusted writers, not against them.
SECOND READ: READY
…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
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 stale workflow run can still overwrite newer lane statuses after the newer run finishes. rule:none
Blocking — correctness: stale worker can leave an obsolete status — scripts/fleet-status.mjs:267-287
// Post, then read everything again and correct what differs.
const have = latestByContext(await ghAll(/repos/${repo}/commits/${read.facts.head}/statuses, token));
for (const s of todo) { await gh(/repos/${repo}/statuses/${read.facts.head}, token, { ... }); }
The post-then-read loop does not establish that the worker which writes last has the freshest facts. For example, run A reads an older review state (pending), run B reads a later approval and posts fleet/review=success, then B completes its second pass. A can then resume, read statuses, see B's success differs from its old desired pending, and post pending. On A's next pass it re-reads the same still-old review state and sees its own pending status as correct, so it exits; no run remains to correct the stale final write. This is precisely the concurrent ordering the new loop is intended to prevent, but it still leaves the final status wrong until another event happens. The new unit case tests correction by a hypothetical later fresh pass, not this interleaving of two live workers.
// Serialize status publication per PR (or retain a server-side freshness
// comparison that prevents a run from publishing after a newer run has
// observed the PR). Then test an interleaving where an older reader writes
// after the newer worker has completed; the final context must remain the
// newer worker's value.
What's good: required CI is green at adb8b0b, and I checked the rework's dismissal handling, required-context exclusion, file-count boundary, and status comparison tests. The dismissal and exact-100-file cases are now explicitly covered.
… 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: request changes — removing the dispatcher kick silently restores timer-only review dispatch. rule:none
Blocking — .github/workflows/review-kick.yml:1-59
-name: Review kick
-on:
- pull_request_target:
- types: [opened, synchronize, reopened, ready_for_review]
- - name: Queue review-dispatch for this head
This change deletes the workflow that calls review-dispatch.service when a same-repository PR is opened or receives a new head. The new fleet-status workflow only publishes statuses; it does not invoke the fleet dispatcher. Consequently, after this merges, a new commit can remain unreviewed until the periodic dispatcher timer runs rather than being queued immediately. That contradicts the deleted workflow's stated purpose and makes the visible lane status stay pending despite all CI being complete. Keep the independent dispatch trigger (or replace it with an equivalent immediate, trusted trigger) rather than deleting it as part of the status-reporting feature.
# Retain .github/workflows/review-kick.yml unchanged, or provide an equivalent
# trusted event handler that queues review-dispatch for opened/synchronized PRs.
Required CI was green at bc827002. I reviewed the live PR diff and, in particular, the new workflow-run coverage and the change since the prior verdict; I did not run the local test suite.
… 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 — required CI can remain pending indefinitely on PRs with more than 100 check runs. rule:none
Blocking — .github workflow status computation (scripts/fleet-status.mjs:382)
const runs = (await (await gh(\/repos/${repo}/commits/${p.head.sha}/check-runs?per_page=100`, token)).json()).check_runs ?? [];`
The new lane is intended to reflect every required check, but this endpoint is read only once and therefore retains at most 100 check runs. A head with (for example) a large matrix or reruns that produces 101 runs, where a required context is in the next page, never adds that context to checks. requiredCiState() then sees an empty state for the missing required context (lines 222–224) and returns pending, even after that required job has passed. Consequently fleet/verify, and then the review lanes, can block the PR permanently.
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 also add a regression case with a required check beyond the first 100 returned runs.
What's good: required CI is green at e5c423d; I reviewed the workflow triggers, status/verification state machine, pagination for PR metadata, and the new unit-test coverage. The status loop’s fresh-read correction and exclusion of the lane contexts from CI evaluation are sound.
The check-runs list was read once with per_page=100, so on a head with more than 100 runs a required check on a later page never counted, and fleet/verify could wait on it indefinitely. Paging is now one pure helper, collectPages(), used by the list endpoints and by check runs alike. Four tests: 101 rows over two pages, a full last page, a short first page, and a required check found on page 2.
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 re-review.
I reviewed the live-head delta from e5c423da0f769fa731454e4ddc96845bab5cc7ff to 6ff4cf15d615c792ba81b91f1454e52f1c431dd7, concentrating on the new paginated check-run fetch. collectPages preserves all rows through the first short page, and the check-run path now supplies every page to requiredCiState; the added tests cover a required check appearing on page two and pagination boundaries. Required CI is green at this head.
What's good: extracting the paging loop into one helper prevents the statuses and check-runs readers from drifting, while retaining the existing fail-closed handling when the API cannot be read.
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 6ff4cf1. The lane rules match the PR body, every reachable boundary I rebuilt has a test that can fail, and the self-test job reports 115 pass, 0 fail at this head.
Scope note: the ticket named bc82700, but the live head is 6ff4cf1 (two commits later: backfill paging, the separator pin, and check-run paging). I reviewed 6ff4cf1, reading the full diff (3 files, +819). I also checked the base branch's ruleset and the on: block of every workflow at this head against the workflow_run list.
Boundaries rebuilt from the diff
| predicate | input | behaviour | pinned by |
|---|---|---|---|
needsVerify files.length > 100 (scripts/fleet-status.mjs:190) |
99 / 100 / 101 docs files | not code / not code / code | "99 docs files", "exactly 100 docs files", "more than 100 files" |
| same, bot branch | 101 files on bot/ |
exempt (bot rule first) | "a bot PR with more than 100 files is still not code" |
isBotPr (:181-184) |
a person on bot/, release/1.2; dependabot on any branch |
not bot, not bot, bot | the three isBotPr blocks |
verifiedAtHead (:194-203) |
label only; comment only; older comment then head; head then older; other login; findings/blocked headings; 7-char sha |
only label + latest verifier comment at head counts | the verifiedAtHead sections |
requiredCiState (:216-228) |
[]; only fleet/* required; missing; in_progress; failure plus pending; rerun that passed; skipped/neutral |
none / none / pending / pending / failed / passed / passed | the "required CI" block |
| own-context exclusion | fleet/* required and pending |
not counted, no self-wait over 3 rounds | "own lanes required, three rounds" |
redlineVerdict + head match (:231-240, :320) |
deterministic approval on code vs docs; dismissal/comment after an older approval; CR after APPROVED at same head; CR at unverified head | pending / success / pending with old sha / failure / pending | "deterministic approvals", "Redline reviews that are not verdicts" |
secondReadAtHead regex (:249) |
no line; READY, mostly; two lines in one body; later lineless review; older-head verdict; Redline carrying the line; bare NOT READY |
none / none / last wins / keeps verdict / none / none / red with no colon | "the Second Read, review by review" |
separator strip (:253) |
-, :, U+2013, U+2014, none; reason starting with a backtick or ( / [ |
separator dropped, first char kept | the separator loop and "a reason that starts with a code span" |
fit (:290) |
140 / 141 chars | kept / 137 + ... |
"the 140-character edge" |
collectPages (:280-287) |
5; 100+1; 100+100 | 1 page / 2 / 3 (last one empty) | the paging block |
statusesToPost / latestByContext (:260-274) |
identical; different state; missing context; same state with a new description; stale overwrite | skip / post / post / post / corrected next pass | the post-then-verify block |
Assertions that could hold whether or not the change is in place: none found. "the latest comment wins" fails if the first comment won; "CI pending: an old Breaker label and comment do not count" fails if ci === 'none' were dropped from :303; the workflow_run coverage test fails if the workflows: list fails to parse (listed becomes []).
PR body claims checked against the diff and the repo
- Required checks are read from branch rules at run time (
:381-383). The live ruleset lists exactly the four the body names. workflow_runcovers everypull_requestworkflow. At this head those areCI,CodeQL,Imageandlabels, which matches.github/workflows/fleet-status.yml:24.PR triageispull_request_targetonly and is correctly left out.- The
statusandbackfilljobs check out the default branch's script only (fleet-status.yml:49-54,:100-105).self-testgetscontents: readonly. - Fork PRs are skipped both in the job
if(:33-39) and inreadFacts(:371). - Unreadable rules or check runs fall back to
pending(:384,:395) and never to green.
Non-blocking note for the "next step" (not this diff)
When fleet/* become required, remember that on pull_request and pull_request_review events GitHub runs the workflow file from the PR's merge ref. A same-repo PR that edits fleet-status.yml can therefore post its own fleet/* statuses with statuses: write. Pinning the contexts to the GitHub Actions integration in the ruleset does not close this, because the edited workflow also runs as Actions. The existing required checks already have this property, and it needs branch write access, so I am not counting it against this PR. It is worth having the dispatcher, or Redline, flag any PR that touches fleet-status.yml.
What's good
The pure/CLI split keeps all the rule logic unit-testable. The post, re-read and correct loop is a reasonable substitute for a concurrency group that would roll up as a failed check. The ASCII-only source with run-time separators is a neat way to cover the real verdict line.
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 bed18a0. The only change since my READY at 6ff4cf1 is a merge of main, and the three files this PR adds are byte-identical to that head.
What changed since the last read
bed18a0isMerge branch 'main' into ci/fleet-statuson top of6ff4cf1.git diff --quiet 6ff4cf1 bed18a0 -- .github/workflows/fleet-status.yml scripts/fleet-status.mjs scripts/fleet-status.test.mjsexits 0.origin/main(a5db6aa) is an ancestor of the head, so the PR diff is still exactly those 3 files (+819/-0).- Everything the merge brought in comes from
main(#80's worker cookie renewal, sponsors sync, docs). None of it is on this PR's review surface.
Re-checked against the merged-in main
workflow_runlist stays complete. The merge adds.github/workflows/sponsors-readme.yml, which triggers onscheduleandworkflow_dispatchonly (sponsors-readme.yml:12-16). It posts no checks on a PR head, soworkflows: [CI, CodeQL, Image, labels](fleet-status.yml:18) still names everypull_requestworkflow, andPR triageremains the onlypull_request_targetone. The coverage test ("workflow_run lists ...",fleet-status.test.mjs:335-360) re-reads the directory, so it would have caught a gap. It passes:node scripts/fleet-status.test.mjsprints115 pass, 0 fail, matching the body.- The sponsors bot PR classifies correctly. That workflow opens its PR from
bot/sponsors-readme(sponsors-readme.yml:66,88). If that PR is opened asaskalf,isBotPrreturns true (fleet-status.mjs:46-48,BOT_BRANCH^bot\/), so all three lanes read "not required / not gating" and a README-only bot PR does not wait on the gate. If it is opened under some other identity, it still touches onlyREADME.md, andisCodePathreturns false for.md(:33). The result is the same. - Core rules re-read at this head, unchanged from my ledger at
6ff4cf1:needsVerify> 100fail-closed (:55),verifiedAtHeadlatest-comment-wins with the prefix match (:59-67),requiredCiStateown-context exclusion and last-result-per-name (:81-92),redlineVerdictskipping deterministic approvals only on code (:96-104),secondReadAtHeadhead/login/dismissed filter, last line wins, and one-separator strip (:108-121),fitat 140 (:155), and the post, re-read and correct loop bounded at 3 passes (:281-298). Each row still has the test I cited last time, and I found no assertion that holds whether or not the change is in place.
CI at this head
gh pr checks 83: Validate HTML and Scripts, Analyze (actions), Analyze (javascript-typescript) and Scorecard (file-based checks) (the four required checks) all pass, as do self-test and status. backfill is skipped, as expected on pull_request.
Not re-read
I did not re-read the merged-in main files beyond the workflow trigger lists. They are not part of this PR's diff.
What's good
Merging main was the right way to pick up the new workflow. The directory-driven coverage test is exactly what makes a merge like this safe to re-gate without re-reading everything.
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 e2f7721. The lane rules match the body, every reachable boundary I rebuilt is pinned by a test that discriminates, and the privileged job runs only default-branch code.
Since my last read at bed18a0, the only change is the merge of main (1d4f6e87, .github/labels.json, which is not in this PR's diff). I re-read the full diff (3 files, +819) at this head anyway.
Boundaries rebuilt from the diff
| Predicate | Input | Behaviour at head | Pinned by |
|---|---|---|---|
facts.files.length > 100 (scripts/fleet-status.mjs:190) |
99 / 100 / 101 docs files | not code / not code / code | "99 docs files", "exactly 100 docs files", "more than 100 files" |
| same, bot PR | 101 files on bot/ |
exempt (bot rule read first) | "a bot PR with more than 100 files is still not code" |
isCodePath .github/ carve-out |
workflows/*.yml, actions/**, scripts/**, .MJS under workflows |
not code, code, code, code | ".github config is not", ".github actions are", ".github scripts are", "keeps its case" |
isBotPr identity AND branch |
person on bot/, release/; askalf on each prefix; dependabot anywhere |
matches the dispatcher rule | the two isBotPr blocks |
requiredCiState own-context filter (:217) |
only fleet/* required; fleet/* required and pending with CI green |
none; passed |
"only own lanes required: none", "own lanes required and pending", plus the three-round fixed-point test. All of them fail without line 217 |
requiredCiState empty required |
[] |
none |
"no required checks -> none" |
| missing / running / failed / rerun / skipped / neutral | as named | pending / pending / failed / passed / passed | the six requiredCiState checks |
rules or checks unreadable (:384, :395) |
throw | pending, never green |
by construction (CLI path, not unit-tested; this is the fail-closed direction) |
verifiedAtHead |
label only, comment only, older sha, later older sha, other login, findings/blocked headings, 7-char prefix |
each correct | the verifiedAtHead blocks |
redlineVerdict |
deterministic approval on code vs docs; DISMISSED/COMMENTED at head after an older approval; CR after APPROVED at same head | not a verdict / verdict; pending naming the older sha; red | "deterministic approvals", "Redline reviews that are not verdicts" |
secondReadAtHead |
no line; READY, mostly; two lines in one body; later lineless review; older-head verdict; DISMISSED; Redline carrying the line |
none; none; last wins; kept; none; skipped; none | "the Second Read, review by review", the dismissed block |
| reason separator | -, :, U+2013, U+2014, none, leading backtick/bracket |
dropped once, first char kept | the separator loop (built with fromCharCode), "keeps it", "[scope] missing" |
fit |
140 / 141 chars | kept / 137 + ... |
"the 140-character edge" |
collectPages |
short first page, 100+1, 100+100 | stops at first short page | the pager block |
statusesToPost |
identical, state differs, missing, same state new description, stale overwrite | skip, post, post, post, corrected then left alone | the post-then-verify block |
I also read the new tests for vacuity. The own-context, dismissed-review and separator checks all fail if you remove the line they cover. "checks nobody requires do not hold it" is a plain non-required-check case and makes no claim to cover the filter.
Body claims checked against the diff and the repo
- Required checks:
rules/branches/mainreturns exactlyValidate HTML and Scripts,Analyze (actions),Analyze (javascript-typescript),Scorecard (file-based checks). The script reads that list at run time (scripts/fleet-status.mjs:381-383). Nothing is hard-coded. - The
workflow_runlist[CI, CodeQL, Image, labels](.github/workflows/fleet-status.yml:24) matches every workflow at this head that has apull_requesttrigger.PR triageispull_request_target-only and is left out. Canary, ClusterFuzzLite, Scorecard, Deploy and Sponsors never run onpull_request. The guard test atscripts/fleet-status.test.mjs(theonBlockloop) would flag a new one. - Trust boundary:
statusandbackfillsparse-check-out the default branch's script only (ref: ${{ github.event.repository.default_branch }},persist-credentials: false).self-testruns the PR's code withcontents: readand nothing else. Fork PRs are dropped both in the jobifand inreadFacts(:371).workflow_dispatchfails thestatusjob's third clause becausegithub.event.pull_requestis null, so onlybackfillruns on dispatch. - "115 unit tests": the self-test log at this head ends
115 pass, 0 fail. All required CI is green ate2f7721. - Backfill:
--limit 1000plus-ge 1000then::error::. That is conservative at exactly 1000 open PRs, which is the direction the body promises.
What's good
The self-reference trap (requiring fleet/* makes fleet/verify wait on itself) is closed at the right place. The three-round fixed-point test shows it converges to all-green instead of only asserting one call. The post-then-reread loop is a sound replacement for a concurrency group that would roll up as a failed check.
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
Validate HTML and Scripts,Analyze (actions),Analyze (javascript-typescript),Scorecard (file-based checks). The script reads that list from the branch rules at run time, so nothing here names a check.workflow_runre-runs it when anypull_requestworkflow 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.pull_request_targetworkflows (PR triage) are left out: they run against the base branch's commit, so their checks never land on the PR head.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.