[kyle-sexton]

Refs: #10596

Summary

Follow-up to #10596. On gate and trust hold paths only a trusted person's approving review lifts the hold, bound to its commit, and it carries to a later head only over merges of the base. Two facts made that carry useless:

1. Ruleset 24475076 ("trust-root") sets dismiss_stale_reviews_on_push: true, so the base merge's push dismisses the approval, and a dismissed approval never counted.
2. sameNetChange fails whenever main touched a file the pull request also changes, as in the feat(pr-pipeline): tidy a small single-area change inside resolve-issue before its one push #10224 replay (f3de238112 to bad82b1b56).

The owner decided on 2026-10-28 to build both parts below. The cost-decision carry is unchanged.

Fix

(a) GitHub's automatic stale dismissal still counts (autoDismissed / auto_dismissed, liftsOf / lifts_of).

A trusted person's review whose REST state is DISMISSED counts as an approval of its own commit_id only when the pull request's issue timeline has exactly one review_dismissed event for that review id, and that event has all of these:

• dismissed_review.state of approved;
• dismissal_message strictly null (an empty string or a missing field revokes);
• an actor of type Bot (the pushing App);
• a 40-hex dismissal_commit_id.

A restored approval lifts nothing by itself:

• it never takes the exact-head shortcut in liftersOf / carried_lifters, and never reaches pathLifters;
• it lifts only through the carry, and approvalCarry also requires its dismissal commit to be one of the commits the current head adds since the approved head, so the push the carry checks is the push that dismissed it.

The Bot actor is what tells GitHub's dismissal from a person's. A dismissal whose actor is a User revokes: a person's own dismissal, or the stale dismissal their push caused (a person's push revokes only when it is the dismissing push). The null message is a second filter: people are already excluded by actor type, and that an App's deliberate dismissal always carries a message is UNVERIFIED (the dismissal API documents message as required). A dismissal with a message, a second dismissal event for the same review, and a later review by the same person also revoke. Approving H, pushing X, then force-pushing back to H leaves no range, so it lifts nothing.

The timeline is read only when a trusted person has a dismissed review. A timeline that does not read restores nothing. GitHub lists GET /repos/{owner}/{repo}/issues/{issue_number}/timeline under the "Pull requests" read permission as well as "Issues" (docs.github.com, "Permissions required for GitHub Apps"). ci-status's pull-requests: write token and the lane App token's pull-requests write both cover it, so no workflow permission changes.

(b) The exact carry (exactBaseCarry / exact_base_carry, approvalCarry / approval_carry).

baseMergesOnly still requires every commit since H to be a merge whose non-first parents the base holds. Then either check carries:

• Fast path, unchanged: sameNetChange(then, now).
• Exact path: the code takes M and M' from the merge_base_commit of the existing <tip>...H and <tip>...H' compares, then reads H...H' (what the merges brought) and M...M' (what the base changed between the two merge bases). Both conditions must hold:
  • H...H' is ahead with behind_by 0, and M...M' is ahead or identical with behind_by 0. Each commit list is read whole (total_commits matches).
  • The two file lists are equal in both directions, file for file, on status, previous_filename and patch (sameNetChange reused, so either list at 300 files, or any file without a patch, fails closed).
  • The recursive trees of H' and M' (git/trees/<sha>?recursive=1, read once each, truncated must be false) give each brought file one mode, and a removed file or a renamed file's old path is in neither tree (sameModes / _same_modes). This closes the mode residual on this path and cross-checks that every brought path exists in H'.

The carry, fast path included, runs only when the pull request's base is the repository's default branch: another base may be non-linear, with criss-cross merge bases. Python's pull re-read now also refuses a head in a fork, as JS does.

Assumption, not checked: the exact path assumes GitHub omits no file from a compare's file list when it lists fewer than 300 files. Nothing reads a second listing to cross-check that; the tree read only confirms each listed path exists in H'.

What the exact path rejects:

• a file the merges changed beyond the base (hold path or not);
• a base hunk the merge altered;
• a base change the merge dropped or reverted;
• a mode flip on a file the base changed, including a hold-path file the pull request never touched.

baseMergeCarry keeps its arity and meaning, because the cost-hold bash step in pr-require-checks.yml calls it positionally from the base's module.

Cost carry left unchanged. Moving it would mean changing three readers in lockstep (JS carries, Python _carries, and the bash carries() step in a security gate). A push does not dismiss a cost decision, so problem (1) does not apply to it.

Security reasoning.

• The Bot actor, the review id match and the dismissal commit among the pushed commits identify a dismissal caused by the very push the carry inspects; the null message is a second filter whose force for Apps is UNVERIFIED. A person, by their own dismissal or by the push that dismissed the review, revokes.
• A restored approval is bound to its original commit and has no exact-head path.
• The exact carry needs the merges to have brought exactly what the base changed between the two merge bases: M' is an ancestor of the tip, so M...M' is base content only, and the two-way equality admits nothing else. Patches include hunk headers and context, so a base hunk lands only where it applied cleanly.
• Every read failure, truncation (300 files, total_commits mismatch, missing patch, truncated tree), fork head, moved head, missing merge base or non-default base fails closed. The mode residual remains only on the same-net-change fast path, which reads no trees.

Verification

Failing first:

• At 7a18685f2c, three new JS tests in merge-holds.test.mjs failed: the stale-dismissal carry, feat(pr-pipeline): tidy a small single-area change inside resolve-issue before its one push #10224's shape, and the pure-function test.
• At base, the new Python test_a_stale_dismissed_approval_carries, test_10224s_shape_carries_when_the_merges_brought_exactly_mains_changes and the parity test TheCarriedLiftIsTheGates failed.
• The new enable-auto-merge case failed against the base's merge-holds.mjs.
• The negative cases pass at base by construction, since base never counted a dismissed approval. Mutation runs are the evidence that they bite: 10 mutations of the new JS rules and 10 of the Python rules, each one killed (the Python behind_by == 0 survivor got its own case).

Covered, in JS and Python:

• the feat(pr-pipeline): tidy a small single-area change inside resolve-issue before its one push #10224 shape carries;
• a stale auto-dismissal carries;
• these do not carry: a dismissal with a message (including an empty one or a missing field), a dismissal by a person, a dismissal with no actor, a dismissed request for changes, a dismissal of another review, a second dismissal event, a dismissal commit the merges did not push, a dismissal at the approved head, a dismissed approval of the head itself, and a later request for changes;
• a merge that also edits a hold or non-hold file does not carry;
• a merge whose second parent is not in the base does not carry;
• a base change reverted inside the merge does not carry;
• 300-file and cut commit lists on either range, a range that is not ahead, and a missing merge base all fail closed;
• a mode flip on a brought file, a brought file missing from either or both trees, a truncated or unflagged tree, an unreadable tree, a removed file still in a tree, a non-default base (exact and fast path), an unreadable repository, and a head re-read in a fork do not carry (added after the independent security review; each failed first, and each new rule survived no mutation run).

Every pre-existing adversarial case now carries consistent H...H' and M...M' data, so it fails for its own reason and not for missing data.

Suites, after merging main (417bdd5639):

• merge-holds.test.mjs: 52 pass. enable-auto-merge.test.mjs: 144 pass.
• babysit-prs scripts/tests: 3344 passed.
• ci-hold-cost-decision: 157. ci-hold-human-gated: 49. ci-hold-lifted-by: 35. ci-hold-merge-group: 67. ci-hold-pr-unblock-status: 27. ci-lanes-pr-hold: 231.

#10224 replay (read-only, cached compares; main at 0218059955, H f3de238112, H' bad82b1b56):

• (a) works. The 23:31:57Z approval (review 5675840882) is restored with dismissal commit bad82b1b56, the lanes App's merge push.
• The 06:25:59Z null-message dismissal by kyle-sexton (a person's own non-merge push, 1f6d28bed7) is treated as revoking.
• (b) does not carry the real replay. It fails closed for two independent reasons:
  • H...H' and M...M' each list exactly 300 files, the compare cap; main moved 17 commits.
  • Even uncapped, the two overlap files differ. collect-base-activity.test.mjs differs in the hunk header (-128 vs -125), and plugin.json differs in a context line (the pull request's description change shows as context).
• So on this repository's busy main, the exact path will rarely fire. The common win is (a) plus the existing fast path when main did not touch the pull request's files.

Related

• fix(pr-merge): carry a hold-path lift over a later base merge #10596: the carry this extends.
• feat(pr-pipeline): tidy a small single-area change inside resolve-issue before its one push #10224: the measured churn.
• Deferred by the coordinator pending measurement: a tree and blob carry (with a 3-way check for overlap files) that would lift the 300-file cap and carry the real feat(pr-pipeline): tidy a small single-area change inside resolve-issue before its one push #10224 shape.
• This changes gate files (merge-holds.mjs, babysit_merge_policy.py), so the security lane may hold it for a person.

🤖 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-28T14:33:34.672736Z	603867a	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 reviewed 417bdd563991aa211daebd57fe19ea745179bde5: 0 finding(s), 0 as inline comments.

Security review: PR #10671 (exact base-merge carry and stale-dismissal restore)

critical-findings: 0

I found no security issues. This change loosens a merge hold, so I checked it for a path where a pull request author gets through content a trusted person never approved.

Scope. I read the full diff of merge-holds.mjs, babysit_merge_policy.py, the babysit_verify_evidence.py comment edit, the tests, the README and the change notes. This lane could not run any code. I read the surrounding code in merge-holds.mjs (baseMergesOnly, pages). No manifests are touched, so there are no manifest <field> lines. Nothing in the diff can start a real charge, so there is no cost hold.

Weakened hold: a stale-dismissed approval now counts.

• A dismissed approval is restored only when its timeline event passes four filters. The event is approved, has a null message, has a Bot actor, and has a 40-hex dismissal commit (merge-holds.mjs:199-213, babysit_merge_policy.py auto_dismissed).
• A second event for the same review, or any non-matching event, revokes it. The JS and Python versions have the same logic.
• A restored approval never takes the exact-head shortcut (dismissal === null is required, in liftersOf and carried_lifters). It lifts only through approvalCarry, and its dismissal commit must be among the commits added since the approved head.
• A forged Bot actor does not widen anything. The carry still has to pass the content check: every added commit must be a merge of base-held parents, and the net or exact change must match. The actor filter only avoids restoring an approval a person dismissed on purpose.
• The null message filter rests on the dismissal API requiring a message. The PR and code comments flag this as unverified. Even if wrong, the content check still stops unapproved content, so I did not rate it above a note.

Weakened hold: the exact base-merge carry. exactBaseCarry requires all of these to hold:

• H...H' is ahead with behind_by 0, and M...M' is ahead or identical with behind_by 0.
• Both commit lists are read whole.
• The two file lists match on status, previous_filename and patch. This reuses sameNetChange, so a list of 300 files or a missing patch fails closed.
• The recursive trees of H' and M' give each brought file one mode, and a truncated tree fails closed.

The first three together mean H' = H + (M→M'). Because M' is an ancestor of the base tip, M→M' is base content only. An evil merge, a dropped or altered base hunk, a reverted base change, a mode flip, a rename or a delete each fails one of the checks. baseMergesOnly is unchanged, so non-merge commits and merges of non-base parents still fail. I found no bypass.

Other controls I checked.

• The carry now runs only when the pull request's base is the repository's default branch, and a fork head fails closed. The Python version re-checks the fork head on its re-read of the pull request.
• A failed read (the timeline, the repository, a tree or a compare) lifts nothing.
• The if (dismissal !== null && !newCommits(...).some(...)) call cannot dereference null, because baseMergesOnly returns false first when newCommits is null.
• The cost-decision carry (baseMergeCarry, which the bash step in pr-require-checks.yml calls positionally) keeps its arity and behavior.
• The added id on fromRest items feeds only the review key.
• _default_branch in the Python module exists at babysit_merge_policy.py:4529.

The base still has the repo's required reviews and the trust-root ruleset, and this change adds no workflow permission. The PR states that timeline reads fit the existing pull-requests scope, and the diff changes no workflow file.

Residuals, all documented in the PR.

• Compare reports no file mode, so the same-net-change path still treats a mode-only change as unchanged. This is pre-existing, and the exact path closes it with the tree reads.
• The exact path assumes GitHub omits no file from a compare under 300 files.
• On a busy base the exact path will rarely fire. That makes it fail closed more often, not less.

This is a hold-path file change under the owner's 2026-10-28 decision, so a person will still see it. That is not a finding.

[github-actions]

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

No blocking or advisory findings in the code-review lane for this head. I read the whole diff and the pull request context. I did not run the tests; the pass counts in the description are the author's claims and I did not verify them.

What I checked

• Stale-dismissal restore. autoDismissed and auto_dismissed agree. Both need exactly one review_dismissed event for the review, state approved, a strictly null message, a Bot actor and a 40-hex commit. Any other event for that review id revokes it. In liftsOf and lifts_of a restored approval takes no exact-head path: the sha === head branch skips it, and it carries only through approvalCarry. That function also requires the dismissal commit to be among the commits added since the approved head.
• Exact carry. exactBaseCarry and exact_base_carry match each other. Both require H...H' to be ahead, M...M' to be ahead or identical, behind_by 0, commit lists read whole, and file-for-file equality through sameNetChange (so 300 files or a missing patch fail closed). Both check modes against the recursive trees of H' and M', with a truncated tree, a missing path or a removed or renamed-away path that still exists failing closed.
• Default-branch gate. The carry now runs only when the base is the repository's default branch, and the Python re-read also checks the head's repo. A failed default-branch read raises PolicyReadError, which the caller catches, so it lifts nothing.
• JS and Python parity. The parity test now drives both implementations over the same cases, including the new carry cases.
• Cost carry. baseMergeCarry keeps its signature, so the positional call from pr-require-checks.yml is unaffected.
• Fragments and docs. Both change fragments are bump: patch, and the README section matches the code.

Remaining risks (not findings)

• Stale description. The pull request body says the compare API's missing file mode remains an open residual for the exact path. The head closes that for the exact path with the tree reads, as the fragments and README describe. The mode-only residual stays only on the same-net-change path. The description is out of date; the code and docs agree with each other.
• Large trees. On a very large repository the recursive tree may come back truncated. That fails closed, so the exact path would simply not fire there. It is not a correctness problem.
• Unverified premise. A bot's deliberate dismissal always carrying a message is documented in the code as unverified. The Bot-actor and dismissal-commit checks are the primary filters, and the code and README say so.

Spend: none beyond normal CI runs; no paid plan or billing setting is touched.

[github-actions]

Post-merge mutation testing

• Range: c3e8018..bcfd9f7
• Merged pull request: #10671
• Outcome: refused

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

File	Killed	Survived	Timeout	No coverage	Invalid	Not run
.github/scripts/pr-merge/merge-holds.mjs	0	0	0	0	0	4
plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py	0	0	0	0	0	4

The run's summary has the full report.