Repository navigation
feat(pr-pipeline): clear a review-lane hold recorded on an older head once the new head reviews clean - #10959
Conversation
…s both lanes at a new head A review lane's hold names the head it judged (claude-lane-hold sha=...), but only a person could lift it, so a hold recorded against an older head stayed after the fix was pushed and reviewed clean (#10743: a CRITICAL hold at 272c9a2 while 23ee4b1 was fixed and reviewed). clear-escalations.mjs now judges a label a review lane added as stale-review-hold: every hold comment since the label was last removed names an earlier head, a trusted move (trustedMove, anchored at the push that landed the newest held head; never a lanes App push) took the branch off it, both review lanes passed on the current head with a review-complete notice and no hold notice, the head changes neither review caller, and the security lane leaves no CRITICAL or IMPORTANT finding (whole review, every finding inline, critical-findings: 0, no unresolved security-lane thread at CRITICAL, IMPORTANT or unreadable severity). A lane that still finds the cause holds the new head again. ci-status's review-lane lift check accepts the lanes App's own removal only after its stale-review-hold comment since the hold names a head no hold comment names; any other App or bot removal still fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VQb9Q3ucKDVyHvpcU5cfU8
… and close its races Fixes from the security review of 990c194, and the direction that ci-status verify the lanes App's stale-review-hold judgment, not trust it. - Every review lane must review the head whole, so a finding at the held head is looked at again; a code-lane open thread at CRITICAL or IMPORTANT keeps the hold, and for a lane a hold names (as for the security lane) every finding must be inline and an unrated open thread counts as CRITICAL. - clear-escalations reads the head and the hold comments again just before the delete, so a hold a lane wrote under the label already on, or a moved head, keeps it. - ci-status counts the App's removal only when no hold comment names the event's head (the head a missed hold names), the marker names a head no hold names, and the base commit's clear-escalations.mjs recheckStaleHold, the same predicate, passes again at the event's head with the job's own token. The base checkout gains that module's import closure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VQb9Q3ucKDVyHvpcU5cfU8
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
claude-review reviewed I found nothing to block or flag in PR #10959. I only read the code. I did not run the Node or shell tests, so the test results in the PR description are the author's claims and I have not confirmed them. What I checked
Observations that are not findings
Spend: this change adds no paid or billed resources. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bee893e2c6
ℹ️ 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".
|
claude-security-review reviewed Security review of #10959 at e5ff94d. The PR narrows a merge hold. A review lane's critical-findings: 0 What I checked and found sound
One finding (IMPORTANT): the "no open HIGH or CRITICAL finding" gate treats a resolved thread as a fixed finding. See the inline comment on Protected-class list. No instruction file, rules file, skill body or hook allowlist is deleted or softened here, so the instruction exception register was not needed. |
|
Unattended merge: risk HIGH at head |
…e it to a fix - ci-status's recheck judges the head the App's marker names, as it was when the App removed the label, so a later clean push stays green before its lanes run and a lanes App push after the clear does not strand it. - The hold-lift step counts hold comments only from the label's last removal before it went on, as holdsSince does, so a hold a person already lifted cannot block a later clear. - A clear needs a commit since each held head that is not a merge, and a critical-security hold needs its lane's CRITICAL inline finding at the held head in a file changed since; a summary-only finding stays for a person. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VQb9Q3ucKDVyHvpcU5cfU8
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VQb9Q3ucKDVyHvpcU5cfU8
…the App's whole predicate, comment only on a real clear - A stale-hold clear counts only commits since the held head that are the pull request's own (not reachable from main), so an Update branch that brings in main's commits lifts nothing; a critical-security hold needs the held finding's file changed by one of those commits. - ci-status's recheck runs the same signed-commits, lanes App marker and person checks clear-escalations runs, as of the removal, through one shared function. - The head and holds are re-read before the clear comment too, so a kept label gets no comment saying it came off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VQb9Q3ucKDVyHvpcU5cfU8
…n when edited after it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VQb9Q3ucKDVyHvpcU5cfU8
|
Unattended merge: risk HIGH at head This refusal has stood unchanged at this head, so a person is needed: the pr-merge lane added the human-gated label. |
melodic-automation-lanes
Bot
added
the
needs-human
…ged the flagged lines A trusted edit anywhere in the held file (a comment, whitespace) no longer counts as a fix: the diff from the held head must remove, or insert beside, a line the CRITICAL inline finding names there. A finding with no line, or a file whose patch does not read, keeps the hold. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The pr-merge lane escalated this pull request, and its head has moved ( |
melodic-automation-lanes
Bot
removed
the
needs-human
|
Removing |
|
The pr-unblock lane stopped
When a retry on this head can change the outcome, the lane runs one on its own, up to its per-head cap, and edits this comment if it stops again; a run that does not stop clears the status. When no retry can (a review thread the lane does not take or settle, or a failing check after the retries are spent), a person resolves the threads or gets the checks green, then dispatches the workflow below: that run finds the cause gone and clears the status. On a pull request the lanes App opened, the dispatchers start that run once per stop; on any other, a person does. A new commit to the branch starts clean. To run it again on this head now, dispatch the gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number=10959 -f activity=resolve-conflicts |
| if (counts.length !== 1 || counts[0] !== "0") { | ||
| return `the security lane's summary of ${head} does not read critical-findings: 0`; | ||
| } | ||
| const open = threads.filter((thread) => !thread.resolved); |
There was a problem hiding this comment.
The "no HIGH or CRITICAL finding" gate counts only unresolved lane threads, so a resolved thread counts as a fixed finding. In staleHoldEvidence the held CRITICAL thread at the old head is expected to be resolved (the tests' heldFinding() is isResolved: true). isResolved says nothing about who resolved the thread or why. GitHub lets the pull request author or anyone with write access resolve a review thread, and that person need not be on the trusted-actor list. Resolving adds no comment or review, so the personSince / standingStop "a person spoke" check does not see it.
Path: a lane holds critical-security at H1 with an inline CRITICAL thread. A trusted human pushes a non-merge commit that touches the flagged lines. patchTouches also counts an insertion adjacent to the flagged line. The branch's author then resolves the CRITICAL thread. The sweep clears the hold as stale-review-hold, and ci-status accepts the App's removal after the recheck. The recheck reads thread state at check time and not at the removal, so it passes too.
What stops it today: a trusted human push, and a whole review from the security lane at the new head that must itself read critical-findings: 0 with no new hold. That makes the lane model's re-review the only check on whether the code is actually fixed. It reads the same PR content the attacker controls, and the thread state is the one input a non-trusted account can set.
Suggested fix: count a thread as dealt with only if a trusted human resolved it (GraphQL resolvedBy), or ignore isResolved and require that no lane thread at the held head still anchors to unchanged lines. heldCauseUnchanged already does the line check for critical-security holds only. Extend it to every strict lane's CRITICAL or IMPORTANT thread.
severity: IMPORTANT
claude-lane: claude-security-review
No related issue: owner standing direction 2026-10-29, needs-human policy brief Part A2
Summary
A review lane's hold (
claude-lane-hold sha=...) names the head it judged, but until now only a person could lift it. So a hold recorded against an older head stayed on the pull request after the fix was pushed and reviewed clean. #10743 shows this: a CRITICAL hold at 272c9a2 stayed while 23ee4b1 was fixed and reviewed.pr-merge's scheduled
clear-escalationsnow removes a human-gated label that a review lane added, recording the reasonstale-review-hold. It does so only when all of these hold:humanon the base trusted-actor list. A push by the lanes App never counts.critical-findings: 0, and every finding is inline for the security lane and for any lane a hold names. No unresolved lane thread is atCRITICALorIMPORTANT(security-review IMPORTANT counts as HIGH). For those strict lanes, an open thread with no readable severity also keeps the hold.A lane that still finds the cause holds the new head again, with its own hold comment and label.
The lanes App's own comment is never taken as proof.
ci-status's review-lane lift step accepts the lanes App's removal only when all of these hold:stale-review-holdcomment written since the hold.recheckStaleHold, re-run from the base commit's copy ofclear-escalations.mjswith the job's own token, passes at the head the App's newest marker names (CLEARED_SHA), judged as it was when the App removed the label. It is the same predicate the App ran. A later head is judged by its own review lanes, as after a person's lift.holdsSincecounts them.Any other App or bot removal still fails.
This touches the gate paths
.github/scripts/pr-merge/clear-escalations.mjsand.github/workflows/pr-require-checks.yml(plus a comment-only edit topr-merge.yml), so a person arms it.Fix
clear-escalations.mjs:staleHoldEvidencepredicate (lane reviews, open-thread severities, trusted move), andrecheckStaleHoldfor ci-status.stale-review-holdpath inverdictfor labels a review lane added.pr-require-checks.yml:HEAD_SHAto the step and the marker's head to the recheck asCLEARED_SHA.clear-escalations.mjs's import closure.docs/conventions/pr-pipeline/README.md: a new row in the clear table. Changelog fragment in.changes/pr-pipeline/.clear-escalations.mjs, not a new module. A new module would need a hold-floor row, andsync-hold-floor.pywould then rewritemerge-holds.mjs.HEAD_SHA, and rechecks at the marker's head. Under strict equality, every clean push after a valid clear would turn ci-status red until a person acts.critical-securityhold needs its lane's CRITICAL inline finding at the held head in a file changed since; a summary-only finding stays for a person. feat(pr-pipeline): bind workflow-edit reviews to a run from main #10743's CRITICAL finding was inline at its held head 272c9a2, so it meets this rule.Verification
node --testin.github/scripts/pr-merge: 665 pass, 0 fail, including 12 stale-hold and 8 recheck tests.scripts/ci-hold-lifted-by.test.sh: PASS=63 FAIL=0.test_merge_holds_sync(unittest): 20 tests OK.scripts/sync-hold-floor.py --check: clean after merging main (feat(pr-pipeline): bind workflow-edit reviews to a run from main #10743).ci-test-node-stop-commands,run-hygiene-gate) fail locally only because PyYAML is missing.actionlintonpr-require-checks.ymlandpr-merge.yml: clean.markdownlint-cli2on the README and the fragment: clean. ShellCheck on the harness: clean.main.Related
enable-auto-merge.mjs:687-695(liftBlocker) counts only person removals as lifts. After a stale-review-hold clear it still reports "a review lane holds it" when lifting pr-merge's own refusal label later.babysit-merge-riskon a moved head) is already cleared by the existinghead-movedpath.🤖 Generated with Claude Code
https://claude.ai/code/session_01VQb9Q3ucKDVyHvpcU5cfU8