Skip to content

feat(pr-pipeline): hold lanes App human-merge pull requests with a label only a listed person lifts - #11036

Merged
kyle-sexton merged 4 commits into
mainfrom
feat/9437-human-merge-hold-label
Oct 29, 2026
Merged

kyle-sexton merged 4 commits into
mainfrom
feat/9437-human-merge-hold-label

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Closes #9437

Summary

A lanes App pull request that holds for a person said so only in its body (Hold: a human merges this), which the App can edit. report-outcome now also labels it human-merge-hold, and ci-status fails while that label is on and unless a person on the trusted-actor list removed it. The coordinator chose a dedicated label (option a) over reusing needs-human under the owner's standing direction, because the lanes App adds and clears needs-human itself (report-escalation, clear-escalations), so counting its needs-human as a hold would turn ci-status red after every automatic clear.

Fix

  • report-outcome.mjs: plans carry hold; a held PR gets human-merge-hold right after it opens, or when the run reuses its open PR. A PR the label cannot reach (after the resend) is closed and the issue retried under the new hold-label-failed stop route.
  • pr-require-checks.yml ci-status: new step Fail a human-merge hold a listed person has not lifted. It fails while the label is on. Once the label is off, it fails unless the first removal at or after the newest add (by any account) came from a human on the base trusted-actor list, matched by numeric id and not made through an App. It reads no other step's outcome, so neither the human-gated step nor a cost decision lifts it, and it fails closed on unreadable labels, events or actor list.
  • enable-auto-merge.mjs HOLD_LABELS, merge-holds.mjs labelHolds (the merge queue) and babysit_merge_policy.MERGE_HOLD_LABELS hold the label.
  • gate-source.mjs is unchanged: option (a) leaves the needs-human hold-bot list alone, so there is nothing to mirror there.

Verification

  • Red first: the new report-outcome, enable-auto-merge and merge-holds cases failed before the change. scripts/ci-hold-human-merge.test.sh against main's workflow fails with no such step.
  • node --test over .github/scripts/pr-merge and .github/scripts/work-resolve-issue: all pass. The scripts/ci-hold-*.test.sh suites: 384 pass, 0 fail (23 new). The babysit test_merge_holds_sync.py, test_babysit_merge_policy.py and test_babysit_merge_prefilter.py suites pass. actionlint is clean, and scripts/check-affected.sh passed 15 checks.

Each refined criterion (intake refinement, 2026-10-26), and what meets it under option (a):

Refined criterion Met by
report-outcome adds the human-gated label (needs-human) on a human-merge hold Replaced: it adds human-merge-hold; report-outcome.test.mjs "main labels a held PR human-merge-hold right after it opens" and "... it reuses"
a non-human-merge PR gets no label "main puts no human-merge-hold label on a PR that does not hold"
the lift step counts a lanes App labeled event as a hold Replaced: a separate ci-status step counts any add of human-merge-hold as a hold
ci-hold-lifted-by.test.sh: a non-listed removal exits 1 ci-hold-human-merge.test.sh "an account off the list removing it fails ..." and "the lanes App removing its own label fails"
ci-hold-lifted-by.test.sh: a listed person's removal exits 0 ci-hold-human-merge.test.sh "a trusted person's removal lifts it"
enable-auto-merge: Hold line deleted, label present, not armed enable-auto-merge.test.mjs "a human-merge-hold label holds a lanes App PR whose body lost its hold line or its Refs line" (PR 1)
enable-auto-merge: Refs rewritten to Closes, label present, not armed the same test (PR 2); holdReason returns it carries human-merge-hold
existing suites pass as above

Left out: the issue's refined criteria still name needs-human; amending them is the owner's call. The label is not created in the repository by this PR. The first labels POST is expected to create it (UNVERIFIED).

Related

🤖 Generated with Claude Code

kyle-sexton and others added 4 commits October 29, 2026 15:37
…bel only a listed person lifts

report-outcome labels a pull request it holds for a person
human-merge-hold right after it opens or reuses it, and closes and retries
one it cannot label (hold-label-failed). A new ci-status step fails while
the label is on and, once it is off, unless the first removal at or after
its newest add was a trusted-actor human acting directly. enable-auto-merge,
the merge queue's labelHolds and the babysit merge lane hold the label too,
so deleting the body's hold line or rewriting Refs to Closes no longer lets
a held pull request merge.

Closes #9437

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…comments

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 29, 2026 20:00
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

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-29T20:02:42.445475Z a221573 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

Copy link
Copy Markdown
Contributor

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

I found nothing to block or flag in this change. I read the whole diff (14 files) and the PR context, and I ran no tests myself. The author's test counts are theirs and unverified.

Closing issue #9437 (checked against its acceptance criteria):

  • A human-merge lanes App PR carries the label from opening. report-outcome.mjs adds human-merge-hold right after the PR opens or when it reuses its open PR. The new report-outcome tests pin both paths.
  • Neither the hold-line deletion nor the Refs: to Closes rewrite arms it, and ci-status stays red until a listed human removes the label. HOLD_LABELS in enable-auto-merge.mjs now holds the label, and the new enable-auto-merge test covers both body edits. The new ci-status step fails while the label is on. Once it is off, it fails unless the first removal at or after the newest add came from a human on the base trusted-actor list, matched by numeric id and not made through an App. scripts/ci-hold-human-merge.test.sh covers the lift, App, stranger, fail-closed and re-add cases.
  • Tests pin both cases. They do.
  • The PR swaps the issue's needs-human for a dedicated label, and it says why: the lanes App adds and clears needs-human itself. That choice is the PR's, so the issue's "reuse the needs-human lift step" wording is not met literally. The PR records the deviation openly, and the label does the same job.

Spot checks

  • The label list is mirrored in merge-holds.mjs labelHolds and in babysit_merge_policy.MERGE_HOLD_LABELS. I found no other non-test list of hold labels that needs the new name. check-wip-cap and pr-run-activity-write.yml keep their own lists, which this change doesn't touch.
  • The new hold-label-failed stop route has the same retry shape as pr-head-moved.
  • The new step reads no other step's outcome, so the human-gated and cost steps can't lift it.

Cost: there is no billing change. Reports of the unverified label creation are the author's own note: the first labels POST is expected to create human-merge-hold.

Minor, not counted as a finding: if the label write fails, report-outcome closes the PR even when it only reused an existing open one. A retry is a safe fallback, so I did not flag it.

@github-actions

Copy link
Copy Markdown
Contributor

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

Security review of #11036 at a221573. I found no security issues. No plugin.json or marketplace.json file changed, so there are no manifest lines.

critical-findings: 0

This change adds a hold; it widens nothing. I read the whole change to each trust-control file on the trusted-path list:

  • merge-holds.mjs labelHolds: adds a fourth label hold. It narrows nothing that was held before.
  • enable-auto-merge.mjs HOLD_LABELS: adds human-merge-hold. holdReason (line 387) compares lowercased names, so Human-Merge-Hold also holds. The body-text holds (LANE_HOLD and the others) are untouched.
  • babysit_merge_policy.MERGE_HOLD_LABELS: adds the same label. Nothing is removed.
  • pr-require-checks.yml ci-status step: it fails closed on every path I read:
    • It fails when the label list, event list, base SHA or trusted-actor list is unreadable.
    • It fails when the label is on.
    • It fails when the label is off and no removal is recorded since the newest add by any account.
    • It fails when the first removal at or after that add came from an account whose numeric id is not a human in the base's trusted-actors.json, or from any App (performed_via_github_app).
    • The actor list comes from BASE_CONFIG, checked out at the base SHA, not from the PR head, so the PR cannot edit the list it is judged by.
    • The job triggers include labeled and unlabeled (line 50), so adding or removing the label re-runs it. The first run can pass before the label is added; the labeled re-run and the arm and queue label holds cover that gap.
  • report-outcome.mjs: a held PR is labeled right after it opens or is reused. If the label write fails after a resend, the PR is closed and the issue goes to the retry route hold-label-failed. This fails closed, and the stop-routes entry is a plain retry route.
  • Workflow step inputs: the step's only expression inputs are github.token, the PR number, and the base SHA, which is regex-checked before use. There is no expression injection.

Cost: none. The only added spend is one extra labels API write per held PR.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 29, 2026
Merged via the queue into main with commit 74739c7 Oct 29, 2026
88 checks passed
@kyle-sexton
kyle-sexton deleted the feat/9437-human-merge-hold-label branch October 29, 2026 21:35
kyle-sexton added a commit that referenced this pull request Oct 29, 2026
…th-holds switch (owner Q16)

#11036 turns report-outcome's hold into a human-merge-hold label that
ci-status enforces, so its denied-path match is a path hold too: with
merge.diff-check.path-holds absent or false it denies no path; an
unreadable config still holds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(pr-pipeline): hold lanes App human-merge pull requests with a label only a listed human can remove

1 participant