[kyle-sexton]

Refs: melodic-software/claude-code-plugins#9436

Summary

A recorded cost decision (**Decision recorded.** Cost:) used to drop the cost hold at every later head of the pull request. That let a change made after the decision through without a second look: in claude-code-plugins#9509, a non-merge commit changed cancel-in-progress. The owner chose option d on 2026-10-24: the decision now covers only the net diff it was made at.

Fix

.github/actions/apply-review-hold/hold.cjs now runs the carry test below. It runs only when the counted review returned a cost hold.

1. Decision. The newest unedited decision by a trusted person. It is the pull request's own if one exists, otherwise its linked issue's (one closing issue, else one Refs: issue, naming #<number> and newer than the pull request). The author, App, edit and created-after-PR checks are unchanged.

2. Decided head. The sha= of the newest <!-- claude-lane-hold lane=<this lane> sha=<40hex> criteria=... --> marker that:

   • is posted by github-actions[bot],
   • has criteria that include cost, and
   • has a created_at earlier than the decision's.

   With no such marker, nothing carries, even at the same head.

3. Carry test.

   • If the current head equals the decided head, the decision carries and no compare is read.
   • Otherwise the step reads GET compare/{base}...{decidedHead} and GET compare/{base}...{currentHead}. Both are three-dot compares, and {base} is the PR's base ref (new BASE_REF env, github.event.pull_request.base.ref).
   • The decision carries only when both files arrays have the same set of filenames, and each file has the same status, previous_filename and patch text. Blob sha and URLs are ignored.

4. Fail closed. The cost hold stays on any of these:

   • a failed read,
   • a missing marker,
   • an unusable base name,
   • a compare with 300 or more files,
   • a file without a patch (binary or oversized),
   • a duplicate filename.

5. Bound. At most 2 compare reads, and none when the head is unchanged. The step logs one core.info line naming the reason category and shas, with no comment bodies.

The pr-review-cli.yml hold-step contract (header) and the allowed-author-ids description now say this in plain words.

Verification

• nice node --test .github/scripts/apply-review-hold.test.cjs: 35 pass, 0 fail.
  • Carries: same head (0 compares); a base-only merge whose blob sha moved but patch did not; a rebase with the same patches listed in another order.
  • Does not carry: the #9509 shape (a later cancel-in-progress hunk); a merge that hand-edits a PR line; a base change that shifts a patch's hunk header; an added, dropped or status-changed file; a missing patch; 300 files; a compare error; no prior cost marker (also: a marker after the decision, another lane's, a non-cost one, one not by the bot); an unusable base name.
  • Call bound: the carry path reads exactly listComments, graphql, compare, compare, listFiles, get.
  • No-carry reasons: each negative case asserts its own logged reason.
• Mutation check: making the per-file comparison always true fails 4 tests; removing the marker requirement fails 1.
• nice node --test .github/scripts/pr-review-cli-api-calls.test.cjs: 10/10. nice node --test .github/scripts/pr-review-cli.test.cjs: 24/24. biome check clean; actionlint clean.

Related

• fix(pr-pipeline): carry an owner-lifted claude-review cost hold forward across base-only merges claude-code-plugins#9436 (the cost decision this carries)
• perf(pr-pipeline): run pr-unblock-dispatch half-hourly after the reconciler claude-code-plugins#9509 (the shape that must not carry)
• fix(pr-merge): bind a cost decision to the pull request it names claude-code-plugins#9569 (merge gate that matches this test)

🤖 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-24T06:24:35.916258Z	37f451d	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 3m 48s —— View job

---

Code review — PR #877

• Gather context (diff, PR context, rules)
• Review hold.cjs carry-test logic
• Review tests
• Review workflow contract changes
• Check cost/trust-gate hold classification
• Post findings

Scope note. REVIEW.md records that this repository runs a dedicated security lane (pr-review-security.yml), so security findings are out of scope for this lane and are left to it. No .claude-plugin/plugin.json or marketplace.json changed, so the manifest checks don't apply. No PR-context file was supplied and the PR body has no Closes/Fixes line (only a cross-repo Refs:), so there's no closing-issue criteria to check against. The only rule file under .claude/rules/ (pr-body-contract.md) carries no paths: frontmatter, so it doesn't scope-match any changed file here; for what it's worth, the PR body is compliant with it anyway (opens with Refs:, has ## Summary/## Fix/## Verification/## Related).

What I checked. Read the full diff (hold.cjs, its test file, and the pr-review-cli.yml doc/env changes) end-to-end: the new carryDecision compare logic, the costDecided "newest unedited decision" change, the marker-regex construction (guarded safely — LANE_ID is validated to ^[a-z-]+$ before interpolation into the regex), the BRANCH validation on BASE_REF, the file-map equality check (then.size === now.size plus a one-directional subset check, which is a valid way to prove set equality here), the fail-closed paths (300-file cap, missing patch, duplicate filenames, failed reads, unusable base name), and the call-bound enforcement (at most two compare reads, none when the head is unchanged). I also confirmed the newly-added BASE_REF env var is wired into the one "Apply the hold" step this diff touches, and that another BASE_REF-consuming step in the same job already existed and is unaffected.

I could not execute node --test in this environment (the sandbox declines to run node here), so I was unable to independently confirm the PR's "35 pass, 0 fail" and mutation-testing claims — those remain author-reported and unverified by me. biome/actionlint cleanliness is likewise author-claimed and unverified.

Result: no findings. Static reading didn't turn up a correctness bug meeting the high-signal bar — the carry logic, its fail-closed defaults, and the bound on compare reads all look internally consistent with the stated design, and the test fixtures I traced (base-only merge, rebase, the #9509 shape, hand-edited merge, shifted hunk header, added/dropped file, missing patch, 300 files, failed compare, missing/foreign/wrong-criteria marker, unusable base name) match the code paths they're named after.

claude-lane-summary: lane=claude-review sha=37f451df97cc6f1be16c251b543c7cc84b6f7451 run=40070002914

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Apply skip gate — not applicable (change touches the cost-hold carry logic, a trust-relevant control path)
• Review .github/actions/apply-review-hold/hold.cjs for trust-boundary / logic issues
• Review .github/workflows/pr-review-cli.yml changes
• Review test file changes (context only)
• Adversarial verification of candidate findings — no candidates to verify
• Post findings / summary

No security issues found. manifest <field>: not applicable — no plugin manifest files changed.

Notes from the review (no action needed):

• The change is a tightening, not a loosening, of the existing control. Before this PR, costDecided returned a plain boolean and any unedited trusted decision covered every future head of the PR forever — exactly the gap exploited conceptually by claude-code-plugins#9509 (a later cancel-in-progress change slipping through under an old decision). carryDecision in hold.cjs#L229-L293 now requires the head to be byte-identical (per-file status/previous_filename/patch) to the decided head's net diff against the base, or an exact sha match, before the decision carries.
• Fail-closed is consistent throughout: a failed compareCommitsWithBasehead read, an unusable BASE_REF, a ≥300-file compare, a file with no patch (binary/oversized), or a duplicate filename all keep the cost hold rather than dropping it (hold.cjs#L247-L292).
• BASE_REF is validated against a branch-name allowlist (BRANCH regex, hold.cjs#L217) before being embedded in the basehead REST parameter; it's only ever consumed by the Octokit REST client (no run:/shell interpolation), and the same env.BASE_REF pattern already exists elsewhere in pr-review-cli.yml, so this isn't a new injection surface.
• The regex built from env.LANE_ID (hold.cjs#L232-L235) is safe because LANE_ID is validated to /^[a-z-]+$/ earlier in run() (hold.cjs#L344-L345) before any carryDecision/marker lookup runs — no regex-injection or ReDoS path from user-controlled content (only github-actions[bot]-authored comments are scanned for the marker).
• Marker detection is scoped to c.user.login === "github-actions[bot]", which an ordinary PR author/commenter cannot forge, so the "decided head" cannot be spoofed by an untrusted comment.

critical-findings: 0

claude-lane-summary: lane=claude-security-review sha=37f451df97cc6f1be16c251b543c7cc84b6f7451 run=40070002914

[github-actions]

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

[github-actions]

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