[kyle-sexton]

Closes #10437

Summary

The cost hold took a decision's decided head from the newest cost notice that a review lane's status job recorded before the decision (#10357). If someone deleted the lane runs of a newer head, the older run's notice became the newest. An older head then became the decided head, and a later head with the same net change as that older head carried the owner's decision. This was the residual #10357 accepted, raised in review thread r4383606958.

The cost hold now checks the head branch's push history against the lane runs on each head pushed in that history. Deleting a run does not remove its push from that history. This change only tightens the hold: every new path keeps the hold, and no condition that cleared it before clears it now.

Fix

The rule is the same in all three readers:

• Where the history comes from. One page (per_page=100, newest first) of the repository activity API for refs/heads/<head branch>: GET /repos/{repo}/activity?ref=refs%2Fheads%2F<branch>&direction=desc&per_page=100. Each entry with a non-zero after sha is a head the branch moved to.
• What must hold. The branch was pushed to the decided head before the decision. Every other head pushed at or after the decided head's last such push, and before the decision, has a run of each review lane (pr-review-hosted.yml and pr-review-security-hosted.yml) from this repository, started before the decision. A run from only one lane is not enough: the lane runs left after a partial deletion would otherwise still promote the older notice.
• What keeps the hold.
  • A head with a missing lane run.
  • No push of the decided head before the decision.
  • A full page of 100 entries, counting pushes after the decision and before the decided head. On a strictly newest-first page, the required push of the decided head already shows that every later push is listed. The full-page check guards against an ordering the docs do not state as strict, and can be narrowed later.
  • A history that does not parse, or a read that fails.
• The window ends at the decision, not at the current head. Only runs started before the decision can set the decided head. The current head's lane runs may not be listed yet when ci-status runs on the same push. Pushing the decided head again after the decision does not hide the earlier gap, because the window is measured from its last push before the decision.

Changes per reader:

• merge-holds.mjs: new exported unranHead(activity, ran, decided, at). noticedHead collects each lane's run heads started before the decision. Once decidedHead names a head, it reads the activity page with the actions reader and returns null when unranHead names a reason. The check runs before the decided === head shortcut.
• babysit_merge_policy.py: unran_head mirrors it. _noticed_head reads the page through _get with BABYSIT_VERIFY_GH_TOKEN when that is set. A failed read raises PolicyReadError, which carries nothing.
• ci-status cost-hold step (.github/workflows/pr-require-checks.yml): the runs loop builds ran, then the step reads the activity page and runs the base's merge-holds.mjs unranHead through holds_js, before carries. A base without unranHead (this pull request's own runs, until it merges) keeps the hold. The step comment states the rule.
• Changelog fragment for source-control.

The token scopes stay as they are. The activity endpoint needs contents: read, per GitHub's permissions for GitHub Apps ("Contents", GET /repos/{owner}/{repo}/activity, read, UAT and IAT tokens). The ci-status job (pr-require-checks.yml) and the pr-merge.yml jobs that pass BABYSIT_VERIFY_GH_TOKEN already grant contents: read. That the Actions GITHUB_TOKEN can read this endpoint in practice is UNVERIFIED: the docs list installation tokens, and only a local gh token was used to probe it. If it cannot, every cost decision keeps its hold, so the change still fails closed.

Known holds this keeps in the safe direction, where a person lifts the label as before:

• a head pushed while the pull request conflicted with its base, since GitHub runs no pull_request workflow then;
• a head whose lane runs never ran, such as a [skip ci] push or a push while the pull request was closed;
• a branch whose activity listing holds 100 or more entries, whenever they were pushed.

Verification

Each new case was run against origin/main's merge-holds.mjs, babysit_merge_policy.py and pr-require-checks.yml, and fails there because the decision carries. With the fix, every case passes.

• node --test .github/scripts/pr-merge/merge-holds.test.mjs: 41 pass, 0 fail. On main: 4 fail. Two new tests: a deleted run of either lane or both, a run started after the decision, a fork run, the decided head pushed back, no push of the decided head, a full page, a failed read, and the non-deleted and pushed-after-decision cases that still carry. Two existing read-order assertions gain the activity read.
• node --test .github/scripts/pr-merge/enable-auto-merge.test.mjs: 137 pass. Its mock gains the activity route.
• python3 -m unittest tests.test_babysit_merge_cost tests.test_merge_holds_sync (babysit-prs scripts): 38 tests OK. On main: 9 failures and 1 error. The JS and Python readers agree on four new parity cases:
  • a newer head with a run of every lane carries;
  • all of its runs deleted keeps the hold;
  • one lane's run deleted keeps the hold;
  • no push of the decided head keeps the hold.
• scripts/ci-hold-cost-decision.test.sh: PASS=151 FAIL=0. On main: 7 fail.
  • The new cases are: a deleted run, a deleted security-lane run, every lane ran (carries), an unreadable history, a push after the decision (carries), no push of the decided head, and the decided head pushed back. A further case checks that the history is read as one page.
  • Four existing decided-head cases gain a security-lane run on HEAD, so they still test their own rule and not the new one.
  • The unreadable-module case now stops at the push-history check.
• actionlint .github/workflows/pr-require-checks.yml: clean. shellcheck scripts/ci-hold-cost-decision.test.sh: clean. scripts/run-ruff.sh check: clean. The Python edits follow ruff format; the format drift left in those files was there before. scripts/check-changelog-fragments.sh --check: valid.
• scripts/check-affected.sh --tests: 15 checks passed; the tests check failed only on two suites that need PyYAML, which is not installed here: ci-test-node-stop-commands.test.sh and run-hygiene-gate.test.sh. fix(pr-merge): take a cost decision's decided head from the review lane's check-run annotation #10357 recorded the same two. In that run the babysit unittest discover ran 3207 tests, OK. node --test .github/scripts/pr-merge/*.test.mjs: 529 pass, 0 fail.
• The encoded activity URL the readers build was probed once with a local gh token on fix(pr-merge): take a cost decision's decided head from the review lane's check-run annotation #10357's branch. It returned that branch's 6 entries, newest first.

Related

• fix(pr-merge): take a cost decision's decided head from the review lane's check-run annotation #10357: took the decided head from lane-run notices and recorded this residual.
• Scope: the issue's Done-when names the three merge-side readers above, which gate the merge. apply-review-hold/hold.cjs in melodic-software/ci-workflows was not read or changed.

🤖 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-27T20:13:43.352031Z	4919c0f	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.

[github-actions]

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

No findings. I read the diff and the pull request context. I did not run any tests, so everything the PR says about passing suites is author-claimed and unverified by me.

What I checked

• Closing issue fix(pr-merge): keep a cost hold when a newer lane run was deleted #10437. The three Done-when items are met:
  • The detector unranHead is in merge-holds.mjs, mirrored as unran_head in babysit_merge_policy.py and as a call through holds_js in the ci-status step.
  • The tests delete a newer head's lane run, either lane's or both, and assert the hold stays.
  • The no-deletion cases still carry: a newer head with both lanes' runs, and a head pushed after the decision.
• Three readers, one rule.
  • The at cutoff, the 40-hex and all-zero sha filters, and the "last push of the decided head before at" window match across JS, Python and the shell step.
  • The non-array, full-page (100 or more), unparseable-timestamp and no-push-of-decided-head cases each keep the hold in all three.
  • An unparseable cutoff keeps the hold in both JS and Python.
  • test_merge_holds_sync.py adds parity cases that run JS and Python against the same worlds.
• Shell step in pr-require-checks.yml.
  • $ref is already @uri-encoded at line 4072, so refs%2Fheads%2F$ref builds the same URL the other two readers and the test stubs expect.
  • $decided_at is epoch seconds, so the ran filter compares like with like. The ISO comment.created_at goes to unranHead separately.
  • ran is built in the main shell, not a subshell, so it survives the loop.
  • A base merge-holds.mjs without unranHead makes holds_js fail, and that keeps the hold.
  • A 404 from the activity read is a read_api failure and keeps the hold.
• Direction of change. Every new branch returns null or calls keep, so the change only adds holds. No earlier clearing path is relaxed.
• Tests. The new cases cover each lane's deletion, a run after the decision, a fork run, the decided head pushed back after the decision, a full page, and a failed or malformed read. The existing read-order and env-count assertions were updated to include the activity read.

Notes, not findings

• The PR itself flags that the Actions GITHUB_TOKEN may not be able to read the activity endpoint. If it can't, every cost decision keeps its hold, which is the safe direction. I can't confirm either way from here.
• The conservative holds listed in the PR (a conflicted push, a [skip ci] push, 100 or more activity entries) leave the lift to a person, as before.
• Spend: the change adds one activity API read per cost-decision evaluation. It starts no billed resource, and there is no cost hold.

[github-actions]

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

critical-findings: 0

No security findings. I read the whole diff, including the trusted-path files merge-holds.mjs, pr-require-checks.yml and babysit_merge_policy.py. I did not run anything: this lane has no shell.

The change only tightens the cost hold. The decided head is kept only if the branch's push history (GET /repos/{repo}/activity) shows a run of both review lanes, started before the decision, on every head pushed since the decided head's last push. I checked each path where it could loosen a hold:

• Fail-closed reads:
  • Reader behavior: unranHead and unran_head return a reason string, which means "keep the hold", for each of these cases: an unreadable or non-array history, a page of 100 or more entries, a malformed ran list, an unparseable timestamp, an unparseable decision time, and no push of the decided head before the decision. The first four cases are fixed in merge-holds.mjs:46 and babysit_merge_policy.py:335. Python also fails closed on a non-object entry, through json_object. Math.max() over an empty list is -Infinity, which is not finite, so the JS reader keeps the hold.
  • Failed read: a failed activity read keeps the hold in all three readers. noticedHead in JS throws and the Python PolicyReadError is documented to carry nothing. In the workflow, read_api … || keep handles it.
• Workflow shell:
  • Branch name: the branch is @uri-encoded at pr-require-checks.yml:4072 before it goes into the branch= and ref=refs%2Fheads%2F$ref query strings, so an attacker-chosen branch name cannot inject query parameters. The JS reader uses encodeURIComponent after the BRANCH check, and Python uses quote(..., safe='').
  • Base module: the step runs unranHead from the base's merge-holds.mjs, as with the other hold functions, so this pull request's own code does not gate itself. A base without the function throws inside holds_js, and the step keeps the hold.
  • Values passed to the base module: the workflow passes the activity page, the run lists, the decided sha and the decision time to holds_js as JSON files. It passes no interpolated shell text into the JavaScript.
• Remaining cases:
  • Re-push of the decided head: a push of the decided head again before the decision moves the window start forward. I read that as the decision then applying to that state, and it is not a bypass.
  • Runs after the decision: the code ignores runs started at or after the decision, and decided/at are bounded the same way in all three readers.
• Trust controls: none is widened. Token scopes are unchanged. The author says in the PR description that the Actions GITHUB_TOKEN may not be able to read the activity endpoint. If so, every cost decision keeps its hold, which is the safe direction.
• Cost: none. The change adds one extra REST read per cost-hold evaluation and starts no billed resource.
• Manifests and instruction surfaces: the diff touches no plugin manifest and removes no standing instruction.

[melodic-automation-lanes]

The pr-unblock lane stopped on this pull request and needs a person: the merge stages a change under .github/workflows/, which the lane does not push.

• Reason: workflow-staged
• Resume key: 4919c0f6f1e58e822d1a5e23503cf481fc9c6e87 (the head the lane stopped on)

To resume, deal with the reason above, remove the needs-human label (and do-not-merge, if set), then dispatch the pr-unblock workflow with pr-number=10515 and activity=resolve-conflicts:

gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number=10515 -f activity=resolve-conflicts

[melodic-automation-lanes]

The pr-unblock lane escalated this pull request as workflow-staged, and that no longer holds at 28c4ee455def8b0b1fd5ced866e37fcdd9c3d8e1: main changes nothing under .github/workflows/ since its merge base with 28c4ee4. pr-merge removed needs-human, so the lanes run on it again. To keep a person on it, add needs-human back: a label a person adds is never removed.