Skip to content
Merged
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
7 changes: 7 additions & 0 deletions .changes/pr-pipeline/perf-pr-merge-read-token-fd7bfe9c.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
bump: patch
---

### Changed

- pr-merge.yml's merge job runs its read-only post-merge check under the job's own `GITHUB_TOKEN` (the job gains `issues: read` for the demotion records) instead of the lane App token; only the `--remediate` run keeps the lane token. The gate's review-evidence reads run under `BABYSIT_VERIFY_GH_TOKEN` too, so the lane token is spent on writes and the reads that need the App's identity.
7 changes: 7 additions & 0 deletions .changes/source-control/perf-pr-merge-read-token-70c894c1.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
bump: patch
---

### Changed

- The unattended merge gate reads its review evidence (the pull request's timeline and comments, and the review-lane runs with their attempts, jobs and annotations) under `BABYSIT_VERIFY_GH_TOKEN` when it is set, as the verdict reader's Actions reads already do, so those GETs no longer spend the lane App's rate limit. Every write keeps the process token; without the variable every read does too. `gh_json`, `fetch_paginated_api` and `collect_lane_runs` take an optional `env_overrides`.
16 changes: 16 additions & 0 deletions .github/scripts/pr-merge/merge-job.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,22 @@ test("a failed lane merge reruns the check with --remediate under the lane token
assert.match(runScript("Check earlier lane merges"), /^echo "code=\$code" >>"\$GITHUB_OUTPUT"$/m);
});

// Only --remediate writes, so it alone takes the lane token. The read-only
// check and the gate's evidence reads spend the job's own token, which this
// job grants no write.
test("the read-only post-merge check and the gate's reads use the job's read-only token", () => {
assert.match(step("Check earlier lane merges"), /\n {10}GH_TOKEN: \$\{\{ github\.token \}\}\n/);
assert.doesNotMatch(runScript("Check earlier lane merges"), /--remediate/);
assert.match(step("Record a failed lane merge"), /\n {10}GH_TOKEN: \$\{\{ steps\.lane-token\.outputs\.token \}\}\n/);
const gate = step("Run the merge gate on each candidate");
assert.match(gate, /\n {10}GH_TOKEN: \$\{\{ steps\.lane-token\.outputs\.token \}\}\n/);
assert.match(gate, /\n {10}BABYSIT_VERIFY_GH_TOKEN: \$\{\{ github\.token \}\}\n/);
const permissions = /\n permissions:\n([\s\S]*?)\n [a-z-]+:/.exec(job)[1];
const grants = [...permissions.matchAll(/^ {6}([a-z-]+): ([a-z]+)/gm)].map((m) => `${m[1]}: ${m[2]}`);
assert.deepEqual(grants.filter((g) => g.endsWith(": write")), ["id-token: write"]);
assert.ok(grants.includes("issues: read"), grants.join(", "));
});

// Runs the record step against a fake python3 that saves its arguments,
// prints `report` and exits `code`.
const runRecord = (code, report = { remediation: { errors: [] } }) => {
Expand Down
17 changes: 11 additions & 6 deletions .github/workflows/pr-merge.yml
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ jobs:
# Read a refused lane run's annotations and its PR head's checks. Never
# write: the gate-check job below writes the check runs.
checks: read
issues: read # the post-merge check's demotion records, read with this job's token
id-token: write # prove to the token broker which workflow this is
outputs:
# `true` when the gate's rows were handed to the gate-check job.
Expand Down Expand Up @@ -638,12 +639,14 @@ jobs:
# (--postmerge-result); a PR that merges during the run is in the
# gate's own `dispatch` output. The post-merge-dispatch job runs the
# same check to start the full run an earlier merge needs, so neither a
# failed listing nor a long gate loop holds that run back.
# failed listing nor a long gate loop holds that run back. Read-only,
# so it reads with this job's own token, off the lane App's quota; only
# --remediate below needs the lane token.
- name: Check earlier lane merges
id: post-merge
if: steps.lane-token.outputs.token != ''
env:
GH_TOKEN: ${{ steps.lane-token.outputs.token }}
GH_TOKEN: ${{ github.token }}
REPO: ${{ github.repository }}
run: |
set -euo pipefail
Expand Down Expand Up @@ -767,10 +770,12 @@ jobs:
if: steps.candidates.outcome == 'success'
env:
GH_TOKEN: ${{ steps.lane-token.outputs.token }}
# Only the verdict reader's Actions calls and the whole-review
# request's limit reads use this, as their own GH_TOKEN
# (babysit_verify_evidence.py, babysit_whole_review.py); the job
# runs main's code alone, never a PR's.
# The verdict reader's Actions calls, the review-evidence reads
# (timeline, comments, lane runs, attempts, jobs, annotations) and
# the whole-review request's limit reads use this, as their own
# GH_TOKEN (babysit_verify_evidence.py, babysit_merge.py
# _lane_evidence, babysit_whole_review.py); every write keeps the
# lane token. The job runs main's code alone, never a PR's.
BABYSIT_VERIFY_GH_TOKEN: ${{ github.token }}
# At most one whole-review request per run: the first candidate to
# reach its limits creates this file (babysit_whole_review.py).
Expand Down
6 changes: 4 additions & 2 deletions docs/conventions/pr-pipeline/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1299,8 +1299,10 @@ saved, and is kept for that candidate's gate run: the pull request and every que
listing being paged again, so a listing outage holds the whole queue rather than merging blind.
The lane token holds no Actions permission, so pr-merge.yml grants its job `actions: read` and passes the job's `GITHUB_TOKEN` as
`BABYSIT_VERIFY_GH_TOKEN`; the reader uses it as `GH_TOKEN` for the artifact listing, the run
records, the download and the compare of a run's head to main (a `contents: read` call) alone, and
every other call keeps the lane token. That job checks out only main and runs no pull request code.
records, the download and the compare of a run's head to main (a `contents: read` call). The gate's
review-evidence reads use it too: the timeline, the comments, and the lane runs with their attempts,
jobs and annotations. Every other call, every write included, keeps the lane token. That job checks
out only main and runs no pull request code.

pr-merge.yml also names a file in `BABYSIT_RUN_CACHE`, where the run's gate processes share reads
that are the same for every candidate. The acting login and a credential pass are reused only under
Expand Down
48 changes: 31 additions & 17 deletions plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,10 @@ def gh_capture(
)


def gh_json(args: list[str]) -> Any:
text = run_gh(args)
def gh_json(args: list[str], env_overrides: Mapping[str, str] | None = None) -> Any:
text = _run_gh(
args, timeout_seconds=None, check=True, env_overrides=env_overrides
).stdout
return json.loads(text) if text.strip() else None


Expand Down Expand Up @@ -945,11 +947,15 @@ def flatten_paginated_items(


def fetch_paginated_api(
endpoint: str, label: str, *, items_key: str | None = None
endpoint: str,
label: str,
*,
items_key: str | None = None,
env_overrides: Mapping[str, str] | None = None,
) -> list[dict[str, Any]]:
return flatten_paginated_items(
gh_json(["api", endpoint, "--paginate", "--slurp"]), label, items_key=items_key
)
args = ["api", endpoint, "--paginate", "--slurp"]
data = gh_json(args, env_overrides) if env_overrides else gh_json(args)
return flatten_paginated_items(data, label, items_key=items_key)


def normalized_rest_author(item: dict[str, Any]) -> dict[str, Any]:
Expand Down Expand Up @@ -1319,6 +1325,7 @@ def collect_lane_runs(
head_ref: str,
lanes: Iterable[tuple[str, str]],
created_at: str = "",
env_overrides: Mapping[str, str] | None = None,
) -> list[dict[str, Any]]:
"""The `runs` input of `babysit_review_evidence.bind()` for one pull request.

Expand All @@ -1333,8 +1340,19 @@ def collect_lane_runs(
from a fresh `GET actions/runs/{id}` taken after the listing; each attempt
carries its jobs, and each lane job its check run's annotations. Any read
failure, a deleted or expired run included, raises `RuntimeError`, which
the caller turns into unbound evidence.
the caller turns into unbound evidence. `env_overrides` applies to every
read, all of them GETs.
"""
env = env_overrides

def read(path: str) -> Any:
return gh_json(["api", path], env) if env else gh_json(["api", path])

def pages(endpoint: str, label: str, items_key: str | None = None) -> list[Any]:
return fetch_paginated_api(
endpoint, label, items_key=items_key, env_overrides=env
)

runs: list[dict[str, Any]] = []
since = (
f"&created={quote('>=' + created_at, safe='')}"
Expand All @@ -1343,12 +1361,12 @@ def collect_lane_runs(
)
for path, job_suffix in lanes:
workflow = quote(path.rsplit("/", 1)[-1], safe="")
listing = fetch_paginated_api(
listing = pages(
f"repos/{repo}/actions/workflows/{workflow}/runs"
f"?branch={quote(head_ref, safe='')}&event=pull_request{since}"
"&per_page=100",
f"{workflow} runs",
items_key="workflow_runs",
"workflow_runs",
)
for item in listing:
if not any(
Expand All @@ -1359,22 +1377,18 @@ def collect_lane_runs(
run_id = item.get("id")
if not isinstance(run_id, int) or isinstance(run_id, bool):
raise RuntimeError(f"{workflow} run without an id")
latest = json_object(
gh_json(["api", f"repos/{repo}/actions/runs/{run_id}"])
)
latest = json_object(read(f"repos/{repo}/actions/runs/{run_id}"))
count = latest.get("run_attempt")
if not isinstance(count, int) or isinstance(count, bool) or count < 1:
raise RuntimeError(f"run {run_id}: no attempt count")
attempts = []
for n in range(1, count + 1):
base = f"repos/{repo}/actions/runs/{run_id}/attempts/{n}"
attempt = json_object(gh_json(["api", base]))
jobs = fetch_paginated_api(
f"{base}/jobs?per_page=100", f"run {run_id} jobs", items_key="jobs"
)
attempt = json_object(read(base))
jobs = pages(f"{base}/jobs?per_page=100", f"run {run_id} jobs", "jobs")
for job in jobs:
if str(job.get("name") or "").rsplit(" / ", 1)[-1] == job_suffix:
job["annotations"] = fetch_paginated_api(
job["annotations"] = pages(
f"repos/{repo}/check-runs/{job.get('id')}/annotations"
"?per_page=100",
f"job {job.get('id')} annotations",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2852,10 +2852,18 @@ def _lane_evidence(
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."""
more commits than the list returns, 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;
every read here is a GET."""
head = json_object(pr.get("head"))
token = os.environ.get(verify_evidence.TOKEN_ENV)
read_env = {"GH_TOKEN": token} if token else None
timeline = fetch_paginated_api(
f"repos/{repo}/issues/{number}/timeline?per_page=100", f"#{number} timeline"
f"repos/{repo}/issues/{number}/timeline?per_page=100",
f"#{number} timeline",
env_overrides=read_env,
)
commits = pull_request_commits_once(
repo,
Expand Down Expand Up @@ -2895,7 +2903,9 @@ def _lane_evidence(
excluded.add(str(json_object(pr.get("user")).get("login") or "").casefold())
excluded.discard("")
comments = fetch_paginated_api(
f"repos/{repo}/issues/{number}/comments?per_page=100", f"#{number} comments"
f"repos/{repo}/issues/{number}/comments?per_page=100",
f"#{number} comments",
env_overrides=read_env,
)
try:
threads: list[Any] | None = fetch_review_threads(
Expand All @@ -2909,6 +2919,7 @@ def _lane_evidence(
str(head.get("ref")),
review_evidence.LANES.values(),
str(pr.get("created_at") or ""),
read_env,
)

def human(actor: Any) -> bool:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1221,7 +1221,9 @@ def fake(args: list[str]) -> Any:
)
self.assertEqual([r["id"] for r in runs], [7])

def test_a_creation_time_that_is_not_a_utc_timestamp_leaves_the_listing_unbounded(self) -> None:
def test_a_creation_time_that_is_not_a_utc_timestamp_leaves_the_listing_unbounded(
self,
) -> None:
answers = self.answers(fresh_attempts=1)

def fake(args: list[str]) -> Any:
Expand All @@ -1239,6 +1241,28 @@ def test_a_deleted_or_expired_attempt_raises(self) -> None:
with self.assertRaises(RuntimeError):
self.collect(answers)

def reads_under(self, env: dict[str, str] | None) -> list[tuple[str, Any]]:
answers = self.answers()
reads: list[tuple[str, Any]] = []

def fake(args: list[str], env_overrides: Any = None) -> Any:
reads.append((args[1], env_overrides))
return answers[args[1]]

with mock.patch.object(gh, "gh_json", side_effect=fake):
gh.collect_lane_runs(self.REPO, 5, "feat/x", self.LANES, env_overrides=env)
# The listing, the run, and two attempts' record, jobs and annotations.
self.assertEqual(sorted(e for e, _ in reads), sorted(answers))
return reads

def test_every_read_runs_under_the_token_it_is_given(self) -> None:
reads = self.reads_under({"GH_TOKEN": "read"})
self.assertEqual([env for _, env in reads], [{"GH_TOKEN": "read"}] * 8)

def test_with_no_token_every_read_keeps_the_process_token(self) -> None:
reads = self.reads_under(None)
self.assertEqual([env for _, env in reads], [None] * 8)


if __name__ == "__main__":
unittest.main()
Loading
Loading