Skip to content

fix(pr-pipeline): sweep lanes App heads whose review dispatch skipped and never retried - #12648

Merged
kyle-sexton merged 3 commits into
mainfrom
fix/unblock-advisory-threads
Nov 3, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
fix/unblock-advisory-threads

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

No related issue: dispatcher stall found by the PR Lane sweep (hub ledger L80)

Summary

On 2026-11-03, three lanes App PRs each sat BLOCKED on one unresolved advisory claude-review thread with no pr-unblock address-feedback round queued, until a person dispatched it: #12578 (thread 01:19Z, manual round 02:27Z) and #12604 and #12605 (threads 02:19Z and 02:21Z, manual rounds 03:21Z and 03:22Z). That is about 1 h of extra age each.

Advisory threads were not filtered out. On an ungated lanes App head, any unresolved thread dispatches (feedback-dispatch.mjs:583-601). The reviewer-id list at :309 only feeds the @codex request check. The cause was the FAIL-cap read plus a missing retry:

Fix

A new sweep pass, sweepUnsent, runs after sweepStale. It sends one lanes App head per tick that has an unresolved review thread and no pr-unblock / address-feedback check run, through its newest completed review run, with all of dispatchFeedback's gates (markers, hold labels, human gate, FAIL cap). It waits until that review run is at least 10 minutes old, the dispatch job's timeout-minutes, so it does not race the review completion's own dispatch, which runs in a different concurrency group. A pr-unblock.yml run for the PR created since that review started holds the head, so a round that ended before posting its check is sent only once.

Each lanes App PR with no round costs one check-run read per tick, plus one thread read when it has none. A head whose skip stands costs the reads dispatchFeedback makes before it skips.

Verification

  • Baseline: 3 PRs on 2026-11-03, about 1 h of extra age each. Root cause: a non-final verify:unreadable skip (feedback-dispatch.mjs:684-700, verify-cap.mjs:152) with no sweep pass for a lanes App head that has no round.
  • The cap read does succeed some of the time. The dispatcher (actor github-actions[bot]) sent rounds for lanes App PRs feat(testing): commit an attended cleanup batch once shown, without a batch-level wait #12609 at 02:37Z (run 41567272072) and feat(session-flow): workflow probes git for missing files before asking #12630 at 03:15Z (run 41571055692), and on a lanes App PR every one of those dispatches runs the cap read. The fix(toolchain): run Biome as npx biomejs biome, not the unrelated biome package #12578 round (run 41566239073) was started by hand. A retry each tick therefore clears the stall on the first read that fits. The sweep runs in the same Dispatch address-feedback step as the review dispatches, so it uses the same verify reader (VERIFY_EVIDENCE_SCRIPT, pr-feedback-dispatch.yml:442), and a head at the final cap still gets no round.
  • node --test .github/scripts/pr-unblock/feedback-dispatch.test.mjs: 208 pass, 0 fail. The two new tests (a head the cap read skipped is sent by the sweep, and the pass leaves heads with a round, no unresolved thread, a review inside the grace, a run since the review, or do-not-merge) fail against the old feedback-dispatch.mjs.
  • The old test "the sweep does not revisit a head the human gate skipped once the label is off" is replaced: that head is now revisited, through the gates as before.

The listing cost itself is addressed elsewhere: FAIL index mode (#12558, merged 03:28:44Z) makes verify-cap's reader read targeted names instead of the whole listing (babysit_verify_evidence.py:4436-4438). This pass stays as the retry for any non-final skip, whatever its cause. Retention stays at 90 days (#8295's scope, deferred by the owner).

Re-measure: after this merges, count lanes App PRs that sit BLOCKED on an unresolved thread for more than 40 min with no pr-unblock / address-feedback check run (one tick plus the 10 min grace). Target: 0 over a 24 h window, against the baseline of 3 PRs at about 1 h each.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01Vcupom3BKF6QuC5U3sRJPj

… and never retried

A review completion's address-feedback dispatch that skips on a cause that
clears later (verify-cap.mjs's unreadable FAIL cap when the artifact listing
does not fit in the caller's limit) records nothing, and no review run follows
on that head. The half-hourly sweep only sent released heads, fix requests,
stale stops and idle owner PRs, so one unresolved advisory thread held
#12578, #12604 and #12605 until a person dispatched the round.

sweepUnsent sends a lanes App head with an unresolved thread and no
address-feedback check run, once its newest review run is past the dispatch
job's 10-minute timeout and no round was created since that review started.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Vcupom3BKF6QuC5U3sRJPj
@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 4ef49a7008490f49e5618388c3d01dedad493ce6: read 4 of 4 changed file(s); 0 finding(s), 0 as inline comments.

Security review: PR #12648

I found no security issues. I read every hunk of all 4 changed files. Two of them match the trusted paths: .github/scripts/pr-unblock/feedback-dispatch.mjs and .github/workflows/pr-feedback-dispatch.yml. I read both changes in full, plus the dispatch code they lead to.

What changed. The tick's sweep gets a new pass, sweepUnsent, that runs after sweepStale (feedback-dispatch.mjs#L773-L777). It looks for a lanes App head that has an unresolved review thread but no pr-unblock / address-feedback check run. When such a head's newest completed review run is at least 10 minutes old and no pr-unblock.yml run for the PR was created since that review started, the pass sends the head through dispatchFeedback (#L932-L958). The workflow change is a comment, and the changelog entry and tests carry no security surface.

Trust boundaries checked

  • Who it can reach: sweepUnsent only runs over pulls. That list holds PRs where pull.user?.id === appSenderId (#L757), and openPulls already narrowed it to non-draft PRs whose head is in this repository, with a valid SHA (#L826-L832). Owner PRs and fork heads never reach this pass.

  • Gates still in force: The send goes through dispatchFeedback, which keeps all of its existing gates:

    • hold labels (#L546-L549);
    • review-lane markers on the head (#L553-L557);
    • the human-gate gateSource / threadFixers check (#L564-L601);
    • the active-run hold;
    • lane slot enablement (#L630-L637);
    • the existing check-run handling;
    • the pr-verify FAIL-cap read (#L687-L704).

    The call matches the one in the existing dispatchAtHead (#L1017-L1026). The new pass only adds filters before it and removes none.

  • Untrusted text reaching a credentialed lane: No new path. The pass sends the same review threads to the same address-feedback activity that the review-completion dispatch already sends them to. Only the timing of the retry is new.

  • Replaced test: The old test "the sweep does not revisit a head the human gate skipped once the label is off" is gone. Such a head can now be sent again, but only after the human-gated label is off. Removing that label is a person's step (or clear-escalations.mjs's), and the human-gate branch still runs whenever the label is present. No merge hold is weakened.

  • Trust controls: The diff does not touch the trusted-trigger check, the lane token scope, the kill switch, the broker's grants or the merge-hold scripts. Nothing is widened.

Spend: Extra pr-unblock address-feedback rounds and the GitHub API reads each tick makes. These draw on prepaid model credit and the API rate budget, not billing.

The instruction-surface and plugin-manifest checks don't apply: the diff touches no standing instruction file and no manifest. No text in the PR context tries to direct this review.

critical-findings: 0

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

claude-review: 1 inline finding(s) on 4ef49a7008490f49e5618388c3d01dedad493ce6; the summary is in this lane's pull request comment.

claude-lane: claude-review

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

claude-review reviewed 4ef49a7008490f49e5618388c3d01dedad493ce6: read 4 of 4 changed file(s); 1 finding(s), 1 as inline comments.

Code review: sweep lanes App heads whose review dispatch skipped (sweepUnsent)

The change fits the stated root cause. The PR body says a non-final verify:unreadable skip leaves the head unrecorded, and nothing reads it again. The new pass runs after sweepStale and goes through dispatchFeedback, so every existing gate still applies: review markers, hold labels, the human gate, the final FAIL cap, and the active-run hold. A pull request whose skip stands does not stop the sweep from reaching the next one, because sweepPulls moves on when a decision returns null. The cost of repeated FAIL-cap reads inside the 10-minute job is bounded: after one deadline miss, verifyReader sets NO_INCREMENTAL_WAITS, so later reads in the same run refuse early.

Finding (advisory, inline): the 10-minute grace is measured from the wrong event. It counts from when the newest-created review run last updated, not from when the head's last dispatch-triggering run completed. A dispatcher in the head's concurrency group can therefore still be deciding while this pass, which runs in the separate sweep group, dispatches the same head. Two cases where that happens: the other review lane finishing later, and a pr-merge completion after a label comes off.

Checked, no finding:

  • The changelog fragment's bump: patch fits a fix to an internal workflow script. No plugin manifest is touched, so there are no manifest lines.
  • The replaced test ("the sweep does not revisit a head the human gate skipped once the label is off") is a deliberate behaviour change, and the PR body says so. The new tests cover:
    • a round at the head
    • no unresolved thread
    • no completed review
    • a review inside the grace
    • a review with no readable completion time
    • a run since the review, and a run from before it
    • do-not-merge
  • run_started_at ?? created_at handles a missing time safely: a NaN reviewStarted makes any listed run count as sent.

Not verified by me: the PR body's test results (node --test … 208 pass) and its run-log evidence are the author's claims. I had no shell to run anything.

Spend (not a finding): each tick adds one check-run read per open lanes App pull request with no round, plus a thread read. A head whose skip stands also gets dispatchFeedback's reads before the skip, including a FAIL-cap verifier read, every tick. All of this is prepaid API usage, as are any extra pr-unblock rounds it sends.

The PR context file has no embedded directives aimed at this lane. There are no prior findings, and the round cap was not reached (0 of 3).

… trigger at the head

reviewRunAt returns the newest-created review run, so another review lane
finishing later, a re-run, or a pr-merge completion could still start a
head-group dispatcher after the grace had passed, and the sweep could send
the same head alongside it. The grace now runs from the newest completion of
any review run (pull_request) or pr-merge run (pull_request_target) at the
head.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Vcupom3BKF6QuC5U3sRJPj

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

claude-review: 1 inline finding(s) on d1989f86dae028d5759c1a041657e2147e79505c; the summary is in this lane's pull request comment.

claude-lane: claude-review

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed 12d2a2a6093e0b1ac2d16b6b959b84401933d883: read 2 of 2 changed file(s); 0 finding(s), 0 as inline comments.

Code review (incremental, d1989f8..12d2a2a): sweepUnsent grace now covers the relay and pr-verify dispatchers

No new findings. I read both files on the incremental list in full: feedback-dispatch.mjs and feedback-dispatch.test.mjs.

This push fixes this lane's earlier advisory, discussion_r4450887180: the grace missed two dispatchers.

  • Comment relay. [RELAY_WORKFLOW, RELAY_EVENT] is now in the trigger list (feedback-dispatch.mjs#L953). Both constants already exist at L367-368 with the values the relay checks elsewhere use. A pull_request_review_comment run carries the PR head's SHA, so the head_sha === head filter matches it.
  • pr-verify FAIL. Its dispatcher runs at main's head, so its run can't be matched to the PR head. The fix uses the newest createdAt across the unresolved threads' comments instead (L956-957). The verify notes are posted as review comments just before that dispatch, so a recent note holds the head for the grace window. THREADS_QUERY already fetches createdAt (L1213). Timestamps that can't be read are dropped with Number.isFinite. A run with a missing updated_at still makes last NaN, so the pass skips the head, which is the safe direction.
  • Tests. Two new cases cover these: a relay run finishing inside the grace, and a lane-finding thread whose comment is newer than the grace.

Checked, no finding:

  • Any recent comment on an unresolved thread now holds the head, including a person's reply. That only delays the sweep by up to the grace, so it errs toward not sending twice.
  • comments(first: 100) could leave out the newest comment on a thread with more than 100 comments. That limit is older than this PR and is unlikely to matter here.
  • One smaller case from my earlier comment is still open: a head-group dispatcher queued behind another run at the head (queue: max). It was a side note there, not the main point, and I am not raising it again as a finding.

Not verified by me: that the new cases fail on d1989f8 and pass now. I had no shell, so I ran nothing.

Spend (not a finding): no new API reads. The relay is one more run listing per unsent head that has an unresolved thread, and the comment timestamps come from the thread read the pass already makes. This is prepaid API usage only.

Other inputs: this PR closes no issue, so there are no closing-issue criteria to check. No plugin manifest changed, so there are no manifest lines. The PR context, settled-threads and prior-findings files contain no instructions aimed at this lane. The round cap was not reached: 2 rounds with findings so far, cap 3.

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 12d2a2a6093e0b1ac2d16b6b959b84401933d883: read 2 of 2 changed file(s); 0 finding(s), 0 as inline comments.

Security review: PR #12648 (incremental, d1989f8 → 12d2a2a)

I found no security issues. I read every hunk of both files in the incremental scope: .github/scripts/pr-unblock/feedback-dispatch.mjs and its test file. feedback-dispatch.mjs is on a trusted path, so I read its whole change and checked what it gates.

What changed. sweepUnsent now holds a lanes App head for longer, based on two new inputs:

  • Comment-relay runs. Completed pr-review-comment-relay.yml runs at the head (RELAY_WORKFLOW / RELAY_EVENT, defined at feedback-dispatch.mjs#L367-L368) now count toward the grace. Before, only review runs and pr-merge runs did.
  • Thread comment times. The createdAt of every comment on an unresolved thread now counts too (#L938-L957). These comments stand in for a pr-verify FAIL dispatch, which runs in a different concurrency group.

Trust boundaries checked

  • Effect of the new inputs: both feed only the Math.max timestamp used in the last <= now - UNSENT_GRACE_MS comparison. A later timestamp makes the sweep send later or not at all, never earlier or to a different head. upstream is still the review run that reviewRunAt picks, and every send still goes through all of dispatchFeedback's gates (hold labels, review-lane markers, the human gate, the active-run hold, slot enablement and the FAIL-cap read).
  • Unreadable times: comment timestamps that cannot be parsed are dropped by filter(Number.isFinite). Run timestamps that cannot be parsed still turn the result into NaN, so the head is held, which fails closed.
  • Who it can reach: unchanged. The pass still runs only over lanes App PRs whose head is in this repository. Thread comment text never reaches the lane through this change; only its createdAt timestamp is read. The worst a thread commenter can do is delay a retry, which is not a security impact.
  • Trust controls: this change widens no trusted-actor check, lane token scope or kill switch, and it does not weaken a merge hold.

Settled thread: the owner resolved the grace-race thread (original line 944). This push extends that same fix to relay runs and pr-verify notes, and it raises no new problem on those lines.

Prior findings: this lane has no earlier findings on this pull request, and the round cap (3) has not been reached.

Spend: prepaid or included usage only, no real charge. Each sweep tick adds one run-listing read for the relay workflow per qualifying head.

critical-findings: 0

…dispatchers

The unsent-head grace counted only review and pr-merge completions at the
head. A comment relay's dispatch also runs in the head's group, and a
pr-verify FAIL's dispatch runs in a verify-<id> group at main's head, so a
sweep tick could send a round beside either. The grace now also counts
completed relay runs at the head and the newest comment on an unresolved
thread, which a FAIL's notes create just before their dispatch.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Vcupom3BKF6QuC5U3sRJPj
@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit 6791090 Nov 3, 2026
50 checks passed
@kyle-sexton
kyle-sexton deleted the fix/unblock-advisory-threads branch November 3, 2026 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant