From 8c6ad054b931b754bab6871c84524cc2442bb2c4 Mon Sep 17 00:00:00 2001 From: teahubxyz Date: Tue, 13 Oct 2026 21:33:35 -0700 Subject: [PATCH] security: a real CI check before auto-merge; .github/ always protected (#46) - checks(): the worker's own time/worker-run status can block or delay a merge but never counts as green - dispatch_ci(): with no other check on the head, Time runs TIME_CI_WORKFLOW (ci.yml, workflow_dispatch) on the PR branch and waits; beat ci-dispatched, once - ci.yml accepts workflow_dispatch - touches_protected(): .github/ is always protected, whatever TIME_PROTECTED says - labs-aicharts.yml: deploy URL via env, not ${{ }} in the script - tests: worker-status-only no longer merges; dispatch once; real context merges Claude-Session: https://claude.ai/code/session_01CYHXFvWMCs1GUmRmpySqQ6 Co-authored-by: claude --- .github/time/test_time.py | 18 +++++++++---- .github/time/time.py | 42 +++++++++++++++++++++++------ .github/workflows/ci.yml | 1 + .github/workflows/labs-aicharts.yml | 3 ++- CHANGELOG.md | 3 +++ 5 files changed, 53 insertions(+), 14 deletions(-) diff --git a/.github/time/test_time.py b/.github/time/test_time.py index dffdade..9816c78 100644 --- a/.github/time/test_time.py +++ b/.github/time/test_time.py @@ -84,6 +84,7 @@ class World: if path.endswith("/status"): return {"statuses": next(d.get("statuses", []) for d in self.gh_pr.values() if d["pr"]["head"]["sha"] in path)} if "/statuses/" in path and method == "POST": self.statuses = getattr(self, "statuses", []) + [body]; return {} + if "/actions/workflows/" in path and path.endswith("/dispatches"): return {} if "/issues?state=open" in path: lab = path.split("labels=")[1].split("&")[0] return [{"number": i["number"], "title": i.get("title", ""), "body": i.get("body", ""), "state": "open"} @@ -107,7 +108,7 @@ def beat(state, h=0, **kw): def run(world, env): for k in ("TIME_MODE", "CURSOR_API_KEY", "TIME_REVIEW", "TIME_DAILY_CAP", "TIME_CURSOR_MODEL", "TIME_AUTOMERGE", - "TIME_REQUIRE_CHECKS", "VENICE_API_KEY"): + "TIME_REQUIRE_CHECKS", "VENICE_API_KEY", "TIME_CI_WORKFLOW"): os.environ.pop(k, None) os.environ.update(env) tm.http = world.http @@ -209,7 +210,7 @@ assert "couldn't find cards" in w.posted[0][1] and w.issues[1]["labels"] == [] # 13 landing: Approve from a trusted reviewer + green checks -> squash merge, card closed, next card dispatched def pr_world(verdict_body="**Approve** looks right", assoc="NONE", bot=True, checks=None, files=None, extra=None): - prd = {"number": 7, "state": "open", "merged_at": None, "head": {"sha": "abc"}, "html_url": "u", "created_at": iso(2)} + prd = {"number": 7, "state": "open", "merged_at": None, "head": {"sha": "abc", "ref": "time/card-1"}, "html_url": "u", "created_at": iso(2)} issues = {2: {"number": 2, "title": "next", "body": "Depends on #1\n\nDONE: d", "state": "open", "labels": ["queued"]}} cm = {1: [beat("pr", 1, pr=7)], 7: [{"created_at": iso(), "author_association": assoc, "user": {"login": "github-actions[bot]" if bot else "someone", "type": "Bot" if bot else "User"}, "body": verdict_body}]} @@ -234,6 +235,8 @@ assert not any(c[0] == "PUT" for c in w.calls) and "protected paths" in w.posted w = run(pr_world(checks=[{"status": "completed", "conclusion": "failure"}]), {**K, "TIME_AUTOMERGE": "on"}) assert not any(c[0] == "PUT" for c in w.calls) and "checks are failing" in w.posted[0][1] w = run(pr_world(checks=[]), {**K, "TIME_AUTOMERGE": "on"}) +assert not any(c[0] == "PUT" for c in w.calls) and "running CI" in w.posted[0][1] # no checks yet: Time dispatches CI +w = run(pr_world(checks=[]), {**K, "TIME_AUTOMERGE": "on", "TIME_CI_WORKFLOW": "off"}) assert not any(c[0] == "PUT" for c in w.calls) and "no checks" in w.posted[0][1] w = run(pr_world(checks=[]), {**K, "TIME_AUTOMERGE": "on", "TIME_REQUIRE_CHECKS": "off"}); assert any(c[0] == "PUT" for c in w.calls) # pending checks -> silent wait @@ -260,11 +263,16 @@ w3 = run(World([card(1, D)], {1: [beat("would-dispatch")]}), {"TIME_MODE": "live assert "VENICE_API_KEY" in w3.posted[-1][1] print("ALL OK 4") -# 15 a commit status from the worker's own test run counts as the check +# 15 the worker's own status can block a merge but never earns one: Time dispatches CI instead, then merges on it w = pr_world(checks=[]); w.gh_pr[7]["statuses"] = [{"state": "success", "context": "time/worker-run"}] -w = run(w, {**K, "TIME_AUTOMERGE": "on"}); assert any(c[0] == "PUT" for c in w.calls), "status should count as a green check" +w = run(w, {**K, "TIME_AUTOMERGE": "on"}); assert not any(c[0] == "PUT" for c in w.calls), "worker status alone is not green" +assert any("/actions/workflows/ci.yml/dispatches" in c[1] for c in w.calls) and "running CI" in w.posted[0][1] w = pr_world(checks=[]); w.gh_pr[7]["statuses"] = [{"state": "failure", "context": "time/worker-run"}] -w = run(w, {**K, "TIME_AUTOMERGE": "on"}); assert not any(c[0] == "PUT" for c in w.calls) +w = run(w, {**K, "TIME_AUTOMERGE": "on"}); assert not any(c[0] == "PUT" for c in w.calls) and "failing" in w.posted[0][1] +w = pr_world(checks=[]); w.gh_pr[7]["statuses"] = [{"state": "success", "context": "time/worker-run"}, {"state": "success", "context": "ci"}] +w = run(w, {**K, "TIME_AUTOMERGE": "on"}); assert any(c[0] == "PUT" for c in w.calls), "a real context counts" +w = pr_world(checks=[]); w.comments[1] = [beat("ci-dispatched", 0, pr=7)] +w = run(w, {**K, "TIME_AUTOMERGE": "on"}); assert not any("/dispatches" in c[1] for c in w.calls), "dispatch once" # a verdict posted in the same run as the `pr` beat still counts (window starts at PR creation) w = pr_world(); w.comments[1] = [beat("pr", 0, pr=7)] w = run(w, {**K, "TIME_AUTOMERGE": "on"}); assert any(c[0] == "PUT" for c in w.calls) diff --git a/.github/time/time.py b/.github/time/time.py index d3aa61a..dd526ac 100644 --- a/.github/time/time.py +++ b/.github/time/time.py @@ -328,6 +328,7 @@ def verdict(pr: dict, since: str) -> str | None: def touches_protected(pr_number: int) -> list[str]: paths = [p.strip() for p in (os.environ.get("TIME_PROTECTED") or DEFAULT_PROTECTED).split(",") if p.strip()] + paths = list(dict.fromkeys([".github/", *paths])) # workflows stay a human's merge whatever TIME_PROTECTED says files, page = [], 1 while True: # every file, not the first 100; a rename counts under both names batch = gh("GET", f"/repos/{REPO}/pulls/{pr_number}/files?per_page=100&page={page}") @@ -341,16 +342,37 @@ def touches_protected(pr_number: int) -> list[str]: return [f for f in files if any(f.startswith(p) or p in f for p in paths)] +WORKER_CONTEXT = "time/worker-run" + + def checks(sha: str) -> str: - """pass | pending | fail | none, over every check run on the PR head.""" + """pass | pending | fail | none, over every check run on the PR head. + + The worker's own status (WORKER_CONTEXT) can fail or delay a merge but never earns one: a model that chose the + command it ran is not a reviewer of it. Without any other check the answer is "none".""" runs = gh("GET", f"/repos/{REPO}/commits/{sha}/check-runs?per_page=100").get("check_runs", []) states = gh("GET", f"/repos/{REPO}/commits/{sha}/status").get("statuses", []) # commit statuses count too - if not runs and not states: - return "none" if any(r["status"] != "completed" for r in runs) or any(st["state"] == "pending" for st in states): return "pending" - ok = all(r["conclusion"] in ("success", "neutral", "skipped") for r in runs) and all(st["state"] == "success" for st in states) - return "pass" if ok else "fail" + if not all(r["conclusion"] in ("success", "neutral", "skipped") for r in runs) or not all(st["state"] == "success" for st in states): + return "fail" + if not runs and not any(st.get("context") != WORKER_CONTEXT for st in states): + return "none" + return "pass" + + +def dispatch_ci(pr: dict) -> bool: + """Run the project's CI on the PR branch by hand. GitHub skips pull_request workflows for pushes made with the + Actions token, so a worker's PR gets no checks unless Time asks; TIME_CI_WORKFLOW (default ci.yml) must accept + workflow_dispatch. Returns False when there is no such workflow.""" + wf = (os.environ.get("TIME_CI_WORKFLOW") or "ci.yml").strip() + if wf == "off": + return False + try: + gh("POST", f"/repos/{REPO}/actions/workflows/{wf}/dispatches", {"ref": pr["head"]["ref"]}) + except RuntimeError: + return False + return True def land(card: dict, last: dict) -> None: @@ -382,9 +404,13 @@ def land(card: dict, last: dict) -> None: c = checks(pr["head"]["sha"]) need = (os.environ.get("TIME_REQUIRE_CHECKS") or "on").strip() != "off" if c == "pending" or (c == "none" and need): - if c == "none" and last["beat"] != "no-checks": - comment(n, f"⏱️ Time: approved, but PR #{pr['number']} has no checks, and I only merge on green. Add a CI " - "workflow (or set `TIME_REQUIRE_CHECKS=off`).", {"beat": "no-checks", "pr": pr["number"]}) + if c == "none" and last["beat"] not in ("no-checks", "ci-dispatched"): + if dispatch_ci(pr): + comment(n, f"⏱️ Time: approved; running CI on PR #{pr['number']} before merging.", + {"beat": "ci-dispatched", "pr": pr["number"]}) + else: + comment(n, f"⏱️ Time: approved, but PR #{pr['number']} has no checks, and I only merge on green. Add a CI " + "workflow that accepts workflow_dispatch (or set `TIME_REQUIRE_CHECKS=off`).", {"beat": "no-checks", "pr": pr["number"]}) return if c == "fail": if last["beat"] != "checks-failed": diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8309b07..7c8178e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,6 +4,7 @@ on: pull_request: push: branches: [main] + workflow_dispatch: # Time runs CI on a worker's PR branch by hand (pull_request skips token pushes) permissions: contents: read jobs: diff --git a/.github/workflows/labs-aicharts.yml b/.github/workflows/labs-aicharts.yml index 322d750..6bd3c3f 100644 --- a/.github/workflows/labs-aicharts.yml +++ b/.github/workflows/labs-aicharts.yml @@ -25,9 +25,10 @@ jobs: CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} TIME_VENTURE_DOMAIN: ${{ vars.TIME_VENTURE_DOMAIN }} - name: smoke + env: + u: ${{ steps.deploy.outputs.url }} # through env, never interpolated into the script run: | set -o pipefail - u="${{ steps.deploy.outputs.url }}" for i in $(seq 1 18); do curl -fsS "${u}api/health" && break; sleep 10; done curl -fsS "${u}api/schema?dataset=orders" | python3 -c 'import json,sys; d=json.load(sys.stdin); assert len(d["sample"])==5, d; print("schema ok")' curl -fsS -X POST "${u}api/spec" -H 'content-type: application/json' \ diff --git a/CHANGELOG.md b/CHANGELOG.md index 0805590..5a09783 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ ## [Unreleased] ### Security +- The worker's own commit status (`time/worker-run`) can fail or delay a merge but never earns one: with no other check on the PR head, Time dispatches the project's CI workflow on the PR branch (`TIME_CI_WORKFLOW`, default `ci.yml`, which must accept `workflow_dispatch`; GitHub skips `pull_request` runs for pushes made with the Actions token) and merges on that. `off` keeps the old "no checks" stop. +- `.github/` is protected from auto-merge whatever `TIME_PROTECTED` says. +- `labs-aicharts.yml` passes the deploy URL through `env` instead of interpolating it into the shell. - Time's state comments (`` beats) count only when written by `github-actions[bot]`; a pasted marker from anyone else is ignored. A stranger on a public repo could otherwise steer a card to merge a chosen PR or build a chosen page. - `/time retry` and "Approve"/"Object" comments count only from the owner, members, collaborators and Time itself; any other bot is a stranger too. - The protected-path check reads every file of a PR (not the first 100) and counts a rename under both names.