fix: pre-landing review fixes (PR reuse runs main's checkout; sizing experiments are never evidence; a re-run never reuses)

Review-Finding: p1/specialist-batched#1 reader=security
Review-Finding: p1/red-team#1 reader=red-team
Review-Finding: p1/red-team#2 reader=red-team
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
garrytanandcapy-ai[bot] committed 2026-11-03 07:50:52 +00:00
1 parent 560c7c9810
commit 284bd9123d
6 files changed
+45 -17

No files matched your search

+6 -1
View File
@@ -101,8 +101,12 @@ jobs:
run_url: ${{ steps.find.outputs.run_url }}
tree: ${{ steps.find.outputs.tree }}
steps:
# On a pull request this checks out main (the base commit), never the
# merge commit, so the reuse script, its imports and the run profile are
# main's code; bun ignores any .env or bunfig.toml in the checkout.
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
with:
ref: ${{ github.event.pull_request.base.sha || github.sha }}
persist-credentials: false
- uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2
with:
@@ -111,7 +115,7 @@ jobs:
name: Find a green run of this exact tree that this run's pull request cannot control
env:
GH_TOKEN: ${{ github.token }}
run: bun run scripts/ci-tree-reuse.ts find --workflow free-tests.yml --profile ci-reuse/free-tests --pull-request
run: bun --no-env-file --config=/dev/null run scripts/ci-tree-reuse.ts find --workflow free-tests.yml --profile ci-reuse/free-tests --pull-request
free-plan:
needs: reuse
@@ -763,6 +767,7 @@ jobs:
if: steps.route.outputs.reused != 'true'
env:
PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }}
SIZING_OVERRIDE: ${{ inputs.free_runner }}${{ inputs.free_shards }}${{ inputs.free_jobs }}
run: bun run scripts/ci-tree-reuse.ts record --plan "$RUNNER_TEMP/free-plan.json" --results "$RUNNER_TEMP/free-results" --os Linux,macOS,Windows --out "$RUNNER_TEMP/tested-tree/tree.json"
- name: Upload the tested tree
if: steps.route.outputs.reused != 'true'
+11 -4
View File
@@ -1063,13 +1063,17 @@ runs nightly and by hand only (docs/support-tiers.md), never on a push, so its
#### Pull requests reuse only a run they can't control
Free Tests' `reuse` job also runs on `pull_request`, as
`find … --pull-request` (`findPrReuse`). Main pushes stay the only consumers
`find … --pull-request` (`findPrReuse`). On a pull request it checks out the
base commit, never the merge commit, and runs bun with `--no-env-file
--config=/dev/null`, so the reuse script, everything it imports and the run
profile are main's code. Main pushes stay the only consumers
of anything a pull request could certify for itself, so a `pull_request` run
is never a source. A pull request run reuses a green run only when all of these
hold:
1. the pull request's head is on this repository (`fork_head`) and it targets
main (`not_main_pr`);
1. the pull request's head is on this repository (`fork_head`), it targets
main (`not_main_pr`), and this is the run's first attempt (`rerun`: a re-run
never reuses, so a red earlier attempt stays visible);
2. GitHub's merge commit has exactly two parents, main and the pull request's
head (`merge_mismatch`);
3. the merge commit leaves `.github/workflows/free-tests.yml`
@@ -1083,7 +1087,10 @@ hold:
5. `verifyRun` accepts it under `ci-reuse/free-tests` with `push` and
`workflow_dispatch` as the accepted events, its `tested-tree` record names
the merge commit's tree, and no newer red run on its head vetoes it (the
pull request's own run in progress is not a veto).
pull request's own run in progress is not a veto). A dispatch that
overrode the runner, shards or sub-shards records `sizing_experiment` in
its `tested-tree`, and `verifyRun` never accepts it (`sizing_experiment`),
for any consumer.
The aggregate's route step accepts a reuse on a `pull_request` run of
`refs/pull/<n>/merge` as it does on a push to main. A reused pull request run
+3 -2
View File
@@ -50,7 +50,7 @@ export interface ApiRun {
repository?: { full_name: string } | null; head_repository?: { full_name: string } | null;
}
export interface ApiJob { name: string; conclusion: string | null; completed_at?: string | null }
export interface TreeRecord { run_id?: unknown; run_attempt?: unknown; event?: unknown; sha?: unknown; head_sha?: unknown; tree?: unknown; reused?: unknown; reuse?: unknown; os?: unknown; files?: unknown; tests?: unknown }
export interface TreeRecord { run_id?: unknown; run_attempt?: unknown; event?: unknown; sha?: unknown; head_sha?: unknown; tree?: unknown; reused?: unknown; reuse?: unknown; sizing_experiment?: unknown; os?: unknown; files?: unknown; tests?: unknown }
/** Everything verifyRun reads, injectable so every refusal has a fixture. */
export interface RunDeps {
@@ -63,7 +63,7 @@ export interface RunDeps {
export type RunRefusal = 'run_ref_invalid' | 'other_repository' | 'fork_run' | 'workflow_mismatch' | 'event_mismatch' | 'run_in_progress'
| 'run_not_success' | 'job_missing' | 'job_ambiguous' | 'job_not_success' | 'artifact_missing' | 'artifact_mismatch'
| 'artifact_reused' | 'tree_mismatch' | 'newer_run_vetoes' | 'run_changed' | 'run_stale' | 'os_mismatch' | 'api_error';
| 'artifact_reused' | 'sizing_experiment' | 'tree_mismatch' | 'newer_run_vetoes' | 'run_changed' | 'run_stale' | 'os_mismatch' | 'api_error';
export interface RunEvidence { run_id: number; run_attempt: number; head_sha: string; tree: string; profile: string; event: string; finished_at: string; files: string[]; tests: number | null }
export type RunVerdict = { ok: true; evidence: RunEvidence } | { ok: false; reason: RunRefusal; detail: string };
@@ -169,6 +169,7 @@ export function verifyRun(deps: RunDeps, o: { repo: string; runRef: string; prof
catch (e: any) { return fail('artifact_missing', `${at} has no readable ${p.artifact} tree.json: ${String(e?.message ?? e).slice(0, 200)}`); }
if (!rec || typeof rec !== 'object') return fail('artifact_mismatch', `${at}'s ${p.artifact} tree.json is not a JSON object`);
if (rec.reused === true || rec.reuse === true) return fail('artifact_reused', `${at} reused another run; a reuse is never evidence`);
if (rec.sizing_experiment !== undefined) return fail('sizing_experiment', `${at} was a runner-sizing experiment (a dispatch that overrode the runner, shards or sub-shards); it is never a Free Tests result`);
const pr = run.event === 'pull_request';
const recHeads = pr ? [['head_sha', rec.head_sha, run.head_sha] as const] : rec.head_sha === undefined && rec.sha === undefined ? [['head', undefined, run.head_sha] as const] : ([['sha', rec.sha, run.head_sha], ['head_sha', rec.head_sha, run.head_sha]] as const).filter(([, got]) => got !== undefined);
const wrong = ([['run_id', rec.run_id, id], ['run_attempt', rec.run_attempt, run.run_attempt], ['event', rec.event, run.event], ...recHeads] as const).filter(([, got, want]) => got !== want);
+7 -4
View File
@@ -6,7 +6,9 @@
* The producer. A Free Tests or Windows Free Tests aggregate runs it only after its strict verification
* passed, and uploads the file as the `tested-tree` artifact: run_id, run_attempt, event, sha (GITHUB_SHA),
* head_sha (the pull request head for pull_request, else GITHUB_SHA), tree (git's tree of GITHUB_SHA),
* workflow, os (the OSes the run's test jobs ran on), files (every test file in the plan) and tests.
* workflow, os (the OSes the run's test jobs ran on), files (every test file in the plan) and tests, plus
* sizing_experiment: true when a dispatch overrode the runner, shards or sub-shards (SIZING_OVERRIDE), which
* verifyRun never accepts as evidence.
*
* bun run scripts/ci-tree-reuse.ts find --workflow <file.yml> --profile <run_profiles name>
* The main-push consumer (the `reuse` job). A push to main whose tree a pull request already tested green
@@ -43,7 +45,7 @@ export interface PrReuseDeps extends RunDeps {
commit(sha: string): { tree: string; parents: string[] } | null;
blob(sha: string, file: string): string | null;
}
export interface PrContext { number: number; mergeSha: string; headSha: string; headRef: string; headRepo: string; baseRef: string; runId: number }
export interface PrContext { number: number; mergeSha: string; headSha: string; headRef: string; headRepo: string; baseRef: string; runId: number; runAttempt: number }
/** The files whose bytes decide what a reuse means: a pull request that edits one runs every job. */
export const reuseGuardedFiles = (workflow: string) => [`.github/workflows/${workflow}`, 'scripts/ci-tree-reuse.ts', 'lib/run-evidence.ts'];
export type ReuseVerdict =
@@ -95,6 +97,7 @@ export function findPrReuse(deps: PrReuseDeps, o: { repo: string; base: string;
const { pr } = o;
if (!SHA.test(pr.mergeSha) || !SHA.test(pr.headSha)) return refuse('usage', `merge ${JSON.stringify(pr.mergeSha)} or head ${JSON.stringify(pr.headSha)} is not a commit sha`);
if (!Number.isSafeInteger(pr.number) || pr.number < 1 || !pr.headRef) return refuse('usage', 'the pull_request event names no pull request number or head branch');
if (pr.runAttempt !== 1) return refuse('rerun', `attempt ${pr.runAttempt} of run ${pr.runId}: a re-run never reuses, so an earlier red attempt can't be hidden`);
if (pr.baseRef !== o.base) return refuse('not_main_pr', `#${pr.number} targets ${pr.baseRef}, not ${o.base}`);
if (o.profile.workflow !== o.workflow) return refuse('profile_mismatch', `profile ${o.profileName} names ${o.profile.workflow}, not ${o.workflow}`);
if (pr.headRepo.toLowerCase() !== o.repo.toLowerCase()) return refuse('fork_head', `#${pr.number}'s head is on ${pr.headRepo || 'a deleted repository'}, not ${o.repo}`);
@@ -146,7 +149,7 @@ export function treeRecord(env: NodeJS.ProcessEnv, o: { plan: { shards: Array<{
if (!files.length) throw new Error('the plan lists no test files');
const counts = o.results.map(r => r.outcome?.summary?.testsRan);
const tests = o.results.length && counts.every(n => Number.isSafeInteger(n) && n! >= 0) ? counts.reduce((a, b) => a! + b!, 0)! : files.length;
return { run_id: int('GITHUB_RUN_ID'), run_attempt: int('GITHUB_RUN_ATTEMPT'), event, sha, head_sha: head, tree: o.tree, workflow, os: o.os, files, tests };
return { run_id: int('GITHUB_RUN_ID'), run_attempt: int('GITHUB_RUN_ATTEMPT'), event, sha, head_sha: head, tree: o.tree, workflow, os: o.os, files, tests, ...(env.SIZING_OVERRIDE ? { sizing_experiment: true } : {}) };
}
function flag(args: string[], name: string): string | undefined { const i = args.indexOf(name); return i >= 0 ? args[i + 1] : undefined; }
@@ -206,7 +209,7 @@ function find(args: string[]): number {
let event: { pull_request?: { number?: number; head?: { sha?: string; ref?: string; repo?: { full_name?: string } | null }; base?: { ref?: string } } };
try { event = JSON.parse(fs.readFileSync(env.GITHUB_EVENT_PATH ?? '', 'utf8')); } catch (e: any) { return settle(refuse('usage', `GITHUB_EVENT_PATH: ${String(e?.message ?? e).slice(0, 200)}`)); }
const p = event.pull_request ?? {};
const pr: PrContext = { number: Number(p.number), mergeSha: env.GITHUB_SHA ?? '', headSha: p.head?.sha ?? '', headRef: p.head?.ref ?? '', headRepo: p.head?.repo?.full_name ?? '', baseRef: p.base?.ref ?? '', runId: Number(env.GITHUB_RUN_ID) };
const pr: PrContext = { number: Number(p.number), mergeSha: env.GITHUB_SHA ?? '', headSha: p.head?.sha ?? '', headRef: p.head?.ref ?? '', headRepo: p.head?.repo?.full_name ?? '', baseRef: p.base?.ref ?? '', runId: Number(env.GITHUB_RUN_ID), runAttempt: Number(env.GITHUB_RUN_ATTEMPT) };
const prDeps: PrReuseDeps = {
...deps,
runsForBranch: (wf, branch) => ghApiJson<{ workflow_runs?: ApiRun[] }>(`repos/${repo}/actions/workflows/${wf}/runs?branch=${encodeURIComponent(branch)}&per_page=${MAX_CANDIDATES}`).workflow_runs ?? [],
+5 -4
View File
@@ -1,8 +1,8 @@
/**
* CI reuse wiring (GSTA-156 P3b) for free-tests.yml and windows-free-tests.yml. Pins the properties that make reuse
* safe: nothing a pull request controls certifies it (a push to main consumes; free-tests.yml's pull_request runs consume
* only through find --pull-request, whose sources and guarded files test/ci-tree-reuse.test.ts pins; dispatch runs never
* consume), the reuse job alone reads Actions with a job-level actions: read,
* only through find --pull-request run from main's checkout, whose sources and guarded files test/ci-tree-reuse.test.ts
* pins; a sizing-experiment dispatch records sizing_experiment and is never evidence; dispatch runs never consume), the reuse job alone reads Actions with a job-level actions: read,
* a skipped or failed reuse job runs every job exactly as before (GitHub skips a job whose needed job was skipped
* unless its condition uses a status function, so every dependent condition starts with !cancelled()), the
* aggregate's reuse branch runs before any download and the strict path is otherwise unchanged, tested-tree is
@@ -52,8 +52,9 @@ for (const [file, spec] of Object.entries(FILES)) {
expect(source).not.toContain('pull_request_target');
const find = jobs.reuse.steps.find((s: any) => s.id === 'find');
expect(find.env).toEqual({ GH_TOKEN: '${{ github.token }}' });
expect(find.run).toBe(`bun run scripts/ci-tree-reuse.ts find --workflow ${file} --profile ${spec.profile}${spec.pullRequest ? ' --pull-request' : ''}`);
expect(find.run).toBe(spec.pullRequest ? `bun --no-env-file --config=/dev/null run scripts/ci-tree-reuse.ts find --workflow ${file} --profile ${spec.profile} --pull-request` : `bun run scripts/ci-tree-reuse.ts find --workflow ${file} --profile ${spec.profile}`);
expect(jobs.reuse.steps[0].with['persist-credentials']).toBe(false);
if (spec.pullRequest) expect(jobs.reuse.steps[0].with.ref).toBe('${{ github.event.pull_request.base.sha || github.sha }}');
expect(Object.keys(jobs.reuse.outputs).sort()).toEqual(['reused', 'run_id', 'run_url', 'tree']);
});
@@ -100,7 +101,7 @@ for (const [file, spec] of Object.entries(FILES)) {
expect(steps.length).toBe(upload + 1);
expect(steps[upload].uses).toStartWith('actions/upload-artifact@');
expect(steps[upload].with['if-no-files-found']).toBe('error');
expect(steps[record].env).toEqual({ PR_HEAD_SHA: '${{ github.event.pull_request.head.sha }}' });
expect(steps[record].env).toEqual({ PR_HEAD_SHA: '${{ github.event.pull_request.head.sha }}', ...(spec.pullRequest ? { SIZING_OVERRIDE: '${{ inputs.free_runner }}${{ inputs.free_shards }}${{ inputs.free_jobs }}' } : {}) });
expect(steps[1].run).toMatch(/set -eu\n(test "\$[A-Z_]+_RESULT" = success\n)+$/);
});
+13 -2
View File
@@ -126,7 +126,7 @@ describe('findPrReuse: a pull request reuses only a run it cannot control', () =
const SELF = 999;
const GUARDED = ['.github/workflows/free-tests.yml', 'scripts/ci-tree-reuse.ts', 'lib/run-evidence.ts'];
interface PrFake { id: number; event: string; branch: string; head: string; conclusion?: string | null; status?: string; started?: string; record?: Partial<TreeRecord> }
interface PrWorld { runs: PrFake[]; trees: Record<string, string>; blobs: Record<string, string>; parents: string[]; headRepo: string }
interface PrWorld { runs: PrFake[]; trees: Record<string, string>; blobs: Record<string, string>; parents: string[]; headRepo: string; attempt?: number }
const blobs = (over: Record<string, string> = {}) => ({ ...Object.fromEntries(GUARDED.flatMap(p => [[`${MERGE}:${p}`, `blob-${p}`], [`${BASE}:${p}`, `blob-${p}`]])), ...over });
const prWorld = (over: Partial<PrWorld> = {}): PrWorld => ({
runs: [{ id: 500, event: 'push', branch: 'main', head: MAIN_HEAD }, { id: SELF, event: 'pull_request', branch: 'feat', head: HEAD, status: 'in_progress', conclusion: null, started: '2026-10-30T11:30:00Z' }],
@@ -148,7 +148,7 @@ describe('findPrReuse: a pull request reuses only a run it cannot control', () =
};
}
const pr = (w: PrWorld) => findPrReuse(prDeps(w), { repo: REPO, base: 'main', workflow: 'free-tests.yml', profileName: 'ci-reuse/free-tests', profile: PROFILE, now: NOW,
pr: { number: 42, mergeSha: MERGE, headSha: HEAD, headRef: 'feat', headRepo: w.headRepo, baseRef: 'main', runId: SELF } });
pr: { number: 42, mergeSha: MERGE, headSha: HEAD, headRef: 'feat', headRepo: w.headRepo, baseRef: 'main', runId: SELF, runAttempt: w.attempt ?? 1 } });
const prWhy = (w: PrWorld) => { const v = pr(w); return v.reused ? 'reused' : v.reason; };
test('a green push run on main at the merge commit\'s exact tree is reused', () => {
@@ -186,6 +186,15 @@ describe('findPrReuse: a pull request reuses only a run it cannot control', () =
expect(prWhy(prWorld({ headRepo: 'someone/gstack' }))).toBe('fork_head');
});
test('a runner-sizing experiment dispatch is never a source (red team p1 #1)', () => {
const v = pr(prWorld({ runs: [{ id: 506, event: 'workflow_dispatch', branch: 'feat', head: HEAD, record: { sizing_experiment: true } }] }));
expect(v).toMatchObject({ reused: false, reason: 'no_verified_run', detail: expect.stringContaining('reason=sizing_experiment') });
});
test('a re-run attempt of the pull request run never reuses, so its own red earlier attempt stays visible (red team p1 #2)', () => {
expect(prWhy(prWorld({ attempt: 2, runs: [{ id: 501, event: 'workflow_dispatch', branch: 'feat', head: HEAD }] }))).toBe('rerun');
});
test('a merge commit that is not base plus this head, or an unreadable one, is refused', () => {
expect(prWhy(prWorld({ parents: [BASE, '6'.repeat(40)] }))).toBe('merge_mismatch');
expect(prWhy(prWorld({ parents: [BASE] }))).toBe('merge_mismatch');
@@ -200,6 +209,8 @@ describe('treeRecord: the tested-tree producer', () => {
expect(treeRecord(env(), { plan, results, os: ['Linux', 'macOS', 'Windows'], tree: TREE })).toEqual({ run_id: 300, run_attempt: 2, event: 'workflow_dispatch', sha: HEAD, head_sha: HEAD, tree: TREE, workflow: '.github/workflows/free-tests.yml', os: ['Linux', 'macOS', 'Windows'], files: ['test/a.test.ts', 'test/b.test.ts', 'test/c.test.ts'], tests: 12 });
expect(treeRecord(env({ GITHUB_EVENT_NAME: 'pull_request', GITHUB_SHA: MERGE, PR_HEAD_SHA: HEAD }), { plan, results, os: ['Windows'], tree: TREE })).toMatchObject({ event: 'pull_request', sha: MERGE, head_sha: HEAD });
expect(treeRecord(env(), { plan, results: [{ outcome: {} }], os: ['Linux'], tree: TREE }).tests).toBe(3);
expect(treeRecord(env({ SIZING_OVERRIDE: '1' }), { plan, results, os: ['Linux'], tree: TREE })).toMatchObject({ sizing_experiment: true });
expect(treeRecord(env({ SIZING_OVERRIDE: '' }), { plan, results, os: ['Linux'], tree: TREE })).not.toHaveProperty('sizing_experiment');
});
test('missing or malformed inputs throw, so the aggregate fails instead of uploading a record', () => {
expect(() => treeRecord(env({ GITHUB_EVENT_NAME: 'pull_request', GITHUB_SHA: MERGE }), { plan, results, os: ['Linux'], tree: TREE })).toThrow(/PR_HEAD_SHA/);