Skip to content

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

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

askalf merged 15 commits into
mainfrom
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 Validate HTML and Scripts, Analyze (actions), Analyze (javascript-typescript), Scorecard (file-based checks). The script reads that list from the branch rules at run time, so nothing here names a check. workflow_run re-runs it when any pull_request 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. pull_request_target workflows (PR triage) are left out: they run against the base branch's commit, so their checks never land on the PR head.

The three fleet/* contexts are never counted as CI themselves, so they can become required checks 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.
  • 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
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Deploying amnesia-site with  Cloudflare Pages  Cloudflare Pages

Latest commit: e2f7721
Status: ✅  Deploy successful!
Preview URL: https://02b16db5.amnesia-site.pages.dev
Branch Preview URL: https://ci-fleet-status.amnesia-site.pages.dev

View logs

@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 — deleting a verification comment can leave its success status green at the same head. rule:none

  • Blocking — .github/workflows/fleet-status.yml:14: issue_comment subscribes only to types: [created, edited]. The lane state is recomputed from the current issue comments (the script fetches /issues/${pr}/comments at scripts/fleet-status.mjs:200), but a deletion of the ## Verification at <head> comment emits an issue_comment.deleted event which this workflow does not handle. For a non-exempt PR on a branch with no required CI checks, start with the verified label and a valid verification comment so fleet/verify is posted successful, then delete that comment. No workflow runs to recompute the lanes, so the previously successful fleet/verify status remains on the head despite the underlying verification no longer satisfying the dispatcher rule; once these contexts are required, that stale success can permit merging without verification.

    issue_comment:
      types: [created, edited, deleted]
    

The required CI checks are green at 743e0f7. I reviewed the workflow event coverage and the status-state logic/tests; I did not run the local test suite per review policy.

…in the bot rule

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

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

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

@sprayberry-secondread 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 blocking defect. The PR body's own next step (make the fleet/* contexts required) makes every code PR's lanes stay pending, which blocks every code merge.

Head note: I was dispatched at 743e0f7. The branch moved twice while I was reading:

  • 93f5850: edited/deleted triggers, the bot-rule comment, isBotPr tests.
  • 6e771d8: concurrency group replaced by postedSince.

I read both deltas, so this review and its verdict are for 6e771d8. The finding below is in all three heads.

Findings

High: the script's own fleet/* contexts count as required CI once they are required

scripts/fleet-status.mjs:225-226

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

scripts/fleet-status.mjs:141

  const verified = code && (ci === 'passed' || (ci === 'none' && verifiedAtHead(facts)));

The PR body's Next step, after this merges says to add fleet/verify, fleet/review and fleet/second-read to the default branch's required status checks. After that, required contains the three contexts this script writes. Here is what happens on a code PR:

  1. On the first run no fleet/* status exists at the head, so last.get('fleet/verify') is '' and requiredCiState returns 'pending'.
  2. ci === 'pending', so verified is false. That makes gated true, and all three lanes are posted as pending.
  3. On the next run, fleet/verify is PENDING at the head (and fleet/review and fleet/second-read too), which matches CHECK_WAITING. So requiredCiState returns 'pending' again. Nothing can ever break the loop.

Because the rules list is non-empty, ci is never 'none', so the verified label and comment fallback can't break the loop either. I reproduced this at 6e771d8 by feeding laneStatuses its own output back through requiredCiState for three rounds. The input was a code PR with all four real CI checks green, the verified label with a ## Verification at <head> comment, Redline APPROVED at the head, and a Second Read READY at the head:

required = 4 contexts (today)
  round 1..3 requiredCi=passed  fleet/verify=success fleet/review=success fleet/second-read=success
required = 7 contexts (after the documented next step)
  round 1..3 requiredCi=pending fleet/verify=pending fleet/review=pending fleet/second-read=pending

Once they are required, GitHub refuses every code merge because fleet/verify is never green. That includes merges by hand and by auto-merge. The backfill job would then post pending on every open PR. The existing test checks nobody requires do not hold it (scripts/fleet-status.test.mjs:227) covers a fleet/verify status that is present but not required. No test covers the case where it is required, which is exactly the configuration the body says comes next.

Suggested fix (exclude the lanes this script owns from the CI it waits on, plus a test that pins it):

const OWN = new Set(Object.values(CONTEXTS));
// in requiredCiState, before the loop:
required = required.filter((r) => !OWN.has(r));
if (!required.length) return 'none';
check('our own lanes being required do not hold verification',
  requiredCiState(['test', CONTEXTS.verify, CONTEXTS.review, CONTEXTS.secondRead], [ok('test')]) === 'passed');

Filtering inside requiredCiState rather than at the CLI call site means the test drives the same code the workflow runs.

Info: the test count in the body is stale

The body says 73 pass, 0 fail and "the script's 73 unit tests". At 6e771d8, node scripts/fleet-status.test.mjs prints 83 pass, 0 fail, which matches the latest commit message.

What I checked

  • Boundaries I rebuilt from the diff:
    • requiredCiState with empty required → 'none', pinned.
    • A required check not yet reported → pending, pinned.
    • A required check failed while another is pending → failed, pinned.
    • A rerun where the last result wins → pinned.
    • Skipped and neutral count as passed → pinned.
    • fit at 140 and 141 characters → pinned.
    • 99 vs 100 files → pinned.
    • A bot branch opened by a person → pinned.
    • A verdict at an older head vs this head, for both Redline and the Second Read → pinned.
    • A NOT READY with no reason and no trailing colon → pinned.
    • A deterministic approval on code vs docs → pinned.
    • postedSince → pinned for all five of its cases: after the read, before it, in the same second, another context, and no Date header.
  • The unguarded row is the one above: the fleet/* contexts in required.
  • postedSince ordering: a stale overwrite would need the older run to read before an event and the newer run to post within the same Date second as that read. Runner startup takes seconds, so the newer run's created_at is always strictly later, and the same-second overwrite only happens between runs that read the same state. It holds.
  • New assertions: each one I read would fail against a plausible wrong implementation. I found none that holds either way.
  • Workflow:
    • The status job checks out the default branch's script only (sparse, persist-credentials: false). It is skipped by hashFiles until merge, and the job token is least-privilege.
    • Fork PRs are excluded both in the job if and again in the script.
    • workflow_run is limited to CI/CodeQL. Both produce the four required contexts; I confirmed this from the run names.
  • CI: I ran self-test locally at 6e771d8 and got 83/0. At 93f5850, all required checks passed; kick failed but is not required.
  • What's good: the rules are pure functions with the I/O confined to the CLI block, a verdict counts only at the head everywhere, and when the script can't read something it falls back to pending/label rather than to green.

SECOND READ: NOT READY — requiring the fleet/* contexts (the body's next step) makes requiredCiState wait on the script's own statuses, so every code PR stays pending forever

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

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

@sprayberry-redline 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 — concurrent same-second runs can still let stale lane state overwrite fresher state. rule:none

  • Blocking correctness — scripts/fleet-status.mjs:256: if (postedSince(posted, s.context, readAt)) { console.log(' (a newer run already posted this; skipped)'); continue; } delegates the ordering decision to a strict timestamp comparison. readAt comes from GitHub's HTTP Date header and status created_at is also second-granularity; postedSince() only returns true when Date.parse(s.created_at) > readAtMs. The added test codifies the unsafe equality case 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));. If run A reads at 03:00:10.100, run B reads newer review data later in that same second and posts its lane status, then A reaches this check, both timestamps parse to 03:00:10 and A overwrites B's fresh status with its stale decision. That is precisely the concurrent ordering race this change is meant to prevent, and it can leave a green or red required lane inconsistent with the newest review state.

    // Treat an equal, server-second timestamp as newer too: this run cannot
    // establish that it read after the already-posted status.
    return statuses.some((s) => s.context === context && Date.parse(s.created_at) >= readAtMs);
    

Required CI is green at 0e1fca7. I re-read the workflow event handling, required-check exclusion, status ordering path, and the accompanying unit tests. I did not run the local suite per review policy.

…0 files

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

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

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

@sprayberry-secondread 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 adb8b0b. The lane rules match what the PR body says, the new tests fail when the code they cover is broken, and every claim in the body checks out against the diff. There is one low-severity note on the post-then-verify loop.

Scope: the dispatch ticket named 0e1fca7, but the head has since moved to adb8b0b9 (two commits: the post-then-verify loop, dismissed Second Read reviews, the >100-file rule, and test renames). I reviewed the live head. I read the full diff (3 files, +727), the base branch's required-check rules, ci.yml's triggers, and the check runs at the head. I did not run the tests; CI's self-test passed at this head.

Findings

Low: scripts/fleet-status.mjs:401-418: on pass 3 the loop posts statuses and never re-reads them

  for (let pass = 1; pass <= 3; pass++) {
    const read = await readFacts();
    ...
    const todo = statusesToPost(want, have);
    if (!todo.length) break;
    ...
    for (const s of todo) {
      await gh(`/repos/${repo}/statuses/${read.facts.head}`, token, {

The comment at 397-399 says "the run that acts last re-reads after its own writes". That holds for passes 1 and 2, but not for pass 3. Pass 3 reads the facts, posts what differs, and the loop exits without checking again.

Failure scenario: two runs overlap on a busy PR, and run A needs a correction on every pass. After A's pass-3 read, Redline posts CHANGES_REQUESTED at the same head. Run B reads that and posts fleet/review failure. Then A's pass-3 POST lands with its older data (success) and A exits. The head now shows green over a red verdict. Nothing corrects it until the next PR event, and there may be no next event.

This needs three consecutive diffs in one run, so it is unlikely. The body's "(up to three passes)" and the comment's "the next event covers anything after that" already describe the limit. The statuses are also informational in this PR. So this is not blocking, but it matters once fleet/* become required checks, because a stale success could then let a merge through.

Suggested fix: after the last pass, only verify and log. Never end the loop on a write.

  for (let pass = 1; pass <= 4; pass++) {
    const read = await readFacts();
    if (!read) break;
    const want = laneStatuses(read.facts);
    if (pass === 1) for (const s of want) console.log(`${s.context.padEnd(18)} ${s.state.padEnd(8)} ${s.description}`);
    if (dryRun) break;
    const have = latestByContext(await ghAll(`/repos/${repo}/commits/${read.facts.head}/statuses`, token));
    const todo = statusesToPost(want, have);
    if (!todo.length) break;
    if (pass === 4) { console.log(`still differs after 3 passes: ${todo.map((s) => s.context).join(', ')}`); process.exitCode = 1; break; }
    ...

A failed status job at least makes the unsettled state visible on the PR.

Boundaries I rebuilt from the diff

Predicate Input Behaviour Pinned by
needsVerify files.length > 100 99 / 100 / 101 docs files not code / not code / code "99 docs files are not code", "exactly 100 docs files is not code", "more than 100 files is code whatever they are"
same, required CI passed 101 docs files verify green "more than 100 docs files still verifies once required CI passes"
isBotPr person on bot/, askalf on feat/, dependabot on any branch not bot / not bot / bot the isBotPr checks
verifiedAtHead !facts.head / no label / older sha / latest comment / other login / findings/blocked heading each false the verifiedAtHead block
requiredCiState empty required / only fleet/* required [], own none "no required checks -> none", "only own lanes required: none"
same, unreported / running / failed / rerun passed / skipped+neutral each pending / pending / failed / passed / passed the "required CI is the verification" block
fleet/* pending while required own lanes PENDING, CI green passed (does not wait on itself) "own lanes required and pending, CI green: still passed" and the three-round loop
redlineVerdict deterministic approval code / docs ignored / counts "on code it is not Redline's verdict" / "on docs it is"
same, DISMISSED or COMMENTED at head after an older APPROVED pending, names the old sha "a dismissal and a comment at head leave..."
secondReadAtHead commitId != head / dismissed / no line / READY, mostly / last line in the body / later review with no line each none / none / none / none / last wins / keeps the verdict the "Second Read, review by review" block and the dismissed block
NOT READY with empty reason SECOND READ: NOT READY NOT READY at 4753643, no trailing colon "NOT READY with no reason is red without a trailing colon"
fit at 140 / 141 chars kept whole / cut to 137 + ... "the 140-character edge"
statusesToPost identical / new state / missing / same state, new description skip / post / post / post the post-then-verify block

I found no reachable row with wrong behaviour and no reachable row left untested, apart from the pass-3 loop exit above, which is CLI I/O with no unit seam.

Assertions that could not fail: I checked each of the 94 check() calls against the branch it targets. The ones most likely to be vacuous do in fact fail if the code they cover is broken. "review pending even with an approval at head" fails without the gated branch. "the latest comment wins" fails with an any-comment rule. "own lanes required and pending, CI green: still passed" fails without the OWN_CONTEXTS filter. "a later READY still counts after a dismissed NOT READY" fails if dismissed reviews are not skipped.

PR body claims, checked

  • Required checks: rules/branches/main returns exactly Validate HTML and Scripts, Analyze (actions), Analyze (javascript-typescript) and Scorecard (file-based checks). The first and last come from ci.yml and the Analyze pair from CodeQL, so workflow_run: [CI, CodeQL] covers every producer.
  • "runs the default branch's copy of scripts/fleet-status.mjs": the sparse checkout pins ref: default_branch, and hashFiles skips the step until merge. The status check passed in 7s at this head, which fits a skipped step. On pull_request* events the workflow YAML itself is still the PR's copy. That is expected and harmless, since only same-repo authors reach this job.
  • Permissions, fork skip in both the if and the script, no concurrency group, self-test read-only, manual backfill: all as the body states.
  • "94 unit tests": there are 94 check( calls.
  • "plain ASCII throughout": the diff contains 0 bytes >= 0x80.
  • "same files as askalf/dario#1419": scripts/fleet-status.mjs has the same blob (8b338ee). The test file differs only in location (test/fleet-status.mjs there) and its import path.

What's good

The script is pure functions over fetched facts, and the CLI is a thin shell around them, so nearly every rule can be unit-tested. Unreadable rules and unreadable checks both fail closed: neither can turn fleet/verify green. Filtering the fleet's own contexts out of the required list prevents the self-wait deadlock ahead of the planned "make them required" step.

Operator note for that step, outside this diff: any workflow run with statuses: write can post a fleet/* context, including a modified workflow on a same-repo PR. Requiring these contexts gates merges by convention among trusted writers, not against them.

SECOND READ: READY

…losed rules

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

99/99.

@sprayberry-redline 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 stale workflow run can still overwrite newer lane statuses after the newer run finishes. rule:none

Blocking — correctness: stale worker can leave an obsolete status — scripts/fleet-status.mjs:267-287

// Post, then read everything again and correct what differs.

const have = latestByContext(await ghAll(/repos/${repo}/commits/${read.facts.head}/statuses, token));

for (const s of todo) { await gh(/repos/${repo}/statuses/${read.facts.head}, token, { ... }); }

The post-then-read loop does not establish that the worker which writes last has the freshest facts. For example, run A reads an older review state (pending), run B reads a later approval and posts fleet/review=success, then B completes its second pass. A can then resume, read statuses, see B's success differs from its old desired pending, and post pending. On A's next pass it re-reads the same still-old review state and sees its own pending status as correct, so it exits; no run remains to correct the stale final write. This is precisely the concurrent ordering the new loop is intended to prevent, but it still leaves the final status wrong until another event happens. The new unit case tests correction by a hypothetical later fresh pass, not this interleaving of two live workers.

// Serialize status publication per PR (or retain a server-side freshness
// comparison that prevents a run from publishing after a newer run has
// observed the PR). Then test an interleaving where an older reader writes
// after the newer worker has completed; the final context must remain the
// newer worker's value.

What's good: required CI is green at adb8b0b, and I checked the rework's dismissal handling, required-context exclusion, file-count boundary, and status comparison tests. The dismissal and exact-100-file cases are now explicitly covered.

… that it does

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

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

@sprayberry-redline 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 — removing the dispatcher kick silently restores timer-only review dispatch. rule:none

Blocking — .github/workflows/review-kick.yml:1-59

-name: Review kick

-on:

- pull_request_target:

- types: [opened, synchronize, reopened, ready_for_review]

- - name: Queue review-dispatch for this head

This change deletes the workflow that calls review-dispatch.service when a same-repository PR is opened or receives a new head. The new fleet-status workflow only publishes statuses; it does not invoke the fleet dispatcher. Consequently, after this merges, a new commit can remain unreviewed until the periodic dispatcher timer runs rather than being queued immediately. That contradicts the deleted workflow's stated purpose and makes the visible lane status stay pending despite all CI being complete. Keep the independent dispatch trigger (or replace it with an equivalent immediate, trusted trigger) rather than deleting it as part of the status-reporting feature.

# Retain .github/workflows/review-kick.yml unchanged, or provide an equivalent
# trusted event handler that queues review-dispatch for opened/synchronized PRs.

Required CI was green at bc827002. I reviewed the live PR diff and, in particular, the new workflow-run coverage and the change since the prior verdict; I did not run the local test suite.

… own separator

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

@sprayberry-redline 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 CI can remain pending indefinitely on PRs with more than 100 check runs. rule:none

Blocking — .github workflow status computation (scripts/fleet-status.mjs:382)

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

The new lane is intended to reflect every required check, but this endpoint is read only once and therefore retains at most 100 check runs. A head with (for example) a large matrix or reruns that produces 101 runs, where a required context is in the next page, never adds that context to checks. requiredCiState() then sees an empty state for the missing required context (lines 222–224) and returns pending, even after that required job has passed. Consequently fleet/verify, and then the review lanes, can block the PR permanently.

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

Please also add a regression case with a required check beyond the first 100 returned runs.

What's good: required CI is green at e5c423d; I reviewed the workflow triggers, status/verification state machine, pagination for PR metadata, and the new unit-test coverage. The status loop’s fresh-read correction and exclusion of the lane contexts from CI evaluation are sound.

The check-runs list was read once with per_page=100, so on a head with more than
100 runs a required check on a later page never counted, and fleet/verify could
wait on it indefinitely. Paging is now one pure helper, collectPages(), used by
the list endpoints and by check runs alike. Four tests: 101 rows over two pages,
a full last page, a short first page, and a required check found on page 2.
@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: APPROVED — no blocking issues found in the live-head re-review.

I reviewed the live-head delta from e5c423da0f769fa731454e4ddc96845bab5cc7ff to 6ff4cf15d615c792ba81b91f1454e52f1c431dd7, concentrating on the new paginated check-run fetch. collectPages preserves all rows through the first short page, and the check-run path now supplies every page to requiredCiState; the added tests cover a required check appearing on page two and pagination boundaries. Required CI is green at this head.

What's good: extracting the paging loop into one helper prevents the statuses and check-runs readers from drifting, while retaining the existing fail-closed handling when the API cannot be read.

@sprayberry-secondread 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 6ff4cf1. The lane rules match the PR body, every reachable boundary I rebuilt has a test that can fail, and the self-test job reports 115 pass, 0 fail at this head.

Scope note: the ticket named bc82700, but the live head is 6ff4cf1 (two commits later: backfill paging, the separator pin, and check-run paging). I reviewed 6ff4cf1, reading the full diff (3 files, +819). I also checked the base branch's ruleset and the on: block of every workflow at this head against the workflow_run list.

Boundaries rebuilt from the diff

predicate input behaviour pinned by
needsVerify files.length > 100 (scripts/fleet-status.mjs:190) 99 / 100 / 101 docs files not code / not code / code "99 docs files", "exactly 100 docs files", "more than 100 files"
same, bot branch 101 files on bot/ exempt (bot rule first) "a bot PR with more than 100 files is still not code"
isBotPr (:181-184) a person on bot/, release/1.2; dependabot on any branch not bot, not bot, bot the three isBotPr blocks
verifiedAtHead (:194-203) label only; comment only; older comment then head; head then older; other login; findings/blocked headings; 7-char sha only label + latest verifier comment at head counts the verifiedAtHead sections
requiredCiState (:216-228) []; only fleet/* required; missing; in_progress; failure plus pending; rerun that passed; skipped/neutral none / none / pending / pending / failed / passed / passed the "required CI" block
own-context exclusion fleet/* required and pending not counted, no self-wait over 3 rounds "own lanes required, three rounds"
redlineVerdict + head match (:231-240, :320) deterministic approval on code vs docs; dismissal/comment after an older approval; CR after APPROVED at same head; CR at unverified head pending / success / pending with old sha / failure / pending "deterministic approvals", "Redline reviews that are not verdicts"
secondReadAtHead regex (:249) no line; READY, mostly; two lines in one body; later lineless review; older-head verdict; Redline carrying the line; bare NOT READY none / none / last wins / keeps verdict / none / none / red with no colon "the Second Read, review by review"
separator strip (:253) -, :, U+2013, U+2014, none; reason starting with a backtick or ( / [ separator dropped, first char kept the separator loop and "a reason that starts with a code span"
fit (:290) 140 / 141 chars kept / 137 + ... "the 140-character edge"
collectPages (:280-287) 5; 100+1; 100+100 1 page / 2 / 3 (last one empty) the paging block
statusesToPost / latestByContext (:260-274) identical; different state; missing context; same state with a new description; stale overwrite skip / post / post / post / corrected next pass the post-then-verify block

Assertions that could hold whether or not the change is in place: none found. "the latest comment wins" fails if the first comment won; "CI pending: an old Breaker label and comment do not count" fails if ci === 'none' were dropped from :303; the workflow_run coverage test fails if the workflows: list fails to parse (listed becomes []).

PR body claims checked against the diff and the repo

  • Required checks are read from branch rules at run time (:381-383). The live ruleset lists exactly the four the body names.
  • workflow_run covers every pull_request workflow. At this head those are CI, CodeQL, Image and labels, which matches .github/workflows/fleet-status.yml:24. PR triage is pull_request_target only and is correctly left out.
  • The status and backfill jobs check out the default branch's script only (fleet-status.yml:49-54, :100-105). self-test gets contents: read only.
  • Fork PRs are skipped both in the job if (:33-39) and in readFacts (:371).
  • Unreadable rules or check runs fall back to pending (:384, :395) and never to green.

Non-blocking note for the "next step" (not this diff)

When fleet/* become required, remember that on pull_request and pull_request_review events GitHub runs the workflow file from the PR's merge ref. A same-repo PR that edits fleet-status.yml can therefore post its own fleet/* statuses with statuses: write. Pinning the contexts to the GitHub Actions integration in the ruleset does not close this, because the edited workflow also runs as Actions. The existing required checks already have this property, and it needs branch write access, so I am not counting it against this PR. It is worth having the dispatcher, or Redline, flag any PR that touches fleet-status.yml.

What's good

The pure/CLI split keeps all the rule logic unit-testable. The post, re-read and correct loop is a reasonable substitute for a concurrency group that would roll up as a failed check. The ASCII-only source with run-time separators is a neat way to cover the real verdict line.

SECOND READ: READY

@sprayberry-secondread 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 bed18a0. The only change since my READY at 6ff4cf1 is a merge of main, and the three files this PR adds are byte-identical to that head.

What changed since the last read

  • bed18a0 is Merge branch 'main' into ci/fleet-status on top of 6ff4cf1. git diff --quiet 6ff4cf1 bed18a0 -- .github/workflows/fleet-status.yml scripts/fleet-status.mjs scripts/fleet-status.test.mjs exits 0. origin/main (a5db6aa) is an ancestor of the head, so the PR diff is still exactly those 3 files (+819/-0).
  • Everything the merge brought in comes from main (#80's worker cookie renewal, sponsors sync, docs). None of it is on this PR's review surface.

Re-checked against the merged-in main

  • workflow_run list stays complete. The merge adds .github/workflows/sponsors-readme.yml, which triggers on schedule and workflow_dispatch only (sponsors-readme.yml:12-16). It posts no checks on a PR head, so workflows: [CI, CodeQL, Image, labels] (fleet-status.yml:18) still names every pull_request workflow, and PR triage remains the only pull_request_target one. The coverage test ("workflow_run lists ...", fleet-status.test.mjs:335-360) re-reads the directory, so it would have caught a gap. It passes: node scripts/fleet-status.test.mjs prints 115 pass, 0 fail, matching the body.
  • The sponsors bot PR classifies correctly. That workflow opens its PR from bot/sponsors-readme (sponsors-readme.yml:66,88). If that PR is opened as askalf, isBotPr returns true (fleet-status.mjs:46-48, BOT_BRANCH ^bot\/), so all three lanes read "not required / not gating" and a README-only bot PR does not wait on the gate. If it is opened under some other identity, it still touches only README.md, and isCodePath returns false for .md (:33). The result is the same.
  • Core rules re-read at this head, unchanged from my ledger at 6ff4cf1: needsVerify > 100 fail-closed (:55), verifiedAtHead latest-comment-wins with the prefix match (:59-67), requiredCiState own-context exclusion and last-result-per-name (:81-92), redlineVerdict skipping deterministic approvals only on code (:96-104), secondReadAtHead head/login/dismissed filter, last line wins, and one-separator strip (:108-121), fit at 140 (:155), and the post, re-read and correct loop bounded at 3 passes (:281-298). Each row still has the test I cited last time, and I found no assertion that holds whether or not the change is in place.

CI at this head

gh pr checks 83: Validate HTML and Scripts, Analyze (actions), Analyze (javascript-typescript) and Scorecard (file-based checks) (the four required checks) all pass, as do self-test and status. backfill is skipped, as expected on pull_request.

Not re-read

I did not re-read the merged-in main files beyond the workflow trigger lists. They are not part of this PR's diff.

What's good

Merging main was the right way to pick up the new workflow. The directory-driven coverage test is exactly what makes a merge like this safe to re-gate without re-reading everything.

SECOND READ: READY

@sprayberry-secondread 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 e2f7721. The lane rules match the body, every reachable boundary I rebuilt is pinned by a test that discriminates, and the privileged job runs only default-branch code.

Since my last read at bed18a0, the only change is the merge of main (1d4f6e87, .github/labels.json, which is not in this PR's diff). I re-read the full diff (3 files, +819) at this head anyway.

Boundaries rebuilt from the diff

Predicate Input Behaviour at head Pinned by
facts.files.length > 100 (scripts/fleet-status.mjs:190) 99 / 100 / 101 docs files not code / not code / code "99 docs files", "exactly 100 docs files", "more than 100 files"
same, bot PR 101 files on bot/ exempt (bot rule read first) "a bot PR with more than 100 files is still not code"
isCodePath .github/ carve-out workflows/*.yml, actions/**, scripts/**, .MJS under workflows not code, code, code, code ".github config is not", ".github actions are", ".github scripts are", "keeps its case"
isBotPr identity AND branch person on bot/, release/; askalf on each prefix; dependabot anywhere matches the dispatcher rule the two isBotPr blocks
requiredCiState own-context filter (:217) only fleet/* required; fleet/* required and pending with CI green none; passed "only own lanes required: none", "own lanes required and pending", plus the three-round fixed-point test. All of them fail without line 217
requiredCiState empty required [] none "no required checks -> none"
missing / running / failed / rerun / skipped / neutral as named pending / pending / failed / passed / passed the six requiredCiState checks
rules or checks unreadable (:384, :395) throw pending, never green by construction (CLI path, not unit-tested; this is the fail-closed direction)
verifiedAtHead label only, comment only, older sha, later older sha, other login, findings/blocked headings, 7-char prefix each correct the verifiedAtHead blocks
redlineVerdict deterministic approval on code vs docs; DISMISSED/COMMENTED at head after an older approval; CR after APPROVED at same head not a verdict / verdict; pending naming the older sha; red "deterministic approvals", "Redline reviews that are not verdicts"
secondReadAtHead no line; READY, mostly; two lines in one body; later lineless review; older-head verdict; DISMISSED; Redline carrying the line none; none; last wins; kept; none; skipped; none "the Second Read, review by review", the dismissed block
reason separator -, :, U+2013, U+2014, none, leading backtick/bracket dropped once, first char kept the separator loop (built with fromCharCode), "keeps it", "[scope] missing"
fit 140 / 141 chars kept / 137 + ... "the 140-character edge"
collectPages short first page, 100+1, 100+100 stops at first short page the pager block
statusesToPost identical, state differs, missing, same state new description, stale overwrite skip, post, post, post, corrected then left alone the post-then-verify block

I also read the new tests for vacuity. The own-context, dismissed-review and separator checks all fail if you remove the line they cover. "checks nobody requires do not hold it" is a plain non-required-check case and makes no claim to cover the filter.

Body claims checked against the diff and the repo

  • Required checks: rules/branches/main returns exactly Validate HTML and Scripts, Analyze (actions), Analyze (javascript-typescript), Scorecard (file-based checks). The script reads that list at run time (scripts/fleet-status.mjs:381-383). Nothing is hard-coded.
  • The workflow_run list [CI, CodeQL, Image, labels] (.github/workflows/fleet-status.yml:24) matches every workflow at this head that has a pull_request trigger. PR triage is pull_request_target-only and is left out. Canary, ClusterFuzzLite, Scorecard, Deploy and Sponsors never run on pull_request. The guard test at scripts/fleet-status.test.mjs (the onBlock loop) would flag a new one.
  • Trust boundary: status and backfill sparse-check-out the default branch's script only (ref: ${{ github.event.repository.default_branch }}, persist-credentials: false). self-test runs the PR's code with contents: read and nothing else. Fork PRs are dropped both in the job if and in readFacts (:371). workflow_dispatch fails the status job's third clause because github.event.pull_request is null, so only backfill runs on dispatch.
  • "115 unit tests": the self-test log at this head ends 115 pass, 0 fail. All required CI is green at e2f7721.
  • Backfill: --limit 1000 plus -ge 1000 then ::error::. That is conservative at exactly 1000 open PRs, which is the direction the body promises.

What's good

The self-reference trap (requiring fleet/* makes fleet/verify wait on itself) is closed at the right place. The three-round fixed-point test shows it converges to all-green instead of only asserting one call. The post-then-reread loop is a sound replacement for a concurrency group that would roll up as a failed check.

SECOND READ: READY

@askalf
askalf merged commit 41e52e0 into main Sep 25, 2026
16 checks passed
@askalf
askalf deleted the ci/fleet-status branch September 25, 2026 05:14
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