[kyle-sexton]

Closes #12809

Summary

The merge gate refuses a lanes App pull request verify:earlier-fail when a FAIL at another head has the same change digest. Once that FAIL's fail-note threads were resolved, the PR could not get out of that state. address-feedback found no thread, ended success without pushing, and the dispatcher kept re-running the same no-op round (#12783, runs 41606722334 and 41610279787).

This change implements the decision record. The reader's earlierFails names the FAILed heads, the dispatcher sends a model round for them even when no thread is unresolved, and the select job reads that FAIL's notes from its verdict artifact. A round that pushes nothing and reports every note already addressed now escalates the new feedback-already-addressed reason with the needs-session label instead of ending success.

Fix

• verify-cap.mjs: on a verify:earlier-fail refusal, the reader's answer carries earlierFailHeads, taken from the report's existing earlierFails (only commit ids other than the head; anything else names no head and warns). That refusal no longer warns.
• feedback-dispatch.mjs: on a lanes App PR whose threads hold a fail note from another head, the "no unresolved review thread" skip now waits for the verifier read, at most EARLIER_FAIL_READS (3) such reads per pass. On an earlier-fail answer it sends a model round with the new verify-earlier-fail-heads input (space-separated SHAs), never on a pass-only round. The cap, hold, owner and handled-check-run skips still come first.
• pr-unblock.yml: the new input sits beside verify-pass-head and verify-superseded-head. address-feedback-select gains actions: read and the VERIFY_EARLIER_FAIL_HEADS env var.
• address-feedback.mjs select: when no thread is kept, nothing untaken is open and FAILed heads were named, it reads their notes (new earlier-fail-notes.mjs) into threads.json verify_notes (VNOTE_<n> items, each with a head, a path and a body). Before reading, it stands down awaiting-session while that head's stop is queued for a session builder, and it keeps the protected-path stop and the round cap. A read that fails escalates feedback-verdict-unreadable (route fail).
• earlier-fail-notes.mjs: finds the FAIL by its exact-name index artifact for the head (pr-verify-index-sha-<head>), then the run's FAIL verdict for this PR and head. It reads only runs that resolve-pr.mjs countedBy accepts (pr-verify.yml on main of this repo) and passes over others, as the gate's reader does. The verdict goes through verify-notes.mjs readVerdict and noteComments. Every other failure, a withheld FAIL, and more than 50 items all throw.
• Model contract: feedback-thread.sh gains notes. record accepts VALID or already-addressed (with evidence) for a VNOTE_ id, and a note's path is in scope like a thread's. comments-ci.md, the SKILL.md mode row and the collect-base-activity preamble name the new subcommand.
• apply-patch.mjs keptThreadsFrom counts notes as threads on their paths, so the fix scope and the Lane-Thread trailer name fixed notes.
• publish-feedback.mjs settleNotes posts and resolves nothing for notes.
  • A push whose one fix commit names a VALID note ends as a fix round does (escalationReason: "").
  • No VALID note with the head unmoved escalates feedback-already-addressed.
  • A moved head with no VALID note, or a VALID note with no landed fix, escalates feedback-not-landed.
  • A missing note escalates feedback-incomplete, and an off-template reply escalates feedback-invalid.
• The routes table, the write runner's reason allowlist and report-escalation.test.sh list both new reasons. The changes include pr-pipeline README text and two changelog fragments, for source-control and pr-pipeline.

Verification

#	Criterion	Met by
1	The reader's earlier-fail report names the FAILed heads	earlierFails already exists at babysit_verify_evidence.py:1132. New test test_an_earlier_fail_refusal_names_the_failed_head (test_babysit_verify_evidence.py:1287). verify-cap.mjs:250 passes the heads on, tested at verify-cap.test.mjs:142.
2	The round names the FAILed head, and select reads its notes from the artifact	The input is set at feedback-dispatch.mjs:751 and read by address-feedback.mjs:1157. The artifact read is earlier-fail-notes.mjs:68-94 (countedBy, readVerdict, noteComments). Tests: earlier-fail-notes.test.mjs:60, address-feedback.test.mjs:2081.
3	No unresolved thread plus earlier-fail gets a model round	feedback-dispatch.mjs:622 and :737, behind the prefilter and cap below. Test feedback-dispatch.test.mjs:1557.
4	All fail-note threads resolved means notes become work items, not stop-resolved	address-feedback.mjs:1008. Test address-feedback.test.mjs:2081.
5	A push answering a named note ends as a pushed fix	publish-feedback.mjs:639 (settleNotes). Test publish-feedback.test.mjs:1790.
6	No push and every note already addressed escalates needs-session, not success	publish-feedback.mjs:652. Test publish-feedback.test.mjs:1801.
7	An artifact that cannot be downloaded or parsed ends failing	address-feedback.mjs:1017 (feedback-verdict-unreadable, route fail). Tests address-feedback.test.mjs:2103 and earlier-fail-notes.test.mjs:82,91,113.
8	The new reason is in the routes file, with a why that names the already-addressed earlier FAIL	escalation-routes.json:27. The why is checked at publish-feedback.test.mjs:1801, and report-escalation.test.sh:185 pins the table.
9	The reader's refusal rules are unchanged	No change to babysit_verify_evidence.py. Its suite passes (430 tests).
10	Test of the artifact path	address-feedback.test.mjs:2081 and earlier-fail-notes.test.mjs:60.
11	Test of the already-addressed path, with no silent success	publish-feedback.test.mjs:1801, :1813. The same-head re-dispatch stands down at address-feedback.test.mjs:2137.

Local runs:

• node --test over address-feedback, earlier-fail-notes, feedback-dispatch, publish-feedback, verify-cap, callers, apply-patch, collect-base-activity and workflow-gates: 1364 passed, 0 failed.
• feedback-thread.test.sh passes all 164 cases.
• report-escalation.test.sh passes.
• babysit_verify_evidence unittest: 430 tests OK.
• scripts/check-affected.sh --base origin/main --no-fetch: 15 passed, 0 skipped.

Beyond the record, each choice taken to meet a criterion:

• feedback-verdict-unreadable is the new reason that gives criterion 7 its failing status, with its lane-stopped check.
• A notes round past the round cap stops feedback-verifier-open.
• A note takes only VALID or already-addressed.
• More than one FAILed head goes in one space-separated input.
• pr-run-activity-write.yml (the reason allowlist) and the collect-base-activity preamble are edited.

Cost bound: one verifier read can take up to VERIFY_CAP_TIMEOUT_MS, 2 minutes (verify-cap.mjs:67), and sweepStale and recoverLost decide PR after PR inside one 10-minute dispatch job. So the no-unresolved-thread path reads only when both of these hold:

• Prefilter: the threads the dispatcher already read hold a pr-verify fail note (github-actions[bot], first line <!-- pr-verify:fail-note -->) from a review of another head (feedback-dispatch.mjs:788 earlierFailNoted). This costs no extra call. Every other PR keeps the free skip. The pr-verify-failed label was not usable as the sign: verify-notes.mjs puts it only on a person's PR, never a lanes App PR. A failed pr-verify check at the head was not usable either: in an earlier-fail refusal the FAIL is at another head. The cost of this choice: a FAIL whose notes were never posted as threads (the PR moved before post-verify-notes ran) gets no round on this path.
• Cap: one dispatcher pass makes at most EARLIER_FAIL_READS (3, feedback-dispatch.mjs:783) such reads, through a budget shared across the pass's sweeps. The rest skip unrecorded.
• Retry window: a read that fails or times out answers readFailed (verify-cap.mjs), and a cap read that could not count every FAIL is treated the same way; neither is taken as "no earlier FAIL". A head left by a failed read or by the cap has no review run coming, so sweepUnsent also sends a head with no unresolved thread that holds a fail note from another head, but only while its newest review run completed within EARLIER_FAIL_WINDOW_MS (2 hours, beside EARLIER_FAIL_READS in feedback-dispatch.mjs). Nothing records an answer, so a head answered "no earlier FAIL" may be read again inside the window: about 4 half-hourly ticks, and never more than the 3-read cap per pass.
• Why bounding rather than persisting: the tick restores no dispatch record (pr-feedback-dispatch.yml:396-405 skips the restore for pr-unblock-dispatch.yml, and the save keys on main's commit on a tick), so a recorded answer never reached the next tick. Persisting it would add two workflow steps (a rolling cache restore and save) and a cache-trust surface (any default-branch job can write the cache) to save at most about 4 bounded re-reads per head.
• Tests (feedback-dispatch.test.mjs): "a thread-less head whose verifier read failed is read again by a later pass inside the window", and "an answered thread-less head may be re-read inside the window, never past the pass's cap, and not at all past the window".

Tests: feedback-dispatch.test.mjs covers both: no fail-note sign means no verifier read (feedback-dispatch.test.mjs:1607), and 4 candidates in one pass get 3 reads (:1626).

Related

• perf(pr-pipeline): re-run ci-status once per head after both review lanes finish #12797 edits .github/workflows/pr-feedback-dispatch.yml. This PR does not touch that file: the dispatcher builds its inputs in feedback-dispatch.mjs.
• docs: replace restated upstream facts with pointers in work-items, context-budget #12783: the PR where this was seen. Its current head already has success address-feedback check runs, so this change re-dispatches it only once its head moves.

Known limits

• A lanes App PR whose earlier FAIL never had its notes posted as threads (the PR moved before post-verify-notes ran) does not pass the prefilter. Today that path dead-ends silently: the merge gate keeps refusing with verify:earlier-fail, the dispatcher skips the PR as thread-less, and no round or escalation follows, as before this change. This is accepted for the case pr-unblock: address-feedback dead-ends on verify:earlier-fail once fail notes are resolved #12809 describes, where the fail-note threads exist and are resolved. A follow-up issue is filed only if a run shows the edge.
• A thread-less head whose verifier read keeps failing past EARLIER_FAIL_WINDOW_MS (2 hours after its newest review run completed), or that the 3-read cap keeps leaving that long, is left alone: no sweep reads it again until a new review run at the head, such as the one a push or a base merge starts.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-03T15:33:54.662542Z	066ef29	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 066ef298ea

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

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

Security review: PR #12843, incremental (afb7ddc0 → abccf5d7)

I found no security issues in this push. I read every hunk of all 3 files in the incremental scope: the .changes/ note, .github/scripts/pr-unblock/feedback-dispatch.mjs (a trusted path) and its test. I also read sweepUnsent and the sweep branch of main in the head file for context.

critical-findings: 0

What I checked

• What changed. The push drops the EARLIER_FAIL_READ dispatch-record key. Previously, a head with no unresolved thread whose verifier read found no earlier FAIL was recorded and never read again. Now the unsent sweep keeps re-reading such a head while its newest review run finished within EARLIER_FAIL_WINDOW_MS (2 hours) (feedback-dispatch.mjs#L1017). If the completion time can't be read, Date.parse returns NaN and the check fails closed: the head is left alone.
• Trust and merge-hold surface. The change touches only when a round gets dispatched, not who is trusted or what the round may do. The trust checks in dispatchFeedback are unchanged: the trusted humans, the App sender ID, the reviewer IDs, and the slot resolution through resolveFeedbackSlot. So is the "sent once per review run" check (L1036-L1043). No merge hold, token scope, kill switch or trusted-actor path is touched. A head with no earlier FAIL still ends in skip, so the change adds no dispatch path that wasn't already gated.
• Record write removed from the sweep branch of main (L1570-L1582). That write only ever carried EARLIER_FAIL_READ. The non-sweep branch still writes the dispatch record, so no other handled-state is lost.
• Instruction surface. The .changes/ edit and the code comments are prose that matches the code. No guardrail is removed.

Usage cost (not a finding): a head that already got a "no earlier FAIL" answer can now be re-read by the verifier on each half-hourly tick for up to 2 hours. Those reads are capped at EARLIER_FAIL_READS = 3 per pass, and a dispatched round is still sent only once per review run. This spends Actions run time and API calls on the existing schedule. It sets up no new billing.

[github-actions]

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

claude-lane: claude-review

[github-actions]

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

Code review (incremental, afb7ddc → abccf5d): PR #12843

Scope. I read every hunk of all 3 files in this round's change:

• .changes/pr-pipeline/fix-address-feedback-earlier-fail-040eae83.md
• .github/scripts/pr-unblock/feedback-dispatch.mjs
• .github/scripts/pr-unblock/feedback-dispatch.test.mjs

For context I also read the head versions of reviewRunAt, dispatchFeedback's parameters, sweepReleased, sweepUnsent, the tick branch of main, and the sweepUnsent test helper. I ran no tests. The PR's local-run results are the author's claim and I have not checked them.

What changed. The earlier-fail-read record is gone.

• dispatchFeedback no longer reads or writes it.
• The tick no longer copies it, writes the dispatch record, or emits record-changed.
• sweepUnsent now limits a head with no unresolved thread by time instead: it still needs a fail note from another head, and the head's newest review run must have completed (updated_at) within EARLIER_FAIL_WINDOW_MS (2 hours).

What I checked:

• Comparison. !(Date.parse(...) >= now - window) counts the window's last moment as inside, and a missing or unparseable time leaves the head alone.
• Order of checks. The window check runs after reviewRunAt and before the grace and already-sent checks, so a head outside the window costs no further reads.
• now. It reaches sweepUnsent through common.now → rest.now.
• No leftovers. Nothing in the repository still uses EARLIER_FAIL_READ or earlier-fail-read. writeRecord is still used on the non-tick path (feedback-dispatch.mjs:1639).
• Record default. With no record passed in, dispatchFeedback falls back to emptyRecord() on the tick, as it did before this PR added the mark.
• Tests. The new tests cover a retry inside the window, a pass whose read cap is spent, the window's last moment (past - 1), and the first moment past it (past, which makes no read and no dispatch). They anchor on REVIEWED.updated_at (12:10), which matches the code.
• Docs. The comments and the changelog fragment now describe the window and drop the old claim that a definitive answer is recorded.

Findings: none.

Prior and settled points

• My earlier advisory finding (feedback-dispatch.mjs ~1010: an answer recorded where the tick never reads it) is answered by this push. The owner removed the record and bounded re-reads with the window instead, and recorded the decision in the settled thread. I am not raising it again.
• The other settled thread (Codex P2: a failed read was taken as "no earlier FAIL") still holds after this push. readFailed and the cap-read skip still return before any answer is taken.
• Accepted limit. The PR's "Known limits" section names it: a head whose fail-note threads are resolved more than 2 hours after its newest review run, with no address-feedback check run at that head, waits for a new review run.

Scope notes

• I left security to the security lane, which runs for this repository.
• No plugin.json or marketplace.json changes, so there are no manifest lines.
• I found no embedded instructions in the PR context, settled-threads or prior-findings files.

Spend: an answered head with no unresolved thread can be read again by the verifier for about 4 half-hourly ticks. Each read takes up to 2 minutes, and a pass makes at most 3. This uses Actions minutes and GitHub API rate budget only, with no billing change.