Skip to content

fix(pr-pipeline): name the merge-holds step for what it holds and drop the stale status re-run - #12725

Merged
kyle-sexton merged 5 commits into
mainfrom
fix/12446-hold-step-names
Nov 3, 2026
Merged

kyle-sexton merged 5 commits into
mainfrom
fix/12446-hold-step-names

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Closes #12446

Summary

The leftovers of #12446, per its close-out comment. ci-status's merge-holds step gets a name that says what it holds, the verdict line stops mentioning a body hold, a ci-status re-run for a status ci-status no longer reads goes away, and two docs stop contradicting the code. The merge-holds behavior does not change, and a member's veto stays a comment read, with no labeling write. Item 5 of the issue, the re-sample, is met by the comment above.

Fix

  • Step name. Fail a lanes App pull request its body or a member holds becomes Fail a lanes App pull request on kill switch, hold path, verifier or veto in pr-require-checks.yml, resolve-pr.mjs (HOLD_STEP), reconcile.test.mjs, resolve-pr.test.mjs, ci-lanes-pr-hold.test.sh and pr-pipeline/README.md.
  • Old step name still read. resolve-pr.mjs runs from main and judges the step names of runs from older commits, and a re-run keeps its commit's workflow. Without the old name, a head held only on a hold path whose newest run predates this merge would read as not-a-passing-run, and pr-verify would not judge it. Both checks in heldShape and heldOnly take either name; OLD_HOLD_STEP can go once no open pull request's newest run reports it.
  • Verdict line. merge-holds.mjs now prints #<n>: no member veto and no hold path.. The grep in pr-require-checks.yml takes that line and the old one: the step copies the module from main's tip (:6287), while the grep comes from the run's own commit, so a lanes App group queued behind this one has the new grep and main's old module. The old alternative can go once this has merged and no such group is queued.
  • pr-feedback-dispatch.yml. A completed pr-unblock.yml run no longer re-runs ci-status. It leaves workflows:, the concurrency group and the rerun-ci-status gate, since no other job uses it (the dispatch job's if never names it), so each pr-unblock completion no longer starts a run in which every job skips. The review-lane and approval upstreams are unchanged. check-trusted-trigger/README.md drops the matching record and residual text.
  • Nothing live depended on the re-run. ci-status reads no pr-unblock/* status: only enable-auto-merge.mjs does (unblockStops). Labels a stop writes go on with the lanes App token, and pr-require-checks.yml:52 runs on labeled and unlabeled, so they start their own run. The reconciler's RECORDED_UPSTREAMS is pr-require-checks.yml alone, and fix(pr-pipeline): converge our own actors' merge holds on do-not-merge #12522 removed the reconciler's pr-unblock stop kind.
  • Docs. loop-lane/README.md now says a member's veto comment holds a lanes App pull request. pr-pipeline/README.md gives "seven places" as the 2026-11-01 count. The AGENTS.md hold paragraph still reads true (merge-holds.mjs:1647-1650; babysit_merge_policy.py:5130 for the body hold), so it is unchanged.

One window stays open: a lanes App run from before this merge that is re-run after it has the old grep and main's new module, so it fails closed with "exited 0 without its verdict" until a new event starts a run on the head. That failure holds rather than passes and clears with the next run. Printing both lines from the module would avoid it, at the cost of a second verdict line to remove later, so this PR leaves it.

Verification

  • bash scripts/ci-lanes-pr-hold.test.sh: PASS=300 FAIL=0.
  • bash scripts/ci-rerun-ci-status.test.sh: PASS=239 FAIL=0.
  • node --test on merge-holds.test.mjs, enable-auto-merge.test.mjs, resolve-pr.test.mjs, reconcile.test.mjs, workflow-gates.test.mjs and log-api-budget.test.mjs: no failures.
  • actionlint on both edited workflows and markdownlint-cli2 on the three edited docs: clean.
  • scripts/check-affected.sh --base origin/main --uncommitted --no-fetch: 12 passed, 3 skipped.
  • scripts/check-line-citations.py: clean.

Left out: CI's full suites, per the task; the changeover itself, which only a live run across the merge can show.

Related

  • #12284: merge-flow audit.
  • #12522: removed the holds these leftovers describe.
  • #12717: edits other lines of the same step and test for the verdict reader.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VQb9Q49h99qKdrunU4xw3S

kyle-sexton and others added 3 commits November 3, 2026 00:24
…p the stale status re-run

Rename ci-status's merge-holds step to say what it holds now: the kill
switch, hold paths, verifier holds and a member's veto. pr-verify's held-run
check keeps taking the old name, because a run reports the step names of its
own commit, re-runs too.

Drop "no body hold" from the merge-holds verdict line. The step's grep still
takes the old line as well: it reads the module from main's tip, which a
queued group carrying this change has not reached.

Stop re-running ci-status when a pr-unblock run completes. ci-status reads no
pr-unblock/* status, and the labels that run writes start their own run, so
pr-unblock also leaves the workflow's upstreams and concurrency groups.

Correct the docs: a member's veto comment holds a lanes App pull request, and
the seven hold places are the 2026-11-01 count.

Closes #12446

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q49h99qKdrunU4xw3S
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 3, 2026 05:27
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 3, 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-11-03T05:31:29.228904Z dc469e7 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

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 28db4ebddeadc562ef3de387dee45b5e01aa3c53: read 17 of 17 changed file(s); 0 finding(s), 0 as inline comments.

Security review: #12725 (no findings)

I read every hunk of all 17 changed files in .claude-lane/pr.diff. Five of them match trusted paths: merge-holds.mjs, resolve-pr.mjs, pr-require-checks.yml, pr-feedback-dispatch.yml and check-trusted-trigger/README.md. For each I read the whole change and what it gates. I found no security issues.

The last review of this lane was at dc469e72 and covered 15 files. This head adds two: a comment-only edit in reconcile.mjs and a line-citation fix in docs/outage-posture-audit.md. Neither touches a security surface.

Merge hold: merge-holds.mjs

  • Only the wording of the line it prints when nothing holds changes (merge-holds.mjs#L1696).
  • The line still prints only when reasons is empty, and the exit code (reasons.length || verifier.length ? 1 : 0) is unchanged. No hold is narrowed.

ci-status verdict reader: pr-require-checks.yml

  • The step is renamed and its body is unchanged.
  • The verdict grep (pr-require-checks.yml#L6512) now accepts the new line and the old one. Both are exact whole-line matches (-qxF).
  • The old line is what the base module printed in the same no-hold case, so the step passes nothing the base did not.
  • The verifier-verdict check after it and the fail-closed exits are unchanged.

pr-verify held-run check: resolve-pr.mjs

  • heldShape and heldOnly now accept the earlier hold-step name as well as the new one.
  • The earlier name is exactly the string the base already accepted, and a run's step names come from its own commit's workflow. So a run judged here was already judged the same way before.
  • The other conditions are unchanged: duplicate step names are refused, the aggregate step must succeed, and the log must account for every error line.

Re-run trigger and recorder: pr-feedback-dispatch.yml

  • The change removes pr-unblock as a workflow_run upstream. With it go its concurrency branch, its clause in the rerun-ci-status gate, RUN_TITLE, the status read, the page_full fail-open path and the vouched path that recorded these re-runs.
  • Every removal narrows: fewer runs can start a recorded ci-status re-run, and nothing new is trusted.
  • If a pr-unblock path reached the job body anyway, it now falls to the *) error exit.
  • I checked the claim that ci-status reads no pr-unblock/* status. The only pr-unblock/ matches in pr-require-checks.yml are hold-path patterns (L5085-5090 and the hold-floor line at L6691).
  • check-trusted-trigger.mjs has no branch that only these re-runs used. Its pr-unblock references are the separate pr-unblock-dispatched-* record.
  • The review-lane and approval-relay paths, including the listed-human check, are unchanged.

Docs and tests

  • check-trusted-trigger/README.md, loop-lane/README.md, pr-pipeline/README.md, outage-posture-audit.md, the .changes entry and the test files are renames, wording changes or removed coverage for the removed upstream. They match the code.
  • None of them deletes or softens a guardrail an agent follows. The loop-lane edit says a member's veto comment holds a lanes App pull request, which describes a hold rather than removing one.

Owner threat model: this change removes no named friction hold.

critical-findings: 0

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed 28db4ebddeadc562ef3de387dee45b5e01aa3c53: read 17 of 17 changed file(s); 0 finding(s), 0 as inline comments.

Code review: #12725 at 28db4eb

I read every hunk of all 17 changed files and found no correctness or maintainability problems, so this review has no findings. The last review of this lane, at dc469e72, also had no findings, so no earlier post repeats here.

What I checked

  • Step rename. The new name Fail a lanes App pull request on kill switch, hold path, verifier or veto is the same in all of these places:

    • pr-require-checks.yml
    • HOLD_STEP in resolve-pr.mjs
    • the regex in reconcile.test.mjs
    • the awk extractor in ci-lanes-pr-hold.test.sh
    • pr-pipeline/README.md

    A search of the repository finds the old name only in OLD_HOLD_STEP (resolve-pr.mjs:261) and in the test for it. heldShape and heldOnly accept either name. The new test covers a path hold, a cost hold that does not pass, do-not-merge beside a hold, and an empty log, all under the old name.

  • Verdict line. merge-holds.mjs now prints #<n>: no member veto and no hold path.. The grep at pr-require-checks.yml:6512 accepts the new line and the old one.

    • The old wording now appears only in the stand-in modules in ci-lanes-pr-hold.test.sh:1034-1037, which test that the old line is still accepted. Nothing else in the repository parses it.
    • Every merge-holds.test.mjs, enable-auto-merge.test.mjs and shell-test assertion uses the new line.
  • pr-feedback-dispatch.yml. pr-unblock no longer appears in workflows:, the concurrency group, the job's if, RUN_TITLE, the case, the status and job reads, or the vouched branch.

    • No $unblock, page_full, RUN_TITLE or $statuses is left in the file.
    • $pr is set again at :825 before its later uses (:826, :848).
    • The test script has no leftover STUB_STATUSES, STUB_UNBLOCK_JOBS, unblock_case or MAIN_TIP references.
    • A new case checks that a pr-unblock path is now refused.
  • Doc citations. The new outage-posture-audit.md anchors match the head file: degraded gates :388, the sweep group is :111, and queue: max is :114.

  • Reconciler comment. The comment at reconcile.mjs:909-913 no longer names the pr-unblock stop kind, which matches the branches below it.

Window the PR leaves open

The PR body names one window. Take a ci-status run that started before the merge and is re-run after it, for example by rerun-ci-status once the review lanes finish. That re-run has the old grep but main's new module, so it fails with "exited 0 without its verdict" until a new event starts a run on the head. It fails closed rather than passing, and the author chose this deliberately, so it is not a finding. A lanes App PR caught in it may still need a person or a new event to start a fresh run.

Closing issue #12446

The PR delivers the leftovers listed in the issue's close-out comment:

  • the step name
  • the verdict wording
  • the stale ci-status re-run in pr-feedback-dispatch.yml
  • the loop-lane/README.md:470-471 contradiction
  • the "seven places" note in pr-pipeline/README.md

The issue's comments record the other done-when items as met. The PR context has no embedded instructions aimed at the review lane.

Changelog

The fragment is bump: patch. That fits renamed text, an internal re-run trigger removed, and doc fixes.

Spend

None. Dropping the pr-unblock upstream means fewer pr-feedback-dispatch runs.

Not verified

I ran nothing in this lane. The test results in the PR body (PASS=300, PASS=239, node --test, actionlint, check-line-citations.py) are the author's claims, and I have not confirmed them.

kyle-sexton and others added 2 commits November 3, 2026 00:34
The native auto-merge section, the reconciler's comment and the hold test's
header still described a body hold and a pr-unblock stop re-run that no longer
exist. The outage audit's two pr-feedback-dispatch.yml citations moved with the
re-run's removal.

Refs: #12446

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q49h99qKdrunU4xw3S
@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit db1cec1 Nov 3, 2026
58 checks passed
@kyle-sexton
kyle-sexton deleted the fix/12446-hold-step-names branch November 3, 2026 05:57
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.

pr-pipeline: converge own-actor merge holds on the do-not-merge label

1 participant