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 <noreply@anthropic.com>
This commit was merged in pull request #46.
This commit is contained in:
1 parent
17e51a6b08
commit
8c6ad054b9
5 files changed
+53
-14
No files matched your search
@@ -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)
|
||||
|
||||
+34
-8
@@ -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":
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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' \
|
||||
|
||||
@@ -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 (`<!-- time {...} -->` 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.
|
||||
|
||||
Reference in new issue
Block a user