From 8e51adb95562e903460c31ccd4ac775f5c5ca250 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Mon, 20 Jul 2026 22:18:09 +0000 Subject: [PATCH 1/6] Commit results to fork PRs with an optional fork-push token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The collector could only push result files to PR branches in this repository: its GITHUB_TOKEN is scoped to ClickHouse/ClickBench, and GitHub grants the "allow edits from maintainers" push permission only to user accounts with write access, never to App installation tokens. Fork PRs got a pastila.nl link and a request to commit the file by hand. With a classic PAT of such a user in the CLICKBENCH_FORK_PUSH_TOKEN secret, the collector now commits results to fork PRs too, when the author allows maintainer edits. The push clears the http extraheader that actions/checkout stores in the git config (it carries GITHUB_TOKEN and would be sent to the fork and rejected) and supplies the token through an inline credential helper reading the environment variable, so it never appears in a command line or an error message. If the push fails — the author unticked the box meanwhile, the branch moved, the fork protects it — the old paste-link fallback is posted instead of failing the PR. When the token is configured but the author disallows maintainer edits, the comment asks them to tick the box. Without the secret, behavior is unchanged. Co-Authored-By: Claude Fable 5 --- .github/workflows/README.md | 14 ++++++ .github/workflows/collect-results.yml | 4 ++ collect-new-results.py | 68 ++++++++++++++++++++++----- 3 files changed, 75 insertions(+), 11 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 8c5714c45..1aa1c39a7 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -53,3 +53,17 @@ apart from the runs of main and are excluded by `collect-results.sh`. systems. `run-benchmark.sh` waits and retries while the quota or the capacity is exhausted, but only within the job's 55-minute limit; what could not be launched by then is reported as failed. + +4. Optional: a `CLICKBENCH_FORK_PUSH_TOKEN` secret for `collect-results.yml`, + holding a **classic** PAT (`public_repo` scope) of a user with write + access to this repository — a dedicated machine account is recommended, + since a classic PAT can push to every public repository its owner can + write to. With it, the collector commits result files directly to fork + PRs whose author allows maintainer edits. Without it (or when the author + unticked "Allow edits by maintainers", or the fork is organization-owned, + where GitHub does not offer maintainer edits), fork results are posted as + pastila.nl links for the author to commit. The workflow's own + `GITHUB_TOKEN` can never push to forks: GitHub grants the maintainer-edit + push permission only to user accounts, not to App installation tokens. + A fine-grained PAT does not work either — it is bound to an explicit + repository list, which cannot include arbitrary contributors' forks. diff --git a/.github/workflows/collect-results.yml b/.github/workflows/collect-results.yml index c85593898..1c44e5c40 100644 --- a/.github/workflows/collect-results.yml +++ b/.github/workflows/collect-results.yml @@ -22,4 +22,8 @@ jobs: - env: GH_TOKEN: ${{ github.token }} CLICKBENCH_DB_PASSWORD: ${{ secrets.CLICKBENCH_DB_PASSWORD }} + # Optional classic PAT of a user with write access: enables + # committing results to fork PRs that allow maintainer edits + # (GITHUB_TOKEN cannot push to forks). See the README. + CLICKBENCH_FORK_PUSH_TOKEN: ${{ secrets.CLICKBENCH_FORK_PUSH_TOKEN }} run: python3 collect-new-results.py diff --git a/collect-new-results.py b/collect-new-results.py index 39ac787fe..44601f730 100755 --- a/collect-new-results.py +++ b/collect-new-results.py @@ -21,6 +21,13 @@ CLICKBENCH_DB_PASSWORD environment variable / GitHub secret) and talks to GitHub through `gh` (GH_TOKEN). Requires a checkout of the repository as the working directory. Set DRY_RUN=1 to print actions instead of performing them. + +The workflow's GITHUB_TOKEN cannot push to forks, so results for fork PRs +are posted as paste links — unless CLICKBENCH_FORK_PUSH_TOKEN holds a classic +PAT of a user with write access to this repository: GitHub grants the "allow +edits from maintainers" push permission to such user accounts (not to App +installation tokens like GITHUB_TOKEN), so with the token the script commits +results to fork PRs whose author left maintainer edits enabled. """ import json @@ -38,6 +45,7 @@ DB_PASSWORD = os.environ.get("CLICKBENCH_DB_PASSWORD") or "" PASTILA_DB_URL = "https://uzg8q0g12h.eu-central-1.aws.clickhouse.cloud/?user=paste" REPO = os.environ.get("GITHUB_REPOSITORY") or "ClickHouse/ClickBench" +FORK_PUSH_TOKEN = os.environ.get("CLICKBENCH_FORK_PUSH_TOKEN") or "" DRY_RUN = bool(os.environ.get("DRY_RUN")) BOT_NAME = "github-actions[bot]" BOT_EMAIL = "41898282+github-actions[bot]@users.noreply.github.com" @@ -309,8 +317,9 @@ def log_links(run_row): # === git === -def fetch_branch(ref, depth=200): - run("git", "fetch", "-q", f"--depth={depth}", "origin", f"+refs/heads/{ref}") +def fetch_branch(ref, remote="origin", depth=200): + """Fetch a branch from origin or from a fork's public URL.""" + run("git", "fetch", "-q", f"--depth={depth}", remote, f"+refs/heads/{ref}") return run("git", "rev-parse", "FETCH_HEAD") @@ -324,7 +333,8 @@ def result_path(run_row): return "{system}/results/{date}/{machine}.json".format(**run_row) -def commit_results(base_sha, rows, removals, message, push_ref, force=False): +def commit_results(base_sha, rows, removals, message, push_ref, force=False, + remote="origin"): """Create a commit with the result files on top of base_sha and push it. Returns the short commit hash, or None if there was nothing to change.""" worktree = os.path.join(tempfile.mkdtemp(prefix="clickbench-results-"), "wt") @@ -343,11 +353,25 @@ def commit_results(base_sha, rows, removals, message, push_ref, force=False): run("git", "-C", worktree, "-c", f"user.name={BOT_NAME}", "-c", f"user.email={BOT_EMAIL}", "commit", "-q", "-m", message) sha = run("git", "-C", worktree, "rev-parse", "HEAD") + auth = [] + if remote != "origin": + # A push to a fork authenticates with the fork-push token. Two + # subtleties: actions/checkout stores the workflow's GITHUB_TOKEN + # as an http..extraheader in the local git config, which + # would be sent to the fork too (and rejected), so it is cleared + # for this push; and the token is handed over through an inline + # credential helper that reads the environment variable, so it + # never appears in a command line or in an error message. + auth = ["-c", "http.https://github.com/.extraheader=", + "-c", "credential.helper=", + "-c", "credential.helper=!f() { echo username=x-access-token;" + ' echo "password=${CLICKBENCH_FORK_PUSH_TOKEN}"; }; f'] if DRY_RUN: - print(f"DRY_RUN: would push {sha} to {push_ref}") + print(f"DRY_RUN: would push {sha} to {remote} {push_ref}") else: - run("git", "-C", worktree, "push", "-q", *(["--force"] if force else []), - "origin", f"HEAD:refs/heads/{push_ref}") + run("git", *auth, "-C", worktree, "push", "-q", + *(["--force"] if force else []), + remote, f"HEAD:refs/heads/{push_ref}") return sha[:10] finally: run("git", "worktree", "remove", "--force", worktree, check=False) @@ -389,20 +413,39 @@ def process_pr(pr_number, rows): head_repo = (meta.get("head") or {}).get("repo") or {} same_repo = head_repo.get("full_name") == REPO - can_commit = good and meta["state"] == "open" and same_repo + # Forks are writable only with the fork-push token (see the module + # docstring) and only while the PR author allows maintainer edits — + # which GitHub does not offer at all for organization-owned forks. + fork_push = (bool(FORK_PUSH_TOKEN) and not same_repo + and bool(head_repo.get("full_name")) + and bool(meta.get("maintainer_can_modify"))) + can_commit = bool(good) and meta["state"] == "open" and (same_repo or fork_push) commit = None removals = [] if can_commit: head_ref = meta["head"]["ref"] - base_sha = fetch_branch(head_ref) + remote = ("origin" if same_repo + else f"https://github.com/{head_repo['full_name']}.git") + base_sha = fetch_branch(head_ref, remote=remote) systems = sorted({r["system"] for r in good}) bot_paths = {result_path(r) for r in good} removals = manual_result_files(pr_number, systems, bot_paths) machines = ", ".join(sorted({r["machine"] for r in good})) - commit = commit_results(base_sha, good, removals, - f"Add benchmark results for {', '.join(systems)} ({machines})", - head_ref) + try: + commit = commit_results(base_sha, good, removals, + f"Add benchmark results for {', '.join(systems)} ({machines})", + head_ref, remote=remote) + except RuntimeError as e: + # A fork push can fail even though maintainer_can_modify said + # yes: the author may have unticked it meanwhile, the branch may + # have moved, or the fork may protect it. Fall back to posting + # paste links instead of failing the whole PR. + if same_repo: + raise + note(f"PR #{pr_number}: pushing to the fork failed: {e}") + can_commit = False + removals = [] lines = [] if good: @@ -421,6 +464,9 @@ def process_pr(pr_number, rows): url = pastila_post(r["output"]) lines.append(f"This pull request is from a fork, so the automation cannot " f"push to it; save [this result]({url}) as `{result_path(r)}`.") + if FORK_PUSH_TOKEN and not meta.get("maintainer_can_modify"): + lines.append('Tick "Allow edits by maintainers" on the pull request ' + "to let the automation commit the results itself.") if removals: lines.append("Removed manually added result files: " + ", ".join(f"`{path}`" for path in removals) + ".") From c142c3181a53a7652534723ff6046d96b54eb571 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Mon, 20 Jul 2026 22:42:03 +0000 Subject: [PATCH 2/6] Use the org-level robot-clickhouse token for fork pushes `ROBOT_CLICKHOUSE_COMMIT_TOKEN` is an org secret already shared with this repository, so no new secret or machine account is needed. The `robot-clickhouse` account currently has read-only access here and must be granted write access for the fork pushes to be accepted; until then the collector falls back to paste links as before. Co-Authored-By: Claude Fable 5 --- .github/workflows/README.md | 28 ++++++++++++++------------- .github/workflows/collect-results.yml | 10 ++++++---- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 1aa1c39a7..f58688e18 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -54,16 +54,18 @@ apart from the runs of main and are excluded by `collect-results.sh`. capacity is exhausted, but only within the job's 55-minute limit; what could not be launched by then is reported as failed. -4. Optional: a `CLICKBENCH_FORK_PUSH_TOKEN` secret for `collect-results.yml`, - holding a **classic** PAT (`public_repo` scope) of a user with write - access to this repository — a dedicated machine account is recommended, - since a classic PAT can push to every public repository its owner can - write to. With it, the collector commits result files directly to fork - PRs whose author allows maintainer edits. Without it (or when the author - unticked "Allow edits by maintainers", or the fork is organization-owned, - where GitHub does not offer maintainer edits), fork results are posted as - pastila.nl links for the author to commit. The workflow's own - `GITHUB_TOKEN` can never push to forks: GitHub grants the maintainer-edit - push permission only to user accounts, not to App installation tokens. - A fine-grained PAT does not work either — it is bound to an explicit - repository list, which cannot include arbitrary contributors' forks. +4. For committing results to fork PRs, `collect-results.yml` uses the + org-level `ROBOT_CLICKHOUSE_COMMIT_TOKEN` secret — a **classic** PAT of + the `robot-clickhouse` machine account, which must have **write access + to this repository** (GitHub grants the "allow edits from maintainers" + push permission to user accounts with write access to the base repo). + With that in place, the collector commits result files directly to fork + PRs whose author allows maintainer edits. Otherwise (no token, no write + access, the author unticked "Allow edits by maintainers", or the fork is + organization-owned, where GitHub does not offer maintainer edits), fork + results are posted as pastila.nl links for the author to commit. The + workflow's own `GITHUB_TOKEN` can never push to forks: the maintainer-edit + push permission is granted only to user accounts, not to App installation + tokens. A fine-grained PAT does not work either — it is bound to an + explicit repository list, which cannot include arbitrary contributors' + forks. diff --git a/.github/workflows/collect-results.yml b/.github/workflows/collect-results.yml index 1c44e5c40..16e479682 100644 --- a/.github/workflows/collect-results.yml +++ b/.github/workflows/collect-results.yml @@ -22,8 +22,10 @@ jobs: - env: GH_TOKEN: ${{ github.token }} CLICKBENCH_DB_PASSWORD: ${{ secrets.CLICKBENCH_DB_PASSWORD }} - # Optional classic PAT of a user with write access: enables - # committing results to fork PRs that allow maintainer edits - # (GITHUB_TOKEN cannot push to forks). See the README. - CLICKBENCH_FORK_PUSH_TOKEN: ${{ secrets.CLICKBENCH_FORK_PUSH_TOKEN }} + # Classic PAT of a user with write access to this repository: + # enables committing results to fork PRs that allow maintainer + # edits (GITHUB_TOKEN cannot push to forks). The org-level + # robot-clickhouse token is used; the robot needs write access + # here, or fork results fall back to paste links. See the README. + CLICKBENCH_FORK_PUSH_TOKEN: ${{ secrets.ROBOT_CLICKHOUSE_COMMIT_TOKEN }} run: python3 collect-new-results.py From f3a5a00709b6e64ed6f57651c3b32b2a5dfb4c0f Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Mon, 20 Jul 2026 22:49:52 +0000 Subject: [PATCH 3/6] Temporary workflow to test the fork push (to be removed) Co-Authored-By: Claude Fable 5 --- .github/workflows/test-fork-push.yml | 29 ++++++++++++++ test-fork-push.py | 56 ++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 .github/workflows/test-fork-push.yml create mode 100644 test-fork-push.py diff --git a/.github/workflows/test-fork-push.yml b/.github/workflows/test-fork-push.yml new file mode 100644 index 000000000..335d65c1a --- /dev/null +++ b/.github/workflows/test-fork-push.yml @@ -0,0 +1,29 @@ +name: "Test fork push" + +# Temporary workflow to test committing results to a fork PR with the +# robot-clickhouse token. To be removed once the test passes. +on: + workflow_dispatch: + inputs: + pr: + description: "Fork PR number" + required: true + comment: + description: "ID of the collect-results comment with the paste links" + required: true + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + - env: + GH_TOKEN: ${{ github.token }} + CLICKBENCH_FORK_PUSH_TOKEN: ${{ secrets.ROBOT_CLICKHOUSE_COMMIT_TOKEN }} + PR_NUMBER: ${{ inputs.pr }} + COMMENT_ID: ${{ inputs.comment }} + run: python3 test-fork-push.py "$PR_NUMBER" "$COMMENT_ID" diff --git a/test-fork-push.py b/test-fork-push.py new file mode 100644 index 000000000..407b10cd0 --- /dev/null +++ b/test-fork-push.py @@ -0,0 +1,56 @@ +#!/usr/bin/env python3 +"""One-off driver to test the fork-push path end to end: read a +collect-results comment, download the paste contents, and commit them to +the PR's fork through the same functions collect-new-results.py uses. +Temporary — to be removed once the test passes.""" + +import importlib.util +import json +import re +import sys + +spec = importlib.util.spec_from_file_location("collect", "collect-new-results.py") +m = importlib.util.module_from_spec(spec) +spec.loader.exec_module(m) + +pr_number, comment_id = sys.argv[1], sys.argv[2] +assert pr_number.isdigit() and comment_id.isdigit() + +body = m.gh("api", f"repos/{m.REPO}/issues/comments/{comment_id}", "--jq", ".body") +pairs = re.findall(r"save \[this result\]\(https://pastila\.nl/" + r"\?([0-9a-f]+)/([0-9a-f]+)\) as `([^`]+)`", body) +assert pairs, "no 'save this result' links found in the comment" + +rows = [] +for fingerprint, content_hash, path in pairs: + path_match = re.match(r"([A-Za-z0-9_-]+)/results/(\d{8})/([a-z0-9._-]+)\.json$", + path) + assert path_match, f"unexpected result path: {path}" + system, date, machine = path_match.groups() + response = m.http_post( + m.PASTILA_DB_URL, + f"SELECT content FROM data_view(fingerprint = '{fingerprint}', " + f"hash = '{content_hash}') FORMAT JSON") + content = json.loads(response)["data"][0]["content"] + json.loads(content) # the result file must be valid JSON + rows.append({"system": system, "date": date, "machine": machine, + "output": content}) + print(f"fetched {path}: {len(content)} bytes") + +meta = m.pr_meta(pr_number) +head_repo = meta["head"]["repo"]["full_name"] +assert head_repo != m.REPO, "this test is for fork PRs" +assert meta["maintainer_can_modify"], "the PR does not allow maintainer edits" + +head_ref = meta["head"]["ref"] +remote = f"https://github.com/{head_repo}.git" +base_sha = m.fetch_branch(head_ref, remote=remote) +print(f"fork head: {base_sha}") + +systems = sorted({r["system"] for r in rows}) +machines = ", ".join(sorted({r["machine"] for r in rows})) +commit = m.commit_results( + base_sha, rows, [], + f"Add benchmark results for {', '.join(systems)} ({machines})", + head_ref, remote=remote) +print(f"pushed commit: {commit}") From 4532951ce6392729504c22e4404aaffe20618630 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Mon, 20 Jul 2026 22:51:48 +0000 Subject: [PATCH 4/6] Temporarily point collect-results at the fork-push test (to be reverted) Co-Authored-By: Claude Fable 5 --- .github/workflows/collect-results.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/collect-results.yml b/.github/workflows/collect-results.yml index 16e479682..30415ce55 100644 --- a/.github/workflows/collect-results.yml +++ b/.github/workflows/collect-results.yml @@ -28,4 +28,6 @@ jobs: # robot-clickhouse token is used; the robot needs write access # here, or fork results fall back to paste links. See the README. CLICKBENCH_FORK_PUSH_TOKEN: ${{ secrets.ROBOT_CLICKHOUSE_COMMIT_TOKEN }} - run: python3 collect-new-results.py + # Temporarily hijacked to test the fork push on this branch: + # dispatched with --ref push-results-to-forks. To be reverted. + run: python3 test-fork-push.py 982 5027900810 From c61fd913260f6282477214fdfbd1f5672f8e6491 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Mon, 20 Jul 2026 22:53:50 +0000 Subject: [PATCH 5/6] Revert "Temporarily point collect-results at the fork-push test (to be reverted)" This reverts commit 4532951ce6392729504c22e4404aaffe20618630. --- .github/workflows/collect-results.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/collect-results.yml b/.github/workflows/collect-results.yml index 30415ce55..16e479682 100644 --- a/.github/workflows/collect-results.yml +++ b/.github/workflows/collect-results.yml @@ -28,6 +28,4 @@ jobs: # robot-clickhouse token is used; the robot needs write access # here, or fork results fall back to paste links. See the README. CLICKBENCH_FORK_PUSH_TOKEN: ${{ secrets.ROBOT_CLICKHOUSE_COMMIT_TOKEN }} - # Temporarily hijacked to test the fork push on this branch: - # dispatched with --ref push-results-to-forks. To be reverted. - run: python3 test-fork-push.py 982 5027900810 + run: python3 collect-new-results.py From 806a457305cdcf25484d611ddc473d45c368cdbf Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Mon, 20 Jul 2026 22:53:53 +0000 Subject: [PATCH 6/6] Revert "Temporary workflow to test the fork push (to be removed)" This reverts commit f3a5a00709b6e64ed6f57651c3b32b2a5dfb4c0f. --- .github/workflows/test-fork-push.yml | 29 -------------- test-fork-push.py | 56 ---------------------------- 2 files changed, 85 deletions(-) delete mode 100644 .github/workflows/test-fork-push.yml delete mode 100644 test-fork-push.py diff --git a/.github/workflows/test-fork-push.yml b/.github/workflows/test-fork-push.yml deleted file mode 100644 index 335d65c1a..000000000 --- a/.github/workflows/test-fork-push.yml +++ /dev/null @@ -1,29 +0,0 @@ -name: "Test fork push" - -# Temporary workflow to test committing results to a fork PR with the -# robot-clickhouse token. To be removed once the test passes. -on: - workflow_dispatch: - inputs: - pr: - description: "Fork PR number" - required: true - comment: - description: "ID of the collect-results comment with the paste links" - required: true - -permissions: - contents: read - -jobs: - test: - runs-on: ubuntu-latest - timeout-minutes: 10 - steps: - - uses: actions/checkout@v4 - - env: - GH_TOKEN: ${{ github.token }} - CLICKBENCH_FORK_PUSH_TOKEN: ${{ secrets.ROBOT_CLICKHOUSE_COMMIT_TOKEN }} - PR_NUMBER: ${{ inputs.pr }} - COMMENT_ID: ${{ inputs.comment }} - run: python3 test-fork-push.py "$PR_NUMBER" "$COMMENT_ID" diff --git a/test-fork-push.py b/test-fork-push.py deleted file mode 100644 index 407b10cd0..000000000 --- a/test-fork-push.py +++ /dev/null @@ -1,56 +0,0 @@ -#!/usr/bin/env python3 -"""One-off driver to test the fork-push path end to end: read a -collect-results comment, download the paste contents, and commit them to -the PR's fork through the same functions collect-new-results.py uses. -Temporary — to be removed once the test passes.""" - -import importlib.util -import json -import re -import sys - -spec = importlib.util.spec_from_file_location("collect", "collect-new-results.py") -m = importlib.util.module_from_spec(spec) -spec.loader.exec_module(m) - -pr_number, comment_id = sys.argv[1], sys.argv[2] -assert pr_number.isdigit() and comment_id.isdigit() - -body = m.gh("api", f"repos/{m.REPO}/issues/comments/{comment_id}", "--jq", ".body") -pairs = re.findall(r"save \[this result\]\(https://pastila\.nl/" - r"\?([0-9a-f]+)/([0-9a-f]+)\) as `([^`]+)`", body) -assert pairs, "no 'save this result' links found in the comment" - -rows = [] -for fingerprint, content_hash, path in pairs: - path_match = re.match(r"([A-Za-z0-9_-]+)/results/(\d{8})/([a-z0-9._-]+)\.json$", - path) - assert path_match, f"unexpected result path: {path}" - system, date, machine = path_match.groups() - response = m.http_post( - m.PASTILA_DB_URL, - f"SELECT content FROM data_view(fingerprint = '{fingerprint}', " - f"hash = '{content_hash}') FORMAT JSON") - content = json.loads(response)["data"][0]["content"] - json.loads(content) # the result file must be valid JSON - rows.append({"system": system, "date": date, "machine": machine, - "output": content}) - print(f"fetched {path}: {len(content)} bytes") - -meta = m.pr_meta(pr_number) -head_repo = meta["head"]["repo"]["full_name"] -assert head_repo != m.REPO, "this test is for fork PRs" -assert meta["maintainer_can_modify"], "the PR does not allow maintainer edits" - -head_ref = meta["head"]["ref"] -remote = f"https://github.com/{head_repo}.git" -base_sha = m.fetch_branch(head_ref, remote=remote) -print(f"fork head: {base_sha}") - -systems = sorted({r["system"] for r in rows}) -machines = ", ".join(sorted({r["machine"] for r in rows})) -commit = m.commit_results( - base_sha, rows, [], - f"Add benchmark results for {', '.join(systems)} ({machines})", - head_ref, remote=remote) -print(f"pushed commit: {commit}")