Skip to content

ci: fleet review lanes show up as commit statuses on the PR head - #212

Merged
askalf merged 15 commits into
masterfrom
ci/fleet-status
Sep 25, 2026
Merged

askalf merged 15 commits into
masterfrom
ci/fleet-status

Conversation

@askalf

@askalf askalf commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

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.

status pending green red
fleet/verify waiting on required CI (or the Breaker) at the head verified at the head, or not required (docs, assets, .github config, bot branch) a required check failed at the head
fleet/review waiting on Redline at the head, or on verification Redline approved the head Redline requested changes at the head
fleet/second-read waiting on the Second Read at the head, or on verification READY at the head, or not gating (non-code PR) NOT READY at the head, with its reason

Here the base branch requires test (6 OS/Node combinations), analyze (javascript-typescript). The script reads that list from the branch rules at run time, so nothing here names a check. workflow_run re-runs it when any pull_request or pull_request_target workflow 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 includes pull_request_target workflows (PR triage): their checks land on the PR head too, so the status job accepts their workflow_run events.

The three fleet/* contexts are never counted as CI themselves, so they can become required checks without fleet/verify waiting 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.

  • The status job runs the default branch's copy of scripts/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.
  • It runs on a GitHub-hosted runner with the workflow's own token (checks: read, statuses: write, reads only otherwise). It does not use the fleet's GitHub quota.
  • Check runs at the head are read across every page, so a busy head's required checks never fall off the first 100.
  • Fork PRs are skipped; the fleet does not review them.
  • No concurrency group: a cancelled run would roll up as a failed check. Instead each run posts only what differs from the head's newest statuses, then re-reads the PR and corrects what differs (up to three passes), so the run that acts last leaves statuses matching data at least as new as anything posted.
  • The statuses are informational. None is added to the ruleset's required checks.
  • self-test runs the script's 116 unit tests on the PR's code, read-only.
  • backfill (manual, workflow_dispatch) posts the lanes on every open same-repo PR, paging past 100 and failing loudly at 1000.

Next step, after this merges: add fleet/verify, fleet/review and fleet/second-read to the default branch's required status checks, then run backfill, so PRs opened earlier report too. With them required, GitHub refuses a merge (by hand or by native auto-merge) until every lane agrees at the head; without them, branch protection knows only CI plus one approval, so a merge by hand or by auto-merge can skip the Second Read.

How to test

  • node scripts/fleet-status.test.mjs: 116 pass, 0 fail.
  • The script and tests are plain ASCII. Tests build the Second Read's own separator character at run time with String.fromCharCode, so its real verdict line is covered with no dash in the source.

@github-actions github-actions Bot added github_actions Pull requests that update GitHub Actions code tests Test suite and CI size/L 200-799 hand-written lines labels Sep 25, 2026
…in the bot rule

A deleted `## Verification at` comment or an edited review's SECOND READ line
changes the lanes, but neither event re-ran the workflow, so a green status could
outlive what it stood for. issue_comment now includes `deleted` and
pull_request_review includes `edited`.

isBotPr is unchanged on purpose: it is the dispatcher's rule. review-dispatch.sh's
`gate` field and needsVerification() in platform's public-automerge-sweep.ts both
exempt a bot-shaped branch only when askalf or github-actions opened it, because
anyone can name a branch `release-x`. The doc comment now says so, and five tests
pin it (a person on bot/ or release/ is still verified).

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).

Verdict: approve — no blocking issues found in the live head.

I reviewed the workflow event filters and permissions, the status calculation (including head-scoped verification, Redline, Second Read, bot, and required-CI paths), and the accompanying unit-test coverage. All seven required CI checks are passing at e047cbc7e0548db6dd9568829098a31ca298d176; I did not run the test suite locally.

What's good: the workflow checks out and executes the default-branch status script rather than PR-controlled code for its write-token job, while the PR-controlled copy is exercised separately by the read-only self-test job. The added tests explicitly cover stale verdicts, deletion-sensitive verification inputs, required-check states, and the author-qualified bot-branch exemption.

No blocking issues found.

The concurrency group cancelled an in-flight run whenever a review, comment or
CI completion landed close behind another event. GitHub rolls a cancelled check
run up as a failure, so the PR's checks read red with nothing wrong (cordon#82,
truecopy-action#32 and checkout-with-retry#20 showed it within minutes).

The group goes. Ordering moves into the script: a run notes GitHub's clock (the
Date header) when it reads the PR, and before posting each context skips it if a
status for that context was posted after that moment, since that run read
fresher data. postedSince() is pure and has five tests (83/83).
The Second Read on amnesia#83 and redstamp#162: once fleet/verify, fleet/review
and fleet/second-read are required checks (the step this PR plans next), the
branch rules list them, and requiredCiState counted them as CI the head waits
on. fleet/verify pending made requiredCi pending, which kept fleet/verify
pending, so every code PR would have stayed blocked for good.

requiredCiState drops the three contexts before it reads anything else. Five
tests pin it, including the Second Read's reproduction: three rounds of feeding
each run's statuses back in as the next run's checks now end all green (88/88).

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: request changes — the status-ordering change still lets an older workflow overwrite a newer result.

Blocking — correctness — scripts/fleet-status.mjs:176-178

export function postedSince(statuses, context, readAtMs) {
if (!Number.isFinite(readAtMs)) return false;
return statuses.some((s) => s.context === context && Date.parse(s.created_at) > readAtMs);

The workflow's ordering guarantee is not valid at GitHub's timestamp precision. readAtMs comes from the HTTP Date header (scripts/fleet-status.mjs:211), and status created_at is also second-granularity. If an older run reads the PR at 03:00:10.x, a newer run observes a review change and posts its context during that same 03:00:10 second, the older run sees that status but Date.parse(created_at) === readAtMs. The strict > returns false, so it posts its stale lane result last and reverses the newer run. The new test explicitly encodes that incorrect case as acceptable at scripts/fleet-status.test.mjs:259:

check('posted in the same second: overwrite', !postedSince([st('fleet/review', '2026-09-25T03:00:10Z')], 'fleet/review', readAt));

Use an ordering mechanism that cannot collapse distinct runs into the same second (for example, a durable monotonic generation/lock for each PR-context, with the generation checked immediately before the status write), and change this test to demonstrate that a status from a fresher same-second run is retained.

// Do not decide freshness from second-granularity HTTP/status timestamps.
// Serialize each PR/context write through a durable generation and write only
// when this run still owns the latest generation.

What's good: required CI is green at the live head; I reviewed the workflow event gating, the lane-state logic, and the new ordering tests without running the local suite.

…0 files

Redline on browser-bridge#114, truecopy#212 and plumbline#52:

- A dismissed Second Read review is not a verdict. secondReadAtHead skips
  DISMISSED reviews, so a dismissed NOT READY no longer keeps the lane red.
- Ordering by the Date header and created_at cannot tell a same-second newer
  post from an older one. postedSince is gone. Each run now posts only what
  differs from the head's newest status per context, then re-reads everything
  and corrects what differs, up to three passes. The run that acts last
  re-reads after its own writes, so what stays on the head matches data at
  least as new as anything posted. latestByContext and statusesToPost are the
  pure parts, with tests for a stale overwrite being corrected.
- The file-count rule matches the live dispatcher: forge's readPrFacts reads
  the first 100 files and fails closed when there are more, so more than 100
  (not exactly 100) is code. A large docs-only PR still verifies once its
  required CI passes.

94/94.
Three test lines described where a case came from instead of what it checks: a
section title naming an old PR, and two comments referring to a review and to
the planned rollout step. They now describe the behavior only. The workflow
comment on ordering matches the post-then-verify loop. No logic change (94/94).

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: no blocking issues at aad8a16. The lane rules, the required-CI reduction and the post-then-verify loop do what the body says. One low-severity cosmetic defect in the NOT READY reason text, which does not affect any status state.

Note: the dispatch ticket pinned d3638ee. The live head is aad8a16, so that is the head I reviewed and the head this verdict applies to.

Findings

Low (non-blocking): scripts/fleet-status.mjs:117, the reason strip eats a leading punctuation character from the reason

      ? { state: 'NOT READY', reason: last.replace(/^NOT READY\W*/, '').trim() }

\W* is meant to drop the separator (-, the em dash, a colon), but it also drops any non-word character at the start of the reason itself. Reproduced against the file at this head: body SECOND READ: NOT READY \u2014 \foo()` returns null→reason: "foo() returns null", so the status reads NOT READY at abc1234: foo()` returns null. The same happens for reasons that start with (, ", [ or .. Reasons that start with a code span are common, because the verdict line usually names a symbol. Only the description text is affected; state: 'failure' is still correct.

Suggested fix:

      ? { state: 'NOT READY', reason: last.replace(/^NOT READY\s*(?:[-:\u2013\u2014]\s*)?/, '').trim() }

Suggested test, next to the existing "NOT READY is red with its reason" case:

check('a reason that starts with a code span keeps it',
  secondReadAtHead(base({ reviews: [review(SECOND_READ_LOGIN, 'COMMENTED', HEAD, 'SECOND READ: NOT READY \u2014 `x` is null')] })).reason === '`x` is null');

Boundaries I rebuilt from the diff, with the test that pins each one

predicate input behaviour pinned by
files.length > 100 (needsVerify) 99 / 100 / 101 docs files not code / not code / code "99 docs files", "exactly 100 docs files", "more than 100 files"
same, bot PR 100 files, bot/ branch exempt "a bot PR with 100 files is not code"
isBotPr identity AND branch person on bot/, release/; askalf on feature branch; dependabot on any branch not / not / not / bot the isBotPr block (both orders are covered)
requiredCiState own contexts only fleet/* required → none; fleet/* required and pending with CI green → passed as stated fleet-status.test.mjs:263-266 (these fail if the OWN_CONTEXTS filter is removed)
required check missing / running / failed with another pending / rerun passed / skipped, neutral pending / pending / failed / passed / passed as stated the requiredCiState block
ci === 'none' vs 'pending' with a label and comment at head Breaker rule applies only when no checks are required "CI pending: an old Breaker label..." / "no required checks: the label and comment still verify"
verifiedAtHead sha prefix 7-char prefix; latest comment at older head; label only; other login; findings/blocked headings true / false / false / false / false the verifiedAtHead blocks
r.commitId !== facts.head (Second Read) verdict at an older head, plus a lineless review at head none "a verdict at an older head plus a lineless review..."
r.state === 'DISMISSED' dismissed NOT READY; dismissed then READY pending / success the last block (fails if the dismissal guard is removed)
READY[ \t\r]*$ READY, mostly; READY\r\n none / READY "READY followed by text is not READY"; CRLF I checked by hand, and it is READY
fit at 140 140 / 141 chars whole / 137 + ... "the 140-character edge"
deterministic approval code / docs not a verdict / a verdict "deterministic approvals"

I read each new assertion to check that it fails without the guard it names. I found none that passes either way. The one that looks vacuous, "checks nobody requires do not hold it" (fleet-status.test.mjs:232), tests a non-required check, and its name says so; the self-exclusion is pinned separately at fleet-status.test.mjs:263-266.

PR body claims checked against the diff

  • "reads that list from the branch rules at run time": rules/branches/<base> at fleet-status.mjs:237. On master it returns the six test (...) contexts plus analyze (javascript-typescript), which matches the body. CI's job names match those contexts one for one.
  • "status runs the default branch's copy... skipped until this merges": ref: default_branch + sparse checkout + hashFiles guard (fleet-status.yml:43-55). The status check on this PR passed; master does not have the script yet.
  • Fork PRs are skipped both in the job if: and in readFacts. self-test has contents: read only. There is no concurrency group, and the 3-pass post/re-read loop is at fleet-status.mjs:271-288.
  • "94 unit tests": 94 check( calls in the diff; the self-test log at this head reads 94 pass, 0 fail.
  • "plain ASCII throughout": no byte above 0x7F in the diff or in the commit messages.

What's good

Unreadable rules fail to the label-and-comment rule and unreadable checks fail to pending, so neither error path can turn fleet/verify green. The workflow never executes PR code with statuses: write. The own-context exclusion avoids a real deadlock once the lanes become required checks.

What I read: the full diff (3 files, 727 lines), the base ruleset, the CI/CodeQL triggers and the self-test log. I ran the parser functions from this head's fleet-status.mjs on the inputs quoted above. I did not run the workflow.

SECOND READ: READY

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: changes requested — public test content contains generated-writing tells. rule:reads-as-generated

Blocking — generated-writing tell — scripts/fleet-status.test.mjs:69,95

review(SECOND_READ_LOGIN, 'COMMENTED', OLD, 'text\nSECOND READ: NOT READY \u2014 stale stack'),

review(SECOND_READ_LOGIN, 'COMMENTED', HEAD, 'SECOND READ: NOT READY \u2014 commit subject has an em dash'),

These added test fixtures construct em-dash text. The first-party public-repo gate treats an em dash anywhere in added public tests as a generated-writing tell. These cases only need punctuation which the NOT READY\W* parser recognizes; a normal hyphen still exercises that branch without shipping the tell.

review(SECOND_READ_LOGIN, 'COMMENTED', OLD, 'text\nSECOND READ: NOT READY - stale stack'),

Apply the same substitution to the fixture at line 95.

What's good: I re-read the live-head diff and the status reconciliation loop. Required CI is green at aad8a16; the fresh-status pass correctly re-reads PR facts and only posts contexts whose newest status differs.

…losed rules

- The two NOT READY test fixtures use an ASCII hyphen instead of a dash escape.
- The reason strip drops the one separator after NOT READY (hyphen, colon, or
  the Second Read's own dash) and keeps a leading backtick, quote or bracket,
  so a reason that names a symbol keeps its code span. Four tests.
- Unreadable branch rules count as pending, as the dispatcher waits on them,
  instead of falling back to the label-and-comment rule.
- The bot-rule test uses 101 files, so it fails if the bot rule is removed,
  and a counterpart pins that a person's 101-file docs PR is code.

99/99.
… that it does

The workflow_run list named only the workflows behind today's required checks,
so a required check added later from another workflow would leave fleet/verify
pending until the next PR event. The list now names every workflow in this
repository that runs on pull requests, and a test reads .github/workflows and
fails if one is missing from it, so adding a workflow without listing it fails
CI instead of stalling the lanes.

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: APPROVED — no blocking issues found in the live head 53049020ec687f1374a24d3859acf359a1f1b889.

What's good: I reviewed the workflow trigger expansion and its regression test. The new test derives every workflow that handles pull_request or pull_request_target from .github/workflows and checks that its displayed name appears in the workflow_run list, which prevents a later PR workflow from silently leaving fleet statuses stale. The workflow continues to execute the default branch copy with minimal read/status-write permissions. All seven required CI checks are green at this head. I did not run the local suite, per review policy.

…et cannot move a required check

A pull_request_target workflow runs against the base branch's latest commit, so
its checks land on that commit and never on the PR head, and the status job
drops its workflow_run events anyway. Listing one (PR triage) could never
refresh the lanes, and the list test passed regardless. The list now names only
pull_request workflows, and the test checks both directions: every
pull_request workflow is listed, and a pull_request_target-only workflow is not.

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: no blocking issues found; approved.

Required CI is green at the live head . I reviewed the added workflow, lane-state computation, status-post/re-read loop, and unit coverage. The workflow enumerates the repository pull-request workflows while excluding the pull_request_target-only workflow, so completion events that can produce head checks refresh the statuses without treating base-commit checks as PR-head signals. The lane contexts are excluded from required-CI evaluation, avoiding self-gating once they are required.

What is good: the implementation reads the default-branch script for privileged status writes, bounds reconciliation to three fresh reads, and its tests cover stale verdicts, dismissed reviews, bot/file-count boundaries, required-check transitions, and status reconciliation.

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: the lane logic looks right to me, but one claim in the PR body is not in the diff: the test for the Second Read's real verdict-line format is missing. Read at head c793292 (the ticket named ac35611, but the head has moved since).

Medium: the Second Read's real separator (U+2014) has no test, and the body says it does

PR body, "How to test":

plain ASCII throughout (the Second Read's dash appears in a test only as an escape)

There is no such test at c793292. scripts/fleet-status.test.mjs has no \u2014 escape and no non-ASCII character (checked by scanning the fetched file). Every NOT READY fixture uses an ASCII hyphen, for example:

scripts/fleet-status.test.mjs:327-330

  check('a reason that starts with a code span keeps it', reasonOf('SECOND READ: NOT READY - `x` is null') === '`x` is null');
  check('a hyphen separator is dropped', reasonOf('SECOND READ: NOT READY - stale stack') === 'stale stack');
  check('a colon separator is dropped', reasonOf('SECOND READ: NOT READY: (a) and (b)') === '(a) and (b)');
  check('no separator: the reason is kept whole', reasonOf('SECOND READ: NOT READY [scope] missing') === '[scope] missing');

The line this seat actually posts is SECOND READ: NOT READY <U+2014> <reason>. The script comment at scripts/fleet-status.mjs:14 also documents the format as SECOND READ: NOT READY - <reason>. So no fixture covers the one format the parser meets in production. Right now the code handles it. I ran secondReadAtHead from this head's file on 'x\r\n\r\nSECOND READ: NOT READY \u2014 x is null\r\n' and got { state: 'NOT READY', reason: 'x is null' } and NOT READY at c793292: x is null. The hazard is regression. Suppose someone narrows the separator class at scripts/fleet-status.mjs:118 to the separators that are tested:

      ? { state: 'NOT READY', reason: last.replace(/^NOT READY\s*(?:[^\w\s`'"([{]\s*)?/, '').trim() }

to (?:[-:]\s*)?. All 104 tests still pass, and every real NOT READY status would then read NOT READY at <sha>: <U+2014> <reason>. So the body describes a guard the diff does not have.

Suggested fix: add the test the body describes, written as an escape so the file stays ASCII:

  check('an em dash separator is dropped', reasonOf('SECOND READ: NOT READY \u2014 `x` is null') === '`x` is null');

and make the :14 comment match the posted format (or say "a dash"). If you would rather keep the test file as it is, drop the sentence from the body. Either way the claim and the diff will agree.

Boundaries I rebuilt from the diff

predicate input behaviour at this head pinned by
needsVerify file count 99 / 100 / 101 docs files not code / not code / code :203, :132, :130
needsVerify bot rule before count bot branch with 101 files exempt :201
isBotPr person on bot/, release/1.2; askalf on bot/; dependabot anywhere no / no / yes / yes :47, :296-300
verifiedAtHead 7-char prefix, older sha, later older comment, other login, findings/blocked heading, label missing per the rules :52-57, :148-150
requiredCiState [], own lanes only, unreported, in_progress, failure plus pending, rerun passed, skipped/neutral none / none / pending / pending / failed / passed / passed :228-235, :306-309
requiredCiState empty or unknown state '', stale, cancelled pending / failed / failed (probed; cancelled failing is correct) '' via unreported row only
redlineVerdict deterministic approval on code vs docs; DISMISSED/COMMENTED at head; CR after APPROVED at the same head skipped / counts / older verdict kept / red :114-119, :153-166
secondReadAtHead older head, no line, READY, mostly, last line wins, other login, DISMISSED, later lineless review per the rules :104-110, :173-180, :315-321
reason separator -, :, none, code span stripped / stripped / kept / kept :327-330; U+2014 not pinned (above)
fit 140 / 141 chars whole / 137 plus ... :220-221
statusesToPost identical, differing state, missing, same state with new description, stale overwrite skip / post / post / post / corrected :283-311
workflow_run list every pull_request workflow listed, pull_request_target excluded CI, CodeQL, docker, truecopy gate listed; PR triage out test loop at the end of the file (I checked it against the eight workflows on the branch)

What I checked and found good

  • Every required context on master (test (ubuntu|windows|macos-latest, 20|22), analyze (javascript-typescript)) comes from CI or CodeQL. Both are in the workflow_run list, so a finishing required check does refresh the lanes.
  • Unreadable rules or checks resolve to pending (scripts/fleet-status.mjs:241, :251). They never turn fleet/verify green.
  • status and backfill check out the default branch's script with persist-credentials: false. self-test runs the PR's code with contents: read only. Fork PRs are filtered in the job if and again in readFacts.
  • The rest of the body's claims match the diff: 104 checks, no concurrency group, post then re-read for up to three passes, own contexts never counted as CI. Both source files are plain ASCII.
  • CI is green at c793292, including self-test.

Not read: the upstream dispatcher (review-events.ts, review-dispatch.sh) that the rules say they mirror. I compared the rules against the PR's own description of them.

SECOND READ: NOT READY — the body says a test covers the Second Read's U+2014 separator, but no test does (fleet-status.test.mjs:327-330 are ASCII-only); add it or drop the claim

… own separator

- backfill read one page of 100 open PRs, so a larger repository would leave
  some PRs without lanes. It now lists up to 1000 (gh pages past 100) and fails
  at that cap instead of skipping PRs. Two tests pin the limit and the guard.
- The Second Read writes its verdict line with its own separator character, and
  after the ASCII fixtures nothing tested that form. The tests now build every
  separator form at run time with String.fromCharCode, the Second Read's
  included, so the real line is pinned with no dash in the source.
@github-actions github-actions Bot added size/XL 800+ hand-written lines and removed size/L 200-799 hand-written lines labels Sep 25, 2026
@askalf askalf added the author-fixes The PR's author answers its reviews; no fleet fix ticket label Sep 25, 2026

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: changes requested — public code adds a generated-writing tell. rule:reads-as-generated

Blocking — generated-writing tell — scripts/fleet-status.mjs:20

// posted line uses an em dash, U+2014; a hyphen or colon is accepted too).

The strict public-repository gate prohibits an em dash in newly added public code, comments, tests, commit messages, or the PR title. This comment embeds one directly; the same character is also newly added in the source/test commentary. Remove the prose tell while retaining the character handling itself.

//     posted line uses U+2014; a hyphen or colon is accepted too).

What I checked: the full three-file diff, status derivation and its unit-test coverage, event triggers and workflow permissions, and the live required CI suite (all seven required checks pass at c68e168). I did not run the local test suite.

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: No blocking issues at df9c8bb. One low, non-blocking note about check-run paging.

The ticket named 40ea3e7. The live head is df9c8bb, and since 40ea3e7 the only changes are two edits to the header comment (scripts/fleet-status.mjs:14-15, in c68e168 and df9c8bb). I read the full diff (all three files) plus the comment-only delta to df9c8bb, gh pr checks at df9c8bb (19 pass, backfill skipped, self-test green), the base branch's live ruleset, and the check runs on this PR's recent heads.

Findings

Low (non-blocking): scripts/fleet-status.mjs:248 reads only the first 100 check runs on the head

const runs = (await (await gh(`/repos/${repo}/commits/${p.head.sha}/check-runs?per_page=100`, token)).json()).check_runs ?? [];

Every other list in readFacts goes through ghAll, but this one is fetched once. GitHub lists check runs on a commit newest first: at c793292, index 0 was id 107942619244 (self-test) and index 29 was 107940069190 (status). So the runs that fall past page 1 are the oldest ones, which are the required test (...) and analyze (...) runs created by the push.

Every pull_request or pull_request_review event adds three check runs to the head through this workflow (status, self-test, backfill). c793292 already had 32 (5 Fleet status runs plus about 17 others). About 25 more review or label events at one head would take it past 100. After that, last.get(r) returns '' for a required check that passed, so requiredCiState returns 'pending' and fleet/verify says "Waiting on required CI" even though CI is green. The review and Second Read lanes stay held behind it too.

This fails closed, which is why it's low. Once the fleet/* contexts are required (the PR's stated next step), though, the PR can't be merged until something is pushed. A repo with a large CI matrix would reach 100 sooner. The script reads its required list at run time and is meant to work on any repo.

Suggested fix: use the response's total_count and page until everything is read (or request each required name with check_name=):

const runs = [];
for (let page = 1; ; page++) {
  const body = await (await gh(`/repos/${repo}/commits/${p.head.sha}/check-runs?per_page=100&page=${page}`, token)).json();
  runs.push(...(body.check_runs ?? []));
  if (runs.length >= (body.total_count ?? 0) || !(body.check_runs ?? []).length) break;
}

Boundaries I rebuilt from the diff

  • needsVerify file count (facts.files.length > 100): 99, 100 and 101 docs files, plus a bot PR with 101 files, are pinned at test file lines 130-134 and 201-203. ghAll reads every page of files, so the length is real.
  • requiredCiState: [] → none; only own contexts required → none; a missing check or one still running → pending; a failure alongside a pending check → failed; a rerun that passed after a failure → passed (last by id wins; check runs are sorted ascending); skipped/neutral → passed; own lanes required and pending → still passed. Each of these is pinned. Commit-status error and check-run cancelled/timed_out/stale fall to failed, which is correct and untested, but it's the same branch as failure.
  • verifiedAtHead: label without comment, comment without label, a 7-char prefix, the latest comment wins in both orders, another login, and findings/blocked headings are all pinned.
  • secondReadAtHead: another reviewer's login, an older head, DISMISSED, the latest review at head wins, a later review without the line keeps the verdict, READY, mostly → none, no reason → no trailing colon, and separators U+002D/U+003A/U+2013/U+2014 are all pinned. The strip regex at :119 keeps a leading backtick, quote or bracket, and that is pinned too. I checked by hand that the \b after NOT READY rejects NOT READYish.
  • redlineVerdict: deterministic approval on code vs docs, dismissal plus comment at head, and changes requested after an approval at the same head are all pinned.
  • fit at 140: 140 kept, 141 → 137 + ..., both pinned exactly.
  • Workflow if:: workflow_dispatch has no pull_request, so status skips and only backfill runs. issue_comment on a plain issue is dropped by pull_request != null. issue_comment on a fork PR passes the job if: but is dropped at readFacts (:229).
  • workflow_run list vs workflows on disk: the test at test file lines 333-361 walks every .yml and fails if a pull_request workflow is missing. I checked: pr-triage.yml runs only on pull_request_target and is correctly left out.

Tests that could pass without the change

I found none that are vacuous. The closest is checks nobody requires do not hold it (test file line 237): it passes whether or not OWN_CONTEXTS is filtered. But the own-lane filter is pinned separately by own lanes required and pending, CI green: still passed and by the three-round loop, and both fail without fleet-status.mjs:83.

PR body claims vs diff

These all match the diff: own contexts excluded from CI (:83), fork skip (:229, and the job if:), post-then-verify capped at three passes (:273-290), backfill paging past 100 and failing at 1000 (fleet-status.yml:114-118), and self-test running with contents: read only. The ruleset lookup matches the live master ruleset: 6 test contexts plus analyze (javascript-typescript).

What's good

The status job runs the default branch's script from a sparse checkout with persist-credentials: false. The PR number goes in through env, not an interpolated run: string. statuses: write is the only write scope. Taking the verdict only at the head and ignoring DISMISSED reviews matches how the dispatcher reads them.

SECOND READ: READY

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).

Verdict: changes requested — a required check can be left pending forever when a head has more than one page of check runs. rule:none

Blocking — correctness — scripts/fleet-status.mjs:248

const runs = (await (await gh(/repos/${repo}/commits/${p.head.sha}/check-runs?per_page=100, token)).json()).check_runs ?? [];

The checks API is paginated, but this request reads only its first 100 check runs. For a PR head with more than 100 runs (for example, a large required matrix or accumulated reruns), a required check outside that first page is absent from checks. requiredCiState consequently treats the required context as unreported and returns pending, so fleet/verify remains pending even after that required check has passed. This contradicts the intended CI-as-verification gate.

Fetch all check-run pages (as is already done for legacy statuses), then derive the final result from the complete set; add a regression test covering a required run on a subsequent page.

const runs = await ghAll(`/repos/${repo}/commits/${p.head.sha}/check-runs`, token);
const checks = runs.sort((a, b) => a.id - b.id)
  .map((c) => ({ name: c.name, state: c.status === 'completed' ? (c.conclusion ?? '') : c.status }));

What's good: required CI is green at df9c8bb30c80b1eee3aa12225b3f1a4bfacc9574; I reviewed the workflow, lane-state logic, pagination handling for statuses/files/reviews/comments, and the added unit coverage. I did not run the local suite, per review policy.

… the lanes too

The Second Read at e4ceacf:
- The check-runs list was read once, and every event at a head adds Fleet
  status runs, so past 100 runs the push-time required checks fell off the page
  and fleet/verify went back to pending. Paging is one pure helper,
  collectPages(), used for check runs and the list endpoints, with a test that
  finds a required check on page 2.
- A pull_request_target run's checks do land on the PR head (PR triage's
  `triage` run is listed at this head), so leaving them out was wrong. The
  status job now accepts workflow_run events from pull_request and
  pull_request_target runs, PR triage is listed again, and the test requires
  both kinds to be listed and checks the job's condition.

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: approve — no blocking issues found in the live-head re-review.

The prior blocking check-run pagination issue is addressed: scripts/fleet-status.mjs:392-395 uses collectPages for the check-runs endpoint, so a required run beyond the first page is included in the required-CI decision. The focused test at scripts/fleet-status.test.mjs:385-394 exercises a required check on page two. The workflow change at .github/workflows/fleet-status.yml:35-38 also accepts pull_request_target workflow-run events; I confirmed the current head has the PR-triage triage check at this PR head and its associated run is a pull_request_target run.

What's good: required CI is green at 16baab38629e1c30c3edcf22008e1029514cae32. I reviewed the 821-line diff and the live-head follow-up commit; I did not run the local suite, per review policy.

No blocking issues found.

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: No blocking issues at 16baab3. The one commit since my READY at df9c8bb fixes the check-run paging gap I raised there and brings pull_request_target runs back into the refresh list. Both changes do what the commit message says.

What I read: the full delta df9c8bb...16baab3 (1 commit, 3 files: fleet-status.yml +3/-2, fleet-status.mjs +17/-8, fleet-status.test.mjs +21/-10), the script and test file at the head, pr-triage.yml's trigger block, the live PR triage run on this head (run 36097084351), and gh pr checks at 16baab3: 19 pass, backfill skipped, and the self-test log reads 116 pass, 0 fail. I did not re-read the unchanged parts of the diff beyond what I covered at df9c8bb.

Findings

No correctness findings.

Info (non-blocking): the check-run call site is not under test. scripts/fleet-status.mjs:256-257:

        const runs = await collectPages(async (n) =>
          (await (await gh(`/repos/${repo}/commits/${p.head.sha}/check-runs?per_page=100&page=${n}`, token)).json()).check_runs ?? []);

The new tests pin collectPages itself, but nothing fails if this line goes back to a single fetch. That is how the rest of the readFacts main block works too (it isn't unit-tested), and the helper is the part with boundaries, so I'm not treating it as blocking.

Boundaries I rebuilt from the delta

  • collectPages stops at rows.length < size. Page 1 short (5) → one page. [100, 1] → 101 rows. [100, 100] → reads a third, empty page and returns 200. All three are pinned (test file lines 389-392). An empty page 1 ([], or a response with no check_runs, which becomes ?? []) → length 0 → returns [] on the first pass, so it can't loop.
  • A required check that only shows up on page 2 → passed. This is pinned at test file line 393 and fails if the pager drops page 2.
  • Runs created while paging: GitHub lists check runs newest first, so a run inserted mid-read pushes rows later. Page 2 then repeats a row instead of skipping one. The duplicates have the same id, and requiredCiState keeps the last value per name after the ascending id sort, so the result doesn't change.
  • ghAll now goes through collectPages with the same per_page=100 and ?/& join as before, so files, reviews, comments and statuses behave the same.
  • Workflow if: for workflow_run: event is pull_request → accepted (unchanged). pull_request_target from a same-repo branch → accepted. I checked this against live data: run 36097084351 has event: pull_request_target, head_sha: 16baab3…, head_repository: askalf/truecopy, pull_requests: [#212], so pull_requests[0].number resolves PR. pull_request_target from a fork → dropped by head_repository.full_name == github.repository, and readFacts drops it again. Any other event (push, schedule) → still dropped.
  • Test's workflow scan, /\bpull_request(_target)?\b/: \b doesn't match between t and _, so a trigger like pull_request_review alone isn't matched and doesn't force a listing. pr-triage.yml (pull_request_target only) now has to be listed, and it is (fleet-status.yml:18). Removing 'PR triage' from the list fails the test, and so does narrowing the if: back to pull_request (test file lines 357-361).

Tests that could pass without the change

None of the new assertions are vacuous. Each collectPages check fails if the helper returns only page 1. The listing check fails without 'PR triage', and the fromJSON check fails without the widened if:. One caveat on the last one: it's a string match against the YAML, so it proves the text is there, not that the text is inside the status job's condition. It is the only place that string occurs in the file.

PR body and commit claims vs the delta

  • "check runs at the head are read across every page": yes, :256-257.
  • "pull_request_target workflows (PR triage)… the status job accepts their workflow_run events": yes, fleet-status.yml:18 and :29-30.
  • "116 unit tests": this matches the self-test log at this head.

What's good

The paging is one pure helper, shared by the list endpoints and check runs, instead of a second copy of the loop. The fork guard stays in two places (workflow if: and readFacts), so widening the event filter to pull_request_target doesn't let fork PRs through.

SECOND READ: READY

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: no blocking issues at 38777ab. This head merges master (#211, 6995e6f) into the branch and leaves the three PR files unchanged. I re-checked the lane rules against the workflows and ruleset the merge brings in, and they still hold.

What I read: the full PR diff at this head (fleet-status.yml, fleet-status.mjs, fleet-status.test.mjs); the compare 16baab3...38777ab (10 files from master, none of them the PR's three); the on: block of every workflow in .github/workflows at this head; the live master ruleset's required checks; the check runs and statuses at 38777ab; and the self-test log for this head (run 36097448534: 116 pass, 0 fail). I did not run anything locally.

Findings

No correctness findings.

Does the merge from master change anything for the lanes?

  • New checks from #211 (test (*, 24), packed tarball (*), actionlint, analyze (actions)): the ruleset requires only test ({ubuntu,windows,macos}-latest, {20,22}) and analyze (javascript-typescript). requiredCiState (scripts/fleet-status.mjs:82-94) looks only at required names, so the new checks can't hold fleet/verify or turn it red. All of them pass at this head anyway.
  • Workflow triggers: #211 adds no new pull_request/pull_request_target workflow. marketplace-watch.yml gains a push trigger only. At this head the self-test's workflow scan lists CI, CodeQL, docker, PR triage and truecopy gate, and all five are listed in workflows: [CI, CodeQL, docker, 'truecopy gate', 'PR triage'] (.github/workflows/fleet-status.yml:18).
  • New linters now cover this PR's workflow: actionlint and CodeQL analyze (actions) both pass at this head. The code-scanning query for refs/pull/212/merge returns no open alerts.

Boundaries I rebuilt, and what pins each

  • facts.files.length > 100 (fleet-status.mjs:56): 99 and 100 docs files are not code, 101 is. A bot PR over 100 files is still exempt (fleet-status.test.mjs:131-135, :202-204). 0 files is not code: verify and second-read go green and review waits on Redline, and there is nothing to verify.
  • requiredCiState: no required checks → none; one not reported yet ('') or running → pending; a failure alongside a pending check → failed; a failure then a passing rerun of the same name → passed; skipped/neutral → passed; the fleet/* contexts are excluded even when they are required and pending. Each has its own check (fleet-status.test.mjs:229-238, :269-286). A completed check run with a null conclusion maps to '', so it stays pending and can't go green.
  • verifiedAtHead: needs both the label and the comment; takes the latest verifier comment; a 7-char prefix counts; findings/blocked headings don't count (fleet-status.test.mjs:52-58, :149-151). It is consulted only when requiredCi === 'none', and pending CI with a stale label stays pending (:249).
  • secondReadAtHead: another login, another head, a DISMISSED state, READY, mostly, a later lineless review, and U+002D/003A/2013/2014 separators are each pinned (fleet-status.test.mjs:106-110, :174-187, :319-330, :370-374). NOT READYISH can't match, because \b needs a word boundary after READY. A CRLF body ends the .* capture before \r.
  • fit at 140/141 characters: fleet-status.test.mjs:221-222.
  • collectPages: pages of [5], [100, 1] and [100, 100], plus a required check that only appears on page 2 (fleet-status.test.mjs:390-393). An empty first page returns [] immediately, so it can't loop.
  • Workflow if:: workflow_dispatch has no pull_request, so status is skipped and only backfill runs. Fork events are dropped by head_repository/head.repo in the workflow and again in readFacts (fleet-status.mjs:237). issue_comment on a plain issue is dropped by issue.pull_request != null.

Tests that could pass without the change

I found none. Each own-context check fails if the OWN_CONTEXTS filter at fleet-status.mjs:83 is removed. Each separator check fails if its character is dropped from the strip. The workflow-list checks fail if a listed workflow is removed.

PR body claims vs the diff at this head

  • "the base branch requires test (6 OS/Node combinations), analyze (javascript-typescript)": the ruleset lists exactly those 7 contexts, even though CI now runs 9 test combinations.
  • "116 unit tests": matches the self-test log at 38777ab.
  • "runs the default branch's copy … never the PR's code": status and backfill check out github.event.repository.default_branch with a sparse checkout of the script and persist-credentials: false. The step is gated on hashFiles, so it is skipped until this merges.
  • "No concurrency group … up to three passes": fleet-status.mjs:282-299.

What's good

The only inputs that could be injected are numeric (PR comes from event number fields), and no ${{ }} reaches a run: script in the status job. Unreadable rules or checks fail closed to pending (fleet-status.mjs:243-261), so an API error can never turn fleet/verify green.

SECOND READ: READY

@askalf
askalf merged commit 160f7b7 into master Sep 25, 2026
26 checks passed
@askalf
askalf deleted the ci/fleet-status branch September 25, 2026 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-fixes The PR's author answers its reviews; no fleet fix ticket github_actions Pull requests that update GitHub Actions code size/XL 800+ hand-written lines tests Test suite and CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants