Repository navigation
fix: keep Castiron budget results valid when main advances #4012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9052d51
2436293
465f305
3354af5
c46e36c
aaf1445
b544336
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ permissions: {} | |
| concurrency: | ||
| group: castiron-custom-code-comment-${{ github.event.workflow_run.head_repository.id }}-${{ github.event.workflow_run.head_branch }} | ||
| cancel-in-progress: false | ||
| queue: max # Preserve newer pending evaluations if older runs arrive out of order. | ||
|
|
||
| jobs: | ||
| compute: | ||
|
|
@@ -31,13 +32,15 @@ jobs: | |
| isolation: ${{ steps.budget.outputs.isolation }} | ||
| budget: ${{ steps.budget.outputs.budget }} | ||
| steps: | ||
| # Selecting main here pins both the executable checker and PR policy base. | ||
| # Subsequent steps use this checkout's SHA even if main advances. | ||
| - name: Check out the trusted reporter | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| ref: main | ||
| persist-credentials: false | ||
|
|
||
| - name: Compute from the current pull request Git objects | ||
| - name: Compute from the selected main and pull request Git objects | ||
| id: report | ||
| if: github.event.workflow_run.event == 'pull_request' | ||
| env: | ||
|
|
@@ -53,6 +56,7 @@ jobs: | |
| run: | | ||
| python3 -I scripts/castiron/custom_code_report.py trusted-report \ | ||
| --repo "$RUNNER_TEMP/castiron-objects.git" \ | ||
| --base "$(git rev-parse HEAD)" \ | ||
| --repository "$REPOSITORY" --run-id "$RUN_ID" --run-attempt "$RUN_ATTEMPT" \ | ||
| --out "$RUNNER_TEMP/castiron-custom-code" | ||
| if test -f "$RUNNER_TEMP/castiron-custom-code/context.json"; then | ||
|
|
@@ -133,17 +137,18 @@ jobs: | |
| HEAD_SHA: ${{ needs.compute.outputs.head-sha }} | ||
| ISOLATION_RESULT: ${{ needs.compute.outputs.isolation }} | ||
| BUDGET_RESULT: ${{ needs.compute.outputs.budget }} | ||
| PUBLISH_ATTEMPT: ${{ github.run_attempt }} | ||
| with: | ||
| script: | | ||
| const event = context.payload.workflow_run; | ||
| const {data: run} = await github.rest.actions.getWorkflowRun({...context.repo, run_id: event.id}); | ||
| if (run.head_sha !== event.head_sha || run.run_attempt !== event.run_attempt || | ||
| if (run.event !== event.event || run.head_sha !== event.head_sha || run.run_attempt !== event.run_attempt || | ||
| run.status !== 'completed' || run.path.split('@', 1)[0] !== '.github/workflows/castiron-custom-code.yml' || | ||
| run.repository.full_name !== `${context.repo.owner}/${context.repo.repo}`) return; | ||
| const head = run.head_sha; | ||
| if (!/^[0-9a-f]{40}$/.test(head)) throw new Error('Invalid candidate SHA'); | ||
| const {data: main} = await github.rest.git.getRef({...context.repo, ref: 'heads/main'}); | ||
| const base = main.object.sha; | ||
| const base = process.env.BASE_SHA; | ||
| let fresh = /^[0-9a-f]{40}$/.test(base) && head === process.env.HEAD_SHA; | ||
| if (run.event === 'pull_request') { | ||
| let pulls = run.pull_requests.length ? run.pull_requests : await github.paginate( | ||
| github.rest.repos.listPullRequestsAssociatedWithCommit, {...context.repo, commit_sha: head}); | ||
|
|
@@ -153,25 +158,40 @@ jobs: | |
| const current = []; | ||
| for (const pull of pulls) { | ||
| const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: pull.number}); | ||
| if (pr.state === 'open' && pr.head.sha === head && pr.base.sha === base && | ||
| if (pr.state === 'open' && pr.head.sha === head && | ||
| pr.base.ref === 'main' && pr.base.repo.full_name === `${context.repo.owner}/${context.repo.repo}`) current.push(pr); | ||
| } | ||
| if (current.length !== 1) return; | ||
| } else if (run.event !== 'merge_group' || !run.head_branch.startsWith('gh-readonly-queue/main/')) { | ||
| return; | ||
| } else { | ||
| const {data: main} = await github.rest.git.getRef({...context.repo, ref: 'heads/main'}); | ||
| fresh = fresh && base === main.object.sha; | ||
| } | ||
| const fresh = base === process.env.BASE_SHA && head === process.env.HEAD_SHA; | ||
| const url = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; | ||
| // Concurrency serializes writes, but does not order runs or retries. | ||
| // Keep an older evaluation from overwriting a newer result for this head. | ||
| const order = [run.id, run.run_attempt, Number(context.runId), Number(process.env.PUBLISH_ATTEMPT)]; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the same PR head has source runs 123 and 124, rerunning run 123 after Useful? React with 👍 / 👎. |
||
| const marker = `[evaluation ${order.join(':')}]`; | ||
| const statuses = await github.paginate(github.rest.repos.listCommitStatusesForRef, {...context.repo, ref: head}); | ||
| const names = ['Castiron / budget-only change', 'Castiron / custom-code budget']; | ||
| for (const status of statuses) { | ||
| if (!names.includes(status.context) || status.creator?.login !== 'github-actions[bot]') continue; | ||
| const prior = status.description?.match(/\[evaluation (\d+):(\d+):(\d+):(\d+)\]$/); | ||
| if (!prior || status.target_url !== `${url.slice(0, url.lastIndexOf('/') + 1)}${prior[3]}`) continue; | ||
| const previous = prior.slice(1).map(Number); | ||
| const different = previous.findIndex((value, index) => value !== order[index]); | ||
| if (different !== -1 && previous[different] > order[different]) return; | ||
| } | ||
| for (const [name, result] of [ | ||
| ['Castiron / budget-only change', process.env.ISOLATION_RESULT], | ||
| ['Castiron / custom-code budget', process.env.BUDGET_RESULT], | ||
| ]) { | ||
| const state = fresh && result === 'success' ? 'success' : 'failure'; | ||
| const description = !fresh ? 'Evaluation unavailable or base changed; rerun against current main.' | ||
| : state === 'success' ? 'Passed against main policy. See the trusted run summary.' | ||
| : 'Budget check failed. See the trusted run summary.'; | ||
| const description = !fresh ? 'Evaluation unavailable or queue base changed; inspect the trusted run and rerun.' | ||
| : `${state === 'success' ? 'Passed' : 'Failed'} against main ${base.slice(0, 12)}. See the trusted run summary.`; | ||
| await github.rest.repos.createCommitStatus({...context.repo, sha: head, context: name, | ||
| state, description, target_url: url}); | ||
| state, description: `${description} ${marker}`, target_url: url}); | ||
| } | ||
| comment: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -605,6 +605,7 @@ def render_report( | |
| ) -> str: | ||
| head = require_sha(report["head_sha"]) | ||
| lines = [MARKER, "", "## Castiron custom code", ""] | ||
| lines.extend([f"Evaluated main: `{require_sha(report['target_base_sha'])}`.", ""]) | ||
| if report.get("status") != "ok": | ||
| reason = html.escape(str(report.get("error", "Report could not be computed"))) | ||
| lines.extend([f"⚠️ Report unavailable for `{head[:12]}`.", "", reason]) | ||
|
|
@@ -803,7 +804,8 @@ def publish_comment( | |
| if ( | ||
| pull["state"] != "open" | ||
| or pull["head"]["sha"] != report["head_sha"] | ||
| or pull["base"]["sha"] != report["target_base_sha"] | ||
| or pull["base"]["ref"] != "main" | ||
| or pull["base"]["repo"]["full_name"] != repository | ||
| ): | ||
| return "Skipped stale report" | ||
| run = api("GET", f"{root}/actions/runs/{run_id}") | ||
|
|
@@ -850,7 +852,8 @@ def publish_comment( | |
| if ( | ||
| pull["state"] != "open" | ||
| or pull["head"]["sha"] != report["head_sha"] | ||
| or pull["base"]["sha"] != report["target_base_sha"] | ||
| or pull["base"]["ref"] != "main" | ||
| or pull["base"]["repo"]["full_name"] != repository | ||
| ): | ||
| return "Skipped stale report" | ||
| if found is not None: | ||
|
|
@@ -893,9 +896,12 @@ def write_report( | |
|
|
||
|
|
||
| def trusted_report( | ||
| repo: Path, repository: str, run_id: int, run_attempt: int, out: Path | ||
| repo: Path, repository: str, run_id: int, run_attempt: int, out: Path, *, base: str | ||
| ) -> None: | ||
| """Recompute from GitHub-associated Git objects, never from PR-produced artifacts.""" | ||
| # The caller pins this to the trusted checkout selected from main. PR base | ||
| # metadata may lag behind main, and main may move again during computation. | ||
| base = require_sha(base) | ||
| if not REPOSITORY.fullmatch(repository) or min(run_id, run_attempt) <= 0: | ||
| raise ReportError("invalid GitHub report target") | ||
| root = f"repos/{repository}" | ||
|
|
@@ -910,7 +916,7 @@ def trusted_report( | |
| return | ||
| head = require_sha(run["head_sha"]) | ||
| associated = associated_pulls(repository, run) | ||
| current: list[tuple[int, str]] = [] | ||
| current: list[int] = [] | ||
| for number in sorted({int(pr["number"]) for pr in associated}): | ||
| if number <= 0: | ||
| raise ReportError("invalid associated pull request") | ||
|
|
@@ -919,13 +925,14 @@ def trusted_report( | |
| pull["state"] == "open" | ||
| and pull["head"]["sha"] == head | ||
| and pull["base"]["repo"]["full_name"] == repository | ||
| and pull["base"]["ref"] == "main" | ||
| ): | ||
| current.append((number, require_sha(pull["base"]["sha"]))) | ||
| current.append(number) | ||
| if not current: | ||
| return | ||
| if len(current) != 1: | ||
| raise ReportError("workflow run has multiple current pull requests") | ||
| number, base = current[0] | ||
| number = current[0] | ||
| public = not api("GET", root)["private"] | ||
| # This must be a new, bare repository: no PR worktree, hooks, configuration, | ||
| # submodules, or Python imports can affect the trusted reporter. | ||
|
|
@@ -935,9 +942,7 @@ def trusted_report( | |
| git(repo, "fetch", "--quiet", "--no-tags", "origin", base, head) | ||
| write_report(repo, base, head, out, fetch=True, require_head_hash=True, public=public) | ||
| (out / "context.json").write_text( | ||
| json.dumps( | ||
| {"pr": number, "repository": repository, "run": run_id, "attempt": run_attempt} | ||
| ) | ||
| json.dumps({"pr": number, "repository": repository, "run": run_id, "attempt": run_attempt}) | ||
| + "\n" | ||
| ) | ||
|
|
||
|
|
@@ -962,6 +967,7 @@ def main() -> int: | |
| trusted.add_argument("--run-id", type=int, required=True) | ||
| trusted.add_argument("--run-attempt", type=int, required=True) | ||
| trusted.add_argument("--out", type=Path, required=True) | ||
| trusted.add_argument("--base", required=True, help="immutable main SHA of the trusted checkout") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If a Useful? React with 👍 / 👎. |
||
| preparing = commands.add_parser("prepare-public") | ||
| preparing.add_argument("--source-repo", type=Path, required=True) | ||
| preparing.add_argument("--source-base", required=True) | ||
|
|
@@ -996,7 +1002,9 @@ def main() -> int: | |
| + "\n" | ||
| ) | ||
| elif args.command == "trusted-report": | ||
| trusted_report(args.repo, args.repository, args.run_id, args.run_attempt, args.out) | ||
| trusted_report( | ||
| args.repo, args.repository, args.run_id, args.run_attempt, args.out, base=args.base | ||
| ) | ||
| elif args.command == "comment": | ||
| if args.report.stat().st_size > 5_000_000: | ||
| raise ReportError("report artifact is too large") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| """Small Git/checkpoint fixture shared by the offline Castiron tests.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import base64 | ||
| import json | ||
| import subprocess | ||
| import tempfile | ||
| import unittest | ||
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| import custom_code_report as report | ||
|
|
||
| GENERATION = "550e8400-e29b-41d4-a716-446655440000" | ||
|
|
||
|
|
||
| class GitTestCase(unittest.TestCase): | ||
| def setUp(self) -> None: # pyright: ignore[reportImplicitOverride] | ||
| self.temporary = tempfile.TemporaryDirectory() | ||
| self.addCleanup(self.temporary.cleanup) | ||
| self.repo = Path(self.temporary.name) | ||
| self.git("init", "-q", "-b", "main") | ||
| self.git("config", "user.name", "Castiron test") | ||
| self.git("config", "user.email", "castiron@example.test") | ||
|
|
||
| def git(self, *args: str) -> str: | ||
| return subprocess.run( | ||
| ["git", "-C", str(self.repo), *args], check=True, capture_output=True, text=True | ||
| ).stdout.strip() | ||
|
|
||
| def write(self, path: str, body: str) -> None: | ||
| target = self.repo / path | ||
| target.parent.mkdir(parents=True, exist_ok=True) | ||
| target.write_text(body) | ||
|
|
||
| def commit(self, message: str = "fixture") -> str: | ||
| self.git("add", "-A") | ||
| self.git("commit", "-q", "--allow-empty", "-m", message) | ||
| return self.git("rev-parse", "HEAD") | ||
|
|
||
| def baseline(self) -> tuple[str, str]: | ||
| self.write("generated.py", "generated\n") | ||
| metadata = { | ||
| "generation_id": GENERATION, | ||
| "source_branch": "test", | ||
| "target": "openai-python", | ||
| "language": "python", | ||
| } | ||
| encoded = base64.b64encode(json.dumps(metadata).encode()).decode() | ||
| generated = self.commit(f"codegen\n\nGeneration metadata: {encoded}") | ||
| self.git("update-ref", "refs/remotes/origin/codegen/test", generated) | ||
| self.write( | ||
| ".castiron.stats.yml", | ||
| f"schema_version: 1\ngeneration_id: {GENERATION}\ncodegen_sha: {generated}\ncodegen_hash: {report.hash_codegen_commit(self.repo, generated)}\n", | ||
| ) | ||
| return generated, self.commit("integrated") | ||
|
|
||
|
|
||
| def source_run(head: str, event: str = "pull_request") -> dict[str, Any]: | ||
| return { | ||
| "id": 123, | ||
| "event": event, | ||
| "head_sha": head, | ||
| "head_branch": "gh-readonly-queue/main/pr-7-example" if event == "merge_group" else "sdk", | ||
| "repository": {"full_name": "openai/example"}, | ||
| "head_repository": {"owner": {"login": "contributor"}}, | ||
| "path": ".github/workflows/castiron-custom-code.yml", | ||
| "status": "completed", | ||
| "run_attempt": 1, | ||
| "pull_requests": [{"number": 3}] if event == "pull_request" else [], | ||
| "conclusion": "failure", # The candidate's result is deliberately ignored. | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.