Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .changes/source-control/babysit-head-pushers-activity-7354a1c0.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
bump: patch
---

### Fixed

- The unattended merge gate takes a pull request's head pushers from the head branch's repository
activity, not only from commit author and committer logins, which GitHub links through an email
the pusher chooses. A collaborator who pushes a commit carrying the agent identity's email is now
excluded from counting as an independent resolver of the lane's review threads. A push history
whose newest move did not bring the head, a move with no actor, or a commits list that does not
hold the pinned head refuses the candidate with `head-pusher-unproven`.
Original file line number Diff line number Diff line change
Expand Up @@ -1105,10 +1105,18 @@ any refusal is the blocker `unattended merge refused: <reason>`:
4. For an eligible pull request: the review-lane evidence bound to the head, and the diff score.
A head commit whose author or committer is linked to no account refuses
(`head-pusher-unproven:<sha>`): its pusher cannot be shown not to be a human. So does a pull
request whose commit count exceeds the commits the list returned.
request whose commit count exceeds the commits the list returned, or whose list lacks the
pinned head (`head-not-listed`). GitHub links a commit's author and committer through an email
the pusher chooses, so every actor in the head branch's repository activity counts as a head
pusher too, and none of them may resolve a finding away. A history whose newest move did not
bring the head refuses (`push-history`), as does a move with no actor (`push-actor`).
- **Pointer**: when the commits list's cap matters, fetch
[list commits on a pull request](https://docs.github.com/en/rest/pulls/pulls#list-commits-on-a-pull-request)
live. **As of**: 2026-10-12. **Recheck trigger**: that section changes the cap or its paging.
- **Pointer**: for the push history, fetch
[list repository activities](https://docs.github.com/en/rest/repos/repos#list-repository-activities)
live; its `actor` may be null. **As of**: 2026-10-29. **Recheck trigger**: that section
changes the activity types or the `actor` field.
No work class is read: the only class-named check is the trust hold, which refuses an issue
carrying `work-class: untrusted-provenance`, or one a non-human removed it from. Eligibility
also needs the linked issue vouched for by a human (`issue-not-vouched:<why>` otherwise): a
Expand Down

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pr-verify verifier failed head 5571c35a9c4cd589554296cf90884426a198f63b. What it wrote, as data:

What it wrote (27 lines)
Summary: Issue #7354 asked for option (b) from owner decision D6: take head pushers from the branch
  activity API, plus the head-in-list hardening. The diff does that, and the code looks correct and
  fails closed. But the owner's latest trusted comment on #7354 (2026-10-29 23:11, after this PR
  opened) closed the issue as "hardening deferred". It says hardening against the organization's own
  actors waits until open issues and open PRs both reach 0, and returns then "as configuration"
  (owner decisions Q15/Q16). This PR delivers now, as hard-coded unattended-merge gate behavior, the
  hardening the owner put off and asked to come back in a different form. The spec's current intent
  is not met, so this should not merge as-is.
- spec-text.json #7354 last comment: the owner closed the issue as "hardening deferred" until open
  issues = 0 and open PRs = 0, to return "as configuration". This PR still says `Closes #7354` and
  hard-codes the hardening. Fix: hold or close this PR until the reopen trigger fires, or get a new
  owner decision that overrides Q15/Q16. When the work resumes, make the activity-based pusher
  exclusion configurable, as the owner asked, not always on.
- pr body: the Python tests and ruff were not run by the lane. They are left to CI, so the changed
  test counts (`3 + 2 * 4`, `1 + 3 + 2 * 4`) have only been traced by hand.
- plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py:3193: the activity read follows
  every page with no cap. A reused branch name with a long history costs unbounded reads. The PR
  body says the merge policy's own branch_activity read stops at 20 pages. When this returns, apply
  a similar cap and refuse with `head-pusher-unproven` past it.
- plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py:3193: `repos/{repo}/activity`
  is read on the base repo. For a cross-repo (fork) head, the activity is empty or belongs to a
  same-named base branch, so the gate refuses with push-history. That fails closed, but no test or
  doc line covers or states it.
- plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py:3196: the activity read uses
  the verdict reader's token (`read_env`). The PR does not confirm that this token can read
  repository activity. A permission error would block every unattended merge rather than refuse with
  a clear reason. Confirm the token's scope or map the error to a `head-pusher-unproven` reason.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pr-unblock / address-feedback: UNCERTAIN, left open

Evidence: plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py:3193, plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py:3196

The main finding is about scope, not code. The verifier reports that the owner closed issue 7354 as hardening deferred until open issues and open PRs both reach 0, and wants it to come back later as configuration (owner decisions Q15/Q16). This PR adds the activity-based head-pusher exclusion now, always on, inside the unattended-merge gate (babysit_merge.py:3193-3201, through _head_pushers at :3098). The code can't settle whether to hold or close this PR, make the exclusion configurable, or get a new owner decision that overrides Q15/Q16. That is the owner's call on shared merge infrastructure, so this lane made no edit. The verifier framed the follow-ups as work for when the issue reopens, so none was applied here. Each one is confirmed in the code: (1) the activity read at :3195 goes through fetch_paginated_api with no page cap; (2) it reads repos/{repo}/activity on the base repo, so a fork head fails closed with head-pusher-unproven:push-history, and no test or doc covers that; (3) it uses the verdict reader token (read_env, :3149), and a permission error raises instead of mapping to a head-pusher-unproven reason. A person needs to decide whether this PR goes ahead before those are fixed.

Original file line number Diff line number Diff line change
Expand Up @@ -3095,6 +3095,29 @@ class HeadPusherUnproven(Exception):
"""A head commit's pusher cannot be shown not to be a human."""


def _head_pushers(activities: Sequence[Any], head_sha: str) -> set[str]:
"""The casefolded login of every account that moved the head branch, from
its repository activity (newest first): a commit's author and committer
are linked through an email the pusher chose, a push's actor is not. A
deletion moves nothing. Raises `HeadPusherUnproven` when the newest move
did not bring `head_sha`, so a push the listing lacks names no pusher, or
when a move has no actor."""
moves = [
json_object(a)
for a in activities
if json_object(a).get("activity_type") != "branch_deletion"
]
if not moves or moves[0].get("after") != head_sha:
raise HeadPusherUnproven("head-pusher-unproven:push-history")
pushers = {
str(json_object(move.get("actor")).get("login") or "").casefold()
for move in moves
}
if "" in pushers:
raise HeadPusherUnproven("head-pusher-unproven:push-actor")
return pushers


def _lane_evidence(
repo: str,
number: int,
Expand All @@ -3108,10 +3131,13 @@ def _lane_evidence(
`changed_paths` is `bind()`'s: None when the changed files are unproven.

The human test bound into `bind()` is the merge policy's, and it also
returns False for the pull request's author and every head pusher (commit
authors and committers, force-push actors): none of them may resolve a
finding away. A commit with no linked author or committer, or a head with
more commits than the list returns, raises `HeadPusherUnproven`. A retargeted base unbinds every lane.
returns False for the pull request's author and every head pusher (the
actors of the head branch's activity, `_head_pushers`, plus commit authors
and committers and force-push actors): none of them may resolve a finding
away. A commit with no linked author or committer, a head with more
commits than the list returns or missing from it, or a push history
`_head_pushers` refuses raises `HeadPusherUnproven`. A retargeted base
unbinds every lane.

The timeline, the comments and the lane runs are read under the verdict
reader's token when one is set, keeping them off the lane App's quota;
Expand Down Expand Up @@ -3140,6 +3166,9 @@ def _lane_evidence(
raise HeadPusherUnproven(
f"head-pusher-unproven:commits-listed:{len(commits)}-of:{total}"
)
# The count is the pinned head's only when the list holds that head.
if head.get("sha") not in {item.get("sha") for item in commits}:
raise HeadPusherUnproven("head-pusher-unproven:head-not-listed")
# A commit whose author or committer email no account claims names no
# login to exclude, so whoever pushed it is not proven non-human.
unlinked = [
Expand All @@ -3161,6 +3190,15 @@ def _lane_evidence(
for e in timeline
if e.get("event") == "head_ref_force_pushed"
}
ref = quote(f"refs/heads/{head.get('ref')}", safe="")
excluded |= _head_pushers(
fetch_paginated_api(
f"repos/{repo}/activity?ref={ref}&direction=desc&per_page=100",
f"#{number} pushes",
env_overrides=read_env,
),
str(head.get("sha")),
)
excluded.add(str(json_object(pr.get("user")).get("login") or "").casefold())
excluded.discard("")
comments = fetch_paginated_api(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2578,10 +2578,21 @@ def lanes_listed(self, changed: list[str]) -> list[Any]:
"head": {"ref": "feat", "sha": HEAD},
"base": {"sha": "b" * 40},
"user": {"login": "someone"},
"commits": 0,
"commits": 1,
}
someone = {"login": "someone"}

def paginated(endpoint: str, label: str, **_: Any) -> list[dict[str, Any]]:
# The head's pushers are proven: the list holds the pinned head
# and the branch's newest push brought it.
if endpoint.endswith("/commits?per_page=100"):
return [{"sha": HEAD, "author": someone, "committer": someone}]
if "/activity?" in endpoint:
return [{"activity_type": "push", "after": HEAD, "actor": someone}]
return []

with (
mock.patch.object(merge, "fetch_paginated_api", return_value=[]),
mock.patch.object(merge, "fetch_paginated_api", side_effect=paginated),
mock.patch.object(merge, "fetch_review_threads", return_value=[]),
mock.patch.object(
merge,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4675,17 +4675,29 @@ def outdated(body: str) -> dict[str, Any]:


class HeadPushers(unittest.TestCase):
"""#7339 item 3: who pushed a commit is known only through a linked login."""
"""#7339 item 3 and #7354: who pushed the head is its branch activity's
actors, never only a commit's email-linked author or committer."""

def read(self, commits: list[dict[str, Any]], total: Any = None) -> Any:
PUSHED = [{"activity_type": "push", "after": HEAD, "actor": {"login": IDENTITY}}]

def read(
self,
commits: list[dict[str, Any]],
total: Any = None,
activity: list[dict[str, Any]] | None = None,
) -> Any:
pr = {
"head": {"ref": "feat", "sha": HEAD},
"user": {"login": IDENTITY},
"commits": len(commits) if total is None else total,
}

def paginated(endpoint: str, label: str, **_: Any) -> list[dict[str, Any]]:
return commits if endpoint.endswith("/commits?per_page=100") else []
if endpoint.endswith("/commits?per_page=100"):
return commits
if "/activity?ref=refs%2Fheads%2Ffeat&direction=desc&" in endpoint:
return self.PUSHED if activity is None else activity
return []

with (
mock.patch.object(merge, "fetch_paginated_api", side_effect=paginated),
Expand All @@ -4705,14 +4717,14 @@ def paginated(endpoint: str, label: str, **_: Any) -> list[dict[str, Any]]:
def test_an_unlinked_author_or_committer_is_unproven(self) -> None:
bot = {"login": IDENTITY}
for name, commit in (
("author", {"sha": "a" * 40, "author": None, "committer": bot}),
("committer", {"sha": "a" * 40, "author": bot, "committer": None}),
("author", {"sha": HEAD, "author": None, "committer": bot}),
("committer", {"sha": HEAD, "author": bot, "committer": None}),
):
with self.subTest(name):
with self.assertRaises(merge.HeadPusherUnproven) as raised:
self.read([commit])
self.assertEqual(
str(raised.exception), "head-pusher-unproven:aaaaaaaaaaaa"
str(raised.exception), f"head-pusher-unproven:{HEAD[:12]}"
)

def test_a_head_longer_than_the_commits_list_is_unproven(self) -> None:
Expand All @@ -4729,11 +4741,54 @@ def test_a_head_longer_than_the_commits_list_is_unproven(self) -> None:
self.read(listed, total=total)
self.assertIn("commits-listed:250", str(raised.exception))

def test_a_list_without_the_pinned_head_is_unproven(self) -> None:
# The count and the list could describe different heads.
bot = {"login": IDENTITY}
with self.assertRaises(merge.HeadPusherUnproven) as raised:
self.read([{"sha": "c" * 40, "author": bot, "committer": bot}])
self.assertEqual(str(raised.exception), "head-pusher-unproven:head-not-listed")

def test_linked_commits_bind_as_before(self) -> None:
bot = {"login": IDENTITY}
lanes, _ = self.read([{"sha": "a" * 40, "author": bot, "committer": bot}])
lanes, _ = self.read([{"sha": HEAD, "author": bot, "committer": bot}])
self.assertEqual(len(lanes), 2)

def test_a_push_history_not_reaching_the_head_is_unproven(self) -> None:
bot = {"login": IDENTITY}
commits = [{"sha": HEAD, "author": bot, "committer": bot}]
older = {"activity_type": "push", "after": "c" * 40, "actor": bot}
for name, activity, reason in (
("none", [], "push-history"),
("older head", [older], "push-history"),
("no actor", [{**self.PUSHED[0], "actor": None}], "push-actor"),
("older no actor", [*self.PUSHED, {**older, "actor": None}], "push-actor"),
):
with self.subTest(name):
with self.assertRaises(merge.HeadPusherUnproven) as raised:
self.read(commits, activity=activity)
self.assertEqual(
str(raised.exception), f"head-pusher-unproven:{reason}"
)

def test_a_pusher_spoofing_the_agent_email_is_no_independent_resolver(
self,
) -> None:
# A collaborator pushes a commit whose author and committer emails
# are the agent identity's: only the push actor names them.
bot = {"login": IDENTITY}
commits = [{"sha": HEAD, "author": bot, "committer": bot}]
deleted = {"activity_type": "branch_deletion", "after": "0" * 40}
activity = [
{"activity_type": "push", "after": HEAD, "actor": {"login": "Collab"}},
*self.PUSHED,
deleted,
]
with mock.patch.object(merge.review_evidence, "bind", return_value=()) as bind:
self.read(commits, activity=activity)
human = bind.call_args.kwargs["is_human"]
self.assertFalse(human({"__typename": "User", "login": "collab"}))
self.assertTrue(human({"__typename": "User", "login": "reviewer"}))


class LaneEvidenceReadToken(unittest.TestCase):
"""The gate's review-evidence reads (timeline, comments, lane runs and
Expand All @@ -4749,6 +4804,8 @@ def answer(self, args: list[str]) -> Any:
bot = {"login": IDENTITY}
if endpoint.endswith(f"/pulls/{PR}/commits?per_page=100"):
return [[{"sha": HEAD, "author": bot, "committer": bot}]]
if endpoint.startswith("repos/owner/repo/activity?"):
return [[{"activity_type": "push", "after": HEAD, "actor": bot}]]
if "/actions/workflows/" in endpoint:
return [
{"workflow_runs": [{"id": self.RUN, "pull_requests": [{"number": PR}]}]}
Expand Down Expand Up @@ -4808,6 +4865,7 @@ def test_the_evidence_reads_carry_the_read_token_and_are_gets(self) -> None:
self.assertFalse(is_write(args), args)
kinds = (
"/timeline?",
"/activity?",
"/comments?",
"/actions/workflows/",
f"/actions/runs/{self.RUN}",
Expand All @@ -4820,14 +4878,14 @@ def test_the_evidence_reads_carry_the_read_token_and_are_gets(self) -> None:
self.assertTrue(any(kind in args[1] for args in carried), kind)
# Two lanes: listing, run, jobs and one annotations read each; the run
# read is the latest attempt's record.
self.assertEqual(len(carried), 2 + 2 * 4)
self.assertEqual(len(carried), 3 + 2 * 4)
# The commits read is shared with other readers and keeps its token.
others = [args[1] for args, env in calls if env is None]
self.assertEqual(others, [f"repos/owner/repo/pulls/{PR}/commits?per_page=100"])

def test_with_no_read_token_every_read_keeps_the_process_token(self) -> None:
calls = self.calls({})
self.assertEqual(len(calls), 1 + 2 + 2 * 4)
self.assertEqual(len(calls), 1 + 3 + 2 * 4)
self.assertEqual([env for _, env in calls], [None] * len(calls))


Expand Down Expand Up @@ -4863,6 +4921,8 @@ def answer(self, endpoint: str) -> Any:
bot = {"login": IDENTITY}
if endpoint.endswith(f"/pulls/{PR}/commits?per_page=100"):
return [[{"sha": HEAD, "author": bot, "committer": bot}]]
if endpoint.startswith("repos/owner/repo/activity?"):
return [[{"activity_type": "push", "after": HEAD, "actor": bot}]]
if endpoint.endswith("/timeline?per_page=100"):
return [[]]
if endpoint.endswith("/comments?per_page=100"):
Expand Down Expand Up @@ -5042,6 +5102,9 @@ def fake_gh(args: list[str], **_: Any) -> subprocess.CompletedProcess[str]:
payload: Any = record["commits"]
elif endpoint.endswith("/commits?per_page=100"):
payload = [rows]
elif endpoint.startswith("repos/owner/repo/activity?"):
pushed = {"activity_type": "push", "after": HEAD}
payload = [[{**pushed, "actor": {"login": IDENTITY}}]]
elif "--paginate" in args:
payload = [[]]
elif endpoint == f"repos/owner/repo/pulls/{PR}":
Expand Down
Loading