[kyle-sexton]

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

Summary

Lets the pr-review-cli.yml review lane hold a pull request for a person: the review can return a hold, a configured path list forces one, and the lane marks the hold with a label that the caller's required check can enforce. This is PR 3 of the claude-code-plugins#9276 plan (review lanes hold risky PRs with needs-human, ci-status enforces it, clean PRs auto-merge natively). It targets pr-review-cli.yml only, because both review lanes now call it. pr-review.yml and pr-review-security.yml are unchanged.

Fix

• Schema and extract (both legs, identical): an optional hold: {criteria: [cost | trust-gate | critical-security] (1-3, unique), reason: string ≤ 2000}. The extract step strips HTML comments from the reason and carries the hold inside the existing review output. A malformed hold makes the review incomplete, the same rule a malformed comment already follows. One prompt line names the field. The review skills decide when to return it (plan PR 7).
• Inputs: hold-label (default '', which turns holds off) and hold-paths (default '', newline-separated globs converted by the same globToRegExp as the scope step; a test keeps the two copies identical).
• New status-job step, "Apply the hold" (.github/actions/apply-review-hold). It runs after "Publish the review" with always(), only in comments mode and only when hold-label is set:
  • It takes the model's criteria only from the leg that counts and only when that leg's review is complete (publish-review's judge).
  • A fresh pulls.listFiles adds trust-gate when any changed file, or a renamed file's old path, matches hold-paths. This applies whatever the model says, including when the review failed. A file list shorter than changed_files also holds.
  • Add-only: it adds the label with issues.addLabels and posts one marker comment, <!-- claude-lane-hold lane=<lane> sha=<head> criteria=<list> -->, followed by the reason and a claude-lane: footer. A rerun does not repeat the marker. It never removes, sets or replaces a label.
  • A failed label write or comment write fails the job (red). The step attempts both writes before failing. A failed file listing counts as a trust-gate hold: the step applies it, then fails.
  • Matched file names in the comment lose backticks and line breaks, and @ mentions in them are broken.
  • The reason is model text, so it goes through stripHtml and the fetched publish gate. A credential-shaped reason, or a gate that cannot load, has its text withheld, and the label is still added.
• Outputs: workflow outputs hold (true/false, false when the step did not run) and hold-criteria.
• Permissions: no change for callers. The status job already holds pull-requests: write. The fine-grained permission table lists POST /repos/{owner}/{repo}/issues/{issue_number}/labels under "Pull requests: write" (docs), and the plan's sandbox canary probes it with the job token. Create the label in the repository first: whether this grant can create a missing label is unverified. The standards runner-policy contract for this SHA (allowedInputs plus hold-label and hold-paths) follows as plan PR 4.
• Checks stay advisory: neither check becomes required. The caller opts in and enforces the label in its own required check (ci-status), under the ADR 0011 supersession tracked in the plan.
• Summary (shadow) mode writes nothing. The SECURITY MODEL header and the README describe the hold.

Verification

• node --test .github/scripts/*.test.cjs: 628 pass, 0 fail (.github/scripts/apply-review-hold.test.cjs is new; .github/scripts/pr-review-cli.test.cjs gains hold cases). node --test .github/actions/report-lane-outcome/*.test.cjs .github/actions/run-claude-cli/*.test.cjs: 146 pass.
• Pinned cases:
  • Add-only: the fake GitHub throws on removeLabel, removeAllLabels, setLabels, deleteLabel and updateLabel, and a static check finds none of them in hold.cjs.
  • A label-write failure fails the step (and the marker is still tried). A comment or listing failure fails it too.
  • A path match forces trust-gate, including a failed review, a renamed file's old path, and root CODEOWNERS matched by **/CODEOWNERS.
  • An empty hold-label makes no API call and outputs hold=false.
  • The step condition requires comments mode and always().
  • The largest review, with a 2000-character hold, still fits the 100,000-byte output cap.
• Mutation check: removing the label-failure error, or the trust-gate add, turns 1 and 6 tests red.
• actionlint: clean. zizmor --offline on the workflow and the new action: no findings. biome check and markdownlint-cli2 README.md: clean. typos: clean.
• A review:security-reviewer subagent reviewed the diff before the push. Fixed: a failed file listing no longer drops the hold, and file names in the comment are sanitized. Left to the enforcing side, by design: whenever the status job is skipped (fork, Dependabot, an unlisted bot) or the review fails, no label is added. The plan's ci-status step (PR 6) requires both review checks to be green on the head, so a missing or red lane holds the merge anyway. Who removed the label is also for the enforcing check to judge.

Related

• Refs: feat(pr-pipeline): review lanes hold a risky PR with needs-human, ci-status enforces it, and clean PRs auto-merge natively claude-code-plugins#9276 (plan PR 3; PR 4 adds the standards runner-policy contract, PR 6 bumps the callers with hold-label: needs-human and the trust-gate hold-paths).
• Builds on feat(review): add pr-review-cli, a review lane on run-claude-cli #862 (pr-review-cli).

🤖 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-23T04:47:27.370503Z	8a90441	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 3m 48s —— View job

---

Security review in progress

• Gather context (diff, PR description, changed files)
• Skip-gate check (change has security-relevant surface: new GitHub label/comment-writing action; proceeding)
• Manual pass over apply-review-hold (hold.cjs, action.yml) and publish-review/publish.cjs
• Manual pass over pr-review-cli.yml workflow changes (schema, status job wiring)
• Adversarial verification via two parallel hunter agents (hold.cjs trust boundaries; YAML wiring/schema divergence)
• Post findings / summary

[claude]

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

---

Review complete

Reviewed the new apply-review-hold action, its tests, and the pr-review-cli.yml changes (schema/extract additions for the optional hold field, the new status-job "Apply the hold" step and outputs).

1 finding posted inline (.github/actions/apply-review-hold/hold.cjs:146-148): the hold-reason secret scan discards scan()'s .warnings, so a warn-only secret-shaped hold reason is posted to the public PR comment with no core.warning logged — unlike publish.cjs's own publish(), which logs every warn-only hit before publishing anyway. Suggested a one-line fix inline.

Things I checked and found sound:

• Add-only guarantee: no label-removal API is named in hold.cjs (matches the static check in the new test file).
• Path-matching (trust-gate) is independent of the model's review outcome, including a failed/incomplete review, matching the pinned test cases.
• A failed listFiles call correctly still forces trust-gate and then fails the job; both writes are attempted before the step throws.
• globToRegExp in hold.cjs is a byte-for-byte copy of the scope step's version, and a test keeps the two in sync.
• The rerun marker dedup checks github-actions[bot], consistent with the existing marker-dedup pattern elsewhere in this workflow (not a new bot identity).
• Output defaulting (hold → 'false' when the step is skipped) is correct and matches its own test.
• The extract step's duplicated hold-criteria/schema blocks between review-key and review-subscription follow the file's existing per-leg duplication pattern; not new scope creep.

Not evaluated: pr-review.yml and pr-review-security.yml are untouched, as the PR states, and this lane only targets pr-review-cli.yml. No closing issue is linked (the PR opens with Refs:, not Closes/Fixes), so no closing-issue acceptance criteria applied. The repo's standards index (docs/standards/README.md) has no paved-path-tagged row, so no paved-path criteria applied either.

Author-claimed and unverified (I did not run them myself — the sandbox here didn't allow invoking node --test): the "628 pass, 0 fail" test run, actionlint/zizmor/biome/markdownlint-cli2/typos results, and the prior security-reviewer subagent pass mentioned in the PR body.

claude-lane-summary: lane=claude-review sha=8a90441cdcf990a1f0cf277fb19bbab0c07b8933 run=39902079656

[github-actions]

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

[claude]

Claude finished @kyle-sexton's task in 3m 59s —— View job

---

Code review complete

• Gather context (diff, PR description, rule files, standards index)
• Skip-gate check (open, not draft, head advanced past the last reviewed commit — reviewing fresh)
• Review changed files against CLAUDE.md / REVIEW.md / path-scoped rules
• Check closing-issue criteria (none linked; PR opens with Refs:, consistent with .claude/rules/pr-body-contract.md)
• Adversarial validation of candidate findings
• Post findings / summary

0 findings. No inline comments posted.

The head (3649a41) has moved three commits past the last lane review (8a90441). The only finding from that prior pass — .github/actions/apply-review-hold/hold.cjs's hold-reason secret scan dropping scan()'s .warnings so a warn-only secret-shaped hit was posted with no core.warning logged — is fixed in 3649a41 exactly as suggested: the loop over scanned.warnings now runs before the withheld check, and a new test (.github/scripts/apply-review-hold.test.cjs's "a warn-only hit in the reason posts it with a warning") pins it.

Re-checked on this pass, nothing new found:

• Schema/extract duplication between the review-key and review-subscription jobs (pr-review-cli.yml#L810, #L876) follows this file's existing per-leg pattern; the repo's standards index (docs/standards/README.md) has no paved-path-tagged row, so no paved-path finding applies, and the duplication is consolidated via the cross-checking test rather than left to drift.
• judge(env) in publish.cjs reads SUB_RESULT, *_PRESENT, *_FAILED, *_CLASS, *_VERDICT, *_REVIEW and KEY_LANE_CLASS; the new "Apply the hold" step (pr-review-cli.yml#L945-969) passes every one of them through env, so modelHold's leg selection can't silently read stale/undefined fields.
• Output wiring (apply-review-hold/action.yml's hold/criteria outputs → the step's steps.hold.outputs.* → the job's hold/hold-criteria outputs, with the || 'false' default only on hold) is internally consistent and matches the pinned wiring test.
• continue-on-error: ${{ inputs.publish != 'comments' }} on the hold step is unreachable-true in the branch where the step actually runs (the if: already requires publish == 'comments'), but that's the tested, intended shape (a failed write must fail the job in comments mode) — not a bug.
• globToRegExp in hold.cjs stays byte-for-byte identical to the scope step's copy per the pinned sync test; $/.github/actions/apply-review-hold and the actions/github-script@3a2844b... pin match this file's existing conventions.

Author-claimed and unverified (sandboxed node --test invocation here required approval I don't have in this lane): the "628 pass, 0 fail" run, actionlint/zizmor/biome/markdownlint-cli2/typos results, and the prior security-reviewer subagent pass.

claude-lane-summary: lane=claude-review sha=3649a4160b1f6b80cb1910845b54db20461db2e4 run=39903610229
(branch)

[claude]

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

---

Security review complete

Scope of this run: HEAD moved from 8a90441 (last reviewed) to 3649a41. The only new change on this branch is commit 3649a41, which fixes the one finding from the prior review. The intervening commit 83b739f (#865) and merge 127e1c8 only bring in a change already merged to origin/main (notice titling / demoted-CRITICAL counting in publish.cjs), unrelated to this PR's own surface and already reviewed separately.

Prior finding — fixed. hold.cjs:146-152 now destructures scan()'s full result and logs every warnings[] entry via core.warning before applying the withheld/reason logic, matching publish.cjs's own publish() pattern. A warn-only secret-shaped hold reason (e.g. password = "...") now leaves an audit trail instead of being posted silently. Confirmed against scan()'s contract in publish.cjs ({ withheld, warnings }, warnings always an array) and the new pinned test (apply-review-hold.test.cjs: "a warn-only hit in the reason posts it with a warning").

No new security issues found in the delta. No manifest changes in this diff (no .claude-plugin/plugin.json or marketplace.json touched), so no manifest <field>: lines apply.

critical-findings: 0

claude-lane-summary: lane=claude-security-review sha=3649a4160b1f6b80cb1910845b54db20461db2e4 run=39903610229

[github-actions]

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