[kyle-sexton]

Closes melodic-software/claude-code-plugins#9635
Refs: melodic-software/claude-code-plugins#9635

Summary

The review lanes' hold step (apply-review-hold/hold.cjs carryDecision) took a cost decision's decided head from the newest unedited github-actions[bot] claude-lane-hold marker comment. Any workflow on GITHUB_TOKEN posts as github-actions[bot], so a workflow could post a marker naming a commit it had not pushed yet and carry an earlier cost decision onto it. The hold step now reads the decided head from this lane's own check-run notices, as the three merge-side readers in claude-code-plugins already do (claude-code-plugins#10357). This is the ci-workflows half of #9635 and merges last.

Fix

• Source of the decided head. The step reads every pull_request run of this run's workflow on the pull request's branch of this repository, started before the decision.
• Vetting each run's head. The compare base...head must list fewer than 300 files and must not change the caller's workflow file, since a pull_request run takes that file from the head.
• Choosing the notice. From each run's status job that finished before the decision, the step takes the claude-lane-hold lane=<lane> ... criteria=...cost... label= notices that name the run's own head, and keeps the newest one.
• What carries nothing. Any of these: two heads noticed at that time, no notice, a head pushed after the decided head with no run of this lane (from the repository activity API, which keeps the push even when the run is deleted; same rule as the merge side's unranHead), or any failed read.
• Removed reads. The comment edit and deletion reads are removed along with the comment read.
• Permissions. The status job now requests actions: read and checks: read, and takes a new STATUS_JOB env. A caller that does not grant both fails at startup, so the title carries !. Both claude-code-plugins callers already grant them (pr-review-hosted.yml:167-171 and pr-review-security-hosted.yml:172,175). The sandbox callers gain them in pr-pipeline-sandbox#243.

Verification

• node --test .github/scripts/*.test.cjs: 735 pass, 0 fail. apply-review-hold.test.cjs has 41 tests.
• Fail before the fix: I ran the committed test file against origin/main's hold.cjs. Two tests failed with the hold output 'false' !== 'true' (apply-review-hold.test.cjs:1025 and :1046). In the first, a github-actions[bot] marker comment with no lane notice made the old code carry the decision. In the second, a newer forged marker naming the current head made it carry.
• Model text cannot forge a notice: no status-job step writes model text raw to stdout. Its notices start with fixed prefixes, and core.notice/core.warning escape newlines.
• Sabotage check: removing any one of 10 guards fails at least one test. The guards are own head, lane, notice level, fork run, run before the decision, job name, workflow-file vet, run-deletion check, tie, and current event. The eleventh, the job-completion cutoff, can be removed without any test failing. It duplicates the cutoff in decidedHead, so removing it changes nothing.
• actionlint and zizmor --offline on pr-review-cli.yml: no findings.

Left out: no live run. The change takes effect once callers re-pin.

Risk

The callers reach this change only by re-pinning. Before the claude-code-plugins callers re-pin (pr-review-hosted.yml:175, pr-review-security-hosted.yml:180), standards runner-policy needs an approved contract for the new SHA. UNVERIFIED: whether another workflow holding checks: write could PATCH an Actions job's check run to add a notice. The merge-side readers from #10357 have the same open question, and it trusts no more than the comment rule did.

Related

• claude-code-plugins#10357: the merge-side readers with the same rule.
• pr-pipeline-sandbox#243: grants checks: read before the sandbox callers re-pin.

🤖 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-10-28T22:06:52.992816Z	b371aae	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.

[claude]

Claude finished @kyle-sexton's task in 4m 55s —— View job

---

Security review complete

• Gather context (diff, PR description, changed files)
• Review hold.cjs trust-boundary logic (decided-head sourcing, guards)
• Review pr-review-cli.yml permission changes (actions: read, checks: read)
• Review test files for coverage of security-relevant guards
• Review README changes
• Adversarial validation of any CRITICAL candidates (none produced)
• Post final findings

No security issues found. This is a trust-boundary hardening change, and I traced it specifically for new attack surface rather than just restating the PR's own claims:

• Scope restriction is correct. carryDecision only considers pull_request runs whose head_repository.full_name matches the current repo (hold.cjs:317) and whose workflow_id matches the current run's own caller workflow (hold.cjs:309). A forged notice from a fork run, or from an unrelated workflow file (even one added by the PR author on the same branch), is never read.
• Self-forgery is closed, not just the comment vector. I traced the scenario where an attacker with push access to a same-repo branch edits the caller workflow file to inject a step that emits a fake claude-lane-hold ... criteria=cost notice naming its own head. That run's own compare(base...head) is checked against the caller's workflow path before its notices are trusted (hold.cjs:322-326), and any run in history that changed that file causes the entire decision to fail closed, not just that run's notice. A new, separate workflow file can't help either, since it would carry a different workflow_id and is excluded by the scoping above.
• No model text reaches the notice. The only core.notice writing the claude-lane-hold line (hold.cjs:496-498) is built from validated env values (lane id, a 40-hex sha, a fixed criteria list, a comma/newline-free label), never from the review's model-generated reason. The notice regex (hold.cjs:197-200) anchors ^ against the whole annotation message with no multiline flag, so even an unrelated step's core.warning echoing attacker text couldn't smuggle a forged line in behind other content. The two core.warning calls inside the status job itself (pr-review-cli.yml) only interpolate a fixed leg name, never PR- or model-controlled text.
• Fail-closed on ambiguity. Two heads tied at the newest notice time, a full page of branch activity, a run with no readable head, an unusable STATUS_JOB/BASE_REF, or any failed read all keep the hold rather than guessing — matches the "fail closed" bar this lane holds changes like this to.
• Permissions match need. The new actions: read / checks: read grants on the status job line up exactly with the new reads (getWorkflowRun, listWorkflowRuns, listJobsForWorkflowRun, listAnnotations); confirmed against both the workflow YAML and the updated pr-review-cli.test.cjs permissions assertion.
• The author's own "UNVERIFIED" note (whether a workflow with checks: write could PATCH a check run to add a forged notice) isn't reachable in this repo — no workflow here grants checks: write (checked). It's a cross-repo question inherited unchanged from the already-accepted merge-side design (claude-code-plugins#10357), not new risk this diff introduces, and the author has already surfaced it rather than hidden it.

I did not execute the test suite directly in this sandbox (the node --test invocation required interactive Bash approval that wasn't available here), so I'm relying on static tracing of the logic plus the PR's own stated verification (735/735 passing, documented sabotage checks against each of the 10 guards) rather than re-running it myself. The guard coverage in the diff (apply-review-hold.test.cjs) does exercise every branch I traced above.

critical-findings: 0

claude-lane-summary: lane=claude-security-review sha=b371aaefd5979b5a6220dea50aa66199faac07cf run=40776900221

[claude]

Claude finished @kyle-sexton's task in 4m 50s —— View job

---

Code review: claude-review lane

• Read context (PR body, diff, custom instructions)
• Read hold.cjs changes
• Read apply-review-hold.test.cjs and pr-review-cli.test.cjs changes
• Read pr-review-cli.yml changes
• Read README.md changes
• Check .claude/rules/ path-scoped rules (pr-body-contract.md)
• Check PR body against pr-body-contract
• Report findings

No blocking or advisory findings. I traced carryDecision's new path (hold.cjs:263-381) end to end against the sabotage checklist in the PR description — own head (m[1] === head, L203), lane, notice level, fork-run exclusion (L317), run-before-decision cutoff (L318), status-job name match (L336), workflow-file vet (L325), the unranHead run-deletion check, the tie case in decidedHead, and the current-event/repo check (L273) — and each guard matches what the comments, the workflow doc block, and the test suite independently claim. The STATUS_JOB plumbing into pr-review-cli.yml's status job permissions (L3657-3659) and the dynamic STATUS_JOB/job-name values line up for both the code and security lanes.

A few notes, none rising to a reportable finding:

• The closing issue, melodic-software/claude-code-plugins#9635, is in a different repository with no PR-context file available to this lane (.claude-lane/pr-context.md is absent) and no local access to that repo, so its acceptance criteria are unread, not verified met.
• I could not execute node --test .github/scripts/*.test.cjs myself in this sandbox (the run required approval this lane doesn't have), so the PR's "735 pass, 0 fail" and "41 tests" verification claims are author-claimed and unverified by me, not independently confirmed. The test additions I read by hand (new laneOf/statusJob/notice/push fixtures, the call-order assertions, and the per-guard table in "the notice fixture carries, and each one-field change to it keeps the cost hold") do match the implementation's branches.
• The pr-body-contract rule's requirements (Conventional Commits title with !, Closes/Refs/No related issue opener, ## Summary/## Fix/## Verification/## Related sections) are all satisfied by this PR body.

manifest: n/a (no .claude-plugin/plugin.json or marketplace.json changed)

claude-lane-summary: lane=claude-review sha=b371aaefd5979b5a6220dea50aa66199faac07cf run=40776900221

[github-actions]

claude-security-review has reviewed this pull request through b371aae; a later push is reviewed from there.

[github-actions]

claude-review has reviewed this pull request through b371aae; a later push is reviewed from there.