Skip to content

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

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

askalf merged 14 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 the seven scan/verify fixtures, analyze (actions). The script reads that list from the branch rules at run time, so nothing here names a check. workflow_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 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/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: 115 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 size/L 200-799 hand-written lines labels 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: approve — no blocking issues found in the workflow, status calculation, or unit-test coverage reviewed.

What's good: the workflow executes the default-branch script with narrowly scoped read/status permissions; the status logic keeps verdicts SHA-scoped and distinguishes required-CI verification from Breaker verification. I also checked the required CI signal at 573c703: all seven required checks are passing. I did not run the local suite; CI is the test signal.

…in the bot rule

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

isBotPr is unchanged on purpose: it is the dispatcher's rule. review-dispatch.sh's
`gate` field and needsVerification() in platform's public-automerge-sweep.ts both
exempt a bot-shaped branch only when askalf or github-actions opened it, because
anyone can name a branch `release-x`. The doc comment now says so, and five tests
pin it (a person on bot/ or release/ is still verified).
The concurrency group cancelled an in-flight run whenever a review, comment or
CI completion landed close behind another event. GitHub rolls a cancelled check
run up as a failure, so the PR's checks read red with nothing wrong (cordon#82,
truecopy-action#32 and checkout-with-retry#20 showed it within minutes).

The group goes. Ordering moves into the script: a run notes GitHub's clock (the
Date header) when it reads the PR, and before posting each context skips it if a
status for that context was posted after that moment, since that run read
fresher data. postedSince() is pure and has five tests (83/83).

@sprayberry-secondread 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 change does what it says as merged. The follow-up the PR body prescribes (make the fleet/* contexts required) would stop fleet/verify from ever going green on a code PR, and after that every code PR is blocked for good.

Findings

1. High: required fleet/* contexts feed back into requiredCiState and deadlock fleet/verify

scripts/fleet-status.mjs:225-226 (CLI block, live head 874c199) builds the required list straight from the branch rules:

    required = rules.filter((r) => r.type === 'required_status_checks')
      .flatMap((r) => (r.parameters?.required_status_checks ?? []).map((c) => c.context));

and then scripts/fleet-status.mjs:236 hands the PR head's commit statuses, which include the script's own fleet/* statuses, to:

      requiredCi = requiredCiState(required, [...statuses, ...checks]);

The PR body's Next step is to "add fleet/verify, fleet/review and fleet/second-read to the default branch's required status checks". Once that is done:

  • On the first run at a new code head, none of the fleet/* contexts has reported yet, so requiredCiState returns 'pending' (s === '').
  • laneStatuses then posts fleet/verify = pending, and gated holds fleet/review and fleet/second-read at pending.
  • On every later run, fleet/verify is required and pending, which matches CHECK_WAITING, so requiredCi stays 'pending'. verified is false on every run.

I reproduced this against the script at both 573c703 and 874c199. Inputs: required = ['analyze (actions)', 'fleet/verify', 'fleet/review', 'fleet/second-read'], CI green, Redline APPROVED and SECOND READ: READY at head. Each run's output was fed back in as statuses:

run1 pending -> fleet/verify=pending fleet/review=pending fleet/second-read=pending
run2 pending -> fleet/verify=pending fleet/review=pending fleet/second-read=pending
run3 pending -> fleet/verify=pending fleet/review=pending fleet/second-read=pending

Because those contexts would be required, GitHub then refuses every merge of a code PR, including when all three lanes agree. Adding the verified label and the Verification comment does not help, because ci !== 'none' skips verifiedAtHead. The test checks nobody requires do not hold it (fleet-status.test.mjs) covers the case where fleet/verify is not required. Nothing covers the case this PR sets up next.

The same list-building code is in askalf/dario#1419 at 2e0f646 (the source this PR says it matches), so the fix probably needs to go there too.

Suggested fix:

    required = rules.filter((r) => r.type === 'required_status_checks')
      .flatMap((r) => (r.parameters?.required_status_checks ?? []).map((c) => c.context))
      .filter((c) => !Object.values(CONTEXTS).includes(c));

A pure-function version of the fix, if you prefer it in requiredCiState so it can be tested:

export function requiredCiState(required, checks) {
  required = required.filter((r) => !Object.values(CONTEXTS).includes(r));
  if (!required.length) return 'none';

Suggested test:

check('the fleet lanes themselves are never required CI',
  requiredCiState(['test', CONTEXTS.verify, CONTEXTS.review, CONTEXTS.secondRead],
    [ok('test'), { name: CONTEXTS.verify, state: 'pending' }]) === 'passed');

What I checked

  • The head moved from 573c703 (the head the ticket named) to 874c199 while I was reviewing, so this review covers the live head. I read the delta (4ff4c1a, 874c199): it adds edited reviews and deleted comments as triggers, drops the concurrency group, and adds postedSince so an older run skips a context a newer run has already posted. The ordering logic and its 5 new tests are sound. Its tie case (a status posted in the same second as the read gets overwritten) is deliberate and pinned by a test. isBotPr is unchanged apart from its doc comment, and it gets 5 more tests.
  • Boundaries in the lane logic. requiredCiState: empty required list gives none; unreported, running and failed checks each give the documented result; the last result per name wins; skipped and neutral pass. verifiedAtHead: label without comment, comment without label, a 7-char prefix, an older head, a later older comment, another login, and the findings/blocked headings. secondReadAtHead: no line, READY, mostly, the last line in a body, the latest review at head, a later review without the line, an old-head verdict. The 140/141-character cut and the 99/100-file threshold. Each has a pinning test that fails when the predicate is flipped, and I found no vacuous assertion.
  • The deterministic-approval exclusion applies only on code, and a docs PR still shows Redline's verdict. That matches the body's table.
  • Workflow: permissions: {} at the top, with per-job grants matching the body. The status and backfill jobs sparse-check-out the default branch's script, so PR code is never run with statuses: write. self-test has contents: read only. The fork filter is in both the job if and the script. workflow_run lists ci and CodeQL, which are the workflows (ci.yml, codeql.yml) that produce the seven required contexts I read from the branch rules.
  • Status and check-run ordering: /statuses is newest-first and gets .reverse()d; check-runs are sorted by id. In both cases "last wins" means newest wins.
  • I ran scripts/fleet-status.test.mjs once at each head: 73 pass at 573c703, 83 pass at 874c199, 0 fail. The body still says 73; at 874c199 the count is 83. CI (gh pr checks) was green at 573c703.

What's good

The lane rules are pure and have thorough tests. Both statuses and the fork filter are handled read-only where they need to be. Running the default branch's script under statuses: write is the right trust boundary for this.

SECOND READ: NOT READY — fleet-status.mjs keeps its own fleet/* contexts in the required list, so once they are made required (the body's next step) fleet/verify stays pending on every code PR

The Second Read on amnesia#83 and redstamp#162: once fleet/verify, fleet/review
and fleet/second-read are required checks (the step this PR plans next), the
branch rules list them, and requiredCiState counted them as CI the head waits
on. fleet/verify pending made requiredCi pending, which kept fleet/verify
pending, so every code PR would have stayed blocked for good.

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

@sprayberry-redline 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 at e1bfa3b5e736d6b959ace73e1c92fae5fd8de571.

What I checked: all seven required CI checks pass at the live head. I reviewed the workflow trust boundary and the status computation, including required-check filtering, status/check ordering, stale-run suppression, fork handling, verification/review gates, and the new regression tests. In particular, scripts/fleet-status.mjs:209-220 filters the three self-posted fleet/* contexts before evaluating required CI, and scripts/fleet-status.test.mjs:664-681 pins both pending self-statuses and three successive status-feedback rounds. This resolves the prior deadlock scenario without excluding actual required CI. I also reviewed the latest head delta and its commit messages; no generated-writing tell or security-sensitive privilege expansion was introduced. I did not run the local suite, per review policy; required CI is green.

What's good: .github/workflows/fleet-status.yml:43-66 checks out the default branch's status script for the job holding statuses: write, while the PR-code self-test remains read-only. That preserves the intended trust boundary.

…0 files

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

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

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

@sprayberry-secondread 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 2a3ba22. The self-wait from my last read (at 874c199) is fixed, and a test pins the fix that fails without it.

The ticket named e1bfa3b. The live head is 2a3ba22, one commit later, and that commit changes only test names, test comments and one workflow comment. This review is at 2a3ba22.

Previous finding, re-checked

scripts/fleet-status.mjs:82

required = required.filter((r) => !OWN_CONTEXTS.has(r));

The three fleet/* contexts are dropped before anything else is read. The tests that pin it would fail without the filter:

  • own lanes required and pending, CI green: still passed. Without the filter, fleet/verify is PENDING, so the result is pending.
  • only own lanes required: none. Without the filter, the result is pending.
  • own lanes required, three rounds: all three green. This feeds each round's output back in as the next round's checks. Without the filter, round 0 has the fleet/* contexts unreported, so the result is pending and stays pending, and the assertion fails.

Boundaries I rebuilt from the diff

predicate input behaviour pinned by
facts.files.length > 100 99 / 100 / 101 docs files not code / not code / code 99 docs files, exactly 100 docs files, more than 100 files
same, with CI 101 docs files, requiredCi: 'passed' verify green more than 100 docs files still verifies once required CI passes
requiredCiState [] / only own / own + CI green / own + CI running none / none / passed / pending four checks in the own-lanes block
requiredCiState last-wins fail then success for the same name passed a rerun that passed
secondReadAtHead DISMISSED at head / lineless later review / older-head verdict skipped / keeps verdict / none three checks, one each
fit 140 / 141 chars kept whole / cut to 137 + ... the 140-character edge
statusesToPost identical / state differs / missing / description differs skip / post / post / post four checks in the post-then-verify block

Every row I could reach has a test. I found no assertion that passes both with and without the code it is meant to cover.

Notes (non-blocking)

  • scripts/fleet-status.mjs:112 skips r.state === 'DISMISSED' for the Second Read. This seat only posts --comment reviews, and GitHub refuses to dismiss a COMMENTED review. So this branch guards a state the live data does not produce today. The skip is harmless and stays correct if that ever changes.
  • scripts/fleet-status.mjs:246 reads check-runs with a single ?per_page=100 and does not paginate. Past 100 runs, a required check could be missed. The code then treats it as pending, so it cannot turn fleet/verify green by mistake. This repo has about 13 runs per head.
  • The PR body says the base requires "the seven scan/verify fixtures, analyze (actions)". The live ruleset on master lists six scan/verify fixtures plus analyze (actions), seven in total. The script reads the list at run time, so this affects only the body's wording.

What I checked

  • The full diff (3 files, +727).
  • gh pr checks at 2a3ba22: all required checks and self-test pass.
  • 94 check( calls in the test file, which matches the body's 94.
  • No non-ASCII bytes anywhere in the diff.
  • scripts/fleet-status.mjs has the same blob (8b338ee) as at askalf/dario#1419's head. I could not fetch the test file there to compare it.
  • The trust boundary: status and backfill check out and run only the default branch's script. The PR number reaches the script only through env, never interpolated into run:. Fork PRs are filtered in both the if: and readFacts. self-test runs with contents: read only.

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: request changes — a public test adds an em dash, a generated-writing tell that must be removed. rule:reads-as-generated

Blocking — generated-writing tell

scripts/fleet-status.test.mjs:70

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

This test input produces an em dash in the Second Read text. First-party strict mode treats an em dash in public code or tests as a blocking generated-writing tell. The parser only needs a NOT READY line to establish that an older-head verdict remains stale; a plain ASCII separator exercises that behavior without the tell.

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

What's good: required CI is green at 2a3ba22; I reviewed the workflow's event/permission boundaries, status reconciliation loop, required-check exclusion, and the unit coverage for lane state transitions.

I did not run the local test suite, per review policy; CI is the test signal.

…losed rules

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

99/99.
… that it does

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

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

@sprayberry-redline 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 complete 773-line addition: the event and permission boundaries in .github/workflows/fleet-status.yml, the status/verification and review-state logic in scripts/fleet-status.mjs, and its unit coverage in scripts/fleet-status.test.mjs. In particular, I checked the required-CI gate, same-head verdict checks, dismissal handling, bot and documentation exemptions, status convergence loop, and the workflow list guard. All required CI checks are passing at c63cfad. I did not run the test suite locally, per review-environment policy; CI is the execution signal.

What's good: the workflow checks out the default branch’s status script with a minimally scoped token, avoids treating its own statuses as CI, and the unit tests cover the relevant stale-head and rerun-state transitions.

No blocking issues found.

@sprayberry-secondread 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: one reachable row is left untested. The Second Read's em dash is the only separator it actually posts, and after the ASCII-fixture change no test covers it. The PR body still says one does.

Read at c63cfad: the full delta since my last read at 2a3ba22 (9c40f53, 006ea23, c63cfad: scripts/fleet-status.mjs +8/-7, scripts/fleet-status.test.mjs +49/-4, .github/workflows/fleet-status.yml +1/-1). I also read laneStatuses, requiredCiState and the readFacts/post loop in context, and the on: blocks of all six workflows. CI is green at this head, and self-test reports 103 pass, 0 fail.

Medium: the em dash separator is a reachable row with no test, and the body says a test covers it

scripts/fleet-status.mjs:117-118

      // Drop the one separator after NOT READY; keep a leading backtick, quote or bracket.
      ? { state: 'NOT READY', reason: last.replace(/^NOT READY\s*(?:[^\w\s`'"([{]\s*)?/, '').trim() }

The Second Read posts its verdict in exactly one shape: SECOND READ: NOT READY + + U+2014 + <reason>. The commit message of 9c40f53 names that case ("hyphen, colon, or the Second Read's own dash"). But 9c40f53 also changed the two fixtures that held it ('...NOT READY \u2014 stale stack' and '...NOT READY \u2014 commit subject has an em dash') to ASCII hyphens, at scripts/fleet-status.test.mjs:73 and :99. The four new separator tests at scripts/fleet-status.test.mjs:327-330 cover hyphen, colon, code span after a hyphen, and no separator. The test file now contains no \u2014 and no non-ASCII character at all.

Failure scenario: someone narrows the class to the separators the tests show, say /^NOT READY\s*(?:[-:]\s*)?/. All 103 tests still pass. I ran both regexes on the real verdict shape:

"NOT READY \u2014 `x` is null"  current => "`x` is null"   narrowed => "\u2014 `x` is null"

With the narrowed regex, every real NOT READY status would read NOT READY at <sha>: — ..., and nothing would fail. The current code handles the case correctly; no test pins it.

The PR body says the opposite: "(the Second Read's dash appears in a test only as an escape)". That was true before 9c40f53 and is false at this head.

Suggested fix (the escape keeps the file ASCII, which is what the body describes):

  check('the Second Read\'s own dash separator is dropped', reasonOf('SECOND READ: NOT READY \u2014 `x` is null') === '`x` is null');

Checked, no finding

  • Fail-closed rules (scripts/fleet-status.mjs:236-252): rules unreadable → required = null → 'pending'. Rules read with none required → 'none' → label-and-comment rule. Rules list only fleet/* contexts → fetched, then requiredCiState filters them out (:82) → 'none', so fleet/verify never waits on itself. A statuses or check-runs read failure → 'pending'. None of these paths can turn fleet/verify green on a read error.
  • Bot rule vs file count (scripts/fleet-status.test.mjs:201-203): the bot test now uses 101 files, so it fails if the bot rule is removed or reordered after the >100 rule. The 101-file person counterpart pins the other side.
  • workflow_run list test (scripts/fleet-status.test.mjs:333-361): \bpull_request\b(?!_target) does not match pull_request_target (_ is a word character). labels.yml runs on path-filtered pull_request and is listed, and pr-triage.yml runs only on pull_request_target and is left out; the CI log shows all four rows. The test can fail: drop labels from .github/workflows/fleet-status.yml:18 and its row fails. The name: regex is anchored at column 0, so a job-level name: cannot shadow the workflow name.
  • The script matches askalf/dario#1419's copy byte for byte at e9aabb5, as the body says.

What's good: the rework makes every read failure pending, and the list test runs in both directions, so it cannot pass just because the list is short.

SECOND READ: NOT READY — no test pins the em dash separator, the only one the Second Read posts (fleet-status.test.mjs has no \u2014), and the body says one does

…n escape)

The ASCII-fixture change left no test on the only separator the Second
Read actually posts. Narrowing the strip class to [-:] now fails this case.

Ticket FIX-6c0a53d9
… own separator

- backfill read one page of 100 open PRs, so a larger repository would leave
  some PRs without lanes. It now lists up to 1000 (gh pages past 100) and fails
  at that cap instead of skipping PRs. Two tests pin the limit and the guard.
- The Second Read writes its verdict line with its own separator character, and
  after the ASCII fixtures nothing tested that form. The tests now build every
  separator form at run time with String.fromCharCode, the Second Read's
  included, so the real line is pinned with no dash in the source.

@sprayberry-redline 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 documentation-only .txt PR is classified as code and is incorrectly held for verification. rule:none

Blocking — scripts/fleet-status.mjs:163-166

if (/\.(md|svg|png|jpe?g|webp|gif)$/i.test(path)) return false;

if (/^docs\/.*\.txt$/i.test(path)) return false;

The first-party exemption covers documentation and assets including .txt files, not only .txt files below docs/. A same-repository PR that changes only a root NOTICE.txt (or changelog/release-notes.txt) reaches line 167 as a code path. needsVerify() then returns true, so fleet/verify remains “Waiting on the Breaker” instead of reporting the exempt success state. This makes a docs-only PR wait for a verification that should not be required.

if (/\.(md|txt|svg|png|jpe?g|webp|gif)$/i.test(path)) return false;
if (/^docs\/.*\.txt$/i.test(path)) return false;

Add a unit case for a .txt file outside docs/ so this classification remains aligned with the exemption rule.

I checked the live-head workflow, status computation, and associated unit-test changes. All required CI checks are passing at fcfaa5a.

@askalf askalf added the author-fixes The PR's author answers its reviews; no fleet fix ticket label Sep 25, 2026

@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 2ad71ce. My last NOT READY (at c63cfad: no test covered the em dash separator) is fixed, and every claim in the body matches the code.

The ticket was cut at fcfaa5a, but the head moved to 2ad71ce (backfill paging) before I started. A verdict only counts at the head, so this read covers 2ad71ce: all three files in full (scripts/fleet-status.mjs, scripts/fleet-status.test.mjs, .github/workflows/fleet-status.yml), plus the c63cfad..2ad71ce delta and gh pr checks.

What I checked

Prior finding, now closed. scripts/fleet-status.test.mjs:364-375:

for (const code of [0x2d, 0x3a, 0x2013, 0x2014]) {
  ...
  check(`separator U+${hex} is dropped`, reasonOf(`SECOND READ: NOT READY ${String.fromCharCode(code)} stale stack`) === 'stale stack');
}
const line = `SECOND READ: NOT READY ${String.fromCharCode(0x2014)} \`x\` is null`;

Both new assertions can fail. If the separator strip in fleet-status.mjs (/^NOT READY\s*(?:[^\w\s'"([{]\s*)?/) were narrowed to ASCII [-:], the U+2014 case would return " stale stack". The end-to-end case goes through laneStatuses, so it pins the real verdict line (em dash, then a reason that opens with a code span) all the way to a red fleet/second-readdescription ending in ``xis null ``. CI's self-test log at this head showsseparator U+2014 is droppedok and110 pass, 0 fail`.

Backfill delta. .github/workflows/fleet-status.yml:113-119. I rebuilt the boundaries myself:

input behaviour pinned by
0 open PRs prs is empty, the loop does nothing, exit 0 not tested, but trivially correct
1 to 999 gh pages on its own, and every same-repo PR is posted backfill reads more than one page (limit > 100)
exactly 1000 (the cap) ::error:: + exit 1. This is conservative because it can't tell 1000 from more. backfill fails at its cap (-ge ${limit})
fork PRs dropped by `select(.isCrossRepository not)`, and the script checks again

The two workflow tests read the YAML's text, so they can fail: lowering --limit to 100 or removing the -ge/::error:: guard breaks them.

Other boundaries I rebuilt from the diff:

  • requiredCiState:
    • [] gives none.
    • A list of only its own fleet/* contexts gives none (lines 237 and 271). So the self-wait I raised at 874c199 stays fixed.
    • A required check that is missing, unreadable or still waiting gives pending, and a non-pass conclusion gives failed.
  • needsVerify:
    • At the 100/101 file edge, 101 is code no matter what the files are (lines 129-133 and 201-202).
    • A bot branch opened by an outside author is still code.
  • secondReadAtHead:
    • A review at an old sha or a DISMISSED review doesn't count, and the last line at the head wins.
    • READY, mostly is not a verdict.
    • CRLF bodies still match, because JS $/. treat \r as a line end.
  • Descriptions:
    • Exactly 140 characters is kept.
    • 141 is truncated to 140 (lines 144 and 214-220).

Body claims:

  • The script, tests and workflow are plain ASCII. I checked this with a byte grep: no bytes ≥ 0x80.
  • workflow_run lists ci, CodeQL and labels, which are exactly the pull_request workflows in .github/workflows/. PR triage (pull_request_target) is left out, and lines 352-361 enforce both.
  • status checks out only the default branch's scripts/fleet-status.mjs and is gated on hashFiles.
  • Permissions are exactly contents/pull-requests/issues/checks: read and statuses: write.
  • There is no concurrency group, and the re-read/correct loop is capped at three passes.

Not a finding, for whoever runs it: on a repo with hundreds of open PRs, backfill (about 8 API calls per PR per pass, timeout-minutes: 10) would hit the timeout or the GITHUB_TOKEN rate limit before it reached the 1000 cap. Both fail loudly under set -e, so no PR is skipped silently. At this repo's size it doesn't matter.

What's good: the lane rules are pure functions with the I/O kept separate. Everything that can't be read counts as pending and can never turn a lane green. The workflow-list test keeps the refresh triggers matching the repo's workflows as they change.

SECOND READ: READY

@askalf
askalf dismissed sprayberry-redline’s stale review September 25, 2026 05:52

Dismissed for a re-read: this verdict came from Redline's pre-398 rule text (.txt exempt everywhere). Migration 398 (live since 04:54Z) states the dispatcher's rule: .txt is exempt only under docs/, which is what this script implements.

@sprayberry-redline 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 — required check-runs beyond the first API page are treated as missing, so the verify lane can remain pending forever after all required CI is green. rule:none

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

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

The endpoint is requested only once, unlike the statuses endpoint immediately above which uses ghAll. On a head with more than 100 check runs (for example matrix jobs, reruns, or check-producing integrations), a required check whose run falls on page two is absent from checks. requiredCiState() then receives no state for that required context and returns pending; fleet/verify consequently stays pending even though every required check passed. The new workflow is intended to make these lane statuses required, so that becomes a merge-blocking false pending state.

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

Please add a regression case covering a required check returned only after the first page.

I checked the live-head diff and the required CI signal (all required checks are green). I did not run the local suite, per review-environment policy.

… the lanes too

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

@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 rework.

The replacement uses collectPages() for check runs at scripts/fleet-status.mjs:255-258, and the added tests exercise a required check returned on page two at scripts/fleet-status.test.mjs:374-383. I also checked the workflow change at .github/workflows/fleet-status.yml:19-20: PR triage is included and both pull-request event variants are admitted; its check run on this live head is attached to the PR head.

Required CI is green at ba0339182cefbf7b5ef8259cafbf650611baed7d. I reviewed the live-head diff and did not run the local suite, per review-environment policy.

What is good: the pagination helper is shared by the list endpoints and has direct boundary coverage for the first short page, a second page, and a full final page.

@sprayberry-secondread 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: I found no blocking issues at ba03391. The one new commit fixes both points I raised at the previous head: check runs are now read across every page, and pull_request_target runs refresh the lanes too. Each fix has a test that can fail.

Scope of this read: I read the delta 2ad71ce..ba03391 (one commit, +41/-20 across the 3 files) in full, plus the surrounding readFacts() / requiredCiState() code in a read-only clone at ba03391. I read the rest of the PR at earlier heads, and it is unchanged since then. I did not run the tests locally. CI self-test at this head logs 115 pass, 0 fail, and every check passes (backfill is skipped, as it should be).

Boundaries rebuilt from the delta

collectPages(page, size = 100), scripts/fleet-status.mjs:145-151:

    const rows = await page(n);
    out.push(...rows);
    if (rows.length < size) return out;
input behaviour pinned by
empty first page (0 rows) returns [] after one call not pinned separately. It is the same < size branch as the 5-row case.
short first page (5) one page, 5 rows a short first page is the only page (test.mjs:392)
exactly size (100) then more reads on 101 rows over two pages are all read (test.mjs:390)
every page full (100, 100) reads a third, empty page, then stops at 200 a full last page reads one more, empty, page (test.mjs:391)
required check only on page 2 requiredCiState sees it and returns passed a required check on page 2 counts (test.mjs:393). This fails if paging stops after page 1 (it would give pending).
check-runs page with no check_runs key ?? [] ends the loop .mjs:255-256. Not tested. An API error throws in gh() first (.mjs:216), and the catch makes it pending.

The workflow condition is fleet-status.yml:29-32:

      (github.event_name == 'workflow_run' &&
       contains(fromJSON('["pull_request","pull_request_target"]'), github.event.workflow_run.event) &&
  • Did a pull_request_target run land on the head? I checked the live run instead of trusting the comment. Run 36100877419 (PR triage) has event: pull_request_target, head_sha: ba03391..., head_repository: askalf/truecopy-action and pull_requests: [{number: 32, head: ba03391...}]. It passes every clause of the condition, and its triage check is listed on this head. This settles the reversal of c63cfad: it is right.
  • Fork PRs: they are still dropped by the head_repository.full_name clause, and again by readFacts() (.mjs:236).
  • Self-trigger loop: fleet-status.yml is not in its own workflow_run list, and the statuses it posts do not raise workflow_run. scorecard.yml never runs on a PR, so leaving it out is correct.

Test assertions

  • test.mjs:355: if (!/\bpull_request(_target)?\b/.test(onBlock(y))) continue; does not match pull_request_review, because \b fails between t and _. So only real PR workflows are required in the list. Removing 'PR triage' from fleet-status.yml:18 fails the per-workflow check for pr-triage.yml.
  • test.mjs:360-361: deleting pull_request_target from the fromJSON array fails this check. It matches the text, not the parsed if:, which is the file's existing convention for workflow tests.

Claims in the body, checked against the diff

  • "Check runs at the head are read across every page": .mjs:255-256. True.
  • "workflow_run re-runs it when any pull_request or pull_request_target workflow finishes": .yml:18 and .yml:30, enforced by test.mjs:353-362. True.
  • "115 unit tests": the CI log says 115 pass, 0 fail. True.
  • "plain ASCII": a byte scan of all three files finds 0 bytes at or above 0x80. True.

Non-blocking note

The tests pin collectPages itself, not the fact that readFacts() calls it for check runs. If .mjs:255 went back to a single fetch, the tests would stay green. This matches the rest of the file, where the I/O block under import.meta.url has no tests and the rules are tested as pure functions. It is not a defect in this head.

What's good

Pulling the pager out into one pure helper, and building ghAll on it, removes the duplicate loop instead of adding a second one. The page-2 test uses the real requiredCiState, so it tests the actual failure (fleet/verify falling back to pending), not just a row count.

SECOND READ: READY

@sprayberry-secondread 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 0248e36. I found one low, non-blocking inaccuracy in the PR body.

What changed since my last read (ba03391)

compare/ba03391...0248e36 has three commits: b2b1b9c (#31, random report delimiter), fdf9ee2 (#33, author-fixes label) and the merge 0248e36. None of them touches .github/workflows/fleet-status.yml, scripts/fleet-status.mjs or scripts/fleet-status.test.mjs, so this PR's own diff is byte-identical to the head I last read. I checked how the merged-in master changes interact with this PR:

  • ci.yml still has name: ci and still runs on pull_request, so the workflow_run list stays complete. The new job scan: a hostile manifest cannot forge the report output has a colon in its name, but nothing in the script parses check names.
  • .github/labels.json gains author-fixes. isCodePath treats it as .github config, and no lane reads that label.

Findings

Low (body only, non-blocking): the required-check count in the body is wrong at this head.
The body says: "Here the base branch requires the seven scan/verify fixtures, analyze (actions)."
GET /repos/askalf/truecopy-action/rules/branches/master lists six scan/verify contexts plus analyze (actions). After the master merge, ci.yml has seven scan/verify jobs, and the new scan: a hostile manifest cannot forge the report output is not required. The script reads the list at run time (scripts/fleet-status.mjs:382-384), so the code is right: fleet/verify will go green without waiting on the new job, and it will stay green if that job fails. Only the sentence is off. If the new job was meant to gate, that is a ruleset change outside this PR.

Suggested fix:

Here the base branch requires the six scan/verify fixtures and `analyze (actions)`.

Boundary pass (rebuilt from the diff)

predicate input behaviour pinned by
fit (fleet-status.mjs:291) 140 / 141 chars kept whole / 137 + ... "exactly 140 characters is kept whole", "141 characters is cut..."
needsVerify files.length > 100 (:191) 99 / 100 / 101 docs files, bot PR with 101 files not code / not code / code / exempt "99 docs files", "exactly 100", "more than 100", "a bot PR with more than 100 files"
requiredCiState own-context filter (:218) only fleet/* required; fleet/* required and pending none / passed "only own lanes required: none", "own lanes required and pending, CI green", three-round loop (this fails without the filter)
requiredCiState empty/unknown state (:224-226) not yet reported, in_progress, failure while another is pending, rerun that passed, skipped/neutral pending / pending / failed / passed / passed the six requiredCiState checks
rules or checks unreadable (:385, :396) HTTP error pending, never green not unit-tested (CLI path); I read the catch arms, and both set a non-green state
verifiedAtHead (:196-203) no label, older sha, 7-char prefix, other login, findings/blocked heading, latest-wins both ways as the rule says the verifiedAtHead block (10 checks)
secondReadAtHead (:248-255) other login, older head, DISMISSED, no line, READY, mostly, last line wins, later lineless review, NOT READY with no reason, separators - : U+2013 U+2014 none / none / skipped / none / none / last / kept / NOT READY at <h> / dropped the Second Read blocks, including the U+2014 line built with String.fromCharCode
Redline deterministic approval (:237) code / docs ignored / counts "on code it is not Redline's verdict", "on docs it is"
collectPages (:286) 100+1, 100+100, 5 101 / 200 (reads one more empty page) / 5 the paging block
backfill cap (fleet-status.yml:121-125) 1000 open PRs ::error::, exit 1 "backfill fails at its cap"

I didn't find a reachable row that behaves wrongly or has no test, apart from the CLI error arms noted above.

Tests that cannot fail

I read each assertion to see whether it could pass without the code it covers. "checks nobody requires do not hold it" would still pass without the own-context filter, because fleet/verify isn't in that required list. Its name says what it covers, and the three-round loop and "own lanes required and pending" tests do pin the filter. None of the other assertions I read would pass without its code.

Claims in the body

  • "115 unit tests": the self-test log at this head shows 115 pass, 0 fail.
  • Runs the default branch's script: yes, ref: default_branch with a sparse checkout, and hashFiles skips the step until the script exists there.
  • Fork PRs skipped: skipped in the job if and again in readFacts (:372).
  • Check runs read across pages: collectPages on check-runs (:391-392).
  • No concurrency group; the script re-reads and corrects for up to three passes (:417-434).
  • pull_request_target accepted: yes (fleet-status.yml:36), and the test checks it.
  • Permissions: checks: read, statuses: write, the rest read-only.

All of these hold, apart from the required-check count above.

What's good

The job runs only default-branch code under a scoped token, and PR is checked against ^\d+$ before it reaches any URL. A lane can't turn green on unreadable data, and fleet/verify can't end up waiting on its own status. CI is green at 0248e36.

SECOND READ: READY

@askalf
askalf merged commit 78a2ffc into master Sep 25, 2026
19 checks passed
@askalf
askalf deleted the ci/fleet-status branch September 25, 2026 06:11
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants