[kyle-sexton]

Refs: #10610

Summary

A work-resolve-issue PR got Hold: a human merges this when the runner's checks did not report. #10610 is an example. That line fails ci-status itself: the merge-holds step reports "its body holds it" on run 40732056082. pr-verify then never runs, because pr-verify.yml runs only on a pr-require-checks run that concluded success (pr-verify.yml:73-74). So nothing a lane does could ever lift the hold, and the PR waited on a person for checks CI runs anyway.

Runner checks that did not report are covered: ci-status reruns them on the head. The reuse leak scan is not covered that way, so it keeps holding on any word but clean. lint-repo's gitleaks honors a gitleaks:allow comment or .gitleaksignore the branch itself can add. Its generic-api-key rule needs 10+ word-class characters at entropy 3.5, where scanLeak flags 8+ of any non-quote character. It also reads only the commits' patches, not every changed file's bytes (464a7e6).

Fix

mustHold in report-outcome.mjs, reason by reason:

Reason	Before	Now
humanMerge (trusted merge: human)	hold	hold
issue labeled untrusted-provenance	hold	hold
denied-path list unreadable	hold	hold
changed-file list unreadable (compare failed)	hold	hold
listing at the 300-file cap while a path is denied	hold	hold
a changed path is denied	hold	hold
a runner check ran and failed	hold	hold
reuse leak scan anything but clean (found, failed, unknown, missing, or no checks record)	hold	hold
no reuse: runner checks missing, unreadable or for another tip	hold	no line
reuse with a clean scan: no runner check reported	hold	no line

• The rule is Boolean(checks?.results.some(({ exit }) => exit !== 0)) || (reused && checks?.scan !== "clean"), in place of !checks || checkFailures(checks).length > 0 || (reused && checks.scan !== "clean").
• A body with no hold says Left out: ...; ci-status, pr-verify and the security review lane check the head before it merges. (report-outcome.mjs:825).
• HOLD_LINE text is unchanged, so every reader (ci-status, enable-auto-merge, check-wip-cap, fix-ci-dispatch, apply-patch) treats the remaining holds as it did.
• Docs: docs/conventions/pr-pipeline/README.md (trust hold section) and pr-run-activity.md (reuse hold conditions). Changelog fragment .changes/pr-pipeline/.

Verification

• report-outcome.test.mjs: 285 pass, 0 fail. The mustHold test (report-outcome.test.mjs:3294) pins: a clean reuse does not hold; on a reuse, scan missing, checks and scan missing, scan failed and scan found each hold; a failed check holds; a reuse with a clean scan and no runner check reported does not hold; no reuse with runner checks missing does not hold.
• test_merge_holds_sync.py: 14 passed. python3 -I scripts/sync-hold-floor.py --check exits 0.
• check-changelog-fragments.sh --check: valid.

Left out:

• refactor(planning): make apply ops the one write path for interview page text #10610 keeps its current line and needs-human label. This change affects PRs opened after it merges.

Related

• refactor(planning): make apply ops the one write path for interview page text #10610
• fix(pr-pipeline): hold reuse PRs on real conditions and leak-scan the reused branch #10624 (introduced the shared mustHold)

🤖 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-28T18:55:44.820246Z	f2aefdb	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 f2aefdbc9a0d540bb89e7d3955249ea61c380e82; the summary is in this lane's pull request comment.

claude-lane: claude-security-review

[github-actions]

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

Security review: #10701 (mustHold in work-resolve-issue/report-outcome.mjs)

critical-findings: 0

No security findings. I read the whole diff, mustHold, checkFailures, decide() and reusePlan() at head 8363f91.

Trust-control check. This change narrows a merge hold, because work-resolve-issue/ is a module the merge-hold scripts import. I checked the code, not the PR text, to see what the narrowing lets through.

• Narrowing 1, main path (decide()). !checks no longer holds. This changes nothing there. decide() already stops at checkFailures(checks) when checks === null (report-outcome.mjs:988-1003, via routeStop("check-failed")), so mustHold at :1020 never sees a null record.
• Narrowing 2, reuse path (reusePlan()). A reused branch with a clean leak scan and an empty results list no longer holds (report-outcome.mjs:721-722). Before, !checks and a null checks record both held.
• What the dropped condition guards. It guarded the runner's lint, fragment and similar checks, which are quality gates. It did not guard the leak scan.
• What still holds. These conditions are unchanged:
  • humanMerge and the untrusted-provenance label.
  • Unreadable denied-path or changed-file lists.
  • A listing cut at the 300-file cap while some path is denied.
  • Any changed path matching a denied glob.
  • Any runner check with a non-zero exit.
  • On a reuse, any scan word other than clean, including a missing record (checks?.scan !== "clean").
• Leak-scan gap. I read the PR's note that lint-repo's gitleaks has a weaker rule than the lane scan, and the diff keeps the lane scan as a hold. That gap is not reopened.
• Part 3 of the severity bar. The unreported-check case is left to ci-status, pr-verify and the security review lane, as the new body text says. The hold-path routing for denied files is untouched. I did not trace the ci-status rerun end to end; I took it from the PR description and the matching merge-holds.mjs / pr-merge.yml references to HOLD_LINE. The dropped hold guarded quality, not a token, privilege or code-execution path, so it does not meet the CRITICAL bar either way.

Other items.

• The HOLD_LINE text is unchanged, so the other readers (enable-auto-merge, check-wip-cap, fix-ci-dispatch, apply-patch) behave as before for the holds that remain.
• No manifest, token scope, kill-switch or trusted-actor file is touched.
• No embedded instructions in the PR context.
• Spend: none.

[github-actions]

claude-review reviewed 8363f9116036aa7d03d7e947a2a210700cdf3c22: 0 finding(s), 0 as inline comments.

No findings. I read the whole diff, the PR context, and report-outcome.mjs around mustHold, reusePlan and decide. I did not run the tests; the pass counts in the PR description are the author's claims and I did not verify them.

• mustHold rewrite: Boolean(checks?.results.some(({ exit }) => exit !== 0)) || (reused && checks?.scan !== "clean") keeps every hold the table in the description says is kept. A missing or null checks record on a reuse still holds through the scan !== "clean" branch, so a reuse cannot clear the hold without a clean scan on a validated record.
• Non-reuse path: decide() already routes checks === null and failed checks to the check-failed stop at report-outcome.mjs:988-1003, before mustHold runs at line 1020. Dropping !checks from mustHold therefore changes only the reuse case and cannot open a PR that was previously stopped.
• checkFailures and checksPassed: mustHold no longer calls them, but they are still used at lines 915 and 988. The resumed-branch gate (checksPassed) still requires at least one check to have run.
• Body text: The no-hold Left out: line is accurate for the cases that now reach it. The tests cover that line, the absence of the hold line, and the mustHold truth table, including the clean reuse with no check results and the no-reuse case with null checks.
• Docs and fragment: They match the code. The fragment's bump: patch fits a behaviour fix.

The change only narrows when the hold line is written. It does not touch the HOLD_LINE text, so the other readers see the remaining holds as before.

There is no closing issue (the PR says Refs: #10610), so there are no acceptance criteria to check. Nothing here can cause a real charge, so there is no hold.

[github-actions]

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

claude-lane: claude-review

[github-actions]

The pr-verify verifier failed head 75c93c6e9266048cc195eb69cf9c4973b6f97039. What it wrote, as data:

Summary: PR #10701 has no linked issue, so its own title and body are the spec (`Refs: #10610` points at a PR). The body says a reuse leak scan that is `failed`, unknown or missing writes no hold line, and gives the rule as `checks?.scan === "found"`. The diff instead keeps `reused && checks?.scan !== "clean"`. On a reuse branch, a missing checks record also means a missing scan, so a reuse PR whose runner checks are missing or for another tip still holds. That is the #10610 case the title says it fixes. The diff's stricter behaviour is defensible: the author's later review comment says gitleaks does not cover what scanLeak flags. But the diff and the spec now contradict each other, and the body's table, rule and verification claims describe code that is not in the PR. Nothing in the diff loosens safety.
- PR body: the spec does not match the diff, so the spec is not delivered as written. The body's table says 'reuse leak scan failed, unknown word or missing: no line' and gives the rule `(reused && checks?.scan === "found")`. `.github/scripts/work-resolve-issue/report-outcome.mjs:722` uses `checks?.scan !== "clean"`. Fix this by rewriting the body (title, Summary, Fix table, rule bullet, Verification) to describe the narrower change. Do not loosen the code: the stricter scan hold is the safer direction, and the author's review comment explains why gitleaks does not cover it.
- PR body: the 'A lane leak scan that did not report is already covered by a required, deterministic control' paragraph contradicts the new code comment at report-outcome.mjs:707-712 and the docs. Remove it or correct it.
- PR body: the Verification text says the mustHold test pins 'scan missing does not hold, checks and scan missing do not hold'. The test in `.github/scripts/work-resolve-issue/report-outcome.test.mjs:95-106` asserts the opposite: both cases hold. Update the text to match the tests.
- PR title / body: on a reused branch, `checks: null` (runner checks missing or for another tip) still holds, because the scan is missing too. So the #10610 case named as the motivating example is still held after this change. Say so plainly in the body, and narrow the title to what the change delivers: non-reuse runs, plus reuse runs with a clean scan and no runner result.
- .github/scripts/work-resolve-issue/report-outcome.mjs:721: the code change itself is correct. `checks?.results.some(...)` with `Boolean` covers a null checks record, and a failed runner check still holds. The non-reuse missing-checks case now writes no hold line, and `ci-status` still runs the checks on the head.

This pull request leaves the merge queue, and ci-status holds it out while it carries pr-verify-failed. A pr-verify pass at a new commit removes the label.

[github-actions]

Post-merge mutation testing

• Range: 308dec1..c9cc7ea
• Merged pull request: #10701
• Outcome: refused

Killed 0, survived 0, timeout 0, no coverage 0, invalid 0, not run 5.
Covered-code score n/a; mutation score n/a.

File	Killed	Survived	Timeout	No coverage	Invalid	Not run
.github/scripts/work-resolve-issue/report-outcome.mjs	0	0	0	0	0	5

The run's summary has the full report.