[kyle-sexton]

Closes #9403

Summary

A review lane re-applies its cost hold on every new head, so needs-human came back after the owner had already recorded the cost decision (#9332). ci-status now clears such a hold itself: the owner's recorded decision is enough for a cost-only hold, and trust-gate and critical-security holds still need a person.

Fix

• A new ci-status step, Clear a cost-only review hold the owner decided (id cost_hold), runs before the human-gated step. It clears the hold only when all of these are true in the window that opens at a trusted person's last removal of the label:
  • only github-actions[bot] or claude[bot] added the label, and each time a held review finished within ten minutes;
  • every hold a review lane recorded has criteria cost alone. The step reads each hold from the claude-lane-hold ... criteria= notice on the lane's status-job check run (every attempt), not from the hold comment, which anyone with write access can edit or delete. Jobs and annotations are read across every page. A pull_request run takes its workflow from the PR, so a lane run whose head changes pr-review-hosted.yml or pr-review-security-hosted.yml from the base keeps the hold, as does a comparison that lists 300 files (the API's cap) or does not parse;
  • the lanes App wrote no comment and no comment was deleted;
  • outside HTML comments, the body links an owner decision under babysit's cost_decision author rule: a trusted-actor human, not posted through an App, never edited, first line **Decision recorded.** Cost:. The decision sits on the PR itself, or (written after the PR) on the one issue GitHub's closingIssuesReferences records the PR as closing, as babysit's cost_decision reads it. A Refs: line or a second closing issue counts for nothing, so an owner decision on an unrelated issue cannot clear the hold.
• When it clears, it posts one comment citing the decision and removes the label with the job token. The hold-lift step skips on cleared=true. An unreadable input clears nothing, so the existing steps judge the label as before.
• ci-status gains checks: read for the annotations, and its timeout goes from 7 to 9 minutes.

Verification

• scripts/ci-hold-cost-decision.test.sh (new): 69 pass, including that a PR with no lane label event stops after the label-event read and one with no link in its body stops before any run or GraphQL read. Each case says whether the hold clears and, for a kept hold, why. A hold comment edited to read cost over a trust-gate notice is kept: the first version of this step cleared it, and this test fails against that version. A decision on an unrelated issue the body names on a Refs: line, or on one of two closing issues, clears nothing: both cases fail against eb5c79e. A lane run on a head that changes a lane workflow, a capped or unparseable comparison, and unpaginated jobs or annotations reads each keep the hold, and all four fail against c77199e.
• scripts/ci-hold-lifted-by.test.sh (33 pass) and scripts/ci-hold-human-gated.test.sh (29 pass).
• feat(pr-pipeline): restore pr-refine as the refine stage for fix-docs #9332 links its decision issue feat(pr-pipeline): restore Refine as its own stage that tidies every PR before review #9306 only through Refs: and closes no issue, so this step no longer clears its hold; a person lifts it.
• scripts/check-affected.sh --tests in the scripts/setup-env.sh venv: 10 passed, 4 skipped. actionlint is clean, and zizmor reports no findings.
• A review:security-reviewer pass on the first version found that hold comments can be edited or deleted. Both are fixed here.

Related

• feat(review): return a hold only for the cost, trust-gate and critical-security classes #9342, feat(pr-pipeline): turn on review-lane holds and require both review lanes in ci-status #9361, feat(source-control): hold a PR that could add or raise a charge for an owner decision #9224
• The step accepts what babysit's cost_decision (plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py) accepts: the PR itself or its one closing issue.
• Residual risk: the author still picks the closing issue, so Closes #M makes a decision on #M count. Merging then closes #M, which is visible.
• Residual risk: ci-status itself runs from the PR head, so a head that edits pr-require-checks.yml can change this step. That trust posture is unchanged.

🤖 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-23T13:56:58.724842Z	f3eb71b	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.

[github-actions]

claude-security-review: 1 inline finding(s) on f3eb71b01eced384ec6f674e8e5d4f9c1ae30d39; the summary is in this lane's pull request comment.

claude-lane: claude-security-review

[github-actions]

claude-security-review reviewed 258792331a8d5fd1991eb08e9493acf214ad4fe3: 0 finding(s), 0 as inline comments.

Incremental security review of .github/workflows/pr-require-checks.yml and scripts/ci-hold-cost-decision.test.sh since c77199e. I found no security issues.

critical-findings: 0

What changed. The cost-hold clearing step now does three more things:

• It compares BASE_SHA...head_sha for each lane run in the window. It keeps the hold if the head changes pr-review-hosted.yml or pr-review-security-hosted.yml, if the comparison lists 300 files or more, or if the comparison is unreadable or has no file list.
• It paginates the jobs and annotations reads.
• It extends the tests to cover these cases.

Checks I made.

• Forged notice. Both lane workflows take their local actions from ./.base, a checkout of main. The review itself is a reusable workflow pinned to a SHA. So changing the two lane workflow files is the only route I found by which a PR head could alter the code that writes the claude-lane-hold notice. The new comparison covers those two files.
• Fail-safe errors. An unreadable compare, a missing file list, a head_sha that is not 40 hex characters, and the 300-file cap all call keep. Each keeps the hold. In the jq filter, the empty-string result means "no change" and any other string means "keep".
• Process substitution. done < <(jq …) leaves the loop in the current shell, so keep still stops the whole step.
• Pagination. inputs.jobs[] and inputs[] iterate every page of a paginated result, so a hold notice on a later page is now seen. Under-reading pages was the earlier bypass.
• Inputs. $sha is regex-validated before it goes into the API path, and $BASE_SHA is validated earlier, so there is no injection surface.
• Tests. The compare stub and the new cases (lane workflow change, file cap, unparseable comparison, pagination, the compare read failing) only exercise the control. Nothing in them is a security concern.

The PR description already names two residual risks: ci-status runs from the PR head, and the author chooses the closing issue. Neither is new in this increment, and I do not report them again.

No manifest fields or plugin manifests are touched. The change is not under a trust-gate path, and nothing here is CRITICAL, so I set no hold.

[github-actions]

claude-review reviewed 258792331a8d5fd1991eb08e9493acf214ad4fe3: 0 finding(s), 0 as inline comments.

Incremental review of c77199e..2587923 (.github/workflows/pr-require-checks.yml, scripts/ci-hold-cost-decision.test.sh). I found no issues. I read the diff and the surrounding step. I did not run the tests, and the PR's pass counts are author-claimed and unverified.

What changed:

• Each lane run's head SHA is now compared to the base (compare/$BASE_SHA...$sha). The hold is kept when that head changes pr-review-hosted.yml or pr-review-security-hosted.yml, when the comparison lists 300 or more files, or when the comparison is unreadable or has no file list.
• The jobs and annotations reads now use --paginate.
• The tests cover each new keep path and the paginated reads, and compare is added to the unreadable-read loop.

Checked and sound:

• Loop and keep: The while read … done < <(jq …) form keeps keep's exit 0 in the main shell, so it still ends the step. The gh api calls inside the loop don't read the loop's stdin.
• Fail-closed paths: A malformed head SHA, a 404 from a garbage-collected head, or a compare response without .files all keep the hold. jq -e with the empty-string result passes as intended.
• Caching: The vetted list avoids repeat compares for the same SHA.
• Workflow scope: Both lane workflows load their local actions from ./.base/.github/actions/... and call a pinned reusable workflow in ci-workflows. So the head's own edits to the two lane workflow files are the only head-controlled workflow code in a lane run, and the new check covers exactly those.
• Tests: The stub's compare case matches $BASE_SHA...<sha> and BASE_SHA reaches the stub through the environment. The new cases (workflow change, 300-file cap, no file list, unreadable compare) each exercise a distinct keep path. The pagination assertion checks the logged gh api arguments rather than real paging, which is fine for a stub.

No hold: the change adds no model or API spend, touches no trust-gate paths, and raises no critical security finding.

[github-actions]

claude-security-review: 2 inline finding(s) on c77199e76079367a2cfd44317b80f081d2b1b853; the summary is in this lane's pull request comment.

claude-lane: claude-security-review

[github-actions]

The pr-verify verifier failed head 258792331a8d5fd1991eb08e9493acf214ad4fe3. What it wrote, as data:

Summary: #9403 asks that a review lane's `cost` hold not stick when the PR's closing or linked issue has an owner-authored `**Decision recorded.** Cost:` comment newer than the PR's createdAt. The new ci-status step only clears the hold when the PR body links that comment, and it ignores linked (`Refs:`) issues. A decision recorded on the closing issue, which is the #9224 rule the issue cites, leaves the PR held unless someone also edits the body to cite it. The PR body itself says the motivating case, #9332, is still not fixed. The rest of the step is careful and well tested: it fails closed, keeps trust-gate and critical-security holds, reads holds from notices rather than comments, and vets lane workflows. But the done-when condition is only met in a narrower case than the issue asks for.
- The review comments on the PR are the owner's (author_id 153232337) and explain design choices. They are context, not spec. I saw no text trying to steer the reviewer.
- .github/workflows/pr-require-checks.yml:124: the step finds a decision only through a `#issuecomment-` link in the PR body (`[ -n "$links" ] || keep "the body links no comment"`). The spec counts a decision when the closing issue carries the comment, with no body citation. A decision must also be newer than the PR's createdAt, so a decision on the closing issue never clears the hold unless the body is edited afterwards. Fix: read the comments of the one closing issue (and of the PR) directly, and apply the same author, unedited, first-line and newer-than-createdAt checks, instead of requiring a body link.
- .github/workflows/pr-require-checks.yml:133: only GitHub's `closingIssuesReferences` (totalCount == 1) counts, and a `Refs:`-linked issue never does. The spec says "closing or linked issue", and its motivating case #9332 links #9306 only through `Refs:`. The PR body admits #9332 stays held. Either support a linked issue in a way that resists the unrelated-issue exploit (for example, require the issue to be referenced in the PR body outside HTML comments and the decision to be newer than createdAt, as the spec states), or get #9403's done-when amended to closing-issue-only before merging this narrowing.
- .github/workflows/pr-require-checks.yml:146: owner identity is any `human` on the base trusted-actor list, not the single owner account the spec names. The PR says this matches babysit's cost_decision rule, which the spec accepts. I could not verify that against babysit_merge_policy.py.
- .github/workflows/pr-require-checks.yml:3117: ci-status gains `checks: read` and its timeout goes from 7 to 9 minutes. Both serve the new step and look proportionate.
- .github/workflows/pr-require-checks.yml:236: the hold-lift step now skips when cleared=true. That also skips it when an untrusted account removed a cost-only hold that the owner's decision covers. This seems to be intended, but a human should confirm it.

[github-actions]

The pr-verify verifier failed head 258792331a8d5fd1991eb08e9493acf214ad4fe3. What it wrote about this file, as data:

- scripts/ci-hold-cost-decision.test.sh:437: the positive case only covers a body that cites the decision link. Add a case where the closing issue carries the owner's decision and the body has no link (expected: cleared), plus whatever linked-issue case the spec ends up requiring.