[kyle-sexton]

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

Summary

This aligns the hold step's cost-decision reader with claude-code-plugins' merge-side readers in claude-code-plugins#9569 (head 6c3b6ae9). It also adds the later-head test that #9436's Verification asks for. The rule is the owner's Q17 (b), recorded at #9436 comment 6380099108: a cost decision binds to one pull request and covers every head of it.

Fix

• #<number> matching. The pattern is now (?:^|[^A-Za-z0-9_#&/.-])#N(?:[^0-9]|$), the same as namesPull at merge-holds.mjs:436-438 on #9569's head. .#7, -#7, ##7,  and o/r#7 do not name the pull request. The #875 matcher accepted .#7 and -#7.
• Refs target. The hold step consults a Refs issue only when issues.get returns the same number with no pull_request, as merge-holds.mjs:484 does. feat(pr-review-cli): log the job token's api budget and skip a decided cost re-hold #875 checked only that pull_request was absent.
• Test. A decision recorded at head A keeps the cost hold off at head B, where B is a base-only merge and the review holds for cost again. This is #9436's case (apply-review-hold.test.cjs:622).

Differences from the merge-side readers that stay

These are deliberate, and each fails closed at merge:

1. A trusted decision on the pull request itself counts with no link in the body. merge-holds.costDecision and babysit's cost_decision read only a decision linked from the body. A merge still needs that link.
2. A decision on the pull request itself counts even when the linked-issue reads fail. Any failed read on the issue side leaves the cost hold in place.
3. Author check: this step and merge-holds check trusted-actor ids (allowed-author-ids, the human ids in trusted-actors.json), while babysit checks admin permission.

Verification

• apply-review-hold.test.cjs adds cases for the exclusion set, a Refs read that answers another number, and the later-head case.
• node --test .github/scripts/*.test.cjs: 672 pass. biome and actionlint are clean.

Related

• ci-workflows#875
• claude-code-plugins#9569
• claude-code-plugins#9589 (the 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-24T04:56:50.992574Z	9d3ed2d	PR opened

ℹ️ 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 1m 28s —— View job

---

Security review — PR #876

• Fetch diff (.github/actions/apply-review-hold/hold.cjs, .github/scripts/apply-review-hold.test.cjs)
• Review trust boundaries around the changed namesPull matcher and its caller costDecided
• Check for manifest changes (none in this diff)
• Check for instruction-surface deletions (none — this PR only touches hold.cjs/its test, not CLAUDE.md/AGENTS.md/rules files)

Scope: hold.cjs#L135-L142 renames and re-implements the #<number> matcher (names_ → namesPull) used only inside costDecided, plus one new table test.

Analysis: namesPull only matters inside the comment loop at hold.cjs#L191-L197, which is already gated by candidate(c) — the comment must be from a trusted author ID, from a User (not an app), and start with the literal **Decision recorded.** Cost: line. An untrusted actor can't forge a qualifying comment, and the number interpolated into the regex comes from Number(env.PR_NUMBER), not attacker-controlled text, so there's no regex-injection surface. The boundary-character set swap (./- now excluded, #/& now included) only changes which already-trusted decision comments bind to this PR number; it doesn't change who can author one. This is a parity fix with claude-code-plugins' namesPull, not a loosening of who can clear the cost hold.

No new vulnerability is introduced by this change, and GitHub Actions trigger/permission hardening (zizmor's lane) doesn't apply — no workflow YAML, permissions:, or trigger config is touched.

Manifest check: not applicable — no .claude-plugin/plugin.json or marketplace.json in this diff.

No security issues found.

critical-findings: 0
claude-lane-summary: lane=claude-security-review sha=9d3ed2de736240d706f5c95d1dad00b07d31d293 run=40064268897
· branch

[claude]

Claude finished @kyle-sexton's task in 1m 51s —— View job

---

CI code review — PR #876

• Gather context (diff, hold.cjs, test file, REVIEW.md, .claude/rules/)
• Verify the new namesPull regex against the test table in apply-review-hold.test.cjs
• Check for other call sites / leftover references to the old names_ helper
• Check security-lane split, closing-issue criteria, manifest/paved-path triggers
• Post findings

Scope. This PR replaces the #<number> matcher in costDecided's linked-issue path (.github/actions/apply-review-hold/hold.cjs) with namesPull, mirroring claude-code-plugins' merge-holds.mjs reader, and adds a table test covering every case the two patterns previously disagreed on.

Findings: none.

• Manually traced the new regex (?:^|[^A-Za-z0-9_/.-])#${number}(?:[^0-9]|$) against all 12 cases in the new test (approved for #7., #7, (#7), ##7, ``, #70, `fix: pin ShellCheck for actionlint parity #77`, `o/r#7`, `x.ci: scope push trigger and add CI-hygiene parity #7`, `x-ci: scope push trigger and add CI-hygiene parity #7`, `a#7`, `_#7`) — every case resolves to the expected boolean.
• The only call site of the old names_ helper (costDecided, hold.cjs:195) was updated to namesPull(c.body, number); no other reference to names_ remains in the repo.
• number passed into the regex is always an integer from GraphQL/Number()-parsed sources (linkedIssue/refsIssue), so no regex-injection surface — not that this is a security-lane concern here anyway, since pr-review-security.yml exists and owns that split per REVIEW.md.
• No .claude/rules/*.md file carries a paths: glob matching the changed files, so no path-scoped rule applies here.
• No plugin manifest (plugin.json/marketplace.json) touched — manifest checks don't apply.
• Closing issue: no .claude-lane/pr-context.md was present, so closing-issue criteria are unread, not verified as met. The PR body's Refs: melodic-software/claude-code-plugins#9436 links a cross-repo issue for context only, not a same-repo closing issue.
• No cost, trust-gate, or critical-security conditions apply — this touches no model/API spend, no trusted-actor/CODEOWNERS/submodule path, and no security-critical surface.
• Author-claimed verification (node --test, biome, actionlint all clean) is unverified by me — noted as author-claimed only.

claude-lane-summary: lane=claude-review sha=9d3ed2de736240d706f5c95d1dad00b07d31d293 run=40064268897

[github-actions]

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

[github-actions]

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

[claude]

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

---

CI code review — PR #876 (incremental)

• Read incremental diff (.claude-lane/incremental.diff): only .github/scripts/apply-review-hold.test.cjs changed since 9d3ed2d
• Trace the new test against hold.cjs's costDecided/namesPull (unchanged in this increment)
• Check .claude/rules/, REVIEW.md, and PR-context for applicable criteria
• Post findings

Scope. Since this lane's last review (9d3ed2d), only one file changed: a new table test, apply-review-hold.test.cjs:619-639, added by the "a cost decision from an earlier head covers a later one" commit. No production code (hold.cjs) changed in this increment.

Analysis. The new test records a cost decision (decision()) on the pull request itself with a decision body naming head HEAD ("a".repeat(40)), then applies the hold step with HEAD_SHA set to a different head ("b".repeat(40)) and a stale claude-lane-hold marker comment for the old head still present. I traced it against costDecided (hold.cjs:147-202): the first loop over prComments.filter(candidate) matches the decision comment (trusted user, unedited, DECISION_LINE matches) with no comparison to HEAD_SHA at all, so costDecided returns true, cost is dropped from model.criteria, and since no other criterion or path match applies, hold is false and no label/comment write happens — matching the test's assertions (core.outputs.hold === "false", writes empty). This correctly captures the scenario from the linked issue (melodic-software/claude-code-plugins#9436): a cost decision binds to the pull request, not to a specific head, so it survives a rebase/base-merge that changes the head sha. The stale marker comment included in the fixture is inert here (it's never read, since the run returns before re-listing PR comments for posting) but doesn't make the test incorrect.

Findings: none. I could not execute node --test in this environment (sandboxed; command required interactive approval unavailable here), so the author-claimed "671 pass" test-suite result is author-claimed and unverified by me — my check above is a manual trace against the current hold.cjs source, not an execution.

• No .claude/rules/*.md file carries a paths: glob matching the changed test file.
• No plugin manifest (plugin.json/marketplace.json) touched.
• .claude-lane/pr-context.md is not present, so closing-issue criteria are unread, not verified as met; the PR's Refs: melodic-software/claude-code-plugins#9436 line is a cross-repo reference for context only.
• No cost, trust-gate, or critical-security conditions apply to this test-only change.
  · branch

claude-lane-summary: lane=claude-review sha=a6edea3f7989c0d4dc01d9a2e892f9f75053e11e run=40064546026

[claude]

Claude finished @kyle-sexton's task in 29s —— View job

---

Security review — PR #876 (incremental)

• Read .claude-lane/incremental.diff (scope: changes since 9d3ed2d)
• Confirm scope is limited to .github/scripts/apply-review-hold.test.cjs
• Apply skip gate

Scope. Since this lane's last review at 9d3ed2d, the only change is one new table test appended to apply-review-hold.test.cjs (+21 lines), covering the case where a cost decision recorded at an earlier head (HEAD) still covers a later head (later) for the purpose of keeping the cost hold off — exercising the costDecided path already reviewed at 9d3ed2d.

Skip gate: met (item 2 — no security-relevant surface). This diff only adds a test case; it does not modify hold.cjs, any workflow YAML, permissions, manifests, or instruction surfaces (CLAUDE.md/AGENTS.md/rules files). No production logic, trust boundary, or input-handling code changed since the prior review.

No security issues found.

critical-findings: 0
claude-lane-summary: lane=claude-security-review sha=a6edea3f7989c0d4dc01d9a2e892f9f75053e11e run=40064546026
· branch

[claude]

Claude finished @kyle-sexton's task in 1m 58s —— View job

---

CI code review — PR #876 (incremental)

• Read .claude-lane/incremental.diff (scope: changes since a6edea3)
• Trace the namesPull regex change against the new/updated test table rows
• Trace the costDecided Refs-number check against the new "misnumbered" test case
• Check .claude/rules/, closing-issue context, and plugin-manifest items
• Post findings

Scope. Since this lane's last review (a6edea3), the change (commit 5c726a1) touches two things in .github/actions/apply-review-hold/hold.cjs:

1. namesPull: the boundary-character class gained # and &, so ##7 and `` no longer count as naming the pull request.
2. costDecided: the Refs-issue read now also rejects when the read's number doesn't match the requested issue number, not just when it's a pull request.

Plus matching coverage in apply-review-hold.test.cjs: a new misnumbered fake-GitHub option, a new "a Refs read that answers another number" case, and the ##7/`` expectations flipped from true to `false`.

Analysis.

• I hand-traced the new namesPull regex ((?:^|[^A-Za-z0-9_#&/.-])#${number}(?:[^0-9]|$)) against ##7 and `` for number=7: in both strings the character immediately before the candidate `ci: scope push trigger and add CI-hygiene parity #7` match (`#` and `&` respectively) is now itself inside the excluded set, so neither the `^` nor the `[^...]` alternative can anchor the match — both correctly resolve to no match, matching the flipped test expectations. The unaffected cases (`"approved for ci: scope push trigger and add CI-hygiene parity #7."`, `"(ci: scope push trigger and add CI-hygiene parity #7)"`, `"x.ci: scope push trigger and add CI-hygiene parity #7"`, etc.) are untouched by the added characters and still resolve as before.
• In costDecided, the new condition read?.number !== issue.number || read.pull_request != null is safe under short-circuit evaluation: when read is undefined, read?.number !== issue.number is already true, so read.pull_request is never dereferenced on read. I traced the new "misnumbered" test fixture (issues.get returns { number: issue_number + 1 }) through this path: read.number (9437) ≠ issue.number (9436), so the function returns false (no decision read), leaving core.outputs.hold === "true" — matching the test's assertion.
• Both changes are read as parity fixes against claude-code-plugins' merge-holds.mjs namesPull/Refs-number check (per the inline comments citing #9569's head 6c3b6ae9); I did not have a local copy of that file to diff against directly, so I verified internal consistency (regex ↔ tests, logic ↔ tests) rather than cross-repo parity itself.

Findings: none.

• No .claude/rules/*.md file carries a paths: glob (pr-body-contract.md has none), so no path-scoped rule applies to these two files.
• No plugin manifest (plugin.json/marketplace.json) touched.
• .claude-lane/pr-context.md is not present, so closing-issue criteria are unread, not verified as met; this PR's Refs: melodic-software/claude-code-plugins#9436 line is a cross-repo reference for context only.
• No cost, trust-gate, or critical-security conditions apply — this is a non-production-spend, non-trust-gate, non-security-critical logic fix.
• I could not execute node --test in this environment; the author-claimed "671 pass" / biome / actionlint results are author-claimed and unverified by me — my analysis above is a manual trace, not a test run.
  · branch

claude-lane-summary: lane=claude-review sha=5c726a1e830c8251a0a4c8e261de576fe7e5f78b run=40064885651

[claude]

Claude finished @kyle-sexton's task in 56s —— View job

---

Security review — PR #876 (incremental)

• Read .claude-lane/incremental.diff (scope: changes since a6edea3)
• Trace the namesPull boundary-char change and the new costDecided issue-number check
• Check for manifest, instruction-surface, or trust-gate path changes (none)
• Post findings

Scope. Since this lane's last review (a6edea3), two things changed in hold.cjs:

1. namesPull now also excludes # and & from the boundary-character class, so ##7 and `` no longer count as naming the pull request (previously true, now `false`).
2. costDecided now also rejects a Refs-linked issue read whose returned number doesn't match the requested issue.number (read?.number !== issue.number || read.pull_request != null), in addition to the existing pull-request check.

Analysis. Both changes tighten costDecided in the fail-closed direction — they only make it harder to satisfy the conditions that drop the cost hold, never easier:

• The regex change only removes matches that used to be true (##7, ``); every case that previously returned false still does. `namesPull` is only consulted inside the `candidate(c)`-filtered loop (hold.cjs#L194-L199), so the comment author is already a trusted, non-app, unedited `Decision recorded. Cost:` author — this change narrows which of their comments bind to this PR, it doesn't change who can author one.
• The added read?.number !== issue.number check only adds a new way for the Refs-issue read to be rejected (falling through to return false, i.e. the hold stays), matching the upstream merge-holds.mjs reader. No case that was previously rejected is now accepted.

No new trust boundary, injection surface, or authorization gap is introduced; this is a parity/hardening fix, not a loosening. No manifest, instruction-surface, or trust-gate path (.github/standards/trusted-actors/**, .github/actions/check-trusted-trigger/**, CODEOWNERS, .gitmodules) is touched.

No security issues found.

critical-findings: 0
claude-lane-summary: lane=claude-security-review sha=5c726a1e830c8251a0a4c8e261de576fe7e5f78b run=40064885651
· branch