From c9a46414cbe856449c9cbf803a961e43cdb02142 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 16:32:58 +0200 Subject: [PATCH 01/10] feat(size): measure a base ref in one command (pnpm size --base ) The Size workflow already compares base and PR builds; locally that needed a manual checkout, install, build, --json, and --compare dance, so budgets were negotiated late. --base does the workflow's recipe in a detached worktree under .tmp/size-base/ (kept for reuse, other bases pruned) and compares against it: first run ~1-2 min, later runs against the same base ~3s. Documents the local caveat: npm tarball/unpacked rows compare a fresh base against a working tree that may carry locally built helper artifacts. --- docs/agents/testing.md | 15 +++++++++ scripts/size-report.mjs | 75 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 9bd5f7ef8..24dc98129 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -228,6 +228,21 @@ Lists are bounded (`--limit`, default 10) and always disclose what they hid; `-- unbounded. The query is read-only, runs in well under a second, and adds no CI work — its model is covered by `pnpm depgraph:test` (the existing `Layering Guard` job). +## Shipped size (`pnpm size --base `) + +The Size workflow posts a base/PR comparison on every PR; the same comparison runs locally in one +command, before the PR exists: + +```sh +pnpm size --base origin/main # first run: detached worktree + install + build of the base (~1-2 min) + # later runs against the same base: ~3s (the worktree is kept under .tmp/size-base/) +``` + +Requires a current `pnpm build` of your own tree. `JS raw`/`JS gzip` are the numbers to quote and to +budget against (ADR 0019 §8 units state theirs before starting); the `npm tarball`/`npm unpacked` +rows compare a fresh base checkout against your working tree, which may carry locally built helper +artifacts CI's fresh checkout does not, so read a tarball delta on GitHub's comment, not here. + ## Gate manifest: proving every check has a CI owner Every gate above answers "is the code right?". None of them can answer "does CI still own this diff --git a/scripts/size-report.mjs b/scripts/size-report.mjs index 08e15366a..5160689b7 100644 --- a/scripts/size-report.mjs +++ b/scripts/size-report.mjs @@ -15,6 +15,7 @@ const VALUE_ARGS = new Map([ ['--json', 'json'], ['--markdown', 'markdown'], ['--compare', 'compare'], + ['--base', 'base'], ['--post-comment', 'postComment'], ['--pr', 'pr'], ['--startup-runs', 'startupRuns'], @@ -33,16 +34,22 @@ if (args.postComment) { process.exit(0); } -const report = collectReport(cwd, { - startupRuns: parseNonNegativeInteger(args.startupRuns ?? '0', '--startup-runs'), -}); -const baseReport = args.compare ? JSON.parse(fs.readFileSync(args.compare, 'utf8')) : null; +if (args.compare && args.base) { + throw new Error('--compare and --base are exclusive: one supplies the base report, the other measures it'); +} +const startupRuns = parseNonNegativeInteger(args.startupRuns ?? '0', '--startup-runs'); +const report = collectReport(cwd, { startupRuns }); +const baseReport = args.compare + ? JSON.parse(fs.readFileSync(args.compare, 'utf8')) + : args.base + ? measureBaseRef(cwd, args.base, { startupRuns }) + : null; if (args.json) { writeFile(args.json, `${JSON.stringify(report, null, 2)}\n`); } -const markdown = formatMarkdown(report, baseReport); +const markdown = formatMarkdown(report, baseReport, args.base); if (args.markdown) { writeFile(args.markdown, markdown); @@ -80,6 +87,9 @@ Options: --json Write the raw size report JSON. --markdown Write the markdown report. --compare Compare against a previously written JSON report. + --base Measure (e.g. origin/main) in a detached worktree under + .tmp/size-base/ and compare against it: the local one-command + equivalent of the Size workflow's base/PR comparison. --startup-runs Measure startup medians for side-effect-free CLI commands. --post-comment Post or update the markdown report on the current PR. --pr Pull request number for --post-comment. @@ -144,6 +154,57 @@ function collectReport(root, options) { }; } +// The Size workflow measures the base by checking it out, installing, and building; this is +// the same recipe in a detached worktree so the working tree is never touched. The worktree +// is kept under .tmp/size-base/ so a second run against the same base skips the +// install+build (mirroring the workflow's dist cache); other bases' worktrees are removed. +function measureBaseRef(root, ref, options) { + const sha = execFileSync('git', ['rev-parse', '--verify', `${ref}^{commit}`], { + cwd: root, + encoding: 'utf8', + }).trim(); + const worktreesRoot = path.join(root, '.tmp', 'size-base'); + const worktreeDir = path.join(worktreesRoot, sha.slice(0, 12)); + pruneOtherBaseWorktrees(root, worktreesRoot, worktreeDir); + const registered = execFileSync('git', ['worktree', 'list', '--porcelain'], { + cwd: root, + encoding: 'utf8', + }).includes(`worktree ${worktreeDir}\n`); + if (!registered) { + fs.rmSync(worktreeDir, { recursive: true, force: true }); + fs.mkdirSync(worktreesRoot, { recursive: true }); + execFileSync('git', ['worktree', 'add', '--detach', worktreeDir, sha], { + cwd: root, + stdio: ['ignore', 'ignore', 'inherit'], + }); + } + const built = fs.existsSync(path.join(worktreeDir, 'dist', 'src')); + if (!built) { + process.stderr.write(`[size] measuring base ${sha.slice(0, 9)} (${ref}): install + build in ${worktreeDir}\n`); + execFileSync('pnpm', ['install', '--frozen-lockfile', '--prefer-offline'], { + cwd: worktreeDir, + stdio: ['ignore', 'ignore', 'inherit'], + }); + execFileSync('pnpm', ['build'], { cwd: worktreeDir, stdio: ['ignore', 'ignore', 'inherit'] }); + } + return collectReport(worktreeDir, options); +} + +function pruneOtherBaseWorktrees(root, worktreesRoot, keep) { + if (!fs.existsSync(worktreesRoot)) return; + for (const entry of fs.readdirSync(worktreesRoot, { withFileTypes: true })) { + if (!entry.isDirectory()) continue; + const dir = path.join(worktreesRoot, entry.name); + if (dir === keep) continue; + try { + execFileSync('git', ['worktree', 'remove', '--force', dir], { cwd: root, stdio: 'ignore' }); + } catch { + // Not a registered worktree (a half-created or hand-copied directory): plain removal. + } + fs.rmSync(dir, { recursive: true, force: true }); + } +} + function prepareGeneratedPackageAssets(root) { const packageAppleRunnerScript = path.join(root, 'scripts', 'package-apple-runner-source.mjs'); if (!fs.existsSync(packageAppleRunnerScript)) { @@ -234,7 +295,7 @@ function countNpmPackEntries(pack) { return Array.isArray(pack.files) ? pack.files.length : 0; } -function formatMarkdown(report, baseReport) { +function formatMarkdown(report, baseReport, baseLabel) { const rows = [ metricRow('JS raw', baseReport?.js.rawBytes, report.js.rawBytes), metricRow('JS gzip', baseReport?.js.gzipBytes, report.js.gzipBytes), @@ -250,7 +311,7 @@ function formatMarkdown(report, baseReport) { return `${COMMENT_MARKER} ## Size Report -| Metric | Base | Current | Diff | +| Metric | Base${baseLabel ? ` (${baseLabel})` : ''} | Current | Diff | |---|---:|---:|---:| ${rows.join('\n')} From 5846edcce7f08656b96ca55dd6b39ab88e324688 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 16:39:54 +0200 Subject: [PATCH 02/10] =?UTF-8?q?feat(tooling):=20pnpm=20pr:evidence=20?= =?UTF-8?q?=E2=80=94=20one=20paste-ready,=20SHA-stamped=20evidence=20block?= =?UTF-8?q?=20for=20PR=20bodies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Composes what the repo already measures instead of hand-transcribing it after every rebase: exact merge-base and head, changed-file areas, the affected selector's plan (local vs GitHub-authoritative, fail-open summarized), the layering guard verdict, depgraph counts with a real delta against the base (a throwaway git worktree, no install — the script analyzes its cwd while its imports resolve from this checkout), and, behind flags, the changed-line coverage table and pnpm size --base. It claims nothing about CI: the last line links the head's checks. ~20s default tier. The pure model (grouping, report parsing, rendering) has node:test coverage registered as the pr-evidence-model gate, run in the Affected-check Selector job next to the selector it reads. --- .github/workflows/ci.yml | 7 ++ docs/agents/pull-requests.md | 6 + package.json | 2 + scripts/check-affected/checks.ts | 1 + scripts/check-affected/model.ts | 2 + scripts/pr-evidence/model.test.ts | 168 ++++++++++++++++++++++++++ scripts/pr-evidence/model.ts | 169 ++++++++++++++++++++++++++ scripts/pr-evidence/run.ts | 194 ++++++++++++++++++++++++++++++ scripts/size-report.mjs | 38 ++++-- 9 files changed, 574 insertions(+), 13 deletions(-) create mode 100644 scripts/pr-evidence/model.test.ts create mode 100644 scripts/pr-evidence/model.ts create mode 100644 scripts/pr-evidence/run.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 015540029..0b1aed6f3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -177,6 +177,13 @@ jobs: uses: ./.github/actions/run-gate with: { gate: gate-manifest } + # `pnpm pr:evidence` composes this job's selector output (plus the depgraph and layering + # reports) into the PR-body evidence block; its pure model lives here with the selector + # it reads. Seconds, no install beyond this job's. + - name: Check the PR evidence composer model + uses: ./.github/actions/run-gate + with: { gate: pr-evidence-model } + # Same family as the manifest above — a CI selection that has stopped selecting what # it claims. ios.yml runs a hand-written subset of the runner XCTest methods through an # `-only-testing:` list, and xcodebuild treats an identifier that matches nothing as diff --git a/docs/agents/pull-requests.md b/docs/agents/pull-requests.md index 285ac3d7e..b98028e0e 100644 --- a/docs/agents/pull-requests.md +++ b/docs/agents/pull-requests.md @@ -60,6 +60,12 @@ asked or when the work is intentionally incomplete. validation does not apply instead of writing a command checklist. - Call out real tradeoffs, known gaps, and follow-ups; omit boilerplate when there are none. - Note touched-file count and whether scope expanded beyond the initial command family. +- Paste the block `pnpm pr:evidence` prints (add `--size` for a unit with a size budget, + `--coverage` after `pnpm test:coverage`) rather than transcribing SHAs, gate lists, layering + counts, or edge deltas by hand: it stamps the exact merge-base and head, composes the affected + plan, layering guard, depgraph deltas, and the two optional reports, and links the head's CI + instead of claiming it. After a rebase, re-run it (~20s) rather than editing the old block; the + stamp is what makes the evidence dated instead of stale. ## Reviewing - Review against the linked issue, not only the diff. State the issue's motivating behavior and diff --git a/package.json b/package.json index 687284a30..aae95895d 100644 --- a/package.json +++ b/package.json @@ -117,6 +117,8 @@ "maestro:conformance:regenerate": "node --experimental-strip-types scripts/maestro-conformance/regenerate.mjs", "maestro:conformance:differential": "node --experimental-strip-types packages/maestro/test/conformance/differential/run.ts", "size": "node scripts/size-report.mjs", + "pr:evidence": "node --experimental-strip-types scripts/pr-evidence/run.ts", + "pr:evidence:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/pr-evidence/model.test.ts", "perf": "node --experimental-strip-types scripts/perf/run.ts", "mutation:run": "node --experimental-strip-types scripts/mutation/run.ts", "mutation:check": "node --experimental-strip-types scripts/mutation/run.ts --no-run", diff --git a/scripts/check-affected/checks.ts b/scripts/check-affected/checks.ts index 088801cb9..572af37f3 100644 --- a/scripts/check-affected/checks.ts +++ b/scripts/check-affected/checks.ts @@ -88,6 +88,7 @@ export const CHECK_CATALOG: readonly CheckSpec[] = [ gate('tmpdir-leaks', 'Leaked test tmpdir detector', 'check:tmpdir-leaks'), gate('tmpdir-leaks-model', 'TMPDIR redirection model', 'check:tmpdir-leaks:test'), gate('coverage-model', 'Changed-line coverage model', 'check:coverage-changed:test'), + gate('pr-evidence-model', 'PR evidence composer model', 'pr:evidence:test'), gate('wire-compat-model', 'Wire-compat rules model', 'check:daemon-wire-compat:test'), gate('production-exports', 'Production-unused exports', 'check:production-exports'), gate('bundle-owner-files', 'Bundle owner-file manifest', 'check:bundle-owner-files'), diff --git a/scripts/check-affected/model.ts b/scripts/check-affected/model.ts index c40480e70..b3573a9d0 100644 --- a/scripts/check-affected/model.ts +++ b/scripts/check-affected/model.ts @@ -59,6 +59,7 @@ export type CheckId = | 'tmpdir-leaks' | 'tmpdir-leaks-model' | 'coverage-model' + | 'pr-evidence-model' | 'wire-compat-model' | 'production-exports' | 'bundle-owner-files' @@ -118,6 +119,7 @@ export const ALL_CHECKS: readonly CheckId[] = [ 'tmpdir-leaks', 'tmpdir-leaks-model', 'coverage-model', + 'pr-evidence-model', 'wire-compat-model', 'production-exports', 'bundle-owner-files', diff --git a/scripts/pr-evidence/model.test.ts b/scripts/pr-evidence/model.test.ts new file mode 100644 index 000000000..4afa7888c --- /dev/null +++ b/scripts/pr-evidence/model.test.ts @@ -0,0 +1,168 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { + coverageSummary, + depgraphFacts, + groupChangedFiles, + parseLayeringReport, + renderEvidence, + sizeSummary, + type EvidenceInputs, +} from './model.ts'; + +const HEAD = 'b03379a55baa9f6da5d863e6dcfccdcfef975f5c'; +const BASE = '9a0d6dead229fc82e7e61064f674f963197afc4b'; + +function inputs(overrides: Partial = {}): EvidenceInputs { + return { + generatedAt: '2026-08-18T14:35:58.105Z', + repository: 'callstack/agent-device', + git: { + branch: 'feat/x', + head: HEAD, + headShort: HEAD.slice(0, 9), + base: BASE, + baseRef: 'origin/main', + baseShort: BASE.slice(0, 9), + dirty: false, + changedFiles: ['src/daemon/a.ts', 'src/daemon/b.ts', 'docs/agents/testing.md', 'AGENTS.md'], + }, + affected: { + failOpen: false, + failOpenReasons: [], + checks: [ + { id: 'typecheck', localRunnable: true, ciJobs: ['Lint & Format'] }, + { id: 'unit', localRunnable: true, ciJobs: ['Unit'] }, + { id: 'swift-runner-ios', localRunnable: false, ciJobs: ['iOS'] }, + ], + }, + layering: { ok: true, violationsByRule: {} }, + depgraph: { + head: { + files: 1312, + edges: 5559, + typeInversions: 7, + daemonToPlatforms: { count: 62, valueCount: 43 }, + }, + base: { + files: 1311, + edges: 5556, + typeInversions: 7, + daemonToPlatforms: { count: 64, valueCount: 45 }, + }, + }, + coverage: { kind: 'skipped', reason: 'pass --coverage' }, + size: { kind: 'skipped', reason: 'pass --size' }, + ...overrides, + }; +} + +test('changed files group by top-level area, largest first, root files under (root)', () => { + assert.deepEqual( + [...groupChangedFiles(['src/a.ts', 'docs/b.md', 'src/c.ts', 'AGENTS.md', 'scripts/d.ts'])], + [ + ['src', 2], + ['(root)', 1], + ['docs', 1], + ['scripts', 1], + ], + ); +}); + +test('the layering report parses per-rule counts and takes OK from the exit code', () => { + const red = parseLayeringReport( + 'Layering guard: 2 violation(s)\n\n [R9 type-cycle-size] 1 violation(s):\n::error …\n [R10 daemon-modularity] 1 violation(s):\n', + 1, + ); + assert.deepEqual(red, { + ok: false, + violationsByRule: { 'R9 type-cycle-size': 1, 'R10 daemon-modularity': 1 }, + }); + assert.deepEqual(parseLayeringReport('Layering guard: OK — 1312 source files …\n', 0), { + ok: true, + violationsByRule: {}, + }); +}); + +test('depgraph facts read the counts and the daemon→platforms zone edge', () => { + assert.deepEqual( + depgraphFacts({ + generated: { files: 3, edges: 4 }, + zoneEdges: [ + { from: 'daemon-server', to: 'contracts', count: 9, valueCount: 5 }, + { from: 'daemon-server', to: 'platforms', count: 62, valueCount: 43 }, + ], + typeInversions: { 'commands -> client': 3, 'core -> daemon-server': 2 }, + }), + { files: 3, edges: 4, typeInversions: 5, daemonToPlatforms: { count: 62, valueCount: 43 } }, + ); + assert.equal( + depgraphFacts({ generated: { files: 1, edges: 0 }, zoneEdges: [], typeInversions: {} }) + .daemonToPlatforms, + undefined, + ); +}); + +test('the block is stamped with full base and head SHAs and reports deltas against base', () => { + const block = renderEvidence(inputs()); + assert.match(block, new RegExp(`^\n`)); + assert.match(block, /at `b03379a55` \(`feat\/x`\) against `origin\/main` @ `9a0d6dead`/); + assert.match(block, /Changed: 4 files \(2 src, 1 \(root\), 1 docs\)/); + assert.match( + block, + /3 selected · local: typecheck, unit · GitHub-authoritative: swift-runner-ios/, + ); + assert.match( + block, + /Layering guard: OK · graph 1312 files \(\+1 vs base\), 5559 edges \(\+3 vs base\), type inversions 7 \(±0\) · daemon→platforms 62 total \(-2 vs base\) \/ 43 value \(-2 vs base\)/, + ); + assert.match(block, /Coverage: not measured \(pass --coverage\)/); + assert.match(block, /Size: not measured \(pass --size\)/); + assert.match(block, new RegExp(`commit/${HEAD}/checks$`, 'm')); + assert.doesNotMatch(block, /dirty/); +}); + +test('a dirty tree is called out and a fail-open plan is summarized, not enumerated', () => { + const block = renderEvidence( + inputs({ + git: { ...inputs().git, dirty: true }, + affected: { + failOpen: true, + failOpenReasons: [ + { path: 'scripts/x.ts', rule: 'workflow-tooling' }, + { path: 'scripts/y.ts', rule: 'workflow-tooling' }, + ], + checks: [ + { id: 'a', localRunnable: true, ciJobs: [] }, + { id: 'b', localRunnable: false, ciJobs: [] }, + ], + }, + layering: { ok: false, violationsByRule: { 'R9 type-cycle-size': 1 } }, + depgraph: { ...inputs().depgraph, base: undefined }, + }), + ); + assert.match(block, /working tree dirty: describes the tree, not the head/); + assert.match( + block, + /fail-open \(workflow-tooling\): full set, 1 local \+ 1 GitHub-authoritative/, + ); + assert.doesNotMatch(block, /local: a/); + assert.match(block, /Layering guard: R9 type-cycle-size ×1 · graph 1312 files, 5559 edges,/); +}); + +test('coverage and size summaries lift one line out of the tools’ own markdown', () => { + assert.equal( + coverageSummary( + '## Changed-line coverage gate: PASS\n\n| Metric | Value |\n| --- | --- |\n| Changed-line coverage (gating, threshold 80%) | 24/26 (92.31%) |\n', + ), + '24/26 (92.31%), threshold 80% — PASS', + ); + assert.equal( + sizeSummary( + '| Metric | Base | Current | Diff |\n|---|---:|---:|---:|\n| JS raw | 2.30 MB | 2.30 MB | +99 B |\n| JS gzip | 756.3 kB | 756.4 kB | +50 B |\n', + ), + 'JS gzip 756.4 kB (+50 B vs base)', + ); + assert.equal(coverageSummary('nothing'), 'report present, no gating row found'); + assert.equal(sizeSummary('nothing'), 'report present, no JS gzip row found'); +}); diff --git a/scripts/pr-evidence/model.ts b/scripts/pr-evidence/model.ts new file mode 100644 index 000000000..bb7feb39f --- /dev/null +++ b/scripts/pr-evidence/model.ts @@ -0,0 +1,169 @@ +// Pure composition for `pnpm pr:evidence`: every input is something an existing tool already +// produced (the affected selector's JSON, the depgraph report, the layering guard's report, the +// coverage gate's table, the size report). This module turns them into one paste-ready block +// stamped with the exact base and head, and never measures anything itself. + +export type GitFacts = Readonly<{ + branch: string; + head: string; + headShort: string; + base: string; + baseRef: string; + baseShort: string; + /** Uncommitted changes exist: the block describes the tree, not the head. */ + dirty: boolean; + changedFiles: readonly string[]; +}>; + +export type AffectedPlan = Readonly<{ + failOpen: boolean; + failOpenReasons: readonly Readonly<{ path: string; rule: string }>[]; + checks: readonly Readonly<{ id: string; localRunnable: boolean; ciJobs: readonly string[] }>[]; +}>; + +export type DepgraphFacts = Readonly<{ + files: number; + edges: number; + typeInversions: number; + daemonToPlatforms: Readonly<{ count: number; valueCount: number }> | undefined; +}>; + +export type LayeringOutcome = Readonly<{ + ok: boolean; + /** `R9 type-cycle-size` → 1, from the guard's own per-rule lines. */ + violationsByRule: Readonly>; +}>; + +export type EvidenceInputs = Readonly<{ + generatedAt: string; + repository: string; + git: GitFacts; + affected: AffectedPlan; + layering: LayeringOutcome; + depgraph: Readonly<{ head: DepgraphFacts; base: DepgraphFacts | undefined }>; + /** Markdown the coverage gate printed, or the reason it was not run. */ + coverage: Readonly<{ kind: 'table'; markdown: string } | { kind: 'skipped'; reason: string }>; + /** Markdown the size report printed, or the reason it was not run. */ + size: Readonly<{ kind: 'table'; markdown: string } | { kind: 'skipped'; reason: string }>; +}>; + +/** Top-level area of a repo path: `src/daemon/x.ts` → `src`, `docs/agents/y.md` → `docs`. */ +export function groupChangedFiles(paths: readonly string[]): ReadonlyMap { + const groups = new Map(); + for (const file of paths) { + const slash = file.indexOf('/'); + const area = slash === -1 ? '(root)' : file.slice(0, slash); + groups.set(area, (groups.get(area) ?? 0) + 1); + } + return new Map([...groups].sort((a, b) => b[1] - a[1] || a[0].localeCompare(b[0]))); +} + +/** The guard prints ` [R9 type-cycle-size] 1 violation(s):` per rule; the OK run prints none. */ +export function parseLayeringReport(output: string, exitCode: number): LayeringOutcome { + const violationsByRule: Record = {}; + for (const match of output.matchAll(/^\s*\[([^\]]+)\] (\d+) violation\(s\):/gm)) { + violationsByRule[match[1] as string] = Number(match[2]); + } + return { ok: exitCode === 0, violationsByRule }; +} + +/** Reads the counts this block reports out of the depgraph JSON report. */ +export function depgraphFacts(report: { + generated: { files: number; edges: number }; + zoneEdges: readonly { from: string; to: string; count: number; valueCount: number }[]; + typeInversions: Record; +}): DepgraphFacts { + const daemonToPlatforms = report.zoneEdges.find( + (edge) => edge.from === 'daemon-server' && edge.to === 'platforms', + ); + return { + files: report.generated.files, + edges: report.generated.edges, + typeInversions: Object.values(report.typeInversions).reduce((sum, n) => sum + n, 0), + daemonToPlatforms: daemonToPlatforms + ? { count: daemonToPlatforms.count, valueCount: daemonToPlatforms.valueCount } + : undefined, + }; +} + +function delta(head: number, base: number | undefined): string { + if (base === undefined) return ''; + const diff = head - base; + return diff === 0 ? ' (±0)' : ` (${diff > 0 ? '+' : ''}${diff} vs base)`; +} + +function bullet(text: string): string { + return `- ${text}`; +} + +export function renderEvidence(inputs: EvidenceInputs): string { + const { git, affected, layering, depgraph } = inputs; + const areas = [...groupChangedFiles(git.changedFiles)] + .map(([area, count]) => `${count} ${area}`) + .join(', '); + const local = affected.checks.filter((check) => check.localRunnable).map((check) => check.id); + const remote = affected.checks.filter((check) => !check.localRunnable).map((check) => check.id); + // A fail-open plan is the whole catalog; naming every id would only bury the reason. + const affectedLine = affected.failOpen + ? `fail-open (${[...new Set(affected.failOpenReasons.map((r) => r.rule))].join(', ')}): ` + + `full set, ${local.length} local + ${remote.length} GitHub-authoritative` + : `${affected.checks.length} selected` + + (local.length > 0 ? ` · local: ${local.join(', ')}` : '') + + (remote.length > 0 ? ` · GitHub-authoritative: ${remote.join(', ')}` : ''); + const layeringLine = layering.ok + ? 'Layering guard: OK' + : `Layering guard: ${Object.entries(layering.violationsByRule) + .map(([rule, count]) => `${rule} ×${count}`) + .join(', ')}`; + const head = depgraph.head; + const base = depgraph.base; + const daemonEdges = head.daemonToPlatforms + ? `daemon→platforms ${head.daemonToPlatforms.count} total${delta(head.daemonToPlatforms.count, base?.daemonToPlatforms?.count)}` + + ` / ${head.daemonToPlatforms.valueCount} value${delta(head.daemonToPlatforms.valueCount, base?.daemonToPlatforms?.valueCount)}` + : 'daemon→platforms edges: none'; + + const lines = [ + ``, + `**Evidence** gathered ${inputs.generatedAt} at \`${git.headShort}\` (\`${git.branch}\`) against \`${git.baseRef}\` @ \`${git.baseShort}\`` + + (git.dirty ? ' — **working tree dirty: describes the tree, not the head**' : ''), + bullet(`Changed: ${git.changedFiles.length} files (${areas || 'none'})`), + bullet(`Affected gates (\`check:affected\`): ${affectedLine}`), + bullet( + `${layeringLine} · graph ${head.files} files${delta(head.files, base?.files)}, ` + + `${head.edges} edges${delta(head.edges, base?.edges)}, ` + + `type inversions ${head.typeInversions}${delta(head.typeInversions, base?.typeInversions)} · ${daemonEdges}`, + ), + bullet( + inputs.coverage.kind === 'table' + ? `Changed-line coverage: ${coverageSummary(inputs.coverage.markdown)}` + : `Coverage: not measured (${inputs.coverage.reason})`, + ), + bullet( + inputs.size.kind === 'table' + ? `Size: ${sizeSummary(inputs.size.markdown)}` + : `Size: not measured (${inputs.size.reason})`, + ), + bullet( + `CI on this head (authoritative, not claimed here): https://github.com/${inputs.repository}/commit/${git.head}/checks`, + ), + ]; + return `${lines.join('\n')}\n`; +} + +/** `| Changed-line coverage (gating, threshold 80%) | 24/26 (92.31%) |` → `24/26 (92.31%), threshold 80%`. */ +export function coverageSummary(markdown: string): string { + const row = markdown.split('\n').find((line) => line.startsWith('| Changed-line coverage')); + if (!row) return 'report present, no gating row found'; + const cells = row.split('|').map((cell) => cell.trim()); + const threshold = /threshold (\d+%)/.exec(cells[1] ?? '')?.[1]; + const verdict = /gate: (\w+)/.exec(markdown)?.[1] ?? 'unknown'; + return `${cells[2] ?? '?'}${threshold ? `, threshold ${threshold}` : ''} — ${verdict}`; +} + +/** Pulls the JS gzip row's current value and diff out of the size report table. */ +export function sizeSummary(markdown: string): string { + const row = markdown.split('\n').find((line) => line.startsWith('| JS gzip')); + if (!row) return 'report present, no JS gzip row found'; + const cells = row.split('|').map((cell) => cell.trim()); + return `JS gzip ${cells[3] ?? '?'} (${cells[4] ?? '?'} vs base)`; +} diff --git a/scripts/pr-evidence/run.ts b/scripts/pr-evidence/run.ts new file mode 100644 index 000000000..94e91be58 --- /dev/null +++ b/scripts/pr-evidence/run.ts @@ -0,0 +1,194 @@ +// `pnpm pr:evidence [--base ] [--coverage] [--size] [--json]` +// +// One paste-ready evidence block for a PR body, stamped with the exact base and head, composed +// from the tools the repo already has: the affected selector (`check:affected --json`), the +// layering guard, the depgraph report (head, and base through a throwaway worktree so the +// dependency-edge delta is real), and — behind flags, because they need a build or a coverage +// run — the changed-line coverage gate and `pnpm size --base`. It measures nothing itself and +// claims nothing about CI: the last line is the link to the head's checks. +// +// The default tier finishes in ~20s (the layering guard is most of it). `--coverage` reads the +// existing coverage/lcov.info (run `pnpm test:coverage` first); `--size` runs the base worktree +// build the first time (~1-2 min) and ~3s afterwards. + +import fs from 'node:fs'; +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { runCmd, runCmdSync } from '../../src/utils/exec.ts'; +import { parseScriptArgs } from '../lib/cli-args.ts'; +import { runEntrypoint } from '../lib/cli-entrypoint.ts'; +import { + depgraphFacts, + parseLayeringReport, + renderEvidence, + type AffectedPlan, + type DepgraphFacts, + type EvidenceInputs, + type GitFacts, +} from './model.ts'; + +const USAGE = + 'Usage: pnpm pr:evidence [--base ] [--coverage] [--size] [--json]\n' + + ' --base Base ref (default origin/main); the block uses its merge-base with HEAD\n' + + ' --coverage Include changed-line coverage from coverage/lcov.info (run pnpm test:coverage first)\n' + + ' --size Include the JS size delta (pnpm size --base ; needs pnpm build)\n' + + ' --json Emit the collected inputs as JSON instead of the markdown block\n'; + +const REPOSITORY = 'callstack/agent-device'; +const repoRoot = runCmdSync('git', ['rev-parse', '--show-toplevel']).stdout.trim(); +const scripts = path.join(repoRoot, 'scripts'); + +function git(args: readonly string[], cwd = repoRoot): string { + return runCmdSync('git', [...args], { cwd }).stdout.trim(); +} + +function collectGitFacts(baseRef: string): GitFacts { + const head = git(['rev-parse', 'HEAD']); + const base = git(['merge-base', baseRef, 'HEAD']); + const branch = git(['rev-parse', '--abbrev-ref', 'HEAD']); + const dirty = git(['status', '--porcelain', '--untracked-files=no']).length > 0; + const changedFiles = git(['diff', '--name-only', '--no-renames', `${base}..HEAD`]) + .split('\n') + .filter(Boolean); + return { + branch, + head, + headShort: head.slice(0, 9), + base, + baseRef, + baseShort: base.slice(0, 9), + dirty, + changedFiles, + }; +} + +async function collectAffected(base: string): Promise { + const result = await runCmd( + process.execPath, + [ + '--experimental-strip-types', + path.join(scripts, 'check-affected', 'run.ts'), + '--base', + base, + '--head', + 'HEAD', + '--json', + ], + { cwd: repoRoot, timeoutMs: 120_000 }, + ); + const parsed = JSON.parse(result.stdout) as AffectedPlan; + return { + failOpen: parsed.failOpen, + failOpenReasons: parsed.failOpenReasons, + checks: parsed.checks.map(({ id, localRunnable, ciJobs }) => ({ id, localRunnable, ciJobs })), + }; +} + +async function collectLayering() { + const result = await runCmd( + process.execPath, + ['--experimental-strip-types', path.join(scripts, 'layering', 'check.ts')], + { cwd: repoRoot, timeoutMs: 300_000, allowFailure: true }, + ); + return parseLayeringReport(`${result.stdout}\n${result.stderr}`, result.exitCode); +} + +// The depgraph script analyzes whichever repository its cwd is inside, while its imports resolve +// from this checkout, so a bare `git worktree add` (no install) of the base is enough for the +// base numbers to come from the same instrument as the head numbers. +async function collectDepgraph(cwd: string, out: string): Promise { + await runCmd( + process.execPath, + ['--experimental-strip-types', path.join(scripts, 'depgraph', 'build.ts'), '--out', out], + { cwd, timeoutMs: 300_000 }, + ); + return depgraphFacts(JSON.parse(fs.readFileSync(out, 'utf8'))); +} + +async function collectBaseDepgraph(base: string, scratch: string): Promise { + const worktree = path.join(scratch, 'base'); + git(['worktree', 'add', '--detach', worktree, base]); + try { + return await collectDepgraph(worktree, path.join(scratch, 'depgraph-base.json')); + } finally { + git(['worktree', 'remove', '--force', worktree]); + } +} + +async function collectCoverage(base: string): Promise { + if (!fs.existsSync(path.join(repoRoot, 'coverage', 'lcov.info'))) { + return { kind: 'skipped', reason: 'no coverage/lcov.info — run pnpm test:coverage first' }; + } + const result = await runCmd( + process.execPath, + [ + '--experimental-strip-types', + path.join(scripts, 'coverage-changed', 'run.ts'), + '--base', + base, + ], + { cwd: repoRoot, timeoutMs: 300_000, allowFailure: true }, + ); + return { kind: 'table', markdown: result.stdout }; +} + +async function collectSize(base: string): Promise { + if (!fs.existsSync(path.join(repoRoot, 'dist', 'src'))) { + return { kind: 'skipped', reason: 'no dist/src — run pnpm build first' }; + } + const result = await runCmd( + process.execPath, + [path.join(scripts, 'size-report.mjs'), '--base', base], + { cwd: repoRoot, timeoutMs: 600_000 }, + ); + return { kind: 'table', markdown: result.stdout }; +} + +/** The two report tiers that need a build or a coverage run stay opt-in; the block says so. */ +async function optional( + enabled: boolean | undefined, + flag: string, + collect: () => Promise, +): Promise> { + return enabled ? await collect() : { kind: 'skipped', reason: `pass ${flag}` }; +} + +async function main(argv: readonly string[]): Promise { + const values = parseScriptArgs(argv, USAGE, { + base: { type: 'string', default: 'origin/main' }, + coverage: { type: 'boolean', default: false }, + size: { type: 'boolean', default: false }, + json: { type: 'boolean', default: false }, + }); + const baseRef = values.base ?? 'origin/main'; + const gitFacts = collectGitFacts(baseRef); + const scratch = fs.mkdtempSync(path.join(repoRoot, '.tmp', 'pr-evidence-')); + try { + const [affected, layering, head, base] = await Promise.all([ + collectAffected(gitFacts.base), + collectLayering(), + collectDepgraph(repoRoot, path.join(scratch, 'depgraph-head.json')), + collectBaseDepgraph(gitFacts.base, scratch), + ]); + const inputs: EvidenceInputs = { + generatedAt: new Date().toISOString(), + repository: REPOSITORY, + git: gitFacts, + affected, + layering, + depgraph: { head, base }, + coverage: await optional(values.coverage, '--coverage', () => collectCoverage(gitFacts.base)), + size: await optional(values.size, '--size', () => collectSize(gitFacts.base)), + }; + process.stdout.write( + values.json ? `${JSON.stringify(inputs, null, 2)}\n` : renderEvidence(inputs), + ); + return 0; + } finally { + fs.rmSync(scratch, { recursive: true, force: true }); + } +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { + runEntrypoint('pr-evidence', () => main(process.argv.slice(2))); +} diff --git a/scripts/size-report.mjs b/scripts/size-report.mjs index 5160689b7..9b6b018aa 100644 --- a/scripts/size-report.mjs +++ b/scripts/size-report.mjs @@ -35,7 +35,9 @@ if (args.postComment) { } if (args.compare && args.base) { - throw new Error('--compare and --base are exclusive: one supplies the base report, the other measures it'); + throw new Error( + '--compare and --base are exclusive: one supplies the base report, the other measures it', + ); } const startupRuns = parseNonNegativeInteger(args.startupRuns ?? '0', '--startup-runs'); const report = collectReport(cwd, { startupRuns }); @@ -180,7 +182,9 @@ function measureBaseRef(root, ref, options) { } const built = fs.existsSync(path.join(worktreeDir, 'dist', 'src')); if (!built) { - process.stderr.write(`[size] measuring base ${sha.slice(0, 9)} (${ref}): install + build in ${worktreeDir}\n`); + process.stderr.write( + `[size] measuring base ${sha.slice(0, 9)} (${ref}): install + build in ${worktreeDir}\n`, + ); execFileSync('pnpm', ['install', '--frozen-lockfile', '--prefer-offline'], { cwd: worktreeDir, stdio: ['ignore', 'ignore', 'inherit'], @@ -192,17 +196,21 @@ function measureBaseRef(root, ref, options) { function pruneOtherBaseWorktrees(root, worktreesRoot, keep) { if (!fs.existsSync(worktreesRoot)) return; - for (const entry of fs.readdirSync(worktreesRoot, { withFileTypes: true })) { - if (!entry.isDirectory()) continue; - const dir = path.join(worktreesRoot, entry.name); - if (dir === keep) continue; - try { - execFileSync('git', ['worktree', 'remove', '--force', dir], { cwd: root, stdio: 'ignore' }); - } catch { - // Not a registered worktree (a half-created or hand-copied directory): plain removal. - } - fs.rmSync(dir, { recursive: true, force: true }); + const others = fs + .readdirSync(worktreesRoot, { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => path.join(worktreesRoot, entry.name)) + .filter((dir) => dir !== keep); + for (const dir of others) removeWorktree(root, dir); +} + +function removeWorktree(root, dir) { + try { + execFileSync('git', ['worktree', 'remove', '--force', dir], { cwd: root, stdio: 'ignore' }); + } catch { + // Not a registered worktree (a half-created or hand-copied directory): plain removal. } + fs.rmSync(dir, { recursive: true, force: true }); } function prepareGeneratedPackageAssets(root) { @@ -311,7 +319,7 @@ function formatMarkdown(report, baseReport, baseLabel) { return `${COMMENT_MARKER} ## Size Report -| Metric | Base${baseLabel ? ` (${baseLabel})` : ''} | Current | Diff | +| Metric | ${baseColumnLabel(baseLabel)} | Current | Diff | |---|---:|---:|---:| ${rows.join('\n')} @@ -320,6 +328,10 @@ ${changedChunks} `; } +function baseColumnLabel(baseLabel) { + return baseLabel ? `Base (${baseLabel})` : 'Base'; +} + function metricRow(label, base, current) { return `| ${label} | ${formatMaybeBytes(base)} | ${formatBytes(current)} | ${formatDiff(base, current)} |`; } From 94def305cd9d43bd68cc33a4d4b0c1ccf9e8e70f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 18:11:02 +0200 Subject: [PATCH 03/10] fix(tooling): pr:evidence measures pristine head/base worktrees from an os.tmpdir scratch; size --base gets a per-SHA lock, completion stamp, and non-destructive eviction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (three P1s): - pr:evidence created its scratch under an untracked .tmp/ that a fresh checkout lacks (ENOENT). Scratch now lives under os.tmpdir(), which exists by construction; a real entrypoint regression runs the whole pipeline with --base HEAD (no origin/main needed) and asserts JSON shape plus cleanup of both worktrees and the scratch. - Untracked or uncommitted production files could move the layering/depgraph numbers the block labels as HEAD's. Head is now measured from a pristine worktree of the head commit exactly like base, and the affected plan takes the head SHA (the literal HEAD folds the working tree in). The dirty flag now counts untracked files and says they are not in the block. - size --base force-pruned other cached bases without locking and trusted a dist/src that could be half-built. Per-SHA .lock (pid, O_EXCL) held from before the worktree exists until the base report is read; a live lock on the same base fails fast, a stale one is replaced; eviction skips worktrees whose lock owner is alive; dist/.size-base-complete marks a finished build. Orchestration tests run the real script against a throwaway git repo with pnpm/npm shimmed on PATH (build once, reuse, live lock, stale lock, interrupted build, guarded vs idle eviction). Also fixes the /tmp → /private/tmp realpath mismatch those tests surfaced (git lists worktrees by real path, so the registration check removed a live worktree). --- docs/agents/testing.md | 8 +- package.json | 2 +- scripts/__tests__/size-report-base.test.ts | 148 +++++++++++++++++++++ scripts/pr-evidence/model.test.ts | 5 +- scripts/pr-evidence/model.ts | 6 +- scripts/pr-evidence/run.test.ts | 53 ++++++++ scripts/pr-evidence/run.ts | 64 +++++---- scripts/size-report.mjs | 140 +++++++++++++++---- vitest.config.ts | 3 + 9 files changed, 372 insertions(+), 57 deletions(-) create mode 100644 scripts/__tests__/size-report-base.test.ts create mode 100644 scripts/pr-evidence/run.test.ts diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 24dc98129..4357b07c2 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -235,9 +235,15 @@ command, before the PR exists: ```sh pnpm size --base origin/main # first run: detached worktree + install + build of the base (~1-2 min) - # later runs against the same base: ~3s (the worktree is kept under .tmp/size-base/) + # later runs against the same base: ~3s (the worktree is kept under .tmp/size-base/) ``` +The cache is per SHA and never destructive toward a run in progress: a `.lock` (pid inside) is held +from before the worktree exists until the base report is read, a concurrent run against the same +base fails fast rather than reading a half-built `dist`, another base's run evicts only worktrees +whose lock is absent or whose owner is dead, and a build that was interrupted before its +`dist/.size-base-complete` stamp is rebuilt. + Requires a current `pnpm build` of your own tree. `JS raw`/`JS gzip` are the numbers to quote and to budget against (ADR 0019 §8 units state theirs before starting); the `npm tarball`/`npm unpacked` rows compare a fresh base checkout against your working tree, which may carry locally built helper diff --git a/package.json b/package.json index aae95895d..0cef658ce 100644 --- a/package.json +++ b/package.json @@ -118,7 +118,7 @@ "maestro:conformance:differential": "node --experimental-strip-types packages/maestro/test/conformance/differential/run.ts", "size": "node scripts/size-report.mjs", "pr:evidence": "node --experimental-strip-types scripts/pr-evidence/run.ts", - "pr:evidence:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/pr-evidence/model.test.ts", + "pr:evidence:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/pr-evidence/model.test.ts scripts/pr-evidence/run.test.ts", "perf": "node --experimental-strip-types scripts/perf/run.ts", "mutation:run": "node --experimental-strip-types scripts/mutation/run.ts", "mutation:check": "node --experimental-strip-types scripts/mutation/run.ts --no-run", diff --git a/scripts/__tests__/size-report-base.test.ts b/scripts/__tests__/size-report-base.test.ts new file mode 100644 index 000000000..b14fea6f5 --- /dev/null +++ b/scripts/__tests__/size-report-base.test.ts @@ -0,0 +1,148 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { afterAll, beforeAll, test } from 'vitest'; +import { runCmd, runCmdSync } from '../../src/utils/exec.ts'; + +// `pnpm size --base ` orchestration against a throwaway git repository, with `pnpm` and +// `npm` shimmed on PATH: the shim `pnpm build` writes dist/src and appends to a log, the shim +// `npm pack` prints a fixed dry-run JSON. No install, no network; every run is git + node. + +const ROOT = path.join(import.meta.dirname, '..', '..'); +const SCRIPT = path.join(ROOT, 'scripts', 'size-report.mjs'); +const NEVER_A_PID = 2_147_483_647; // outside every platform's pid range: dead by construction + +let repo: string; +let bin: string; +let buildLog: string; +let first: string; +let second: string; + +function git(args: string[], cwd = repo): string { + return runCmdSync('git', args, { cwd }).stdout.trim(); +} + +function writeExecutable(file: string, body: string): void { + fs.writeFileSync(file, body); + fs.chmodSync(file, 0o755); +} + +beforeAll(() => { + const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'size-report-base-')); + repo = path.join(scratch, 'repo'); + bin = path.join(scratch, 'bin'); + buildLog = path.join(scratch, 'build.log'); + fs.mkdirSync(repo); + fs.mkdirSync(bin); + writeExecutable( + path.join(bin, 'pnpm'), + `#!/bin/sh +echo "$PWD $*" >> "${buildLog}" +if [ "$1" = "build" ]; then mkdir -p dist/src && printf 'export const built = 1;\\n' > dist/src/index.js; fi +`, + ); + writeExecutable( + path.join(bin, 'npm'), + `#!/bin/sh +echo '[{"filename":"pkg.tgz","size":100,"unpackedSize":200,"entryCount":2}]' +`, + ); + git(['init', '-q', '-b', 'main']); + git(['config', 'user.email', 'size@test']); + git(['config', 'user.name', 'size test']); + fs.writeFileSync(path.join(repo, 'package.json'), '{"name":"probe","version":"1.0.0"}\n'); + fs.writeFileSync(path.join(repo, '.gitignore'), '.tmp/\ndist/\n'); + git(['add', '.']); + git(['commit', '-q', '-m', 'first']); + first = git(['rev-parse', 'HEAD']); + fs.writeFileSync(path.join(repo, 'README.md'), 'second\n'); + git(['add', '.']); + git(['commit', '-q', '-m', 'second']); + second = git(['rev-parse', 'HEAD']); + // The head side of the comparison needs a dist too. + fs.mkdirSync(path.join(repo, 'dist', 'src'), { recursive: true }); + fs.writeFileSync(path.join(repo, 'dist', 'src', 'index.js'), 'export const head = 1;\n'); +}); + +afterAll(() => { + fs.rmSync(path.dirname(repo), { recursive: true, force: true }); +}); + +async function size(base: string) { + return await runCmd(process.execPath, [SCRIPT, '--cwd', repo, '--base', base], { + cwd: repo, + env: { ...process.env, PATH: `${bin}:${process.env.PATH ?? ''}` }, + allowFailure: true, + timeoutMs: 60_000, + }); +} + +const worktreeOf = (sha: string) => path.join(repo, '.tmp', 'size-base', sha.slice(0, 12)); +const lockOf = (sha: string) => `${worktreeOf(sha)}.lock`; +const stampOf = (sha: string) => path.join(worktreeOf(sha), 'dist', '.size-base-complete'); +const builds = () => + fs + .readFileSync(buildLog, 'utf8') + .split('\n') + .filter((l) => l.endsWith(' build')); + +test('first run builds the base in a per-SHA worktree, stamps it, releases its lock; second run reuses it', async () => { + const one = await size(first); + assert.equal(one.exitCode, 0, one.stderr); + assert.match(one.stdout, /\| JS raw \|/); + assert.ok(fs.existsSync(stampOf(first)), 'completion stamp written after build'); + assert.equal(fs.existsSync(lockOf(first)), false, 'lock released after the report was read'); + assert.equal(builds().length, 1); + + const two = await size(first); + assert.equal(two.exitCode, 0, two.stderr); + assert.equal(builds().length, 1, 'a stamped base is not rebuilt'); +}); + +test('a base whose lock is held by a live pid fails fast without touching its worktree', async () => { + fs.writeFileSync(lockOf(first), `${process.pid}\n`); // this test process: alive + const before = fs.statSync(stampOf(first)).mtimeMs; + const result = await size(first); + assert.notEqual(result.exitCode, 0); + assert.match( + result.stderr, + new RegExp(`another \`size --base\` \\(pid ${process.pid}\\) is building`), + ); + assert.equal(fs.statSync(stampOf(first)).mtimeMs, before); + assert.equal(builds().length, 1); + fs.rmSync(lockOf(first)); +}); + +test('a stale lock (dead pid) is replaced and the run proceeds', async () => { + fs.writeFileSync(lockOf(first), `${NEVER_A_PID}\n`); + const result = await size(first); + assert.equal(result.exitCode, 0, result.stderr); + assert.equal(fs.existsSync(lockOf(first)), false); +}); + +test('an unstamped worktree (interrupted build) is rebuilt rather than trusted', async () => { + fs.rmSync(stampOf(first)); + const result = await size(first); + assert.equal(result.exitCode, 0, result.stderr); + assert.equal(builds().length, 2, 'dist/src existing without the stamp is not enough'); + assert.ok(fs.existsSync(stampOf(first))); +}); + +test('measuring another base evicts an idle cached base but never one whose lock is live', async () => { + fs.writeFileSync(lockOf(first), `${process.pid}\n`); // in use by "another run" + const guarded = await size(second); + assert.equal(guarded.exitCode, 0, guarded.stderr); + assert.ok(fs.existsSync(worktreeOf(first)), 'a live-locked worktree survives eviction'); + assert.ok(fs.existsSync(stampOf(second))); + fs.rmSync(lockOf(first)); + + const evicting = await size(first); + assert.equal(evicting.exitCode, 0, evicting.stderr); + assert.equal(fs.existsSync(worktreeOf(second)), false, 'an idle other base is evicted'); + assert.equal( + git(['worktree', 'list', '--porcelain']).includes(worktreeOf(second)), + false, + 'and unregistered from git', + ); +}); diff --git a/scripts/pr-evidence/model.test.ts b/scripts/pr-evidence/model.test.ts index 4afa7888c..5f7a4acd5 100644 --- a/scripts/pr-evidence/model.test.ts +++ b/scripts/pr-evidence/model.test.ts @@ -141,7 +141,10 @@ test('a dirty tree is called out and a fail-open plan is summarized, not enumera depgraph: { ...inputs().depgraph, base: undefined }, }), ); - assert.match(block, /working tree dirty: describes the tree, not the head/); + assert.match( + block, + /working tree has uncommitted\/untracked changes: none of them are in this block/, + ); assert.match( block, /fail-open \(workflow-tooling\): full set, 1 local \+ 1 GitHub-authoritative/, diff --git a/scripts/pr-evidence/model.ts b/scripts/pr-evidence/model.ts index bb7feb39f..3a0c8587b 100644 --- a/scripts/pr-evidence/model.ts +++ b/scripts/pr-evidence/model.ts @@ -10,7 +10,7 @@ export type GitFacts = Readonly<{ base: string; baseRef: string; baseShort: string; - /** Uncommitted changes exist: the block describes the tree, not the head. */ + /** Uncommitted or untracked changes exist; everything measured here is the head, not them. */ dirty: boolean; changedFiles: readonly string[]; }>; @@ -125,7 +125,9 @@ export function renderEvidence(inputs: EvidenceInputs): string { const lines = [ ``, `**Evidence** gathered ${inputs.generatedAt} at \`${git.headShort}\` (\`${git.branch}\`) against \`${git.baseRef}\` @ \`${git.baseShort}\`` + - (git.dirty ? ' — **working tree dirty: describes the tree, not the head**' : ''), + (git.dirty + ? ' — **working tree has uncommitted/untracked changes: none of them are in this block**' + : ''), bullet(`Changed: ${git.changedFiles.length} files (${areas || 'none'})`), bullet(`Affected gates (\`check:affected\`): ${affectedLine}`), bullet( diff --git a/scripts/pr-evidence/run.test.ts b/scripts/pr-evidence/run.test.ts new file mode 100644 index 000000000..d708fb272 --- /dev/null +++ b/scripts/pr-evidence/run.test.ts @@ -0,0 +1,53 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from 'node:test'; +import { runCmd, runCmdSync } from '../../src/utils/exec.ts'; + +const REPOSITORY_ROOT = path.resolve(import.meta.dirname, '..', '..'); +const RUN = path.join(REPOSITORY_ROOT, 'scripts', 'pr-evidence', 'run.ts'); + +function scratchDirectories(): string[] { + return fs.readdirSync(os.tmpdir()).filter((name) => name.startsWith('agent-device-pr-evidence-')); +} + +function evidenceWorktrees(): string[] { + return runCmdSync('git', ['worktree', 'list', '--porcelain'], { cwd: REPOSITORY_ROOT }) + .stdout.split('\n') + .filter((line) => line.startsWith('worktree ') && line.includes('agent-device-pr-evidence-')); +} + +// The real entrypoint, end to end, in this repository: `--base HEAD` makes the merge-base HEAD +// itself, so it needs no origin/main and no network (a depth-1 CI checkout is enough) while +// still creating both pristine worktrees, running the selector, the layering guard, and the +// depgraph twice, and rendering the block. It is the regression for the fresh-checkout failure +// (scratch used to be created under an untracked, possibly absent `.tmp/`) and for cleanup. +test('pr:evidence runs end to end from a pristine head worktree and cleans up after itself', async () => { + const before = { scratch: scratchDirectories(), worktrees: evidenceWorktrees() }; + const result = await runCmd( + process.execPath, + ['--experimental-strip-types', RUN, '--base', 'HEAD', '--json'], + { cwd: REPOSITORY_ROOT, timeoutMs: 300_000 }, + ); + const inputs = JSON.parse(result.stdout) as { + git: { head: string; base: string; changedFiles: string[]; dirty: boolean }; + affected: { checks: unknown[] }; + layering: { ok: boolean }; + depgraph: { head: { files: number; edges: number }; base: { files: number; edges: number } }; + coverage: { kind: string }; + size: { kind: string }; + }; + const head = runCmdSync('git', ['rev-parse', 'HEAD'], { cwd: REPOSITORY_ROOT }).stdout.trim(); + assert.equal(inputs.git.head, head); + assert.equal(inputs.git.base, head, '--base HEAD makes the merge-base the head itself'); + assert.deepEqual(inputs.git.changedFiles, []); + assert.ok(inputs.depgraph.head.files > 500, 'the head worktree was analyzed, not an empty tree'); + assert.deepEqual(inputs.depgraph.base, inputs.depgraph.head, 'same commit, same numbers'); + assert.equal(typeof inputs.layering.ok, 'boolean'); + assert.equal(inputs.coverage.kind, 'skipped'); + assert.equal(inputs.size.kind, 'skipped'); + // Both worktrees and the os.tmpdir() scratch are gone, whatever else was there before. + assert.deepEqual(scratchDirectories(), before.scratch); + assert.deepEqual(evidenceWorktrees(), before.worktrees); +}); diff --git a/scripts/pr-evidence/run.ts b/scripts/pr-evidence/run.ts index 94e91be58..f50b48cd9 100644 --- a/scripts/pr-evidence/run.ts +++ b/scripts/pr-evidence/run.ts @@ -2,16 +2,22 @@ // // One paste-ready evidence block for a PR body, stamped with the exact base and head, composed // from the tools the repo already has: the affected selector (`check:affected --json`), the -// layering guard, the depgraph report (head, and base through a throwaway worktree so the -// dependency-edge delta is real), and — behind flags, because they need a build or a coverage -// run — the changed-line coverage gate and `pnpm size --base`. It measures nothing itself and -// claims nothing about CI: the last line is the link to the head's checks. +// layering guard, the depgraph report, and — behind flags, because they need a build or a +// coverage run — the changed-line coverage gate and `pnpm size --base`. It measures nothing +// itself and claims nothing about CI: the last line is the link to the head's checks. +// +// Everything labelled "at " is measured from a throwaway `git worktree` of that exact +// commit, and the base likewise, so an untracked or uncommitted file in the working tree can +// change nothing the block reports as HEAD's; the working tree only contributes the "dirty" +// flag. The worktrees need no install: the scripts analyze whichever repository their cwd is +// in, while their imports resolve from this checkout. // // The default tier finishes in ~20s (the layering guard is most of it). `--coverage` reads the // existing coverage/lcov.info (run `pnpm test:coverage` first); `--size` runs the base worktree // build the first time (~1-2 min) and ~3s afterwards. import fs from 'node:fs'; +import os from 'node:os'; import path from 'node:path'; import { pathToFileURL } from 'node:url'; import { runCmd, runCmdSync } from '../../src/utils/exec.ts'; @@ -46,7 +52,8 @@ function collectGitFacts(baseRef: string): GitFacts { const head = git(['rev-parse', 'HEAD']); const base = git(['merge-base', baseRef, 'HEAD']); const branch = git(['rev-parse', '--abbrev-ref', 'HEAD']); - const dirty = git(['status', '--porcelain', '--untracked-files=no']).length > 0; + // Untracked files count: they are exactly what a pristine-worktree measurement leaves out. + const dirty = git(['status', '--porcelain']).length > 0; const changedFiles = git(['diff', '--name-only', '--no-renames', `${base}..HEAD`]) .split('\n') .filter(Boolean); @@ -62,7 +69,9 @@ function collectGitFacts(baseRef: string): GitFacts { }; } -async function collectAffected(base: string): Promise { +// The head SHA, not the literal `HEAD`: the selector folds working-tree changes into a plan for +// `HEAD`, and this block describes the commit. +async function collectAffected(base: string, head: string): Promise { const result = await runCmd( process.execPath, [ @@ -71,7 +80,7 @@ async function collectAffected(base: string): Promise { '--base', base, '--head', - 'HEAD', + head, '--json', ], { cwd: repoRoot, timeoutMs: 120_000 }, @@ -84,18 +93,15 @@ async function collectAffected(base: string): Promise { }; } -async function collectLayering() { +async function collectLayering(cwd: string) { const result = await runCmd( process.execPath, ['--experimental-strip-types', path.join(scripts, 'layering', 'check.ts')], - { cwd: repoRoot, timeoutMs: 300_000, allowFailure: true }, + { cwd, timeoutMs: 300_000, allowFailure: true }, ); return parseLayeringReport(`${result.stdout}\n${result.stderr}`, result.exitCode); } -// The depgraph script analyzes whichever repository its cwd is inside, while its imports resolve -// from this checkout, so a bare `git worktree add` (no install) of the base is enough for the -// base numbers to come from the same instrument as the head numbers. async function collectDepgraph(cwd: string, out: string): Promise { await runCmd( process.execPath, @@ -105,14 +111,15 @@ async function collectDepgraph(cwd: string, out: string): Promise return depgraphFacts(JSON.parse(fs.readFileSync(out, 'utf8'))); } -async function collectBaseDepgraph(base: string, scratch: string): Promise { - const worktree = path.join(scratch, 'base'); - git(['worktree', 'add', '--detach', worktree, base]); - try { - return await collectDepgraph(worktree, path.join(scratch, 'depgraph-base.json')); - } finally { - git(['worktree', 'remove', '--force', worktree]); - } +/** A pristine checkout of one commit; removed by the caller's scratch cleanup. */ +function addWorktree(scratch: string, name: string, commit: string): string { + const worktree = path.join(scratch, name); + git(['worktree', 'add', '--detach', worktree, commit]); + return worktree; +} + +function removeWorktree(worktree: string): void { + git(['worktree', 'remove', '--force', worktree]); } async function collectCoverage(base: string): Promise { @@ -162,13 +169,19 @@ async function main(argv: readonly string[]): Promise { }); const baseRef = values.base ?? 'origin/main'; const gitFacts = collectGitFacts(baseRef); - const scratch = fs.mkdtempSync(path.join(repoRoot, '.tmp', 'pr-evidence-')); + // os.tmpdir() always exists; a repo-local scratch would have to be created first and is one + // more thing a fresh checkout can lack. + const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-pr-evidence-')); + const worktrees: string[] = []; try { + const headTree = addWorktree(scratch, 'head', gitFacts.head); + const baseTree = addWorktree(scratch, 'base', gitFacts.base); + worktrees.push(headTree, baseTree); const [affected, layering, head, base] = await Promise.all([ - collectAffected(gitFacts.base), - collectLayering(), - collectDepgraph(repoRoot, path.join(scratch, 'depgraph-head.json')), - collectBaseDepgraph(gitFacts.base, scratch), + collectAffected(gitFacts.base, gitFacts.head), + collectLayering(headTree), + collectDepgraph(headTree, path.join(scratch, 'depgraph-head.json')), + collectDepgraph(baseTree, path.join(scratch, 'depgraph-base.json')), ]); const inputs: EvidenceInputs = { generatedAt: new Date().toISOString(), @@ -185,6 +198,7 @@ async function main(argv: readonly string[]): Promise { ); return 0; } finally { + for (const worktree of worktrees) removeWorktree(worktree); fs.rmSync(scratch, { recursive: true, force: true }); } } diff --git a/scripts/size-report.mjs b/scripts/size-report.mjs index 9b6b018aa..04f2d4441 100644 --- a/scripts/size-report.mjs +++ b/scripts/size-report.mjs @@ -157,51 +157,137 @@ function collectReport(root, options) { } // The Size workflow measures the base by checking it out, installing, and building; this is -// the same recipe in a detached worktree so the working tree is never touched. The worktree -// is kept under .tmp/size-base/ so a second run against the same base skips the -// install+build (mirroring the workflow's dist cache); other bases' worktrees are removed. +// the same recipe in a detached worktree so the working tree is never touched. Cache semantics +// are per SHA and non-destructive toward anything in use: +// - .tmp/size-base// is the worktree; a second run against the same base finds the +// completeness stamp and skips install+build (mirroring the workflow's dist cache); +// - .tmp/size-base/.lock (pid inside, created O_EXCL) is held from before the worktree +// is created until the base report has been read, so a concurrent run against the same base +// fails fast instead of reading a half-built dist, and a run against another base never +// removes a worktree whose lock is held by a live pid; a lock whose pid is dead is stale; +// - dist/.size-base-complete is written after a successful build, so an interrupted build is +// rebuilt rather than trusted because dist/src happens to exist. function measureBaseRef(root, ref, options) { const sha = execFileSync('git', ['rev-parse', '--verify', `${ref}^{commit}`], { cwd: root, encoding: 'utf8', }).trim(); - const worktreesRoot = path.join(root, '.tmp', 'size-base'); + fs.mkdirSync(path.join(root, '.tmp', 'size-base'), { recursive: true }); + // Canonical: git lists worktrees by real path (/tmp is /private/tmp on macOS), and the + // registration check below compares against that listing. + const worktreesRoot = fs.realpathSync(path.join(root, '.tmp', 'size-base')); const worktreeDir = path.join(worktreesRoot, sha.slice(0, 12)); - pruneOtherBaseWorktrees(root, worktreesRoot, worktreeDir); + const release = acquireBaseLock(worktreeDir, sha); + try { + pruneOtherBaseWorktrees(root, worktreesRoot, worktreeDir); + ensureBaseWorktree(root, worktreeDir, sha); + if (!fs.existsSync(baseCompletionStamp(worktreeDir))) { + process.stderr.write( + `[size] measuring base ${sha.slice(0, 9)} (${ref}): install + build in ${worktreeDir}\n`, + ); + execFileSync('pnpm', ['install', '--frozen-lockfile', '--prefer-offline'], { + cwd: worktreeDir, + stdio: ['ignore', 'ignore', 'inherit'], + }); + execFileSync('pnpm', ['build'], { cwd: worktreeDir, stdio: ['ignore', 'ignore', 'inherit'] }); + fs.writeFileSync(baseCompletionStamp(worktreeDir), `${sha}\n`); + } + return collectReport(worktreeDir, options); + } finally { + release(); + } +} + +function baseCompletionStamp(worktreeDir) { + return path.join(worktreeDir, 'dist', '.size-base-complete'); +} + +function baseLockPath(worktreeDir) { + return `${worktreeDir}.lock`; +} + +// O_EXCL create is the atomic step; the pid inside is what makes a leftover lock recognizable +// as stale (its owner died) rather than a permanent wedge. +function acquireBaseLock(worktreeDir, sha) { + const lockPath = baseLockPath(worktreeDir); + for (;;) { + const release = tryCreateLock(lockPath); + if (release) return release; + if (isLockHeldByLiveProcess(lockPath)) { + throw new Error( + `another \`size --base\` (pid ${lockOwnerPid(lockPath)}) is building base ${sha.slice(0, 9)} in ${worktreeDir}; wait for it or measure a different base`, + ); + } + fs.rmSync(lockPath, { force: true }); // stale: its owner is gone + } +} + +function isLockHeldByLiveProcess(lockPath) { + const owner = lockOwnerPid(lockPath); + return owner !== undefined && isProcessAlive(owner); +} + +/** Returns the release function on success, or undefined when the lock already exists. */ +function tryCreateLock(lockPath) { + let fd; + try { + fd = fs.openSync(lockPath, 'wx'); + } catch (error) { + if (error.code === 'EEXIST') return undefined; + throw error; + } + fs.writeSync(fd, `${process.pid}\n`); + fs.closeSync(fd); + return () => fs.rmSync(lockPath, { force: true }); +} + +function lockOwnerPid(lockPath) { + try { + const pid = Number(fs.readFileSync(lockPath, 'utf8').trim()); + return Number.isInteger(pid) && pid > 0 ? pid : undefined; + } catch { + return undefined; + } +} + +function isProcessAlive(pid) { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return error.code === 'EPERM'; + } +} + +function ensureBaseWorktree(root, worktreeDir, sha) { const registered = execFileSync('git', ['worktree', 'list', '--porcelain'], { cwd: root, encoding: 'utf8', }).includes(`worktree ${worktreeDir}\n`); - if (!registered) { - fs.rmSync(worktreeDir, { recursive: true, force: true }); - fs.mkdirSync(worktreesRoot, { recursive: true }); - execFileSync('git', ['worktree', 'add', '--detach', worktreeDir, sha], { - cwd: root, - stdio: ['ignore', 'ignore', 'inherit'], - }); - } - const built = fs.existsSync(path.join(worktreeDir, 'dist', 'src')); - if (!built) { - process.stderr.write( - `[size] measuring base ${sha.slice(0, 9)} (${ref}): install + build in ${worktreeDir}\n`, - ); - execFileSync('pnpm', ['install', '--frozen-lockfile', '--prefer-offline'], { - cwd: worktreeDir, - stdio: ['ignore', 'ignore', 'inherit'], - }); - execFileSync('pnpm', ['build'], { cwd: worktreeDir, stdio: ['ignore', 'ignore', 'inherit'] }); - } - return collectReport(worktreeDir, options); + if (registered && fs.existsSync(worktreeDir)) return; + // Registered but gone (hand-deleted), or present but unregistered (hand-copied): start clean. + fs.rmSync(worktreeDir, { recursive: true, force: true }); + execFileSync('git', ['worktree', 'prune'], { cwd: root, stdio: 'ignore' }); + execFileSync('git', ['worktree', 'add', '--detach', worktreeDir, sha], { + cwd: root, + stdio: ['ignore', 'ignore', 'pipe'], + encoding: 'utf8', + }); } +// Cache eviction of idle entries only: a worktree whose lock is held by a live pid is in use +// by another run and is left alone. function pruneOtherBaseWorktrees(root, worktreesRoot, keep) { - if (!fs.existsSync(worktreesRoot)) return; const others = fs .readdirSync(worktreesRoot, { withFileTypes: true }) .filter((entry) => entry.isDirectory()) .map((entry) => path.join(worktreesRoot, entry.name)) .filter((dir) => dir !== keep); - for (const dir of others) removeWorktree(root, dir); + for (const dir of others) { + if (isLockHeldByLiveProcess(baseLockPath(dir))) continue; + removeWorktree(root, dir); + fs.rmSync(baseLockPath(dir), { force: true }); + } } function removeWorktree(root, dir) { diff --git a/vitest.config.ts b/vitest.config.ts index 21f5a9b0e..3ee1701c8 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -78,6 +78,9 @@ export default defineConfig({ // The Bundle Size lane's PR-comment path: spawns the real script against a // stubbed fetch, so it needs no network; pins retry/reconcile/fatal outcomes. 'scripts/__tests__/size-report-post-comment.test.ts', + // `--base` orchestration (per-SHA worktree, lock, completion stamp, eviction) against + // a throwaway git repo with pnpm/npm shimmed on PATH: git + node only. + 'scripts/__tests__/size-report-base.test.ts', // Parses CI configuration only, so this action guard needs no device or subprocess lane. 'test/ci/upload-agent-device-artifacts.test.ts', // #1781 A9: pins the root-doc paths-ignore entries directly against the From fd185047f0e24d89d7ffd9a469bc4bbc6c007b80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 19:17:59 +0200 Subject: [PATCH 04/10] fix(tooling): symlink-identity locks with compare-then-unlink; evict under the victim's lock; pr:evidence registers worktrees on add and cleans up exhaustively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (three P1s): - Lock creation/takeover races: the lock is now a symlink whose target is the owner identity (pid:nonce), created with its identity in one syscall (no empty-file window), taken over only by compare-then-unlink on the exact identity judged stale, and verified after creation; release unlinks only a link that still names this run. Real overlapping-process tests: two runs on one base (exactly one builds, the other fails fast), and a takeover race against a simulated other taker across delays straddling the acquire window (a live lock is never unlinked, both never proceed). - Cross-base eviction: a victim is removed only while holding its own lock, acquired through the same path, so a run wanting it after the check finds it locked rather than half-removed; a live-locked victim is skipped. - pr:evidence worktrees: withWorktrees registers each worktree the moment its add succeeds and sweeps every resource on the way out, collecting failures instead of stopping at the first; planted reds for both (second add fails → first removed; removal of the middle one throws → the others still go). --- package.json | 2 +- scripts/__tests__/size-report-base.test.ts | 105 +++++++++++++++++- scripts/pr-evidence/run.ts | 73 ++++++------- scripts/pr-evidence/worktrees.test.ts | 119 +++++++++++++++++++++ scripts/pr-evidence/worktrees.ts | 76 +++++++++++++ scripts/size-report.mjs | 104 +++++++++++++----- 6 files changed, 406 insertions(+), 73 deletions(-) create mode 100644 scripts/pr-evidence/worktrees.test.ts create mode 100644 scripts/pr-evidence/worktrees.ts diff --git a/package.json b/package.json index 0cef658ce..aaf675332 100644 --- a/package.json +++ b/package.json @@ -118,7 +118,7 @@ "maestro:conformance:differential": "node --experimental-strip-types packages/maestro/test/conformance/differential/run.ts", "size": "node scripts/size-report.mjs", "pr:evidence": "node --experimental-strip-types scripts/pr-evidence/run.ts", - "pr:evidence:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/pr-evidence/model.test.ts scripts/pr-evidence/run.test.ts", + "pr:evidence:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/pr-evidence/model.test.ts scripts/pr-evidence/worktrees.test.ts scripts/pr-evidence/run.test.ts", "perf": "node --experimental-strip-types scripts/perf/run.ts", "mutation:run": "node --experimental-strip-types scripts/mutation/run.ts", "mutation:check": "node --experimental-strip-types scripts/mutation/run.ts --no-run", diff --git a/scripts/__tests__/size-report-base.test.ts b/scripts/__tests__/size-report-base.test.ts index b14fea6f5..a1eeb24ec 100644 --- a/scripts/__tests__/size-report-base.test.ts +++ b/scripts/__tests__/size-report-base.test.ts @@ -80,6 +80,7 @@ async function size(base: string) { const worktreeOf = (sha: string) => path.join(repo, '.tmp', 'size-base', sha.slice(0, 12)); const lockOf = (sha: string) => `${worktreeOf(sha)}.lock`; +const holdLock = (sha: string, pid: number) => fs.symlinkSync(`${pid}:test`, lockOf(sha)); const stampOf = (sha: string) => path.join(worktreeOf(sha), 'dist', '.size-base-complete'); const builds = () => fs @@ -101,13 +102,13 @@ test('first run builds the base in a per-SHA worktree, stamps it, releases its l }); test('a base whose lock is held by a live pid fails fast without touching its worktree', async () => { - fs.writeFileSync(lockOf(first), `${process.pid}\n`); // this test process: alive + holdLock(first, process.pid); // this test process: alive const before = fs.statSync(stampOf(first)).mtimeMs; const result = await size(first); assert.notEqual(result.exitCode, 0); assert.match( result.stderr, - new RegExp(`another \`size --base\` \\(pid ${process.pid}\\) is building`), + new RegExp(`another \`size --base\` \\(pid ${process.pid}\\) is using`), ); assert.equal(fs.statSync(stampOf(first)).mtimeMs, before); assert.equal(builds().length, 1); @@ -115,7 +116,7 @@ test('a base whose lock is held by a live pid fails fast without touching its wo }); test('a stale lock (dead pid) is replaced and the run proceeds', async () => { - fs.writeFileSync(lockOf(first), `${NEVER_A_PID}\n`); + holdLock(first, NEVER_A_PID); const result = await size(first); assert.equal(result.exitCode, 0, result.stderr); assert.equal(fs.existsSync(lockOf(first)), false); @@ -130,7 +131,7 @@ test('an unstamped worktree (interrupted build) is rebuilt rather than trusted', }); test('measuring another base evicts an idle cached base but never one whose lock is live', async () => { - fs.writeFileSync(lockOf(first), `${process.pid}\n`); // in use by "another run" + holdLock(first, process.pid); // in use by "another run" const guarded = await size(second); assert.equal(guarded.exitCode, 0, guarded.stderr); assert.ok(fs.existsSync(worktreeOf(first)), 'a live-locked worktree survives eviction'); @@ -146,3 +147,99 @@ test('measuring another base evicts an idle cached base but never one whose lock 'and unregistered from git', ); }); + +test('two overlapping runs on the same base: exactly one builds, the other fails fast on the live lock', async () => { + // A slower shim build widens the overlap window: the second run must find the first run's + // symlink lock (identity in place from its single creating syscall) and refuse. + fs.rmSync(worktreeOf(first), { recursive: true, force: true }); + runCmdSync('git', ['worktree', 'prune'], { cwd: repo }); + fs.rmSync(stampOf(first), { force: true }); + const slowBin = path.join(path.dirname(bin), 'slow-bin'); + fs.mkdirSync(slowBin, { recursive: true }); + writeExecutable( + path.join(slowBin, 'pnpm'), + `#!/bin/sh +echo "$PWD $*" >> "${buildLog}" +if [ "$1" = "build" ]; then sleep 1; mkdir -p dist/src && printf 'export const built = 1;\\n' > dist/src/index.js; fi +`, + ); + fs.copyFileSync(path.join(bin, 'npm'), path.join(slowBin, 'npm')); + fs.chmodSync(path.join(slowBin, 'npm'), 0o755); + const buildsBefore = builds().length; + const env = { ...process.env, PATH: `${slowBin}:${process.env.PATH ?? ''}` }; + const run = () => + runCmd(process.execPath, [SCRIPT, '--cwd', repo, '--base', first], { + cwd: repo, + env, + allowFailure: true, + timeoutMs: 60_000, + }); + const [a, b] = await Promise.all([run(), run()]); + const outcomes = [a, b].map((r) => r.exitCode === 0); + assert.deepEqual( + outcomes.sort(), + [false, true], + `one wins, one refuses: ${a.stderr} ${b.stderr}`, + ); + const loser = a.exitCode === 0 ? b : a; + assert.match(loser.stderr, /another `size --base` \(pid \d+\) is using base/); + assert.equal(builds().length - buildsBefore, 1, 'exactly one build across the two runs'); + assert.ok(fs.existsSync(stampOf(first))); + assert.equal( + fs.existsSync(lockOf(first)), + false, + 'the winner released; the loser removed nothing', + ); +}); + +test('stale-lock takeover racing another taker: the live lock is never unlinked, and never both proceed', async () => { + // Another process C also finds the stale lock and takes it over (compare-then-unlink, then + // create), at a random moment while our run is doing the same. Whatever the interleaving: + // if our run acquired, C's compare sees a foreign identity and skips; if C acquired first (or + // in our run's window between judging stale and unlinking), our run finds a live lock and + // refuses. It must never unlink C's live lock, and both must never proceed. + const stale = `${NEVER_A_PID}:stale`; + const live = `${process.pid}:taker-c`; + const takeOver = (): 'took-over' | 'held-by-run' | 'absent' => { + let current: string | undefined; + try { + current = fs.readlinkSync(lockOf(first)); + } catch { + current = undefined; + } + if (current === undefined) return 'absent'; // our run already finished and released: no overlap + if (current === stale) fs.unlinkSync(lockOf(first)); // compare-then-unlink, as the script does + try { + fs.symlinkSync(live, lockOf(first)); + return 'took-over'; + } catch { + return 'held-by-run'; // our run holds it: C skips + } + }; + let ourWins = 0; + let cWins = 0; + // Delays straddle the run's time-to-acquire (node start + git rev-parse ≈ 300ms here) so both + // orders occur in practice; the assertions hold under either, so the mix is reported, not required. + const delays = [0, 200, 400, 650]; + for (const [round, delay] of delays.entries()) { + fs.rmSync(lockOf(first), { force: true }); + fs.symlinkSync(stale, lockOf(first)); + const [result, c] = await Promise.all([ + size(first), + new Promise>((resolve) => + setTimeout(() => resolve(takeOver()), delay), + ), + ]); + if (result.exitCode === 0) { + ourWins += 1; + assert.notEqual(c, 'took-over', `round ${round}: C must not acquire while our run holds it`); + } else { + cWins += 1; + assert.match(result.stderr, /is using base/); + assert.equal(fs.readlinkSync(lockOf(first)), live, `round ${round}: C's live lock intact`); + } + fs.rmSync(lockOf(first), { force: true }); + } + assert.equal(ourWins + cWins, delays.length); + process.stderr.write(`[size-report-base] takeover race: run won ${ourWins}, C won ${cWins}\n`); +}); diff --git a/scripts/pr-evidence/run.ts b/scripts/pr-evidence/run.ts index f50b48cd9..862843312 100644 --- a/scripts/pr-evidence/run.ts +++ b/scripts/pr-evidence/run.ts @@ -32,6 +32,7 @@ import { type EvidenceInputs, type GitFacts, } from './model.ts'; +import { withWorktrees } from './worktrees.ts'; const USAGE = 'Usage: pnpm pr:evidence [--base ] [--coverage] [--size] [--json]\n' + @@ -111,17 +112,6 @@ async function collectDepgraph(cwd: string, out: string): Promise return depgraphFacts(JSON.parse(fs.readFileSync(out, 'utf8'))); } -/** A pristine checkout of one commit; removed by the caller's scratch cleanup. */ -function addWorktree(scratch: string, name: string, commit: string): string { - const worktree = path.join(scratch, name); - git(['worktree', 'add', '--detach', worktree, commit]); - return worktree; -} - -function removeWorktree(worktree: string): void { - git(['worktree', 'remove', '--force', worktree]); -} - async function collectCoverage(base: string): Promise { if (!fs.existsSync(path.join(repoRoot, 'coverage', 'lcov.info'))) { return { kind: 'skipped', reason: 'no coverage/lcov.info — run pnpm test:coverage first' }; @@ -172,35 +162,38 @@ async function main(argv: readonly string[]): Promise { // os.tmpdir() always exists; a repo-local scratch would have to be created first and is one // more thing a fresh checkout can lack. const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-pr-evidence-')); - const worktrees: string[] = []; - try { - const headTree = addWorktree(scratch, 'head', gitFacts.head); - const baseTree = addWorktree(scratch, 'base', gitFacts.base); - worktrees.push(headTree, baseTree); - const [affected, layering, head, base] = await Promise.all([ - collectAffected(gitFacts.base, gitFacts.head), - collectLayering(headTree), - collectDepgraph(headTree, path.join(scratch, 'depgraph-head.json')), - collectDepgraph(baseTree, path.join(scratch, 'depgraph-base.json')), - ]); - const inputs: EvidenceInputs = { - generatedAt: new Date().toISOString(), - repository: REPOSITORY, - git: gitFacts, - affected, - layering, - depgraph: { head, base }, - coverage: await optional(values.coverage, '--coverage', () => collectCoverage(gitFacts.base)), - size: await optional(values.size, '--size', () => collectSize(gitFacts.base)), - }; - process.stdout.write( - values.json ? `${JSON.stringify(inputs, null, 2)}\n` : renderEvidence(inputs), - ); - return 0; - } finally { - for (const worktree of worktrees) removeWorktree(worktree); - fs.rmSync(scratch, { recursive: true, force: true }); - } + return await withWorktrees( + repoRoot, + scratch, + [ + { name: 'head', commit: gitFacts.head }, + { name: 'base', commit: gitFacts.base }, + ], + async ([headTree, baseTree]) => { + const [affected, layering, head, base] = await Promise.all([ + collectAffected(gitFacts.base, gitFacts.head), + collectLayering(headTree), + collectDepgraph(headTree, path.join(scratch, 'depgraph-head.json')), + collectDepgraph(baseTree, path.join(scratch, 'depgraph-base.json')), + ]); + const inputs: EvidenceInputs = { + generatedAt: new Date().toISOString(), + repository: REPOSITORY, + git: gitFacts, + affected, + layering, + depgraph: { head, base }, + coverage: await optional(values.coverage, '--coverage', () => + collectCoverage(gitFacts.base), + ), + size: await optional(values.size, '--size', () => collectSize(gitFacts.base)), + }; + process.stdout.write( + values.json ? `${JSON.stringify(inputs, null, 2)}\n` : renderEvidence(inputs), + ); + return 0; + }, + ); } if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { diff --git a/scripts/pr-evidence/worktrees.test.ts b/scripts/pr-evidence/worktrees.test.ts new file mode 100644 index 000000000..73e45947c --- /dev/null +++ b/scripts/pr-evidence/worktrees.test.ts @@ -0,0 +1,119 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from 'node:test'; +import { runCmdSync } from '../../src/utils/exec.ts'; +import { withWorktrees } from './worktrees.ts'; + +// A throwaway repository with one commit; every case below plants a failure somewhere in the +// add → run → cleanup sequence and asserts that nothing the helper created outlives it. + +function makeRepo(): { repo: string; commit: string } { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'pr-evidence-worktrees-')); + const repo = path.join(root, 'repo'); + fs.mkdirSync(repo); + const git = (args: string[]) => runCmdSync('git', args, { cwd: repo }).stdout.trim(); + git(['init', '-q', '-b', 'main']); + git(['config', 'user.email', 'wt@test']); + git(['config', 'user.name', 'wt']); + fs.writeFileSync(path.join(repo, 'file'), 'x\n'); + git(['add', '.']); + git(['commit', '-q', '-m', 'one']); + return { repo, commit: git(['rev-parse', 'HEAD']) }; +} + +function registered(repo: string): string[] { + return runCmdSync('git', ['worktree', 'list', '--porcelain'], { cwd: repo }) + .stdout.split('\n') + .filter((line) => line.startsWith('worktree ')) + .map((line) => line.slice('worktree '.length)) + .filter((dir) => dir !== fs.realpathSync(repo)); +} + +test('worktrees are created in order, handed over as a tuple, and removed with the scratch after success', async () => { + const { repo, commit } = makeRepo(); + const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'pr-evidence-scratch-')); + const seen = await withWorktrees( + repo, + scratch, + [ + { name: 'head', commit }, + { name: 'base', commit }, + ], + async ([head, base]) => { + assert.ok(fs.existsSync(path.join(head, 'file'))); + assert.ok(fs.existsSync(path.join(base, 'file'))); + assert.equal(registered(repo).length, 2); + return [path.basename(head), path.basename(base)]; + }, + ); + assert.deepEqual(seen, ['head', 'base']); + assert.deepEqual(registered(repo), []); + assert.equal(fs.existsSync(scratch), false); +}); + +test('a second add that fails leaks nothing: the first worktree is already registered and gets removed', async () => { + const { repo, commit } = makeRepo(); + const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'pr-evidence-scratch-')); + await assert.rejects( + withWorktrees( + repo, + scratch, + [ + { name: 'head', commit }, + { name: 'base', commit: 'not-a-commit' }, + ], + async () => { + throw new Error('fn must not run when an add failed'); + }, + ), + /git exited with code 128/, + ); + assert.deepEqual(registered(repo), [], 'the successful first add was cleaned up'); + assert.equal(fs.existsSync(scratch), false); +}); + +test('a throwing fn still gets every worktree and the scratch removed', async () => { + const { repo, commit } = makeRepo(); + const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'pr-evidence-scratch-')); + await assert.rejects( + withWorktrees(repo, scratch, [{ name: 'only', commit }], async () => { + throw new Error('measurement failed'); + }), + /measurement failed/, + ); + assert.deepEqual(registered(repo), []); + assert.equal(fs.existsSync(scratch), false); +}); + +test('one cleanup failure never skips the remaining resources, and every failure is reported', async () => { + const { repo, commit } = makeRepo(); + const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'pr-evidence-scratch-')); + const removed: string[] = []; + await assert.rejects( + withWorktrees( + repo, + scratch, + [ + { name: 'first', commit }, + { name: 'second', commit }, + { name: 'third', commit }, + ], + async () => 'ok', + (root, worktree) => { + if (worktree.endsWith('second')) throw new Error('planted removal failure'); + runCmdSync('git', ['worktree', 'remove', '--force', worktree], { cwd: root }); + removed.push(path.basename(worktree)); + }, + ), + (error: unknown) => { + assert.ok(error instanceof Error); + assert.match(error.message, /cleanup left resources behind/); + assert.match(error.message, /second: planted removal failure/); + return true; + }, + ); + assert.deepEqual(removed, ['first', 'third'], 'the failure in the middle skipped nothing'); + assert.equal(fs.existsSync(scratch), false, 'the scratch was still attempted'); +}); diff --git a/scripts/pr-evidence/worktrees.ts b/scripts/pr-evidence/worktrees.ts new file mode 100644 index 000000000..c25a0ca37 --- /dev/null +++ b/scripts/pr-evidence/worktrees.ts @@ -0,0 +1,76 @@ +// Throwaway `git worktree` checkouts for pr:evidence, with two guarantees the runner leans on: +// every worktree is registered for cleanup the moment its `add` succeeds (a later add failing +// leaks nothing), and cleanup is exhaustive — one resource's failure to be removed never skips +// the rest, and every failure is reported after the last one was attempted. + +import fs from 'node:fs'; +import path from 'node:path'; +import { runCmdSync } from '../../src/utils/exec.ts'; + +export type WorktreeSpec = Readonly<{ name: string; commit: string }>; + +function git(cwd: string, args: readonly string[]): void { + runCmdSync('git', [...args], { cwd }); +} + +/** + * Creates the requested worktrees under `scratch`, runs `fn` with their paths (in spec order), + * and removes every worktree that was created plus `scratch` itself, whether `fn` or a later + * `add` threw. Cleanup errors are collected and thrown together after every resource was tried. + */ +export async function withWorktrees( + repoRoot: string, + scratch: string, + specs: Specs, + fn: (paths: { readonly [Index in keyof Specs]: string }) => Promise, + removeWorktree: (repoRoot: string, worktree: string) => void = defaultRemoveWorktree, +): Promise { + const created: string[] = []; + let outcome: { ok: true; value: T } | { ok: false; error: unknown }; + try { + for (const spec of specs) { + const worktree = path.join(scratch, spec.name); + git(repoRoot, ['worktree', 'add', '--detach', worktree, spec.commit]); + created.push(worktree); // registered before the next add can fail + } + // One path per spec, in order: the tuple type mirrors `specs` so callers destructure safely. + outcome = { + ok: true, + value: await fn(created as unknown as { readonly [Index in keyof Specs]: string }), + }; + } catch (error) { + outcome = { ok: false, error }; + } + const failures = cleanUp(repoRoot, created, scratch, removeWorktree); + if (failures.length > 0) { + throw new Error(`pr-evidence cleanup left resources behind:\n${failures.join('\n')}`, { + cause: outcome.ok ? undefined : outcome.error, + }); + } + if (!outcome.ok) throw outcome.error; + return outcome.value; +} + +/** Every resource is attempted; the failures come back together instead of aborting the sweep. */ +function cleanUp( + repoRoot: string, + worktrees: readonly string[], + scratch: string, + removeWorktree: (repoRoot: string, worktree: string) => void, +): string[] { + const failures: string[] = []; + const attempt = (label: string, action: () => void) => { + try { + action(); + } catch (error) { + failures.push(`${label}: ${error instanceof Error ? error.message : String(error)}`); + } + }; + for (const worktree of worktrees) attempt(worktree, () => removeWorktree(repoRoot, worktree)); + attempt(scratch, () => fs.rmSync(scratch, { recursive: true, force: true })); + return failures; +} + +function defaultRemoveWorktree(repoRoot: string, worktree: string): void { + git(repoRoot, ['worktree', 'remove', '--force', worktree]); +} diff --git a/scripts/size-report.mjs b/scripts/size-report.mjs index 04f2d4441..ae977f514 100644 --- a/scripts/size-report.mjs +++ b/scripts/size-report.mjs @@ -2,10 +2,13 @@ import fs from 'node:fs'; import path from 'node:path'; import { execFileSync } from 'node:child_process'; +import crypto from 'node:crypto'; import { performance } from 'node:perf_hooks'; import { gzipSync } from 'node:zlib'; const COMMENT_MARKER = ''; +// This run's identity as a base-worktree lock owner: pid for liveness, nonce against pid reuse. +const LOCK_IDENTITY = `${process.pid}:${crypto.randomUUID()}`; const GITHUB_REQUEST_ATTEMPTS = 4; // Overridable so the regression tests do not sleep through real backoff. const GITHUB_RETRY_BASE_MS = Number(process.env.SIZE_REPORT_RETRY_BASE_MS ?? 1000); @@ -206,51 +209,86 @@ function baseLockPath(worktreeDir) { return `${worktreeDir}.lock`; } -// O_EXCL create is the atomic step; the pid inside is what makes a leftover lock recognizable -// as stale (its owner died) rather than a permanent wedge. +// The lock is a symlink whose *target* is the owner identity (`:`): one syscall +// creates it with its identity in place (no empty-file window for another run to misread as +// stale) and fails EEXIST while held. A stale lock (its pid is dead) is taken over by +// compare-then-unlink on that identity — the unlink is skipped if the link no longer names the +// identity that was judged stale — and every acquisition verifies the link names this run +// before returning. Release unlinks only a link that still names this run. function acquireBaseLock(worktreeDir, sha) { const lockPath = baseLockPath(worktreeDir); - for (;;) { - const release = tryCreateLock(lockPath); - if (release) return release; - if (isLockHeldByLiveProcess(lockPath)) { - throw new Error( - `another \`size --base\` (pid ${lockOwnerPid(lockPath)}) is building base ${sha.slice(0, 9)} in ${worktreeDir}; wait for it or measure a different base`, - ); + for (let attempt = 0; attempt < 8; attempt += 1) { + if (tryCreateLock(lockPath) && readLockIdentity(lockPath) === LOCK_IDENTITY) { + return () => releaseLock(lockPath); } - fs.rmSync(lockPath, { force: true }); // stale: its owner is gone + clearStaleLockOrThrow(lockPath, worktreeDir, sha); } + throw new Error(`could not acquire ${lockPath} after repeated stale-lock takeovers`); } -function isLockHeldByLiveProcess(lockPath) { - const owner = lockOwnerPid(lockPath); - return owner !== undefined && isProcessAlive(owner); +/** Held by a live process → throw; stale (dead owner) → compare-then-unlink; garbage → remove. */ +function clearStaleLockOrThrow(lockPath, worktreeDir, sha) { + const holder = readLockIdentity(lockPath); + if (holder === undefined) { + removeIfNotSymlink(lockPath); // a regular file or directory here is not a lock of this scheme + return; // or it vanished between our create and read: the caller retries + } + if (isProcessAlive(pidOfIdentity(holder))) { + throw new Error( + `another \`size --base\` (pid ${pidOfIdentity(holder)}) is using base ${sha.slice(0, 9)} in ${worktreeDir}; wait for it or measure a different base`, + ); + } + unlinkIfIdentity(lockPath, holder); // stale: its owner is gone; the caller retries the create } -/** Returns the release function on success, or undefined when the lock already exists. */ function tryCreateLock(lockPath) { - let fd; try { - fd = fs.openSync(lockPath, 'wx'); + fs.symlinkSync(LOCK_IDENTITY, lockPath); + return true; } catch (error) { - if (error.code === 'EEXIST') return undefined; + if (error.code === 'EEXIST') return false; throw error; } - fs.writeSync(fd, `${process.pid}\n`); - fs.closeSync(fd); - return () => fs.rmSync(lockPath, { force: true }); } -function lockOwnerPid(lockPath) { +function releaseLock(lockPath) { + unlinkIfIdentity(lockPath, LOCK_IDENTITY); +} + +/** Compare-then-unlink: never remove a lock that has since come to name someone else. */ +function unlinkIfIdentity(lockPath, identity) { + if (readLockIdentity(lockPath) !== identity) return; + try { + fs.unlinkSync(lockPath); + } catch (error) { + if (error.code !== 'ENOENT') throw error; + } +} + +function removeIfNotSymlink(lockPath) { try { - const pid = Number(fs.readFileSync(lockPath, 'utf8').trim()); - return Number.isInteger(pid) && pid > 0 ? pid : undefined; + if (!fs.lstatSync(lockPath).isSymbolicLink()) + fs.rmSync(lockPath, { recursive: true, force: true }); + } catch { + // already gone + } +} + +function readLockIdentity(lockPath) { + try { + return fs.readlinkSync(lockPath); } catch { return undefined; } } +function pidOfIdentity(identity) { + const pid = Number(identity.split(':')[0]); + return Number.isInteger(pid) && pid > 0 ? pid : undefined; +} + function isProcessAlive(pid) { + if (pid === undefined) return false; try { process.kill(pid, 0); return true; @@ -275,8 +313,10 @@ function ensureBaseWorktree(root, worktreeDir, sha) { }); } -// Cache eviction of idle entries only: a worktree whose lock is held by a live pid is in use -// by another run and is left alone. +// Cache eviction of idle entries only, and only while holding the victim's own lock: a +// worktree whose lock is held by a live pid is in use by another run and is left alone, and a +// run that wants the victim after this check finds it locked (fails fast) rather than finding +// it half-removed. function pruneOtherBaseWorktrees(root, worktreesRoot, keep) { const others = fs .readdirSync(worktreesRoot, { withFileTypes: true }) @@ -284,9 +324,17 @@ function pruneOtherBaseWorktrees(root, worktreesRoot, keep) { .map((entry) => path.join(worktreesRoot, entry.name)) .filter((dir) => dir !== keep); for (const dir of others) { - if (isLockHeldByLiveProcess(baseLockPath(dir))) continue; - removeWorktree(root, dir); - fs.rmSync(baseLockPath(dir), { force: true }); + let releaseVictim; + try { + releaseVictim = acquireBaseLock(dir, path.basename(dir)); + } catch { + continue; // in use by a live run: not ours to evict + } + try { + removeWorktree(root, dir); + } finally { + releaseVictim(); + } } } From 52f025df1b083759d9692db9cf698195104677d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 08:40:08 +0200 Subject: [PATCH 05/10] test(size): serialize the size --base orchestration file with the other real spawners MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught running the full unit suite on the rebased branch: the file passed in isolation but intermittently failed under broad file parallelism, where it took 14s versus ~5.5s alone. It spawns node scripts/size-report.mjs per case, which spawns git and the shimmed package managers under it — the SUBPROCESS_STUB_TESTS class exactly (starved spawns surface as a vitest test timeout instead of the orchestration assertion the case is about), so it joins that serialized project with its spawn named at the entry, per docs/agents/testing.md. No rerun layer is involved: the flake is removed, not retried. Two full-suite runs green after. --- docs/agents/testing.md | 5 ++++- vitest.config.ts | 5 +++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 4357b07c2..af1fbedd5 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -542,8 +542,11 @@ would silently enroll every future file under a directory — and run in their o exactly that list, and both projects run inside one `vitest run`, so the serialized chain runs alongside the main pool rather than after it (~0 added CI wall clock). -Issue #1823 owns the membership and the project's deletion test: if the three run un-serialized in +Issue #1823 owns the membership and the project's deletion test: if they run un-serialized in the default pool for 20 consecutive CI runs with no timeout-shaped failure, the project goes. +`size-report-base.test.ts` joined in #1842 — it drives `pnpm size --base`'s worktree/lock +orchestration through a real `node scripts/size-report.mjs` per case, which spawns git and the +shimmed package managers under it. Adding a file needs the concrete spawn named at the entry; per-file `process.env` isolation is not a reason, since `pool: forks` + `isolate: true` already give every project that. diff --git a/vitest.config.ts b/vitest.config.ts index 3ee1701c8..2e8c2394b 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -15,6 +15,11 @@ export const SUBPROCESS_STUB_TESTS: readonly string[] = [ 'scripts/fuzz/harness.test.ts', // Replays the fuzz corpus through that same worker watchdog, waiting its per-case budget. 'scripts/fuzz/corpus-replay.test.ts', + // Spawns `node scripts/size-report.mjs` per case, which itself spawns git plus the shimmed + // pnpm/npm — several real subprocesses deep. Un-serialized it took 14s for the file under the + // full suite versus ~5.5s alone, and starved spawns surfaced as a vitest test timeout instead + // of the orchestration assertion the case is about (#1842). + 'scripts/__tests__/size-report-base.test.ts', ]; const SETUP_FILES = ['src/__tests__/hermetic-env-setup.ts', 'src/__tests__/process-memo-setup.ts']; From 81544f3264505c8bde6a1e3fc9f5067931e3bc97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 09:34:25 +0200 Subject: [PATCH 06/10] refactor(size): extract the base-cache claim protocol and make stale takeover atomic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (P1 + architecture): Stale-claim removal was compare-then-unlink (readlink then unlink; lstat then rm for a stray file), so another taker could replace the observed entry with its live claim between the two syscalls and this run would delete the replacement. Removal now happens only while holding the entry's takeover mutex — an atomically created directory — and re-verifies the claim inside it. A replacement can appear only by creating one on a free path (the abandoned claim occupies it until the unlink) or by another takeover (needs the mutex), so removal cannot delete a replacement. A mutex leaked by a process killed inside its sub-millisecond critical section is reclaimed by age, and even a wrong reclamation is contained: both takers re-verify inside, and the winner is still decided by the atomic symlink() that follows. The protocol moves out of size-report.mjs into scripts/size-base-cache.mjs (AGENTS.md: extract past 500 LOC) — 719 → 536, with the entry lifecycle (claim → evict others → ensure worktree → build if unstamped → measure → release) owned by the module behind withPreparedBaseWorktree. Mirrored tests in scripts/__tests__/size-base-cache.test.ts plant every dangerous interleaving directly on the filesystem: replacement-after-observation, a takeover held by another run, age reclamation, release-after-retarget, and a stray non-symlink. They need no subprocess and run in 9ms, so the raced single-process case was dropped from the orchestration file, which keeps only what real processes can show. Planted red: removing the mutex makes the contended case delete the claim it must not touch. --- scripts/__tests__/size-base-cache.test.ts | 112 +++++++++ scripts/__tests__/size-report-base.test.ts | 57 +---- scripts/size-base-cache.mjs | 276 +++++++++++++++++++++ scripts/size-report.mjs | 193 +------------- vitest.config.ts | 3 + 5 files changed, 401 insertions(+), 240 deletions(-) create mode 100644 scripts/__tests__/size-base-cache.test.ts create mode 100644 scripts/size-base-cache.mjs diff --git a/scripts/__tests__/size-base-cache.test.ts b/scripts/__tests__/size-base-cache.test.ts new file mode 100644 index 000000000..a71b71572 --- /dev/null +++ b/scripts/__tests__/size-base-cache.test.ts @@ -0,0 +1,112 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, test } from 'vitest'; +import { + CLAIM_IDENTITY, + acquireBaseClaim, + claimPath, + readClaimIdentity, + removeAbandonedClaim, + takeoverPath, +} from '../size-base-cache.mjs'; + +// The claim protocol on its own: pure filesystem, no git and no subprocess, so the dangerous +// interleavings can be planted directly instead of hoped for under load. The orchestration this +// protects (worktree reuse, eviction, build stamping) is covered by size-report-base.test.ts. + +const NEVER_A_PID = 2_147_483_647; // outside every platform's pid range: dead by construction +const ABANDONED = `${NEVER_A_PID}:abandoned`; +const OTHER_LIVE = `${process.pid}:another-run`; + +let entry: string; + +beforeEach(() => { + entry = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'size-base-cache-')), 'abc123456789'); +}); + +afterEach(() => { + fs.rmSync(path.dirname(entry), { recursive: true, force: true }); +}); + +test('an unclaimed entry is claimed, names this run, and is released', () => { + const release = acquireBaseClaim(entry, 'abc123456'); + assert.equal(readClaimIdentity(claimPath(entry)), CLAIM_IDENTITY); + release(); + assert.equal(readClaimIdentity(claimPath(entry)), undefined); +}); + +test('a claim held by a live run is refused, and nothing about it is touched', () => { + fs.symlinkSync(OTHER_LIVE, claimPath(entry)); + assert.throws(() => acquireBaseClaim(entry, 'abc123456'), /is using base abc123456/); + assert.equal(readClaimIdentity(claimPath(entry)), OTHER_LIVE, 'the live claim survives'); +}); + +test('an abandoned claim is taken over', () => { + fs.symlinkSync(ABANDONED, claimPath(entry)); + const release = acquireBaseClaim(entry, 'abc123456'); + assert.equal(readClaimIdentity(claimPath(entry)), CLAIM_IDENTITY); + release(); +}); + +test('the exact interleaving: an abandoned claim replaced by a live one is never deleted', () => { + // The window the protocol has to survive — observe abandoned, then another run takes over + // before the removal. `removeAbandonedClaim` re-verifies under the takeover mutex, so the + // replacement it finds is reported, not unlinked. + fs.symlinkSync(ABANDONED, claimPath(entry)); + const observed = readClaimIdentity(claimPath(entry)); + // …the replacement lands here, in the window between observing and removing… + fs.unlinkSync(claimPath(entry)); + fs.symlinkSync(OTHER_LIVE, claimPath(entry)); + + assert.equal(removeAbandonedClaim(entry, observed), 'changed'); + assert.equal(readClaimIdentity(claimPath(entry)), OTHER_LIVE, 'the replacement is intact'); + // And the full acquire path refuses rather than stealing it. + assert.throws(() => acquireBaseClaim(entry, 'abc123456'), /is using base abc123456/); + assert.equal(readClaimIdentity(claimPath(entry)), OTHER_LIVE); +}); + +test('removal cannot run at all while another run holds the takeover mutex', () => { + // A second taker is mid-takeover: the mutex is held, so this run must not remove anything, + // and after CLAIM_ATTEMPTS it reports the contention instead of forcing its way in. + fs.symlinkSync(ABANDONED, claimPath(entry)); + fs.mkdirSync(takeoverPath(entry)); + try { + assert.equal(removeAbandonedClaim(entry, ABANDONED), 'busy'); + assert.equal(readClaimIdentity(claimPath(entry)), ABANDONED, 'untouched while contended'); + assert.throws(() => acquireBaseClaim(entry, 'abc123456'), /taking over the abandoned claim/); + assert.equal(readClaimIdentity(claimPath(entry)), ABANDONED); + } finally { + fs.rmSync(takeoverPath(entry), { recursive: true, force: true }); + } + // Once the other taker finishes, the entry is claimable again. + const release = acquireBaseClaim(entry, 'abc123456'); + assert.equal(readClaimIdentity(claimPath(entry)), CLAIM_IDENTITY); + release(); +}); + +test('a takeover mutex abandoned by a killed process is reclaimed by age, not wedged forever', () => { + fs.symlinkSync(ABANDONED, claimPath(entry)); + fs.mkdirSync(takeoverPath(entry)); + const longAgo = new Date(Date.now() - 60_000); + fs.utimesSync(takeoverPath(entry), longAgo, longAgo); + const release = acquireBaseClaim(entry, 'abc123456'); + assert.equal(readClaimIdentity(claimPath(entry)), CLAIM_IDENTITY); + release(); +}); + +test('release leaves a claim that has come to name another run alone', () => { + const release = acquireBaseClaim(entry, 'abc123456'); + fs.unlinkSync(claimPath(entry)); + fs.symlinkSync(OTHER_LIVE, claimPath(entry)); + release(); + assert.equal(readClaimIdentity(claimPath(entry)), OTHER_LIVE, "another run's claim survives"); +}); + +test('a stray non-symlink at the claim path is cleared instead of wedging the entry', () => { + fs.writeFileSync(claimPath(entry), 'not a claim of this scheme\n'); + const release = acquireBaseClaim(entry, 'abc123456'); + assert.equal(readClaimIdentity(claimPath(entry)), CLAIM_IDENTITY); + release(); +}); diff --git a/scripts/__tests__/size-report-base.test.ts b/scripts/__tests__/size-report-base.test.ts index a1eeb24ec..975529a02 100644 --- a/scripts/__tests__/size-report-base.test.ts +++ b/scripts/__tests__/size-report-base.test.ts @@ -8,6 +8,11 @@ import { runCmd, runCmdSync } from '../../src/utils/exec.ts'; // `pnpm size --base ` orchestration against a throwaway git repository, with `pnpm` and // `npm` shimmed on PATH: the shim `pnpm build` writes dist/src and appends to a log, the shim // `npm pack` prints a fixed dry-run JSON. No install, no network; every run is git + node. +// +// This file owns what only real processes can show: that two concurrent runs build once, that a +// cached entry is reused, and that eviction respects a claim. The claim protocol's own +// interleavings (takeover, replacement, contention) are planted directly in +// size-base-cache.test.ts, which needs no subprocess at all. const ROOT = path.join(import.meta.dirname, '..', '..'); const SCRIPT = path.join(ROOT, 'scripts', 'size-report.mjs'); @@ -191,55 +196,3 @@ if [ "$1" = "build" ]; then sleep 1; mkdir -p dist/src && printf 'export const b 'the winner released; the loser removed nothing', ); }); - -test('stale-lock takeover racing another taker: the live lock is never unlinked, and never both proceed', async () => { - // Another process C also finds the stale lock and takes it over (compare-then-unlink, then - // create), at a random moment while our run is doing the same. Whatever the interleaving: - // if our run acquired, C's compare sees a foreign identity and skips; if C acquired first (or - // in our run's window between judging stale and unlinking), our run finds a live lock and - // refuses. It must never unlink C's live lock, and both must never proceed. - const stale = `${NEVER_A_PID}:stale`; - const live = `${process.pid}:taker-c`; - const takeOver = (): 'took-over' | 'held-by-run' | 'absent' => { - let current: string | undefined; - try { - current = fs.readlinkSync(lockOf(first)); - } catch { - current = undefined; - } - if (current === undefined) return 'absent'; // our run already finished and released: no overlap - if (current === stale) fs.unlinkSync(lockOf(first)); // compare-then-unlink, as the script does - try { - fs.symlinkSync(live, lockOf(first)); - return 'took-over'; - } catch { - return 'held-by-run'; // our run holds it: C skips - } - }; - let ourWins = 0; - let cWins = 0; - // Delays straddle the run's time-to-acquire (node start + git rev-parse ≈ 300ms here) so both - // orders occur in practice; the assertions hold under either, so the mix is reported, not required. - const delays = [0, 200, 400, 650]; - for (const [round, delay] of delays.entries()) { - fs.rmSync(lockOf(first), { force: true }); - fs.symlinkSync(stale, lockOf(first)); - const [result, c] = await Promise.all([ - size(first), - new Promise>((resolve) => - setTimeout(() => resolve(takeOver()), delay), - ), - ]); - if (result.exitCode === 0) { - ourWins += 1; - assert.notEqual(c, 'took-over', `round ${round}: C must not acquire while our run holds it`); - } else { - cWins += 1; - assert.match(result.stderr, /is using base/); - assert.equal(fs.readlinkSync(lockOf(first)), live, `round ${round}: C's live lock intact`); - } - fs.rmSync(lockOf(first), { force: true }); - } - assert.equal(ourWins + cWins, delays.length); - process.stderr.write(`[size-report-base] takeover race: run won ${ourWins}, C won ${cWins}\n`); -}); diff --git a/scripts/size-base-cache.mjs b/scripts/size-base-cache.mjs new file mode 100644 index 000000000..e531b1edd --- /dev/null +++ b/scripts/size-base-cache.mjs @@ -0,0 +1,276 @@ +// Ownership protocol for the `pnpm size --base ` worktree cache. +// +// The cache is `.tmp/size-base//` — a detached worktree of the base commit, built once +// and reused — plus, per entry, a *claim* that says which run is currently using it. Two runs on +// one machine must never build the same entry at once, read a half-built `dist`, or delete an +// entry another run is using. +// +// A claim is a symlink whose target is the owning run's identity (`:`): +// - `symlink()` creates it with its identity already in place — one syscall, so there is no +// window where a claim exists without an owner — and fails EEXIST while another run holds it. +// - A claim whose owning pid is gone is *abandoned*. Removing one is the only dangerous step in +// the protocol: between observing an abandoned claim and unlinking it, another run could have +// removed it and taken the entry, and the unlink would then delete that live claim. So +// removal happens only while holding the entry's takeover mutex — an atomically created +// directory — and re-verifies the claim inside it. A replacement claim can appear only by +// creating one on a free path (impossible: the abandoned claim occupies it until we unlink) +// or by another takeover (impossible: that needs this mutex). Removal therefore cannot +// delete a replacement. +// - Release unlinks only a claim that still names this run, under the same mutex. +// +// The mutex protects a few syscalls with no I/O in between, so a leaked one means a process died +// inside a sub-millisecond window; one older than TAKEOVER_STALE_MS is reclaimed. Even a wrong +// reclamation is contained: two takers would both re-verify inside, at most one unlink succeeds, +// and the winner is still decided by the atomic `symlink()` that follows. + +import { randomUUID } from 'node:crypto'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import path from 'node:path'; + +/** This run's claim identity: pid for liveness, nonce so a reused pid is still a different run. */ +export const CLAIM_IDENTITY = `${process.pid}:${randomUUID()}`; + +const TAKEOVER_STALE_MS = 30_000; +const CLAIM_ATTEMPTS = 8; + +export function claimPath(worktreeDir) { + return `${worktreeDir}.lock`; +} + +export function takeoverPath(worktreeDir) { + return `${worktreeDir}.takeover`; +} + +/** The stamp a finished build writes; its absence means "rebuild", never "trust dist/src". */ +function completionStampPath(worktreeDir) { + return path.join(worktreeDir, 'dist', '.size-base-complete'); +} + +export function readClaimIdentity(claim) { + try { + return fs.readlinkSync(claim); + } catch { + return undefined; + } +} + +function pidOfIdentity(identity) { + const pid = Number(String(identity).split(':')[0]); + return Number.isInteger(pid) && pid > 0 ? pid : undefined; +} + +function isProcessAlive(pid) { + if (pid === undefined) return false; + try { + process.kill(pid, 0); + return true; + } catch (error) { + return error.code === 'EPERM'; // exists, not signalable by us + } +} + +function tryCreateClaim(claim) { + try { + fs.symlinkSync(CLAIM_IDENTITY, claim); + return true; + } catch (error) { + if (error.code === 'EEXIST') return false; + throw error; + } +} + +function enterTakeover(takeover) { + try { + fs.mkdirSync(takeover); + return true; + } catch (error) { + if (error.code !== 'EEXIST') throw error; + } + let age; + try { + age = Date.now() - fs.statSync(takeover).mtimeMs; + } catch { + return false; // vanished; the caller retries + } + if (age < TAKEOVER_STALE_MS) return false; + try { + fs.rmSync(takeover, { recursive: true, force: true }); + fs.mkdirSync(takeover); + return true; + } catch { + return false; + } +} + +function leaveTakeover(takeover) { + fs.rmSync(takeover, { recursive: true, force: true }); +} + +/** + * Removes an abandoned claim, under the takeover mutex so it can never remove a replacement. + * Returns what it found: 'removed' | 'gone' | 'changed' | 'live' | 'busy'. + */ +export function removeAbandonedClaim(worktreeDir, observedIdentity) { + const claim = claimPath(worktreeDir); + const takeover = takeoverPath(worktreeDir); + if (!enterTakeover(takeover)) return 'busy'; + try { + let stats; + try { + stats = fs.lstatSync(claim); + } catch { + return 'gone'; + } + if (!stats.isSymbolicLink()) { + // Not a claim of this scheme at all (a stray file or directory): safe to clear here. + fs.rmSync(claim, { recursive: true, force: true }); + return 'removed'; + } + const current = fs.readlinkSync(claim); + if (current !== observedIdentity) return 'changed'; + if (isProcessAlive(pidOfIdentity(current))) return 'live'; + fs.unlinkSync(claim); + return 'removed'; + } finally { + leaveTakeover(takeover); + } +} + +function heldError(worktreeDir, holder, label) { + const pid = pidOfIdentity(holder); + return new Error( + `another \`size --base\` (pid ${pid ?? 'unknown'}) is using base ${label} in ${worktreeDir}; ` + + `wait for it, measure a different base, or remove ${claimPath(worktreeDir)} if that run is gone`, + ); +} + +/** + * Claims one cache entry for this run. Returns the release function; throws when another live + * run holds it, or when a takeover by another run keeps the claim contended. + */ +export function acquireBaseClaim(worktreeDir, label) { + for (let attempt = 0; attempt < CLAIM_ATTEMPTS; attempt += 1) { + const claim = claimPath(worktreeDir); + if (tryCreateClaim(claim) && readClaimIdentity(claim) === CLAIM_IDENTITY) { + return () => releaseBaseClaim(worktreeDir); + } + const holder = readClaimIdentity(claim); + if (holder !== undefined && isProcessAlive(pidOfIdentity(holder))) { + throw heldError(worktreeDir, holder, label); + } + const outcome = removeAbandonedClaim(worktreeDir, holder); + if (outcome === 'live') throw heldError(worktreeDir, readClaimIdentity(claim), label); + if (outcome === 'busy' && attempt === CLAIM_ATTEMPTS - 1) { + throw new Error( + `another run is taking over the abandoned claim on base ${label} in ${worktreeDir}; retry shortly`, + ); + } + } + throw new Error( + `could not claim base ${label} in ${worktreeDir} after ${CLAIM_ATTEMPTS} attempts`, + ); +} + +/** Releases this run's claim; a claim that has come to name someone else is left alone. */ +function releaseBaseClaim(worktreeDir) { + const claim = claimPath(worktreeDir); + const takeover = takeoverPath(worktreeDir); + if (!enterTakeover(takeover)) { + // Someone is mid-takeover of this entry; they re-verify identity, so they cannot remove ours + // while we still own it, and our claim is removed by the next run that finds it abandoned. + return; + } + try { + if (readClaimIdentity(claim) === CLAIM_IDENTITY) fs.unlinkSync(claim); + } finally { + leaveTakeover(takeover); + } +} + +function removeWorktree(root, dir) { + try { + execFileSync('git', ['worktree', 'remove', '--force', dir], { cwd: root, stdio: 'ignore' }); + } catch { + // Not a registered worktree (half-created, or hand-copied): plain removal. + } + fs.rmSync(dir, { recursive: true, force: true }); +} + +/** Creates the entry's worktree when it is missing or unregistered; otherwise leaves it. */ +function ensureBaseWorktree(root, worktreeDir, sha) { + const registered = execFileSync('git', ['worktree', 'list', '--porcelain'], { + cwd: root, + encoding: 'utf8', + }).includes(`worktree ${worktreeDir}\n`); + if (registered && fs.existsSync(worktreeDir)) return; + // Registered but gone (hand-deleted), or present but unregistered (hand-copied): start clean. + fs.rmSync(worktreeDir, { recursive: true, force: true }); + execFileSync('git', ['worktree', 'prune'], { cwd: root, stdio: 'ignore' }); + execFileSync('git', ['worktree', 'add', '--detach', worktreeDir, sha], { + cwd: root, + stdio: ['ignore', 'ignore', 'pipe'], + encoding: 'utf8', + }); +} + +/** + * Evicts every cache entry except `keep`, each under its own claim: an entry a live run holds is + * skipped, and one this run evicts cannot be adopted mid-removal. + */ +function pruneOtherBaseWorktrees(root, worktreesRoot, keep) { + const others = fs + .readdirSync(worktreesRoot, { withFileTypes: true }) + .filter((entry) => entry.isDirectory() && !entry.name.endsWith('.takeover')) + .map((entry) => path.join(worktreesRoot, entry.name)) + .filter((dir) => dir !== keep); + for (const dir of others) { + let release; + try { + release = acquireBaseClaim(dir, path.basename(dir)); + } catch { + continue; // in use by a live run, or contended: not ours to evict + } + try { + removeWorktree(root, dir); + } finally { + release(); + } + } +} + +/** + * Prepares (or reuses) the cache entry for `ref` under this run's claim and hands its worktree to + * `measure`. The install+build runs only when the entry carries no completion stamp, so an + * interrupted build is redone rather than trusted because `dist/src` happens to exist. + */ +export function withPreparedBaseWorktree(root, ref, measure) { + const sha = execFileSync('git', ['rev-parse', '--verify', `${ref}^{commit}`], { + cwd: root, + encoding: 'utf8', + }).trim(); + fs.mkdirSync(path.join(root, '.tmp', 'size-base'), { recursive: true }); + // Canonical: git lists worktrees by real path (/tmp is /private/tmp on macOS), and + // ensureBaseWorktree compares against that listing. + const worktreesRoot = fs.realpathSync(path.join(root, '.tmp', 'size-base')); + const worktreeDir = path.join(worktreesRoot, sha.slice(0, 12)); + const release = acquireBaseClaim(worktreeDir, sha.slice(0, 9)); + try { + pruneOtherBaseWorktrees(root, worktreesRoot, worktreeDir); + ensureBaseWorktree(root, worktreeDir, sha); + if (!fs.existsSync(completionStampPath(worktreeDir))) { + process.stderr.write( + `[size] measuring base ${sha.slice(0, 9)} (${ref}): install + build in ${worktreeDir}\n`, + ); + execFileSync('pnpm', ['install', '--frozen-lockfile', '--prefer-offline'], { + cwd: worktreeDir, + stdio: ['ignore', 'ignore', 'inherit'], + }); + execFileSync('pnpm', ['build'], { cwd: worktreeDir, stdio: ['ignore', 'ignore', 'inherit'] }); + fs.writeFileSync(completionStampPath(worktreeDir), `${sha}\n`); + } + return measure(worktreeDir); + } finally { + release(); + } +} diff --git a/scripts/size-report.mjs b/scripts/size-report.mjs index ae977f514..33c0b116b 100644 --- a/scripts/size-report.mjs +++ b/scripts/size-report.mjs @@ -2,13 +2,11 @@ import fs from 'node:fs'; import path from 'node:path'; import { execFileSync } from 'node:child_process'; -import crypto from 'node:crypto'; import { performance } from 'node:perf_hooks'; import { gzipSync } from 'node:zlib'; +import { withPreparedBaseWorktree } from './size-base-cache.mjs'; const COMMENT_MARKER = ''; -// This run's identity as a base-worktree lock owner: pid for liveness, nonce against pid reuse. -const LOCK_IDENTITY = `${process.pid}:${crypto.randomUUID()}`; const GITHUB_REQUEST_ATTEMPTS = 4; // Overridable so the regression tests do not sleep through real backoff. const GITHUB_RETRY_BASE_MS = Number(process.env.SIZE_REPORT_RETRY_BASE_MS ?? 1000); @@ -159,192 +157,11 @@ function collectReport(root, options) { }; } -// The Size workflow measures the base by checking it out, installing, and building; this is -// the same recipe in a detached worktree so the working tree is never touched. Cache semantics -// are per SHA and non-destructive toward anything in use: -// - .tmp/size-base// is the worktree; a second run against the same base finds the -// completeness stamp and skips install+build (mirroring the workflow's dist cache); -// - .tmp/size-base/.lock (pid inside, created O_EXCL) is held from before the worktree -// is created until the base report has been read, so a concurrent run against the same base -// fails fast instead of reading a half-built dist, and a run against another base never -// removes a worktree whose lock is held by a live pid; a lock whose pid is dead is stale; -// - dist/.size-base-complete is written after a successful build, so an interrupted build is -// rebuilt rather than trusted because dist/src happens to exist. +// The Size workflow measures the base by checking it out, installing, and building; this is the +// same recipe in a detached worktree, so the working tree is never touched. Entry reuse, claim +// ownership, and eviction belong to scripts/size-base-cache.mjs. function measureBaseRef(root, ref, options) { - const sha = execFileSync('git', ['rev-parse', '--verify', `${ref}^{commit}`], { - cwd: root, - encoding: 'utf8', - }).trim(); - fs.mkdirSync(path.join(root, '.tmp', 'size-base'), { recursive: true }); - // Canonical: git lists worktrees by real path (/tmp is /private/tmp on macOS), and the - // registration check below compares against that listing. - const worktreesRoot = fs.realpathSync(path.join(root, '.tmp', 'size-base')); - const worktreeDir = path.join(worktreesRoot, sha.slice(0, 12)); - const release = acquireBaseLock(worktreeDir, sha); - try { - pruneOtherBaseWorktrees(root, worktreesRoot, worktreeDir); - ensureBaseWorktree(root, worktreeDir, sha); - if (!fs.existsSync(baseCompletionStamp(worktreeDir))) { - process.stderr.write( - `[size] measuring base ${sha.slice(0, 9)} (${ref}): install + build in ${worktreeDir}\n`, - ); - execFileSync('pnpm', ['install', '--frozen-lockfile', '--prefer-offline'], { - cwd: worktreeDir, - stdio: ['ignore', 'ignore', 'inherit'], - }); - execFileSync('pnpm', ['build'], { cwd: worktreeDir, stdio: ['ignore', 'ignore', 'inherit'] }); - fs.writeFileSync(baseCompletionStamp(worktreeDir), `${sha}\n`); - } - return collectReport(worktreeDir, options); - } finally { - release(); - } -} - -function baseCompletionStamp(worktreeDir) { - return path.join(worktreeDir, 'dist', '.size-base-complete'); -} - -function baseLockPath(worktreeDir) { - return `${worktreeDir}.lock`; -} - -// The lock is a symlink whose *target* is the owner identity (`:`): one syscall -// creates it with its identity in place (no empty-file window for another run to misread as -// stale) and fails EEXIST while held. A stale lock (its pid is dead) is taken over by -// compare-then-unlink on that identity — the unlink is skipped if the link no longer names the -// identity that was judged stale — and every acquisition verifies the link names this run -// before returning. Release unlinks only a link that still names this run. -function acquireBaseLock(worktreeDir, sha) { - const lockPath = baseLockPath(worktreeDir); - for (let attempt = 0; attempt < 8; attempt += 1) { - if (tryCreateLock(lockPath) && readLockIdentity(lockPath) === LOCK_IDENTITY) { - return () => releaseLock(lockPath); - } - clearStaleLockOrThrow(lockPath, worktreeDir, sha); - } - throw new Error(`could not acquire ${lockPath} after repeated stale-lock takeovers`); -} - -/** Held by a live process → throw; stale (dead owner) → compare-then-unlink; garbage → remove. */ -function clearStaleLockOrThrow(lockPath, worktreeDir, sha) { - const holder = readLockIdentity(lockPath); - if (holder === undefined) { - removeIfNotSymlink(lockPath); // a regular file or directory here is not a lock of this scheme - return; // or it vanished between our create and read: the caller retries - } - if (isProcessAlive(pidOfIdentity(holder))) { - throw new Error( - `another \`size --base\` (pid ${pidOfIdentity(holder)}) is using base ${sha.slice(0, 9)} in ${worktreeDir}; wait for it or measure a different base`, - ); - } - unlinkIfIdentity(lockPath, holder); // stale: its owner is gone; the caller retries the create -} - -function tryCreateLock(lockPath) { - try { - fs.symlinkSync(LOCK_IDENTITY, lockPath); - return true; - } catch (error) { - if (error.code === 'EEXIST') return false; - throw error; - } -} - -function releaseLock(lockPath) { - unlinkIfIdentity(lockPath, LOCK_IDENTITY); -} - -/** Compare-then-unlink: never remove a lock that has since come to name someone else. */ -function unlinkIfIdentity(lockPath, identity) { - if (readLockIdentity(lockPath) !== identity) return; - try { - fs.unlinkSync(lockPath); - } catch (error) { - if (error.code !== 'ENOENT') throw error; - } -} - -function removeIfNotSymlink(lockPath) { - try { - if (!fs.lstatSync(lockPath).isSymbolicLink()) - fs.rmSync(lockPath, { recursive: true, force: true }); - } catch { - // already gone - } -} - -function readLockIdentity(lockPath) { - try { - return fs.readlinkSync(lockPath); - } catch { - return undefined; - } -} - -function pidOfIdentity(identity) { - const pid = Number(identity.split(':')[0]); - return Number.isInteger(pid) && pid > 0 ? pid : undefined; -} - -function isProcessAlive(pid) { - if (pid === undefined) return false; - try { - process.kill(pid, 0); - return true; - } catch (error) { - return error.code === 'EPERM'; - } -} - -function ensureBaseWorktree(root, worktreeDir, sha) { - const registered = execFileSync('git', ['worktree', 'list', '--porcelain'], { - cwd: root, - encoding: 'utf8', - }).includes(`worktree ${worktreeDir}\n`); - if (registered && fs.existsSync(worktreeDir)) return; - // Registered but gone (hand-deleted), or present but unregistered (hand-copied): start clean. - fs.rmSync(worktreeDir, { recursive: true, force: true }); - execFileSync('git', ['worktree', 'prune'], { cwd: root, stdio: 'ignore' }); - execFileSync('git', ['worktree', 'add', '--detach', worktreeDir, sha], { - cwd: root, - stdio: ['ignore', 'ignore', 'pipe'], - encoding: 'utf8', - }); -} - -// Cache eviction of idle entries only, and only while holding the victim's own lock: a -// worktree whose lock is held by a live pid is in use by another run and is left alone, and a -// run that wants the victim after this check finds it locked (fails fast) rather than finding -// it half-removed. -function pruneOtherBaseWorktrees(root, worktreesRoot, keep) { - const others = fs - .readdirSync(worktreesRoot, { withFileTypes: true }) - .filter((entry) => entry.isDirectory()) - .map((entry) => path.join(worktreesRoot, entry.name)) - .filter((dir) => dir !== keep); - for (const dir of others) { - let releaseVictim; - try { - releaseVictim = acquireBaseLock(dir, path.basename(dir)); - } catch { - continue; // in use by a live run: not ours to evict - } - try { - removeWorktree(root, dir); - } finally { - releaseVictim(); - } - } -} - -function removeWorktree(root, dir) { - try { - execFileSync('git', ['worktree', 'remove', '--force', dir], { cwd: root, stdio: 'ignore' }); - } catch { - // Not a registered worktree (a half-created or hand-copied directory): plain removal. - } - fs.rmSync(dir, { recursive: true, force: true }); + return withPreparedBaseWorktree(root, ref, (worktreeDir) => collectReport(worktreeDir, options)); } function prepareGeneratedPackageAssets(root) { diff --git a/vitest.config.ts b/vitest.config.ts index 2e8c2394b..78a10bac9 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -83,6 +83,9 @@ export default defineConfig({ // The Bundle Size lane's PR-comment path: spawns the real script against a // stubbed fetch, so it needs no network; pins retry/reconcile/fatal outcomes. 'scripts/__tests__/size-report-post-comment.test.ts', + // The `size --base` cache's claim protocol: pure filesystem, so the dangerous + // takeover interleavings are planted directly rather than raced for. Milliseconds. + 'scripts/__tests__/size-base-cache.test.ts', // `--base` orchestration (per-SHA worktree, lock, completion stamp, eviction) against // a throwaway git repo with pnpm/npm shimmed on PATH: git + node only. 'scripts/__tests__/size-report-base.test.ts', From dcf88ae8d67f80e6f70a9198d2beade8df92c999 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 09:40:45 +0200 Subject: [PATCH 07/10] ci(size): preserve the reporter's whole module graph, and gate that it stays whole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Size workflow measures the base commit with the PR's reporter, so it copies the reporter out of the tree before checking the base out. Extracting size-base-cache.mjs made the reporter a two-file graph while the step still copied one file, and the base measurement died with ERR_MODULE_NOT_FOUND — after every deterministic gate had passed, because nothing local reproduces that copy. The step now copies the scripts directory, so a further split cannot leave an import behind, and size-report-preserved-closure.test.ts holds it to the reporter's real relative-import closure and to running the preserved copy rather than the checked-out tree. Planted red: restoring the single-file copy fails both cases, naming scripts/size-base-cache.mjs. Verified by running the reporter from a copied directory exactly as the workflow does. --- .github/workflows/size.yml | 10 ++- .../size-report-preserved-closure.test.ts | 68 +++++++++++++++++++ vitest.config.ts | 4 ++ 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 scripts/__tests__/size-report-preserved-closure.test.ts diff --git a/.github/workflows/size.yml b/.github/workflows/size.yml index 63ca37e18..869328a52 100644 --- a/.github/workflows/size.yml +++ b/.github/workflows/size.yml @@ -40,8 +40,14 @@ jobs: - name: Setup toolchain uses: ./.github/actions/setup-node-pnpm + # The base is measured with the PR's instrument, so the reporter is copied out before the + # checkout moves. It is a module graph, not one file: copy the whole directory so a future + # split cannot silently leave an import behind (a one-file copy broke exactly that way). + # scripts/__tests__/size-report-preserved-closure.test.ts holds this step to the closure. - name: Preserve report script - run: cp scripts/size-report.mjs /tmp/agent-device-size-report.mjs + run: | + rm -rf /tmp/agent-device-size-report + cp -R scripts /tmp/agent-device-size-report # dist is fully determined by the base commit, so reuse it across PR runs # against the same base. Startup medians are still measured fresh on this @@ -60,7 +66,7 @@ jobs: if [ "${{ steps.base-dist-cache.outputs.cache-hit }}" != "true" ]; then pnpm build fi - node /tmp/agent-device-size-report.mjs \ + node /tmp/agent-device-size-report/size-report.mjs \ --startup-runs 7 \ --json /tmp/agent-device-size-base.json diff --git a/scripts/__tests__/size-report-preserved-closure.test.ts b/scripts/__tests__/size-report-preserved-closure.test.ts new file mode 100644 index 000000000..a800d46c8 --- /dev/null +++ b/scripts/__tests__/size-report-preserved-closure.test.ts @@ -0,0 +1,68 @@ +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { expect, test } from 'vitest'; + +// The Size workflow measures the base commit with the PR's reporter, so it copies the reporter +// out of the tree before `git checkout` moves under it. That copy has to carry the reporter's +// whole relative-import closure: when the closure grew to a second file and the step still copied +// one, the base measurement died with ERR_MODULE_NOT_FOUND — after every deterministic gate had +// passed, because nothing local reproduces the copy. This test is that reproduction. + +const ROOT = path.resolve(import.meta.dirname, '..', '..'); +const ENTRY = 'scripts/size-report.mjs'; +const WORKFLOW = '.github/workflows/size.yml'; + +/** Repo-relative paths the entry reaches through relative (`./`, `../`) static imports. */ +function relativeImportClosure(entry: string): string[] { + const seen = new Set(); + const queue = [entry]; + while (queue.length > 0) { + const file = queue.shift() as string; + if (seen.has(file)) continue; + seen.add(file); + const source = readFileSync(path.join(ROOT, file), 'utf8'); + for (const match of source.matchAll( + /(?:^|\n)\s*(?:import|export)[^'"\n]*['"](\.[^'"]+)['"]/g, + )) { + const resolved = path.posix.join(path.posix.dirname(file), match[1] as string); + if (!seen.has(resolved)) queue.push(resolved); + } + } + return [...seen].sort(); +} + +test('the reporter is more than one file, so the workflow may not preserve it as one file', () => { + const closure = relativeImportClosure(ENTRY); + const workflow = readFileSync(path.join(ROOT, WORKFLOW), 'utf8'); + const preserve = workflow.slice( + workflow.indexOf('- name: Preserve report script'), + workflow.indexOf('- name: Restore base dist cache'), + ); + expect(preserve).not.toEqual(''); + + // Copying the whole directory covers any closure inside it, including files a later split adds. + const copiesDirectory = /cp\s+-R\s+scripts\s/.test(preserve); + const outside = closure.filter((file) => !file.startsWith('scripts/')); + if (copiesDirectory) { + expect(outside, 'a closure member outside scripts/ is not covered by copying scripts/').toEqual( + [], + ); + return; + } + // Otherwise every member must be named explicitly — the shape that already broke once. + for (const file of closure) { + expect(preserve, `the preserve step must copy ${file}`).toContain(file); + } +}); + +test('the workflow runs the preserved copy, not the checked-out tree', () => { + const workflow = readFileSync(path.join(ROOT, WORKFLOW), 'utf8'); + const base = workflow.slice( + workflow.indexOf('- name: Measure base size'), + workflow.indexOf('- name: Save base dist cache'), + ); + // Measuring the base with `pnpm size` would run the base commit's own reporter, so base and PR + // would be measured by different instruments — the reason the copy exists at all. + expect(base).toMatch(/node \/tmp\/agent-device-size-report\/size-report\.mjs/); + expect(base).not.toMatch(/pnpm size/); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 78a10bac9..627839a42 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -86,6 +86,10 @@ export default defineConfig({ // The `size --base` cache's claim protocol: pure filesystem, so the dangerous // takeover interleavings are planted directly rather than raced for. Milliseconds. 'scripts/__tests__/size-base-cache.test.ts', + // The Size workflow copies the reporter out of the tree to measure the base with the + // PR's instrument; nothing local reproduces that copy, so this holds the step to the + // reporter's real import closure. + 'scripts/__tests__/size-report-preserved-closure.test.ts', // `--base` orchestration (per-SHA worktree, lock, completion stamp, eviction) against // a throwaway git repo with pnpm/npm shimmed on PATH: git + node only. 'scripts/__tests__/size-report-base.test.ts', From f6dd6f505a6fdf576d2de435cbbb6d3ee8a5b4eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 10:43:21 +0200 Subject: [PATCH 08/10] fix(size): the takeover mutex has one holder for life; split report publishing out of the reporter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (P1 + architecture): Age-based reclamation of the takeover mutex reintroduced the split ownership the mutex exists to prevent: a holder that is merely slow — paused or SIGSTOPed past any threshold — could have its mutex force-removed and replaced, putting two takers inside the supposedly exclusive section, where either could unlink the claim the other had just created; the unconditional pathname-based release could also delete the replacement mutex. The mutex is now a symlink naming its holder, created in one syscall, never reclaimed at any age, and released only by the run that owns it. A mutex leaked by a process killed inside a three-syscall critical section wedges one cache entry with the path to clear in the message, rather than silently deleting another run's live claim. Planted red: restoring age reclamation displaces a day-old delayed holder, which the new case pins. Publishing the report to a PR is a separate question from measuring and formatting it, so it moves to scripts/size-report-comment.mjs with the marker and retry policy it owns; its existing regression drives it through the real script unchanged. scripts/size-report.mjs is 386 LOC — under the 500 tripwire and below the 512 it had on base. --- scripts/__tests__/size-base-cache.test.ts | 38 ++++- scripts/size-base-cache.mjs | 47 ++++--- scripts/size-report-comment.mjs | 164 ++++++++++++++++++++++ scripts/size-report.mjs | 152 +------------------- 4 files changed, 221 insertions(+), 180 deletions(-) create mode 100644 scripts/size-report-comment.mjs diff --git a/scripts/__tests__/size-base-cache.test.ts b/scripts/__tests__/size-base-cache.test.ts index a71b71572..62e6fe23b 100644 --- a/scripts/__tests__/size-base-cache.test.ts +++ b/scripts/__tests__/size-base-cache.test.ts @@ -86,13 +86,39 @@ test('removal cannot run at all while another run holds the takeover mutex', () release(); }); -test('a takeover mutex abandoned by a killed process is reclaimed by age, not wedged forever', () => { +test('a delayed takeover holder is never displaced, however old its mutex looks', () => { + // The interleaving that age-based reclamation created: holder A is merely slow — paused, or + // SIGSTOPed past any threshold — while still inside the section. Reclaiming its mutex would put + // B inside too, and then A's release could remove B's mutex and either could unlink the claim + // the other just created. A mutex is therefore never taken from its holder, at any age. fs.symlinkSync(ABANDONED, claimPath(entry)); - fs.mkdirSync(takeoverPath(entry)); - const longAgo = new Date(Date.now() - 60_000); - fs.utimesSync(takeoverPath(entry), longAgo, longAgo); - const release = acquireBaseClaim(entry, 'abc123456'); - assert.equal(readClaimIdentity(claimPath(entry)), CLAIM_IDENTITY); + fs.symlinkSync(`${process.pid}:delayed-holder`, takeoverPath(entry)); + const longAgo = new Date(Date.now() - 24 * 60 * 60 * 1000); + fs.lutimesSync(takeoverPath(entry), longAgo, longAgo); + + assert.equal(removeAbandonedClaim(entry, ABANDONED), 'busy'); + assert.equal( + fs.readlinkSync(takeoverPath(entry)), + `${process.pid}:delayed-holder`, + "the holder's mutex is intact", + ); + assert.equal(readClaimIdentity(claimPath(entry)), ABANDONED, 'and it removed nothing'); + assert.throws(() => acquireBaseClaim(entry, 'abc123456'), /taking over the abandoned claim/); + assert.equal(fs.readlinkSync(takeoverPath(entry)), `${process.pid}:delayed-holder`); +}); + +test('a leaked mutex wedges only its own entry, and says how to clear it', () => { + // The price of never reclaiming: one entry needs a human. The message has to name the path. + fs.symlinkSync(ABANDONED, claimPath(entry)); + fs.symlinkSync(`${NEVER_A_PID}:leaked`, takeoverPath(entry)); + assert.throws( + () => acquireBaseClaim(entry, 'abc123456'), + new RegExp(`remove ${takeoverPath(entry).replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`), + ); + // A different entry is unaffected: the mutex is per entry, so nothing else is wedged. + const other = path.join(path.dirname(entry), 'def987654321'); + const release = acquireBaseClaim(other, 'def987654'); + assert.equal(readClaimIdentity(claimPath(other)), CLAIM_IDENTITY); release(); }); diff --git a/scripts/size-base-cache.mjs b/scripts/size-base-cache.mjs index e531b1edd..1d92c1d6d 100644 --- a/scripts/size-base-cache.mjs +++ b/scripts/size-base-cache.mjs @@ -18,10 +18,9 @@ // delete a replacement. // - Release unlinks only a claim that still names this run, under the same mutex. // -// The mutex protects a few syscalls with no I/O in between, so a leaked one means a process died -// inside a sub-millisecond window; one older than TAKEOVER_STALE_MS is reclaimed. Even a wrong -// reclamation is contained: two takers would both re-verify inside, at most one unlink succeeds, -// and the winner is still decided by the atomic `symlink()` that follows. +// The mutex has exactly one holder for its whole life: it is never reclaimed by age or by any +// other guess about the holder, because a second holder would restore the split ownership the +// mutex removes. See enterTakeover for what that costs and why it is the safe direction. import { randomUUID } from 'node:crypto'; import { execFileSync } from 'node:child_process'; @@ -31,7 +30,6 @@ import path from 'node:path'; /** This run's claim identity: pid for liveness, nonce so a reused pid is still a different run. */ export const CLAIM_IDENTITY = `${process.pid}:${randomUUID()}`; -const TAKEOVER_STALE_MS = 30_000; const CLAIM_ATTEMPTS = 8; export function claimPath(worktreeDir) { @@ -80,31 +78,33 @@ function tryCreateClaim(claim) { } } +// The takeover mutex is a symlink naming its holder, created in one syscall like a claim, and it +// is *never* force-reclaimed: an age-based reclamation would hand a second holder the section +// whenever the first is merely slow (a paused or SIGSTOPed process crosses any threshold), and +// two holders is exactly the split ownership the mutex exists to prevent. So there is at most one +// holder, ever, and release removes only a mutex that still names this run — it can neither +// force-remove nor release a replacement. +// +// The cost of never reclaiming is a mutex leaked by a process killed inside a critical section of +// three syscalls with no I/O between them. That wedges one cache entry, loudly and with the path +// to remove in the message, instead of silently deleting another run's live claim. function enterTakeover(takeover) { try { - fs.mkdirSync(takeover); + fs.symlinkSync(CLAIM_IDENTITY, takeover); return true; } catch (error) { - if (error.code !== 'EEXIST') throw error; - } - let age; - try { - age = Date.now() - fs.statSync(takeover).mtimeMs; - } catch { - return false; // vanished; the caller retries - } - if (age < TAKEOVER_STALE_MS) return false; - try { - fs.rmSync(takeover, { recursive: true, force: true }); - fs.mkdirSync(takeover); - return true; - } catch { - return false; + if (error.code === 'EEXIST') return false; + throw error; } } function leaveTakeover(takeover) { - fs.rmSync(takeover, { recursive: true, force: true }); + try { + if (fs.readlinkSync(takeover) !== CLAIM_IDENTITY) return; // someone else's: not ours to remove + fs.unlinkSync(takeover); + } catch { + // Already gone, or not a symlink we own: nothing this run may remove. + } } /** @@ -163,7 +163,8 @@ export function acquireBaseClaim(worktreeDir, label) { if (outcome === 'live') throw heldError(worktreeDir, readClaimIdentity(claim), label); if (outcome === 'busy' && attempt === CLAIM_ATTEMPTS - 1) { throw new Error( - `another run is taking over the abandoned claim on base ${label} in ${worktreeDir}; retry shortly`, + `another run is taking over the abandoned claim on base ${label} in ${worktreeDir}; ` + + `retry shortly, or remove ${takeoverPath(worktreeDir)} if no other \`size --base\` is running`, ); } } diff --git a/scripts/size-report-comment.mjs b/scripts/size-report-comment.mjs new file mode 100644 index 000000000..863c855af --- /dev/null +++ b/scripts/size-report-comment.mjs @@ -0,0 +1,164 @@ +// Publishing the size report as a PR comment — the convenience half of `scripts/size-report.mjs`, +// kept separate from measuring and formatting it. The same markdown always reaches the job +// summary, so this surface is best-effort by design: a GitHub outage must not fail the job, while +// a real misconfiguration still must. +// +// `scripts/__tests__/size-report-post-comment.test.ts` drives this through the real script with a +// stubbed fetch, pinning the retry, reconcile, and fatal outcomes. + +import fs from 'node:fs'; + +/** Identifies this job's comment so re-runs update one comment instead of appending new ones. */ +export const COMMENT_MARKER = ''; + +const GITHUB_REQUEST_ATTEMPTS = 4; +// Overridable so the regression tests do not sleep through real backoff. +const GITHUB_RETRY_BASE_MS = Number(process.env.SIZE_REPORT_RETRY_BASE_MS ?? 1000); + +class TransientGitHubError extends Error {} + +// The PR comment is a convenience surface: the same markdown is already in the +// job summary. A GitHub outage (5xx / 429 / network error) must not fail the +// job, but a real misconfiguration (bad token, missing permissions) still does. +export async function postGitHubCommentBestEffort(markdownPath, explicitPrNumber) { + try { + await postGitHubComment(markdownPath, explicitPrNumber); + } catch (error) { + if (!(error instanceof TransientGitHubError)) throw error; + const message = `Skipping PR size comment after transient GitHub failure: ${error.message}`; + process.stdout.write(`::warning::${message}\n`); + appendStepSummary(`> ⚠️ ${message} The size report above is authoritative.\n`); + } +} + +function appendStepSummary(text) { + const summaryPath = process.env.GITHUB_STEP_SUMMARY; + if (summaryPath) fs.appendFileSync(summaryPath, text); +} + +async function postGitHubComment(markdownPath, explicitPrNumber) { + const config = readGitHubCommentConfig(explicitPrNumber); + const body = fs.readFileSync(markdownPath, 'utf8'); + const commentsUrl = buildCommentsUrl(config.repository, config.prNumber); + await retryTransient(() => syncGitHubComment(commentsUrl, config.headers, body)); +} + +// Every attempt re-lists before writing: a create whose response was lost +// (network error / 5xx) may still have landed server-side, and re-listing turns +// that into an update of the existing marker comment instead of a duplicate. +async function syncGitHubComment(commentsUrl, headers, body) { + const comments = await listGitHubComments(commentsUrl, headers); + const existing = comments.find((comment) => comment.body?.includes(COMMENT_MARKER)); + await writeGitHubComment(commentsUrl, headers, body, existing?.url); +} + +function readGitHubCommentConfig(explicitPrNumber) { + const token = process.env.GITHUB_TOKEN; + const repository = process.env.GITHUB_REPOSITORY; + const prNumber = explicitPrNumber ?? process.env.GITHUB_PR_NUMBER; + assertGitHubCommentConfig(token, repository, prNumber); + return { + repository, + prNumber, + headers: buildGitHubHeaders(token), + }; +} + +function assertGitHubCommentConfig(token, repository, prNumber) { + for (const value of [token, repository, prNumber]) { + if (!value) { + throw new Error( + 'GITHUB_TOKEN, GITHUB_REPOSITORY, and PR number are required to post a comment.', + ); + } + } +} + +function buildGitHubHeaders(token) { + return { + accept: 'application/vnd.github+json', + authorization: `Bearer ${token}`, + 'content-type': 'application/json', + 'x-github-api-version': '2022-11-28', + }; +} + +function buildCommentsUrl(repository, prNumber) { + const [owner, repo] = repository.split('/'); + return `https://api.github.com/repos/${owner}/${repo}/issues/${prNumber}/comments`; +} + +async function listGitHubComments(commentsUrl, headers) { + const response = await githubRequest( + `${commentsUrl}?per_page=100`, + { headers }, + 'list PR comments', + ); + return await response.json(); +} + +async function writeGitHubComment(commentsUrl, headers, body, existingUrl) { + const target = commentWriteTarget(commentsUrl, existingUrl); + await githubRequest( + target.url, + { method: target.method, headers, body: JSON.stringify({ body }) }, + `${target.action} PR comment`, + ); +} + +function commentWriteTarget(commentsUrl, existingUrl) { + if (existingUrl) { + return { url: existingUrl, method: 'PATCH', action: 'update' }; + } + return { url: commentsUrl, method: 'POST', action: 'create' }; +} + +// Re-runs `operation` with exponential backoff while it throws +// TransientGitHubError; any other error (a non-transient HTTP status, i.e. a +// configuration problem) propagates immediately and fails the job. +async function retryTransient(operation) { + for (let attempt = 1; ; attempt += 1) { + try { + return await operation(); + } catch (error) { + await backoffOrRethrow(error, attempt); + } + } +} + +async function backoffOrRethrow(error, attempt) { + if (!(error instanceof TransientGitHubError)) throw error; + if (attempt >= GITHUB_REQUEST_ATTEMPTS) { + throw new TransientGitHubError(`${error.message} after ${attempt} attempts`); + } + const delayMs = GITHUB_RETRY_BASE_MS * 2 ** (attempt - 1); + process.stderr.write(`${error.message} (retrying in ${delayMs}ms)\n`); + await new Promise((resolve) => setTimeout(resolve, delayMs)); +} + +// One attempt: network errors and 5xx / 429 throw TransientGitHubError; +// any other non-OK status throws a plain (fatal) Error. +async function githubRequest(url, init, action) { + const response = await fetchOrTransient(url, init, action); + if (response.ok) return response; + throw await githubStatusError(response, action); +} + +async function fetchOrTransient(url, init, action) { + try { + return await fetch(url, init); + } catch (error) { + throw new TransientGitHubError(`Failed to ${action}: ${error?.message ?? error}`); + } +} + +async function githubStatusError(response, action) { + const failure = `Failed to ${action}: ${response.status} ${await response.text()}`; + return isTransientGitHubStatus(response.status) + ? new TransientGitHubError(failure) + : new Error(failure); +} + +function isTransientGitHubStatus(status) { + return status === 429 || status >= 500; +} diff --git a/scripts/size-report.mjs b/scripts/size-report.mjs index 33c0b116b..04db9e9ae 100644 --- a/scripts/size-report.mjs +++ b/scripts/size-report.mjs @@ -5,12 +5,8 @@ import { execFileSync } from 'node:child_process'; import { performance } from 'node:perf_hooks'; import { gzipSync } from 'node:zlib'; import { withPreparedBaseWorktree } from './size-base-cache.mjs'; +import { COMMENT_MARKER, postGitHubCommentBestEffort } from './size-report-comment.mjs'; -const COMMENT_MARKER = ''; -const GITHUB_REQUEST_ATTEMPTS = 4; -// Overridable so the regression tests do not sleep through real backoff. -const GITHUB_RETRY_BASE_MS = Number(process.env.SIZE_REPORT_RETRY_BASE_MS ?? 1000); -class TransientGitHubError extends Error {} const VALUE_ARGS = new Map([ ['--cwd', 'cwd'], ['--json', 'json'], @@ -388,149 +384,3 @@ function writeFile(filePath, contents) { fs.mkdirSync(path.dirname(path.resolve(filePath)), { recursive: true }); fs.writeFileSync(filePath, contents); } - -// The PR comment is a convenience surface: the same markdown is already in the -// job summary. A GitHub outage (5xx / 429 / network error) must not fail the -// job, but a real misconfiguration (bad token, missing permissions) still does. -async function postGitHubCommentBestEffort(markdownPath, explicitPrNumber) { - try { - await postGitHubComment(markdownPath, explicitPrNumber); - } catch (error) { - if (!(error instanceof TransientGitHubError)) throw error; - const message = `Skipping PR size comment after transient GitHub failure: ${error.message}`; - process.stdout.write(`::warning::${message}\n`); - appendStepSummary(`> ⚠️ ${message} The size report above is authoritative.\n`); - } -} - -function appendStepSummary(text) { - const summaryPath = process.env.GITHUB_STEP_SUMMARY; - if (summaryPath) fs.appendFileSync(summaryPath, text); -} - -async function postGitHubComment(markdownPath, explicitPrNumber) { - const config = readGitHubCommentConfig(explicitPrNumber); - const body = fs.readFileSync(markdownPath, 'utf8'); - const commentsUrl = buildCommentsUrl(config.repository, config.prNumber); - await retryTransient(() => syncGitHubComment(commentsUrl, config.headers, body)); -} - -// Every attempt re-lists before writing: a create whose response was lost -// (network error / 5xx) may still have landed server-side, and re-listing turns -// that into an update of the existing marker comment instead of a duplicate. -async function syncGitHubComment(commentsUrl, headers, body) { - const comments = await listGitHubComments(commentsUrl, headers); - const existing = comments.find((comment) => comment.body?.includes(COMMENT_MARKER)); - await writeGitHubComment(commentsUrl, headers, body, existing?.url); -} - -function readGitHubCommentConfig(explicitPrNumber) { - const token = process.env.GITHUB_TOKEN; - const repository = process.env.GITHUB_REPOSITORY; - const prNumber = explicitPrNumber ?? process.env.GITHUB_PR_NUMBER; - assertGitHubCommentConfig(token, repository, prNumber); - return { - repository, - prNumber, - headers: buildGitHubHeaders(token), - }; -} - -function assertGitHubCommentConfig(token, repository, prNumber) { - for (const value of [token, repository, prNumber]) { - if (!value) { - throw new Error( - 'GITHUB_TOKEN, GITHUB_REPOSITORY, and PR number are required to post a comment.', - ); - } - } -} - -function buildGitHubHeaders(token) { - return { - accept: 'application/vnd.github+json', - authorization: `Bearer ${token}`, - 'content-type': 'application/json', - 'x-github-api-version': '2022-11-28', - }; -} - -function buildCommentsUrl(repository, prNumber) { - const [owner, repo] = repository.split('/'); - return `https://api.github.com/repos/${owner}/${repo}/issues/${prNumber}/comments`; -} - -async function listGitHubComments(commentsUrl, headers) { - const response = await githubRequest( - `${commentsUrl}?per_page=100`, - { headers }, - 'list PR comments', - ); - return await response.json(); -} - -async function writeGitHubComment(commentsUrl, headers, body, existingUrl) { - const target = commentWriteTarget(commentsUrl, existingUrl); - await githubRequest( - target.url, - { method: target.method, headers, body: JSON.stringify({ body }) }, - `${target.action} PR comment`, - ); -} - -function commentWriteTarget(commentsUrl, existingUrl) { - if (existingUrl) { - return { url: existingUrl, method: 'PATCH', action: 'update' }; - } - return { url: commentsUrl, method: 'POST', action: 'create' }; -} - -// Re-runs `operation` with exponential backoff while it throws -// TransientGitHubError; any other error (a non-transient HTTP status, i.e. a -// configuration problem) propagates immediately and fails the job. -async function retryTransient(operation) { - for (let attempt = 1; ; attempt += 1) { - try { - return await operation(); - } catch (error) { - await backoffOrRethrow(error, attempt); - } - } -} - -async function backoffOrRethrow(error, attempt) { - if (!(error instanceof TransientGitHubError)) throw error; - if (attempt >= GITHUB_REQUEST_ATTEMPTS) { - throw new TransientGitHubError(`${error.message} after ${attempt} attempts`); - } - const delayMs = GITHUB_RETRY_BASE_MS * 2 ** (attempt - 1); - process.stderr.write(`${error.message} (retrying in ${delayMs}ms)\n`); - await new Promise((resolve) => setTimeout(resolve, delayMs)); -} - -// One attempt: network errors and 5xx / 429 throw TransientGitHubError; -// any other non-OK status throws a plain (fatal) Error. -async function githubRequest(url, init, action) { - const response = await fetchOrTransient(url, init, action); - if (response.ok) return response; - throw await githubStatusError(response, action); -} - -async function fetchOrTransient(url, init, action) { - try { - return await fetch(url, init); - } catch (error) { - throw new TransientGitHubError(`Failed to ${action}: ${error?.message ?? error}`); - } -} - -async function githubStatusError(response, action) { - const failure = `Failed to ${action}: ${response.status} ${await response.text()}`; - return isTransientGitHubStatus(response.status) - ? new TransientGitHubError(failure) - : new Error(failure); -} - -function isTransientGitHubStatus(status) { - return status === 429 || status >= 500; -} From bb22b46067980d516e0e40bdef5363d6b0230be3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 11:36:30 +0200 Subject: [PATCH 09/10] test: prove delayed size cache holder is preserved --- scripts/__tests__/size-base-cache.test.ts | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/scripts/__tests__/size-base-cache.test.ts b/scripts/__tests__/size-base-cache.test.ts index 62e6fe23b..d8e736eb5 100644 --- a/scripts/__tests__/size-base-cache.test.ts +++ b/scripts/__tests__/size-base-cache.test.ts @@ -88,23 +88,25 @@ test('removal cannot run at all while another run holds the takeover mutex', () test('a delayed takeover holder is never displaced, however old its mutex looks', () => { // The interleaving that age-based reclamation created: holder A is merely slow — paused, or - // SIGSTOPed past any threshold — while still inside the section. Reclaiming its mutex would put - // B inside too, and then A's release could remove B's mutex and either could unlink the claim - // the other just created. A mutex is therefore never taken from its holder, at any age. + // SIGSTOPed past any threshold — while still inside the section. Use the old implementation's + // directory-shaped mutex so this is also a planted regression against that exact code: it would + // reclaim the aged directory and enter concurrently. Reclaiming the mutex would put B inside too, + // and then A's release could remove B's mutex and either could unlink the claim the other just + // created. A mutex is therefore never taken from its holder, at any age. fs.symlinkSync(ABANDONED, claimPath(entry)); - fs.symlinkSync(`${process.pid}:delayed-holder`, takeoverPath(entry)); + fs.mkdirSync(takeoverPath(entry)); const longAgo = new Date(Date.now() - 24 * 60 * 60 * 1000); - fs.lutimesSync(takeoverPath(entry), longAgo, longAgo); + fs.utimesSync(takeoverPath(entry), longAgo, longAgo); assert.equal(removeAbandonedClaim(entry, ABANDONED), 'busy'); assert.equal( - fs.readlinkSync(takeoverPath(entry)), - `${process.pid}:delayed-holder`, + fs.lstatSync(takeoverPath(entry)).isDirectory(), + true, "the holder's mutex is intact", ); assert.equal(readClaimIdentity(claimPath(entry)), ABANDONED, 'and it removed nothing'); assert.throws(() => acquireBaseClaim(entry, 'abc123456'), /taking over the abandoned claim/); - assert.equal(fs.readlinkSync(takeoverPath(entry)), `${process.pid}:delayed-holder`); + assert.equal(fs.lstatSync(takeoverPath(entry)).isDirectory(), true); }); test('a leaked mutex wedges only its own entry, and says how to clear it', () => { From c21ccbc1492885d13f547c876bd20a8ebdbd6852 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 14:18:45 +0200 Subject: [PATCH 10/10] docs: keep size review in CI --- .github/workflows/ci.yml | 7 - .github/workflows/size.yml | 10 +- docs/agents/adr-0019-unit.md | 6 +- docs/agents/pull-requests.md | 14 +- docs/agents/testing.md | 32 +- package.json | 2 - scripts/__tests__/size-base-cache.test.ts | 140 --------- scripts/__tests__/size-report-base.test.ts | 198 ------------- .../size-report-preserved-closure.test.ts | 68 ----- scripts/check-affected/checks.ts | 1 - scripts/check-affected/model.ts | 2 - scripts/pr-evidence/model.test.ts | 171 ----------- scripts/pr-evidence/model.ts | 171 ----------- scripts/pr-evidence/run.test.ts | 53 ---- scripts/pr-evidence/run.ts | 201 ------------- scripts/pr-evidence/worktrees.test.ts | 119 -------- scripts/pr-evidence/worktrees.ts | 76 ----- scripts/size-base-cache.mjs | 277 ------------------ scripts/size-report-comment.mjs | 164 ----------- scripts/size-report.mjs | 190 ++++++++++-- vitest.config.ts | 15 - 21 files changed, 178 insertions(+), 1739 deletions(-) delete mode 100644 scripts/__tests__/size-base-cache.test.ts delete mode 100644 scripts/__tests__/size-report-base.test.ts delete mode 100644 scripts/__tests__/size-report-preserved-closure.test.ts delete mode 100644 scripts/pr-evidence/model.test.ts delete mode 100644 scripts/pr-evidence/model.ts delete mode 100644 scripts/pr-evidence/run.test.ts delete mode 100644 scripts/pr-evidence/run.ts delete mode 100644 scripts/pr-evidence/worktrees.test.ts delete mode 100644 scripts/pr-evidence/worktrees.ts delete mode 100644 scripts/size-base-cache.mjs delete mode 100644 scripts/size-report-comment.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0b1aed6f3..015540029 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -177,13 +177,6 @@ jobs: uses: ./.github/actions/run-gate with: { gate: gate-manifest } - # `pnpm pr:evidence` composes this job's selector output (plus the depgraph and layering - # reports) into the PR-body evidence block; its pure model lives here with the selector - # it reads. Seconds, no install beyond this job's. - - name: Check the PR evidence composer model - uses: ./.github/actions/run-gate - with: { gate: pr-evidence-model } - # Same family as the manifest above — a CI selection that has stopped selecting what # it claims. ios.yml runs a hand-written subset of the runner XCTest methods through an # `-only-testing:` list, and xcodebuild treats an identifier that matches nothing as diff --git a/.github/workflows/size.yml b/.github/workflows/size.yml index 869328a52..63ca37e18 100644 --- a/.github/workflows/size.yml +++ b/.github/workflows/size.yml @@ -40,14 +40,8 @@ jobs: - name: Setup toolchain uses: ./.github/actions/setup-node-pnpm - # The base is measured with the PR's instrument, so the reporter is copied out before the - # checkout moves. It is a module graph, not one file: copy the whole directory so a future - # split cannot silently leave an import behind (a one-file copy broke exactly that way). - # scripts/__tests__/size-report-preserved-closure.test.ts holds this step to the closure. - name: Preserve report script - run: | - rm -rf /tmp/agent-device-size-report - cp -R scripts /tmp/agent-device-size-report + run: cp scripts/size-report.mjs /tmp/agent-device-size-report.mjs # dist is fully determined by the base commit, so reuse it across PR runs # against the same base. Startup medians are still measured fresh on this @@ -66,7 +60,7 @@ jobs: if [ "${{ steps.base-dist-cache.outputs.cache-hit }}" != "true" ]; then pnpm build fi - node /tmp/agent-device-size-report/size-report.mjs \ + node /tmp/agent-device-size-report.mjs \ --startup-runs 7 \ --json /tmp/agent-device-size-base.json diff --git a/docs/agents/adr-0019-unit.md b/docs/agents/adr-0019-unit.md index 8a3ae7c6a..207bdfedc 100644 --- a/docs/agents/adr-0019-unit.md +++ b/docs/agents/adr-0019-unit.md @@ -45,9 +45,9 @@ Each step names its declaration site; read that, not prose. §4–5 lifecycle evidence. Importing durable machinery promotes the tier — say so. - Planted-red for anything new that is *not* a table row (a facet, a package boundary): revert, run, quote the failing line. A row needs no planted-red of its own. -- Size: root-bytes-removed vs package-bytes-added and the four checkpoint metrics - (`pnpm size --compare` against the base build), against the posted budget. Move-dominated is the - rule; net growth is itemized, not explained away. +- Size: use the CI Size workflow's root-bytes-removed vs package-bytes-added and four checkpoint + metrics against the posted budget. Do not build a base checkout or run a local size comparison by + default. Move-dominated is the rule; net growth is itemized, not explained away. - Live evidence for the changed path on at least one real target per family the denominator claims (`docs/agents/device-verification.md`); fixture-backed parity does not replace it. - Layering: `pnpm check:layering` green — R3 seam list narrows in the unit that removes an area's diff --git a/docs/agents/pull-requests.md b/docs/agents/pull-requests.md index b98028e0e..8baa9ac5b 100644 --- a/docs/agents/pull-requests.md +++ b/docs/agents/pull-requests.md @@ -60,12 +60,6 @@ asked or when the work is intentionally incomplete. validation does not apply instead of writing a command checklist. - Call out real tradeoffs, known gaps, and follow-ups; omit boilerplate when there are none. - Note touched-file count and whether scope expanded beyond the initial command family. -- Paste the block `pnpm pr:evidence` prints (add `--size` for a unit with a size budget, - `--coverage` after `pnpm test:coverage`) rather than transcribing SHAs, gate lists, layering - counts, or edge deltas by hand: it stamps the exact merge-base and head, composes the affected - plan, layering guard, depgraph deltas, and the two optional reports, and links the head's CI - instead of claiming it. After a rebase, re-run it (~20s) rather than editing the old block; the - stamp is what makes the evidence dated instead of stale. ## Reviewing - Review against the linked issue, not only the diff. State the issue's motivating behavior and @@ -89,3 +83,11 @@ asked or when the work is intentionally incomplete. before/after evidence when an issue reports a concrete divergence. - Green CI is necessary but insufficient for device-facing or routing-sensitive work. - Check whether the tightening pass removed code/tests the change made obsolete. +- Treat the CI Size workflow as review evidence; local size comparisons are not required by default. + Escalate scrutiny when a PR adds roughly 700 or more net production lines (excluding tests, + generated data, fixtures, and documentation) or increases npm unpacked size by more than 3 kB. + Consider gross additions and deletions too, so a move-dominated change is not mistaken for pure + growth. These thresholds trigger investigation, not automatic rejection: ask an independent + reviewer whether a deeper owning interface, stronger types, less ceremony, reuse of an existing + construction path, or deletion of superseded code can make the change materially smaller. The PR + should itemize justified growth and record why a smaller design was rejected. diff --git a/docs/agents/testing.md b/docs/agents/testing.md index af1fbedd5..9ec84e26c 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -136,6 +136,12 @@ the generator, where every property inherits it — not in a new hand-pinned cas ## Affected-check selector (`pnpm check:affected`) +Fast local feedback is a project value: the default developer loop should run the smallest relevant +gate set and return as quickly as correctness allows. Expensive informational measurements belong in +CI unless they are needed to diagnose a reported result. In particular, do not build a base checkout +or run package-size comparisons locally by default; use the authoritative Size workflow report during +review. + `pnpm check:affected --base ` derives which local checks a diff needs, so agents stop interpreting the testing matrix by hand. It is a **fail-open advisory**: existing GitHub CI stays authoritative and required, and this only @@ -228,27 +234,6 @@ Lists are bounded (`--limit`, default 10) and always disclose what they hid; `-- unbounded. The query is read-only, runs in well under a second, and adds no CI work — its model is covered by `pnpm depgraph:test` (the existing `Layering Guard` job). -## Shipped size (`pnpm size --base `) - -The Size workflow posts a base/PR comparison on every PR; the same comparison runs locally in one -command, before the PR exists: - -```sh -pnpm size --base origin/main # first run: detached worktree + install + build of the base (~1-2 min) - # later runs against the same base: ~3s (the worktree is kept under .tmp/size-base/) -``` - -The cache is per SHA and never destructive toward a run in progress: a `.lock` (pid inside) is held -from before the worktree exists until the base report is read, a concurrent run against the same -base fails fast rather than reading a half-built `dist`, another base's run evicts only worktrees -whose lock is absent or whose owner is dead, and a build that was interrupted before its -`dist/.size-base-complete` stamp is rebuilt. - -Requires a current `pnpm build` of your own tree. `JS raw`/`JS gzip` are the numbers to quote and to -budget against (ADR 0019 §8 units state theirs before starting); the `npm tarball`/`npm unpacked` -rows compare a fresh base checkout against your working tree, which may carry locally built helper -artifacts CI's fresh checkout does not, so read a tarball delta on GitHub's comment, not here. - ## Gate manifest: proving every check has a CI owner Every gate above answers "is the code right?". None of them can answer "does CI still own this @@ -542,11 +527,8 @@ would silently enroll every future file under a directory — and run in their o exactly that list, and both projects run inside one `vitest run`, so the serialized chain runs alongside the main pool rather than after it (~0 added CI wall clock). -Issue #1823 owns the membership and the project's deletion test: if they run un-serialized in +Issue #1823 owns the membership and the project's deletion test: if the three run un-serialized in the default pool for 20 consecutive CI runs with no timeout-shaped failure, the project goes. -`size-report-base.test.ts` joined in #1842 — it drives `pnpm size --base`'s worktree/lock -orchestration through a real `node scripts/size-report.mjs` per case, which spawns git and the -shimmed package managers under it. Adding a file needs the concrete spawn named at the entry; per-file `process.env` isolation is not a reason, since `pool: forks` + `isolate: true` already give every project that. diff --git a/package.json b/package.json index aaf675332..687284a30 100644 --- a/package.json +++ b/package.json @@ -117,8 +117,6 @@ "maestro:conformance:regenerate": "node --experimental-strip-types scripts/maestro-conformance/regenerate.mjs", "maestro:conformance:differential": "node --experimental-strip-types packages/maestro/test/conformance/differential/run.ts", "size": "node scripts/size-report.mjs", - "pr:evidence": "node --experimental-strip-types scripts/pr-evidence/run.ts", - "pr:evidence:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/pr-evidence/model.test.ts scripts/pr-evidence/worktrees.test.ts scripts/pr-evidence/run.test.ts", "perf": "node --experimental-strip-types scripts/perf/run.ts", "mutation:run": "node --experimental-strip-types scripts/mutation/run.ts", "mutation:check": "node --experimental-strip-types scripts/mutation/run.ts --no-run", diff --git a/scripts/__tests__/size-base-cache.test.ts b/scripts/__tests__/size-base-cache.test.ts deleted file mode 100644 index d8e736eb5..000000000 --- a/scripts/__tests__/size-base-cache.test.ts +++ /dev/null @@ -1,140 +0,0 @@ -import assert from 'node:assert/strict'; -import fs from 'node:fs'; -import os from 'node:os'; -import path from 'node:path'; -import { afterEach, beforeEach, test } from 'vitest'; -import { - CLAIM_IDENTITY, - acquireBaseClaim, - claimPath, - readClaimIdentity, - removeAbandonedClaim, - takeoverPath, -} from '../size-base-cache.mjs'; - -// The claim protocol on its own: pure filesystem, no git and no subprocess, so the dangerous -// interleavings can be planted directly instead of hoped for under load. The orchestration this -// protects (worktree reuse, eviction, build stamping) is covered by size-report-base.test.ts. - -const NEVER_A_PID = 2_147_483_647; // outside every platform's pid range: dead by construction -const ABANDONED = `${NEVER_A_PID}:abandoned`; -const OTHER_LIVE = `${process.pid}:another-run`; - -let entry: string; - -beforeEach(() => { - entry = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'size-base-cache-')), 'abc123456789'); -}); - -afterEach(() => { - fs.rmSync(path.dirname(entry), { recursive: true, force: true }); -}); - -test('an unclaimed entry is claimed, names this run, and is released', () => { - const release = acquireBaseClaim(entry, 'abc123456'); - assert.equal(readClaimIdentity(claimPath(entry)), CLAIM_IDENTITY); - release(); - assert.equal(readClaimIdentity(claimPath(entry)), undefined); -}); - -test('a claim held by a live run is refused, and nothing about it is touched', () => { - fs.symlinkSync(OTHER_LIVE, claimPath(entry)); - assert.throws(() => acquireBaseClaim(entry, 'abc123456'), /is using base abc123456/); - assert.equal(readClaimIdentity(claimPath(entry)), OTHER_LIVE, 'the live claim survives'); -}); - -test('an abandoned claim is taken over', () => { - fs.symlinkSync(ABANDONED, claimPath(entry)); - const release = acquireBaseClaim(entry, 'abc123456'); - assert.equal(readClaimIdentity(claimPath(entry)), CLAIM_IDENTITY); - release(); -}); - -test('the exact interleaving: an abandoned claim replaced by a live one is never deleted', () => { - // The window the protocol has to survive — observe abandoned, then another run takes over - // before the removal. `removeAbandonedClaim` re-verifies under the takeover mutex, so the - // replacement it finds is reported, not unlinked. - fs.symlinkSync(ABANDONED, claimPath(entry)); - const observed = readClaimIdentity(claimPath(entry)); - // …the replacement lands here, in the window between observing and removing… - fs.unlinkSync(claimPath(entry)); - fs.symlinkSync(OTHER_LIVE, claimPath(entry)); - - assert.equal(removeAbandonedClaim(entry, observed), 'changed'); - assert.equal(readClaimIdentity(claimPath(entry)), OTHER_LIVE, 'the replacement is intact'); - // And the full acquire path refuses rather than stealing it. - assert.throws(() => acquireBaseClaim(entry, 'abc123456'), /is using base abc123456/); - assert.equal(readClaimIdentity(claimPath(entry)), OTHER_LIVE); -}); - -test('removal cannot run at all while another run holds the takeover mutex', () => { - // A second taker is mid-takeover: the mutex is held, so this run must not remove anything, - // and after CLAIM_ATTEMPTS it reports the contention instead of forcing its way in. - fs.symlinkSync(ABANDONED, claimPath(entry)); - fs.mkdirSync(takeoverPath(entry)); - try { - assert.equal(removeAbandonedClaim(entry, ABANDONED), 'busy'); - assert.equal(readClaimIdentity(claimPath(entry)), ABANDONED, 'untouched while contended'); - assert.throws(() => acquireBaseClaim(entry, 'abc123456'), /taking over the abandoned claim/); - assert.equal(readClaimIdentity(claimPath(entry)), ABANDONED); - } finally { - fs.rmSync(takeoverPath(entry), { recursive: true, force: true }); - } - // Once the other taker finishes, the entry is claimable again. - const release = acquireBaseClaim(entry, 'abc123456'); - assert.equal(readClaimIdentity(claimPath(entry)), CLAIM_IDENTITY); - release(); -}); - -test('a delayed takeover holder is never displaced, however old its mutex looks', () => { - // The interleaving that age-based reclamation created: holder A is merely slow — paused, or - // SIGSTOPed past any threshold — while still inside the section. Use the old implementation's - // directory-shaped mutex so this is also a planted regression against that exact code: it would - // reclaim the aged directory and enter concurrently. Reclaiming the mutex would put B inside too, - // and then A's release could remove B's mutex and either could unlink the claim the other just - // created. A mutex is therefore never taken from its holder, at any age. - fs.symlinkSync(ABANDONED, claimPath(entry)); - fs.mkdirSync(takeoverPath(entry)); - const longAgo = new Date(Date.now() - 24 * 60 * 60 * 1000); - fs.utimesSync(takeoverPath(entry), longAgo, longAgo); - - assert.equal(removeAbandonedClaim(entry, ABANDONED), 'busy'); - assert.equal( - fs.lstatSync(takeoverPath(entry)).isDirectory(), - true, - "the holder's mutex is intact", - ); - assert.equal(readClaimIdentity(claimPath(entry)), ABANDONED, 'and it removed nothing'); - assert.throws(() => acquireBaseClaim(entry, 'abc123456'), /taking over the abandoned claim/); - assert.equal(fs.lstatSync(takeoverPath(entry)).isDirectory(), true); -}); - -test('a leaked mutex wedges only its own entry, and says how to clear it', () => { - // The price of never reclaiming: one entry needs a human. The message has to name the path. - fs.symlinkSync(ABANDONED, claimPath(entry)); - fs.symlinkSync(`${NEVER_A_PID}:leaked`, takeoverPath(entry)); - assert.throws( - () => acquireBaseClaim(entry, 'abc123456'), - new RegExp(`remove ${takeoverPath(entry).replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`), - ); - // A different entry is unaffected: the mutex is per entry, so nothing else is wedged. - const other = path.join(path.dirname(entry), 'def987654321'); - const release = acquireBaseClaim(other, 'def987654'); - assert.equal(readClaimIdentity(claimPath(other)), CLAIM_IDENTITY); - release(); -}); - -test('release leaves a claim that has come to name another run alone', () => { - const release = acquireBaseClaim(entry, 'abc123456'); - fs.unlinkSync(claimPath(entry)); - fs.symlinkSync(OTHER_LIVE, claimPath(entry)); - release(); - assert.equal(readClaimIdentity(claimPath(entry)), OTHER_LIVE, "another run's claim survives"); -}); - -test('a stray non-symlink at the claim path is cleared instead of wedging the entry', () => { - fs.writeFileSync(claimPath(entry), 'not a claim of this scheme\n'); - const release = acquireBaseClaim(entry, 'abc123456'); - assert.equal(readClaimIdentity(claimPath(entry)), CLAIM_IDENTITY); - release(); -}); diff --git a/scripts/__tests__/size-report-base.test.ts b/scripts/__tests__/size-report-base.test.ts deleted file mode 100644 index 975529a02..000000000 --- a/scripts/__tests__/size-report-base.test.ts +++ /dev/null @@ -1,198 +0,0 @@ -import assert from 'node:assert/strict'; -import fs from 'node:fs'; -import os from 'node:os'; -import path from 'node:path'; -import { afterAll, beforeAll, test } from 'vitest'; -import { runCmd, runCmdSync } from '../../src/utils/exec.ts'; - -// `pnpm size --base ` orchestration against a throwaway git repository, with `pnpm` and -// `npm` shimmed on PATH: the shim `pnpm build` writes dist/src and appends to a log, the shim -// `npm pack` prints a fixed dry-run JSON. No install, no network; every run is git + node. -// -// This file owns what only real processes can show: that two concurrent runs build once, that a -// cached entry is reused, and that eviction respects a claim. The claim protocol's own -// interleavings (takeover, replacement, contention) are planted directly in -// size-base-cache.test.ts, which needs no subprocess at all. - -const ROOT = path.join(import.meta.dirname, '..', '..'); -const SCRIPT = path.join(ROOT, 'scripts', 'size-report.mjs'); -const NEVER_A_PID = 2_147_483_647; // outside every platform's pid range: dead by construction - -let repo: string; -let bin: string; -let buildLog: string; -let first: string; -let second: string; - -function git(args: string[], cwd = repo): string { - return runCmdSync('git', args, { cwd }).stdout.trim(); -} - -function writeExecutable(file: string, body: string): void { - fs.writeFileSync(file, body); - fs.chmodSync(file, 0o755); -} - -beforeAll(() => { - const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'size-report-base-')); - repo = path.join(scratch, 'repo'); - bin = path.join(scratch, 'bin'); - buildLog = path.join(scratch, 'build.log'); - fs.mkdirSync(repo); - fs.mkdirSync(bin); - writeExecutable( - path.join(bin, 'pnpm'), - `#!/bin/sh -echo "$PWD $*" >> "${buildLog}" -if [ "$1" = "build" ]; then mkdir -p dist/src && printf 'export const built = 1;\\n' > dist/src/index.js; fi -`, - ); - writeExecutable( - path.join(bin, 'npm'), - `#!/bin/sh -echo '[{"filename":"pkg.tgz","size":100,"unpackedSize":200,"entryCount":2}]' -`, - ); - git(['init', '-q', '-b', 'main']); - git(['config', 'user.email', 'size@test']); - git(['config', 'user.name', 'size test']); - fs.writeFileSync(path.join(repo, 'package.json'), '{"name":"probe","version":"1.0.0"}\n'); - fs.writeFileSync(path.join(repo, '.gitignore'), '.tmp/\ndist/\n'); - git(['add', '.']); - git(['commit', '-q', '-m', 'first']); - first = git(['rev-parse', 'HEAD']); - fs.writeFileSync(path.join(repo, 'README.md'), 'second\n'); - git(['add', '.']); - git(['commit', '-q', '-m', 'second']); - second = git(['rev-parse', 'HEAD']); - // The head side of the comparison needs a dist too. - fs.mkdirSync(path.join(repo, 'dist', 'src'), { recursive: true }); - fs.writeFileSync(path.join(repo, 'dist', 'src', 'index.js'), 'export const head = 1;\n'); -}); - -afterAll(() => { - fs.rmSync(path.dirname(repo), { recursive: true, force: true }); -}); - -async function size(base: string) { - return await runCmd(process.execPath, [SCRIPT, '--cwd', repo, '--base', base], { - cwd: repo, - env: { ...process.env, PATH: `${bin}:${process.env.PATH ?? ''}` }, - allowFailure: true, - timeoutMs: 60_000, - }); -} - -const worktreeOf = (sha: string) => path.join(repo, '.tmp', 'size-base', sha.slice(0, 12)); -const lockOf = (sha: string) => `${worktreeOf(sha)}.lock`; -const holdLock = (sha: string, pid: number) => fs.symlinkSync(`${pid}:test`, lockOf(sha)); -const stampOf = (sha: string) => path.join(worktreeOf(sha), 'dist', '.size-base-complete'); -const builds = () => - fs - .readFileSync(buildLog, 'utf8') - .split('\n') - .filter((l) => l.endsWith(' build')); - -test('first run builds the base in a per-SHA worktree, stamps it, releases its lock; second run reuses it', async () => { - const one = await size(first); - assert.equal(one.exitCode, 0, one.stderr); - assert.match(one.stdout, /\| JS raw \|/); - assert.ok(fs.existsSync(stampOf(first)), 'completion stamp written after build'); - assert.equal(fs.existsSync(lockOf(first)), false, 'lock released after the report was read'); - assert.equal(builds().length, 1); - - const two = await size(first); - assert.equal(two.exitCode, 0, two.stderr); - assert.equal(builds().length, 1, 'a stamped base is not rebuilt'); -}); - -test('a base whose lock is held by a live pid fails fast without touching its worktree', async () => { - holdLock(first, process.pid); // this test process: alive - const before = fs.statSync(stampOf(first)).mtimeMs; - const result = await size(first); - assert.notEqual(result.exitCode, 0); - assert.match( - result.stderr, - new RegExp(`another \`size --base\` \\(pid ${process.pid}\\) is using`), - ); - assert.equal(fs.statSync(stampOf(first)).mtimeMs, before); - assert.equal(builds().length, 1); - fs.rmSync(lockOf(first)); -}); - -test('a stale lock (dead pid) is replaced and the run proceeds', async () => { - holdLock(first, NEVER_A_PID); - const result = await size(first); - assert.equal(result.exitCode, 0, result.stderr); - assert.equal(fs.existsSync(lockOf(first)), false); -}); - -test('an unstamped worktree (interrupted build) is rebuilt rather than trusted', async () => { - fs.rmSync(stampOf(first)); - const result = await size(first); - assert.equal(result.exitCode, 0, result.stderr); - assert.equal(builds().length, 2, 'dist/src existing without the stamp is not enough'); - assert.ok(fs.existsSync(stampOf(first))); -}); - -test('measuring another base evicts an idle cached base but never one whose lock is live', async () => { - holdLock(first, process.pid); // in use by "another run" - const guarded = await size(second); - assert.equal(guarded.exitCode, 0, guarded.stderr); - assert.ok(fs.existsSync(worktreeOf(first)), 'a live-locked worktree survives eviction'); - assert.ok(fs.existsSync(stampOf(second))); - fs.rmSync(lockOf(first)); - - const evicting = await size(first); - assert.equal(evicting.exitCode, 0, evicting.stderr); - assert.equal(fs.existsSync(worktreeOf(second)), false, 'an idle other base is evicted'); - assert.equal( - git(['worktree', 'list', '--porcelain']).includes(worktreeOf(second)), - false, - 'and unregistered from git', - ); -}); - -test('two overlapping runs on the same base: exactly one builds, the other fails fast on the live lock', async () => { - // A slower shim build widens the overlap window: the second run must find the first run's - // symlink lock (identity in place from its single creating syscall) and refuse. - fs.rmSync(worktreeOf(first), { recursive: true, force: true }); - runCmdSync('git', ['worktree', 'prune'], { cwd: repo }); - fs.rmSync(stampOf(first), { force: true }); - const slowBin = path.join(path.dirname(bin), 'slow-bin'); - fs.mkdirSync(slowBin, { recursive: true }); - writeExecutable( - path.join(slowBin, 'pnpm'), - `#!/bin/sh -echo "$PWD $*" >> "${buildLog}" -if [ "$1" = "build" ]; then sleep 1; mkdir -p dist/src && printf 'export const built = 1;\\n' > dist/src/index.js; fi -`, - ); - fs.copyFileSync(path.join(bin, 'npm'), path.join(slowBin, 'npm')); - fs.chmodSync(path.join(slowBin, 'npm'), 0o755); - const buildsBefore = builds().length; - const env = { ...process.env, PATH: `${slowBin}:${process.env.PATH ?? ''}` }; - const run = () => - runCmd(process.execPath, [SCRIPT, '--cwd', repo, '--base', first], { - cwd: repo, - env, - allowFailure: true, - timeoutMs: 60_000, - }); - const [a, b] = await Promise.all([run(), run()]); - const outcomes = [a, b].map((r) => r.exitCode === 0); - assert.deepEqual( - outcomes.sort(), - [false, true], - `one wins, one refuses: ${a.stderr} ${b.stderr}`, - ); - const loser = a.exitCode === 0 ? b : a; - assert.match(loser.stderr, /another `size --base` \(pid \d+\) is using base/); - assert.equal(builds().length - buildsBefore, 1, 'exactly one build across the two runs'); - assert.ok(fs.existsSync(stampOf(first))); - assert.equal( - fs.existsSync(lockOf(first)), - false, - 'the winner released; the loser removed nothing', - ); -}); diff --git a/scripts/__tests__/size-report-preserved-closure.test.ts b/scripts/__tests__/size-report-preserved-closure.test.ts deleted file mode 100644 index a800d46c8..000000000 --- a/scripts/__tests__/size-report-preserved-closure.test.ts +++ /dev/null @@ -1,68 +0,0 @@ -import { readFileSync } from 'node:fs'; -import path from 'node:path'; -import { expect, test } from 'vitest'; - -// The Size workflow measures the base commit with the PR's reporter, so it copies the reporter -// out of the tree before `git checkout` moves under it. That copy has to carry the reporter's -// whole relative-import closure: when the closure grew to a second file and the step still copied -// one, the base measurement died with ERR_MODULE_NOT_FOUND — after every deterministic gate had -// passed, because nothing local reproduces the copy. This test is that reproduction. - -const ROOT = path.resolve(import.meta.dirname, '..', '..'); -const ENTRY = 'scripts/size-report.mjs'; -const WORKFLOW = '.github/workflows/size.yml'; - -/** Repo-relative paths the entry reaches through relative (`./`, `../`) static imports. */ -function relativeImportClosure(entry: string): string[] { - const seen = new Set(); - const queue = [entry]; - while (queue.length > 0) { - const file = queue.shift() as string; - if (seen.has(file)) continue; - seen.add(file); - const source = readFileSync(path.join(ROOT, file), 'utf8'); - for (const match of source.matchAll( - /(?:^|\n)\s*(?:import|export)[^'"\n]*['"](\.[^'"]+)['"]/g, - )) { - const resolved = path.posix.join(path.posix.dirname(file), match[1] as string); - if (!seen.has(resolved)) queue.push(resolved); - } - } - return [...seen].sort(); -} - -test('the reporter is more than one file, so the workflow may not preserve it as one file', () => { - const closure = relativeImportClosure(ENTRY); - const workflow = readFileSync(path.join(ROOT, WORKFLOW), 'utf8'); - const preserve = workflow.slice( - workflow.indexOf('- name: Preserve report script'), - workflow.indexOf('- name: Restore base dist cache'), - ); - expect(preserve).not.toEqual(''); - - // Copying the whole directory covers any closure inside it, including files a later split adds. - const copiesDirectory = /cp\s+-R\s+scripts\s/.test(preserve); - const outside = closure.filter((file) => !file.startsWith('scripts/')); - if (copiesDirectory) { - expect(outside, 'a closure member outside scripts/ is not covered by copying scripts/').toEqual( - [], - ); - return; - } - // Otherwise every member must be named explicitly — the shape that already broke once. - for (const file of closure) { - expect(preserve, `the preserve step must copy ${file}`).toContain(file); - } -}); - -test('the workflow runs the preserved copy, not the checked-out tree', () => { - const workflow = readFileSync(path.join(ROOT, WORKFLOW), 'utf8'); - const base = workflow.slice( - workflow.indexOf('- name: Measure base size'), - workflow.indexOf('- name: Save base dist cache'), - ); - // Measuring the base with `pnpm size` would run the base commit's own reporter, so base and PR - // would be measured by different instruments — the reason the copy exists at all. - expect(base).toMatch(/node \/tmp\/agent-device-size-report\/size-report\.mjs/); - expect(base).not.toMatch(/pnpm size/); -}); diff --git a/scripts/check-affected/checks.ts b/scripts/check-affected/checks.ts index 572af37f3..088801cb9 100644 --- a/scripts/check-affected/checks.ts +++ b/scripts/check-affected/checks.ts @@ -88,7 +88,6 @@ export const CHECK_CATALOG: readonly CheckSpec[] = [ gate('tmpdir-leaks', 'Leaked test tmpdir detector', 'check:tmpdir-leaks'), gate('tmpdir-leaks-model', 'TMPDIR redirection model', 'check:tmpdir-leaks:test'), gate('coverage-model', 'Changed-line coverage model', 'check:coverage-changed:test'), - gate('pr-evidence-model', 'PR evidence composer model', 'pr:evidence:test'), gate('wire-compat-model', 'Wire-compat rules model', 'check:daemon-wire-compat:test'), gate('production-exports', 'Production-unused exports', 'check:production-exports'), gate('bundle-owner-files', 'Bundle owner-file manifest', 'check:bundle-owner-files'), diff --git a/scripts/check-affected/model.ts b/scripts/check-affected/model.ts index b3573a9d0..c40480e70 100644 --- a/scripts/check-affected/model.ts +++ b/scripts/check-affected/model.ts @@ -59,7 +59,6 @@ export type CheckId = | 'tmpdir-leaks' | 'tmpdir-leaks-model' | 'coverage-model' - | 'pr-evidence-model' | 'wire-compat-model' | 'production-exports' | 'bundle-owner-files' @@ -119,7 +118,6 @@ export const ALL_CHECKS: readonly CheckId[] = [ 'tmpdir-leaks', 'tmpdir-leaks-model', 'coverage-model', - 'pr-evidence-model', 'wire-compat-model', 'production-exports', 'bundle-owner-files', diff --git a/scripts/pr-evidence/model.test.ts b/scripts/pr-evidence/model.test.ts deleted file mode 100644 index 5f7a4acd5..000000000 --- a/scripts/pr-evidence/model.test.ts +++ /dev/null @@ -1,171 +0,0 @@ -import assert from 'node:assert/strict'; -import { test } from 'node:test'; -import { - coverageSummary, - depgraphFacts, - groupChangedFiles, - parseLayeringReport, - renderEvidence, - sizeSummary, - type EvidenceInputs, -} from './model.ts'; - -const HEAD = 'b03379a55baa9f6da5d863e6dcfccdcfef975f5c'; -const BASE = '9a0d6dead229fc82e7e61064f674f963197afc4b'; - -function inputs(overrides: Partial = {}): EvidenceInputs { - return { - generatedAt: '2026-08-18T14:35:58.105Z', - repository: 'callstack/agent-device', - git: { - branch: 'feat/x', - head: HEAD, - headShort: HEAD.slice(0, 9), - base: BASE, - baseRef: 'origin/main', - baseShort: BASE.slice(0, 9), - dirty: false, - changedFiles: ['src/daemon/a.ts', 'src/daemon/b.ts', 'docs/agents/testing.md', 'AGENTS.md'], - }, - affected: { - failOpen: false, - failOpenReasons: [], - checks: [ - { id: 'typecheck', localRunnable: true, ciJobs: ['Lint & Format'] }, - { id: 'unit', localRunnable: true, ciJobs: ['Unit'] }, - { id: 'swift-runner-ios', localRunnable: false, ciJobs: ['iOS'] }, - ], - }, - layering: { ok: true, violationsByRule: {} }, - depgraph: { - head: { - files: 1312, - edges: 5559, - typeInversions: 7, - daemonToPlatforms: { count: 62, valueCount: 43 }, - }, - base: { - files: 1311, - edges: 5556, - typeInversions: 7, - daemonToPlatforms: { count: 64, valueCount: 45 }, - }, - }, - coverage: { kind: 'skipped', reason: 'pass --coverage' }, - size: { kind: 'skipped', reason: 'pass --size' }, - ...overrides, - }; -} - -test('changed files group by top-level area, largest first, root files under (root)', () => { - assert.deepEqual( - [...groupChangedFiles(['src/a.ts', 'docs/b.md', 'src/c.ts', 'AGENTS.md', 'scripts/d.ts'])], - [ - ['src', 2], - ['(root)', 1], - ['docs', 1], - ['scripts', 1], - ], - ); -}); - -test('the layering report parses per-rule counts and takes OK from the exit code', () => { - const red = parseLayeringReport( - 'Layering guard: 2 violation(s)\n\n [R9 type-cycle-size] 1 violation(s):\n::error …\n [R10 daemon-modularity] 1 violation(s):\n', - 1, - ); - assert.deepEqual(red, { - ok: false, - violationsByRule: { 'R9 type-cycle-size': 1, 'R10 daemon-modularity': 1 }, - }); - assert.deepEqual(parseLayeringReport('Layering guard: OK — 1312 source files …\n', 0), { - ok: true, - violationsByRule: {}, - }); -}); - -test('depgraph facts read the counts and the daemon→platforms zone edge', () => { - assert.deepEqual( - depgraphFacts({ - generated: { files: 3, edges: 4 }, - zoneEdges: [ - { from: 'daemon-server', to: 'contracts', count: 9, valueCount: 5 }, - { from: 'daemon-server', to: 'platforms', count: 62, valueCount: 43 }, - ], - typeInversions: { 'commands -> client': 3, 'core -> daemon-server': 2 }, - }), - { files: 3, edges: 4, typeInversions: 5, daemonToPlatforms: { count: 62, valueCount: 43 } }, - ); - assert.equal( - depgraphFacts({ generated: { files: 1, edges: 0 }, zoneEdges: [], typeInversions: {} }) - .daemonToPlatforms, - undefined, - ); -}); - -test('the block is stamped with full base and head SHAs and reports deltas against base', () => { - const block = renderEvidence(inputs()); - assert.match(block, new RegExp(`^\n`)); - assert.match(block, /at `b03379a55` \(`feat\/x`\) against `origin\/main` @ `9a0d6dead`/); - assert.match(block, /Changed: 4 files \(2 src, 1 \(root\), 1 docs\)/); - assert.match( - block, - /3 selected · local: typecheck, unit · GitHub-authoritative: swift-runner-ios/, - ); - assert.match( - block, - /Layering guard: OK · graph 1312 files \(\+1 vs base\), 5559 edges \(\+3 vs base\), type inversions 7 \(±0\) · daemon→platforms 62 total \(-2 vs base\) \/ 43 value \(-2 vs base\)/, - ); - assert.match(block, /Coverage: not measured \(pass --coverage\)/); - assert.match(block, /Size: not measured \(pass --size\)/); - assert.match(block, new RegExp(`commit/${HEAD}/checks$`, 'm')); - assert.doesNotMatch(block, /dirty/); -}); - -test('a dirty tree is called out and a fail-open plan is summarized, not enumerated', () => { - const block = renderEvidence( - inputs({ - git: { ...inputs().git, dirty: true }, - affected: { - failOpen: true, - failOpenReasons: [ - { path: 'scripts/x.ts', rule: 'workflow-tooling' }, - { path: 'scripts/y.ts', rule: 'workflow-tooling' }, - ], - checks: [ - { id: 'a', localRunnable: true, ciJobs: [] }, - { id: 'b', localRunnable: false, ciJobs: [] }, - ], - }, - layering: { ok: false, violationsByRule: { 'R9 type-cycle-size': 1 } }, - depgraph: { ...inputs().depgraph, base: undefined }, - }), - ); - assert.match( - block, - /working tree has uncommitted\/untracked changes: none of them are in this block/, - ); - assert.match( - block, - /fail-open \(workflow-tooling\): full set, 1 local \+ 1 GitHub-authoritative/, - ); - assert.doesNotMatch(block, /local: a/); - assert.match(block, /Layering guard: R9 type-cycle-size ×1 · graph 1312 files, 5559 edges,/); -}); - -test('coverage and size summaries lift one line out of the tools’ own markdown', () => { - assert.equal( - coverageSummary( - '## Changed-line coverage gate: PASS\n\n| Metric | Value |\n| --- | --- |\n| Changed-line coverage (gating, threshold 80%) | 24/26 (92.31%) |\n', - ), - '24/26 (92.31%), threshold 80% — PASS', - ); - assert.equal( - sizeSummary( - '| Metric | Base | Current | Diff |\n|---|---:|---:|---:|\n| JS raw | 2.30 MB | 2.30 MB | +99 B |\n| JS gzip | 756.3 kB | 756.4 kB | +50 B |\n', - ), - 'JS gzip 756.4 kB (+50 B vs base)', - ); - assert.equal(coverageSummary('nothing'), 'report present, no gating row found'); - assert.equal(sizeSummary('nothing'), 'report present, no JS gzip row found'); -}); diff --git a/scripts/pr-evidence/model.ts b/scripts/pr-evidence/model.ts deleted file mode 100644 index 3a0c8587b..000000000 --- a/scripts/pr-evidence/model.ts +++ /dev/null @@ -1,171 +0,0 @@ -// Pure composition for `pnpm pr:evidence`: every input is something an existing tool already -// produced (the affected selector's JSON, the depgraph report, the layering guard's report, the -// coverage gate's table, the size report). This module turns them into one paste-ready block -// stamped with the exact base and head, and never measures anything itself. - -export type GitFacts = Readonly<{ - branch: string; - head: string; - headShort: string; - base: string; - baseRef: string; - baseShort: string; - /** Uncommitted or untracked changes exist; everything measured here is the head, not them. */ - dirty: boolean; - changedFiles: readonly string[]; -}>; - -export type AffectedPlan = Readonly<{ - failOpen: boolean; - failOpenReasons: readonly Readonly<{ path: string; rule: string }>[]; - checks: readonly Readonly<{ id: string; localRunnable: boolean; ciJobs: readonly string[] }>[]; -}>; - -export type DepgraphFacts = Readonly<{ - files: number; - edges: number; - typeInversions: number; - daemonToPlatforms: Readonly<{ count: number; valueCount: number }> | undefined; -}>; - -export type LayeringOutcome = Readonly<{ - ok: boolean; - /** `R9 type-cycle-size` → 1, from the guard's own per-rule lines. */ - violationsByRule: Readonly>; -}>; - -export type EvidenceInputs = Readonly<{ - generatedAt: string; - repository: string; - git: GitFacts; - affected: AffectedPlan; - layering: LayeringOutcome; - depgraph: Readonly<{ head: DepgraphFacts; base: DepgraphFacts | undefined }>; - /** Markdown the coverage gate printed, or the reason it was not run. */ - coverage: Readonly<{ kind: 'table'; markdown: string } | { kind: 'skipped'; reason: string }>; - /** Markdown the size report printed, or the reason it was not run. */ - size: Readonly<{ kind: 'table'; markdown: string } | { kind: 'skipped'; reason: string }>; -}>; - -/** Top-level area of a repo path: `src/daemon/x.ts` → `src`, `docs/agents/y.md` → `docs`. */ -export function groupChangedFiles(paths: readonly string[]): ReadonlyMap { - const groups = new Map(); - for (const file of paths) { - const slash = file.indexOf('/'); - const area = slash === -1 ? '(root)' : file.slice(0, slash); - groups.set(area, (groups.get(area) ?? 0) + 1); - } - return new Map([...groups].sort((a, b) => b[1] - a[1] || a[0].localeCompare(b[0]))); -} - -/** The guard prints ` [R9 type-cycle-size] 1 violation(s):` per rule; the OK run prints none. */ -export function parseLayeringReport(output: string, exitCode: number): LayeringOutcome { - const violationsByRule: Record = {}; - for (const match of output.matchAll(/^\s*\[([^\]]+)\] (\d+) violation\(s\):/gm)) { - violationsByRule[match[1] as string] = Number(match[2]); - } - return { ok: exitCode === 0, violationsByRule }; -} - -/** Reads the counts this block reports out of the depgraph JSON report. */ -export function depgraphFacts(report: { - generated: { files: number; edges: number }; - zoneEdges: readonly { from: string; to: string; count: number; valueCount: number }[]; - typeInversions: Record; -}): DepgraphFacts { - const daemonToPlatforms = report.zoneEdges.find( - (edge) => edge.from === 'daemon-server' && edge.to === 'platforms', - ); - return { - files: report.generated.files, - edges: report.generated.edges, - typeInversions: Object.values(report.typeInversions).reduce((sum, n) => sum + n, 0), - daemonToPlatforms: daemonToPlatforms - ? { count: daemonToPlatforms.count, valueCount: daemonToPlatforms.valueCount } - : undefined, - }; -} - -function delta(head: number, base: number | undefined): string { - if (base === undefined) return ''; - const diff = head - base; - return diff === 0 ? ' (±0)' : ` (${diff > 0 ? '+' : ''}${diff} vs base)`; -} - -function bullet(text: string): string { - return `- ${text}`; -} - -export function renderEvidence(inputs: EvidenceInputs): string { - const { git, affected, layering, depgraph } = inputs; - const areas = [...groupChangedFiles(git.changedFiles)] - .map(([area, count]) => `${count} ${area}`) - .join(', '); - const local = affected.checks.filter((check) => check.localRunnable).map((check) => check.id); - const remote = affected.checks.filter((check) => !check.localRunnable).map((check) => check.id); - // A fail-open plan is the whole catalog; naming every id would only bury the reason. - const affectedLine = affected.failOpen - ? `fail-open (${[...new Set(affected.failOpenReasons.map((r) => r.rule))].join(', ')}): ` + - `full set, ${local.length} local + ${remote.length} GitHub-authoritative` - : `${affected.checks.length} selected` + - (local.length > 0 ? ` · local: ${local.join(', ')}` : '') + - (remote.length > 0 ? ` · GitHub-authoritative: ${remote.join(', ')}` : ''); - const layeringLine = layering.ok - ? 'Layering guard: OK' - : `Layering guard: ${Object.entries(layering.violationsByRule) - .map(([rule, count]) => `${rule} ×${count}`) - .join(', ')}`; - const head = depgraph.head; - const base = depgraph.base; - const daemonEdges = head.daemonToPlatforms - ? `daemon→platforms ${head.daemonToPlatforms.count} total${delta(head.daemonToPlatforms.count, base?.daemonToPlatforms?.count)}` + - ` / ${head.daemonToPlatforms.valueCount} value${delta(head.daemonToPlatforms.valueCount, base?.daemonToPlatforms?.valueCount)}` - : 'daemon→platforms edges: none'; - - const lines = [ - ``, - `**Evidence** gathered ${inputs.generatedAt} at \`${git.headShort}\` (\`${git.branch}\`) against \`${git.baseRef}\` @ \`${git.baseShort}\`` + - (git.dirty - ? ' — **working tree has uncommitted/untracked changes: none of them are in this block**' - : ''), - bullet(`Changed: ${git.changedFiles.length} files (${areas || 'none'})`), - bullet(`Affected gates (\`check:affected\`): ${affectedLine}`), - bullet( - `${layeringLine} · graph ${head.files} files${delta(head.files, base?.files)}, ` + - `${head.edges} edges${delta(head.edges, base?.edges)}, ` + - `type inversions ${head.typeInversions}${delta(head.typeInversions, base?.typeInversions)} · ${daemonEdges}`, - ), - bullet( - inputs.coverage.kind === 'table' - ? `Changed-line coverage: ${coverageSummary(inputs.coverage.markdown)}` - : `Coverage: not measured (${inputs.coverage.reason})`, - ), - bullet( - inputs.size.kind === 'table' - ? `Size: ${sizeSummary(inputs.size.markdown)}` - : `Size: not measured (${inputs.size.reason})`, - ), - bullet( - `CI on this head (authoritative, not claimed here): https://github.com/${inputs.repository}/commit/${git.head}/checks`, - ), - ]; - return `${lines.join('\n')}\n`; -} - -/** `| Changed-line coverage (gating, threshold 80%) | 24/26 (92.31%) |` → `24/26 (92.31%), threshold 80%`. */ -export function coverageSummary(markdown: string): string { - const row = markdown.split('\n').find((line) => line.startsWith('| Changed-line coverage')); - if (!row) return 'report present, no gating row found'; - const cells = row.split('|').map((cell) => cell.trim()); - const threshold = /threshold (\d+%)/.exec(cells[1] ?? '')?.[1]; - const verdict = /gate: (\w+)/.exec(markdown)?.[1] ?? 'unknown'; - return `${cells[2] ?? '?'}${threshold ? `, threshold ${threshold}` : ''} — ${verdict}`; -} - -/** Pulls the JS gzip row's current value and diff out of the size report table. */ -export function sizeSummary(markdown: string): string { - const row = markdown.split('\n').find((line) => line.startsWith('| JS gzip')); - if (!row) return 'report present, no JS gzip row found'; - const cells = row.split('|').map((cell) => cell.trim()); - return `JS gzip ${cells[3] ?? '?'} (${cells[4] ?? '?'} vs base)`; -} diff --git a/scripts/pr-evidence/run.test.ts b/scripts/pr-evidence/run.test.ts deleted file mode 100644 index d708fb272..000000000 --- a/scripts/pr-evidence/run.test.ts +++ /dev/null @@ -1,53 +0,0 @@ -import assert from 'node:assert/strict'; -import fs from 'node:fs'; -import os from 'node:os'; -import path from 'node:path'; -import { test } from 'node:test'; -import { runCmd, runCmdSync } from '../../src/utils/exec.ts'; - -const REPOSITORY_ROOT = path.resolve(import.meta.dirname, '..', '..'); -const RUN = path.join(REPOSITORY_ROOT, 'scripts', 'pr-evidence', 'run.ts'); - -function scratchDirectories(): string[] { - return fs.readdirSync(os.tmpdir()).filter((name) => name.startsWith('agent-device-pr-evidence-')); -} - -function evidenceWorktrees(): string[] { - return runCmdSync('git', ['worktree', 'list', '--porcelain'], { cwd: REPOSITORY_ROOT }) - .stdout.split('\n') - .filter((line) => line.startsWith('worktree ') && line.includes('agent-device-pr-evidence-')); -} - -// The real entrypoint, end to end, in this repository: `--base HEAD` makes the merge-base HEAD -// itself, so it needs no origin/main and no network (a depth-1 CI checkout is enough) while -// still creating both pristine worktrees, running the selector, the layering guard, and the -// depgraph twice, and rendering the block. It is the regression for the fresh-checkout failure -// (scratch used to be created under an untracked, possibly absent `.tmp/`) and for cleanup. -test('pr:evidence runs end to end from a pristine head worktree and cleans up after itself', async () => { - const before = { scratch: scratchDirectories(), worktrees: evidenceWorktrees() }; - const result = await runCmd( - process.execPath, - ['--experimental-strip-types', RUN, '--base', 'HEAD', '--json'], - { cwd: REPOSITORY_ROOT, timeoutMs: 300_000 }, - ); - const inputs = JSON.parse(result.stdout) as { - git: { head: string; base: string; changedFiles: string[]; dirty: boolean }; - affected: { checks: unknown[] }; - layering: { ok: boolean }; - depgraph: { head: { files: number; edges: number }; base: { files: number; edges: number } }; - coverage: { kind: string }; - size: { kind: string }; - }; - const head = runCmdSync('git', ['rev-parse', 'HEAD'], { cwd: REPOSITORY_ROOT }).stdout.trim(); - assert.equal(inputs.git.head, head); - assert.equal(inputs.git.base, head, '--base HEAD makes the merge-base the head itself'); - assert.deepEqual(inputs.git.changedFiles, []); - assert.ok(inputs.depgraph.head.files > 500, 'the head worktree was analyzed, not an empty tree'); - assert.deepEqual(inputs.depgraph.base, inputs.depgraph.head, 'same commit, same numbers'); - assert.equal(typeof inputs.layering.ok, 'boolean'); - assert.equal(inputs.coverage.kind, 'skipped'); - assert.equal(inputs.size.kind, 'skipped'); - // Both worktrees and the os.tmpdir() scratch are gone, whatever else was there before. - assert.deepEqual(scratchDirectories(), before.scratch); - assert.deepEqual(evidenceWorktrees(), before.worktrees); -}); diff --git a/scripts/pr-evidence/run.ts b/scripts/pr-evidence/run.ts deleted file mode 100644 index 862843312..000000000 --- a/scripts/pr-evidence/run.ts +++ /dev/null @@ -1,201 +0,0 @@ -// `pnpm pr:evidence [--base ] [--coverage] [--size] [--json]` -// -// One paste-ready evidence block for a PR body, stamped with the exact base and head, composed -// from the tools the repo already has: the affected selector (`check:affected --json`), the -// layering guard, the depgraph report, and — behind flags, because they need a build or a -// coverage run — the changed-line coverage gate and `pnpm size --base`. It measures nothing -// itself and claims nothing about CI: the last line is the link to the head's checks. -// -// Everything labelled "at " is measured from a throwaway `git worktree` of that exact -// commit, and the base likewise, so an untracked or uncommitted file in the working tree can -// change nothing the block reports as HEAD's; the working tree only contributes the "dirty" -// flag. The worktrees need no install: the scripts analyze whichever repository their cwd is -// in, while their imports resolve from this checkout. -// -// The default tier finishes in ~20s (the layering guard is most of it). `--coverage` reads the -// existing coverage/lcov.info (run `pnpm test:coverage` first); `--size` runs the base worktree -// build the first time (~1-2 min) and ~3s afterwards. - -import fs from 'node:fs'; -import os from 'node:os'; -import path from 'node:path'; -import { pathToFileURL } from 'node:url'; -import { runCmd, runCmdSync } from '../../src/utils/exec.ts'; -import { parseScriptArgs } from '../lib/cli-args.ts'; -import { runEntrypoint } from '../lib/cli-entrypoint.ts'; -import { - depgraphFacts, - parseLayeringReport, - renderEvidence, - type AffectedPlan, - type DepgraphFacts, - type EvidenceInputs, - type GitFacts, -} from './model.ts'; -import { withWorktrees } from './worktrees.ts'; - -const USAGE = - 'Usage: pnpm pr:evidence [--base ] [--coverage] [--size] [--json]\n' + - ' --base Base ref (default origin/main); the block uses its merge-base with HEAD\n' + - ' --coverage Include changed-line coverage from coverage/lcov.info (run pnpm test:coverage first)\n' + - ' --size Include the JS size delta (pnpm size --base ; needs pnpm build)\n' + - ' --json Emit the collected inputs as JSON instead of the markdown block\n'; - -const REPOSITORY = 'callstack/agent-device'; -const repoRoot = runCmdSync('git', ['rev-parse', '--show-toplevel']).stdout.trim(); -const scripts = path.join(repoRoot, 'scripts'); - -function git(args: readonly string[], cwd = repoRoot): string { - return runCmdSync('git', [...args], { cwd }).stdout.trim(); -} - -function collectGitFacts(baseRef: string): GitFacts { - const head = git(['rev-parse', 'HEAD']); - const base = git(['merge-base', baseRef, 'HEAD']); - const branch = git(['rev-parse', '--abbrev-ref', 'HEAD']); - // Untracked files count: they are exactly what a pristine-worktree measurement leaves out. - const dirty = git(['status', '--porcelain']).length > 0; - const changedFiles = git(['diff', '--name-only', '--no-renames', `${base}..HEAD`]) - .split('\n') - .filter(Boolean); - return { - branch, - head, - headShort: head.slice(0, 9), - base, - baseRef, - baseShort: base.slice(0, 9), - dirty, - changedFiles, - }; -} - -// The head SHA, not the literal `HEAD`: the selector folds working-tree changes into a plan for -// `HEAD`, and this block describes the commit. -async function collectAffected(base: string, head: string): Promise { - const result = await runCmd( - process.execPath, - [ - '--experimental-strip-types', - path.join(scripts, 'check-affected', 'run.ts'), - '--base', - base, - '--head', - head, - '--json', - ], - { cwd: repoRoot, timeoutMs: 120_000 }, - ); - const parsed = JSON.parse(result.stdout) as AffectedPlan; - return { - failOpen: parsed.failOpen, - failOpenReasons: parsed.failOpenReasons, - checks: parsed.checks.map(({ id, localRunnable, ciJobs }) => ({ id, localRunnable, ciJobs })), - }; -} - -async function collectLayering(cwd: string) { - const result = await runCmd( - process.execPath, - ['--experimental-strip-types', path.join(scripts, 'layering', 'check.ts')], - { cwd, timeoutMs: 300_000, allowFailure: true }, - ); - return parseLayeringReport(`${result.stdout}\n${result.stderr}`, result.exitCode); -} - -async function collectDepgraph(cwd: string, out: string): Promise { - await runCmd( - process.execPath, - ['--experimental-strip-types', path.join(scripts, 'depgraph', 'build.ts'), '--out', out], - { cwd, timeoutMs: 300_000 }, - ); - return depgraphFacts(JSON.parse(fs.readFileSync(out, 'utf8'))); -} - -async function collectCoverage(base: string): Promise { - if (!fs.existsSync(path.join(repoRoot, 'coverage', 'lcov.info'))) { - return { kind: 'skipped', reason: 'no coverage/lcov.info — run pnpm test:coverage first' }; - } - const result = await runCmd( - process.execPath, - [ - '--experimental-strip-types', - path.join(scripts, 'coverage-changed', 'run.ts'), - '--base', - base, - ], - { cwd: repoRoot, timeoutMs: 300_000, allowFailure: true }, - ); - return { kind: 'table', markdown: result.stdout }; -} - -async function collectSize(base: string): Promise { - if (!fs.existsSync(path.join(repoRoot, 'dist', 'src'))) { - return { kind: 'skipped', reason: 'no dist/src — run pnpm build first' }; - } - const result = await runCmd( - process.execPath, - [path.join(scripts, 'size-report.mjs'), '--base', base], - { cwd: repoRoot, timeoutMs: 600_000 }, - ); - return { kind: 'table', markdown: result.stdout }; -} - -/** The two report tiers that need a build or a coverage run stay opt-in; the block says so. */ -async function optional( - enabled: boolean | undefined, - flag: string, - collect: () => Promise, -): Promise> { - return enabled ? await collect() : { kind: 'skipped', reason: `pass ${flag}` }; -} - -async function main(argv: readonly string[]): Promise { - const values = parseScriptArgs(argv, USAGE, { - base: { type: 'string', default: 'origin/main' }, - coverage: { type: 'boolean', default: false }, - size: { type: 'boolean', default: false }, - json: { type: 'boolean', default: false }, - }); - const baseRef = values.base ?? 'origin/main'; - const gitFacts = collectGitFacts(baseRef); - // os.tmpdir() always exists; a repo-local scratch would have to be created first and is one - // more thing a fresh checkout can lack. - const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-pr-evidence-')); - return await withWorktrees( - repoRoot, - scratch, - [ - { name: 'head', commit: gitFacts.head }, - { name: 'base', commit: gitFacts.base }, - ], - async ([headTree, baseTree]) => { - const [affected, layering, head, base] = await Promise.all([ - collectAffected(gitFacts.base, gitFacts.head), - collectLayering(headTree), - collectDepgraph(headTree, path.join(scratch, 'depgraph-head.json')), - collectDepgraph(baseTree, path.join(scratch, 'depgraph-base.json')), - ]); - const inputs: EvidenceInputs = { - generatedAt: new Date().toISOString(), - repository: REPOSITORY, - git: gitFacts, - affected, - layering, - depgraph: { head, base }, - coverage: await optional(values.coverage, '--coverage', () => - collectCoverage(gitFacts.base), - ), - size: await optional(values.size, '--size', () => collectSize(gitFacts.base)), - }; - process.stdout.write( - values.json ? `${JSON.stringify(inputs, null, 2)}\n` : renderEvidence(inputs), - ); - return 0; - }, - ); -} - -if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { - runEntrypoint('pr-evidence', () => main(process.argv.slice(2))); -} diff --git a/scripts/pr-evidence/worktrees.test.ts b/scripts/pr-evidence/worktrees.test.ts deleted file mode 100644 index 73e45947c..000000000 --- a/scripts/pr-evidence/worktrees.test.ts +++ /dev/null @@ -1,119 +0,0 @@ -import assert from 'node:assert/strict'; -import fs from 'node:fs'; -import os from 'node:os'; -import path from 'node:path'; -import { test } from 'node:test'; -import { runCmdSync } from '../../src/utils/exec.ts'; -import { withWorktrees } from './worktrees.ts'; - -// A throwaway repository with one commit; every case below plants a failure somewhere in the -// add → run → cleanup sequence and asserts that nothing the helper created outlives it. - -function makeRepo(): { repo: string; commit: string } { - const root = fs.mkdtempSync(path.join(os.tmpdir(), 'pr-evidence-worktrees-')); - const repo = path.join(root, 'repo'); - fs.mkdirSync(repo); - const git = (args: string[]) => runCmdSync('git', args, { cwd: repo }).stdout.trim(); - git(['init', '-q', '-b', 'main']); - git(['config', 'user.email', 'wt@test']); - git(['config', 'user.name', 'wt']); - fs.writeFileSync(path.join(repo, 'file'), 'x\n'); - git(['add', '.']); - git(['commit', '-q', '-m', 'one']); - return { repo, commit: git(['rev-parse', 'HEAD']) }; -} - -function registered(repo: string): string[] { - return runCmdSync('git', ['worktree', 'list', '--porcelain'], { cwd: repo }) - .stdout.split('\n') - .filter((line) => line.startsWith('worktree ')) - .map((line) => line.slice('worktree '.length)) - .filter((dir) => dir !== fs.realpathSync(repo)); -} - -test('worktrees are created in order, handed over as a tuple, and removed with the scratch after success', async () => { - const { repo, commit } = makeRepo(); - const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'pr-evidence-scratch-')); - const seen = await withWorktrees( - repo, - scratch, - [ - { name: 'head', commit }, - { name: 'base', commit }, - ], - async ([head, base]) => { - assert.ok(fs.existsSync(path.join(head, 'file'))); - assert.ok(fs.existsSync(path.join(base, 'file'))); - assert.equal(registered(repo).length, 2); - return [path.basename(head), path.basename(base)]; - }, - ); - assert.deepEqual(seen, ['head', 'base']); - assert.deepEqual(registered(repo), []); - assert.equal(fs.existsSync(scratch), false); -}); - -test('a second add that fails leaks nothing: the first worktree is already registered and gets removed', async () => { - const { repo, commit } = makeRepo(); - const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'pr-evidence-scratch-')); - await assert.rejects( - withWorktrees( - repo, - scratch, - [ - { name: 'head', commit }, - { name: 'base', commit: 'not-a-commit' }, - ], - async () => { - throw new Error('fn must not run when an add failed'); - }, - ), - /git exited with code 128/, - ); - assert.deepEqual(registered(repo), [], 'the successful first add was cleaned up'); - assert.equal(fs.existsSync(scratch), false); -}); - -test('a throwing fn still gets every worktree and the scratch removed', async () => { - const { repo, commit } = makeRepo(); - const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'pr-evidence-scratch-')); - await assert.rejects( - withWorktrees(repo, scratch, [{ name: 'only', commit }], async () => { - throw new Error('measurement failed'); - }), - /measurement failed/, - ); - assert.deepEqual(registered(repo), []); - assert.equal(fs.existsSync(scratch), false); -}); - -test('one cleanup failure never skips the remaining resources, and every failure is reported', async () => { - const { repo, commit } = makeRepo(); - const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'pr-evidence-scratch-')); - const removed: string[] = []; - await assert.rejects( - withWorktrees( - repo, - scratch, - [ - { name: 'first', commit }, - { name: 'second', commit }, - { name: 'third', commit }, - ], - async () => 'ok', - (root, worktree) => { - if (worktree.endsWith('second')) throw new Error('planted removal failure'); - runCmdSync('git', ['worktree', 'remove', '--force', worktree], { cwd: root }); - removed.push(path.basename(worktree)); - }, - ), - (error: unknown) => { - assert.ok(error instanceof Error); - assert.match(error.message, /cleanup left resources behind/); - assert.match(error.message, /second: planted removal failure/); - return true; - }, - ); - assert.deepEqual(removed, ['first', 'third'], 'the failure in the middle skipped nothing'); - assert.equal(fs.existsSync(scratch), false, 'the scratch was still attempted'); -}); diff --git a/scripts/pr-evidence/worktrees.ts b/scripts/pr-evidence/worktrees.ts deleted file mode 100644 index c25a0ca37..000000000 --- a/scripts/pr-evidence/worktrees.ts +++ /dev/null @@ -1,76 +0,0 @@ -// Throwaway `git worktree` checkouts for pr:evidence, with two guarantees the runner leans on: -// every worktree is registered for cleanup the moment its `add` succeeds (a later add failing -// leaks nothing), and cleanup is exhaustive — one resource's failure to be removed never skips -// the rest, and every failure is reported after the last one was attempted. - -import fs from 'node:fs'; -import path from 'node:path'; -import { runCmdSync } from '../../src/utils/exec.ts'; - -export type WorktreeSpec = Readonly<{ name: string; commit: string }>; - -function git(cwd: string, args: readonly string[]): void { - runCmdSync('git', [...args], { cwd }); -} - -/** - * Creates the requested worktrees under `scratch`, runs `fn` with their paths (in spec order), - * and removes every worktree that was created plus `scratch` itself, whether `fn` or a later - * `add` threw. Cleanup errors are collected and thrown together after every resource was tried. - */ -export async function withWorktrees( - repoRoot: string, - scratch: string, - specs: Specs, - fn: (paths: { readonly [Index in keyof Specs]: string }) => Promise, - removeWorktree: (repoRoot: string, worktree: string) => void = defaultRemoveWorktree, -): Promise { - const created: string[] = []; - let outcome: { ok: true; value: T } | { ok: false; error: unknown }; - try { - for (const spec of specs) { - const worktree = path.join(scratch, spec.name); - git(repoRoot, ['worktree', 'add', '--detach', worktree, spec.commit]); - created.push(worktree); // registered before the next add can fail - } - // One path per spec, in order: the tuple type mirrors `specs` so callers destructure safely. - outcome = { - ok: true, - value: await fn(created as unknown as { readonly [Index in keyof Specs]: string }), - }; - } catch (error) { - outcome = { ok: false, error }; - } - const failures = cleanUp(repoRoot, created, scratch, removeWorktree); - if (failures.length > 0) { - throw new Error(`pr-evidence cleanup left resources behind:\n${failures.join('\n')}`, { - cause: outcome.ok ? undefined : outcome.error, - }); - } - if (!outcome.ok) throw outcome.error; - return outcome.value; -} - -/** Every resource is attempted; the failures come back together instead of aborting the sweep. */ -function cleanUp( - repoRoot: string, - worktrees: readonly string[], - scratch: string, - removeWorktree: (repoRoot: string, worktree: string) => void, -): string[] { - const failures: string[] = []; - const attempt = (label: string, action: () => void) => { - try { - action(); - } catch (error) { - failures.push(`${label}: ${error instanceof Error ? error.message : String(error)}`); - } - }; - for (const worktree of worktrees) attempt(worktree, () => removeWorktree(repoRoot, worktree)); - attempt(scratch, () => fs.rmSync(scratch, { recursive: true, force: true })); - return failures; -} - -function defaultRemoveWorktree(repoRoot: string, worktree: string): void { - git(repoRoot, ['worktree', 'remove', '--force', worktree]); -} diff --git a/scripts/size-base-cache.mjs b/scripts/size-base-cache.mjs deleted file mode 100644 index 1d92c1d6d..000000000 --- a/scripts/size-base-cache.mjs +++ /dev/null @@ -1,277 +0,0 @@ -// Ownership protocol for the `pnpm size --base ` worktree cache. -// -// The cache is `.tmp/size-base//` — a detached worktree of the base commit, built once -// and reused — plus, per entry, a *claim* that says which run is currently using it. Two runs on -// one machine must never build the same entry at once, read a half-built `dist`, or delete an -// entry another run is using. -// -// A claim is a symlink whose target is the owning run's identity (`:`): -// - `symlink()` creates it with its identity already in place — one syscall, so there is no -// window where a claim exists without an owner — and fails EEXIST while another run holds it. -// - A claim whose owning pid is gone is *abandoned*. Removing one is the only dangerous step in -// the protocol: between observing an abandoned claim and unlinking it, another run could have -// removed it and taken the entry, and the unlink would then delete that live claim. So -// removal happens only while holding the entry's takeover mutex — an atomically created -// directory — and re-verifies the claim inside it. A replacement claim can appear only by -// creating one on a free path (impossible: the abandoned claim occupies it until we unlink) -// or by another takeover (impossible: that needs this mutex). Removal therefore cannot -// delete a replacement. -// - Release unlinks only a claim that still names this run, under the same mutex. -// -// The mutex has exactly one holder for its whole life: it is never reclaimed by age or by any -// other guess about the holder, because a second holder would restore the split ownership the -// mutex removes. See enterTakeover for what that costs and why it is the safe direction. - -import { randomUUID } from 'node:crypto'; -import { execFileSync } from 'node:child_process'; -import fs from 'node:fs'; -import path from 'node:path'; - -/** This run's claim identity: pid for liveness, nonce so a reused pid is still a different run. */ -export const CLAIM_IDENTITY = `${process.pid}:${randomUUID()}`; - -const CLAIM_ATTEMPTS = 8; - -export function claimPath(worktreeDir) { - return `${worktreeDir}.lock`; -} - -export function takeoverPath(worktreeDir) { - return `${worktreeDir}.takeover`; -} - -/** The stamp a finished build writes; its absence means "rebuild", never "trust dist/src". */ -function completionStampPath(worktreeDir) { - return path.join(worktreeDir, 'dist', '.size-base-complete'); -} - -export function readClaimIdentity(claim) { - try { - return fs.readlinkSync(claim); - } catch { - return undefined; - } -} - -function pidOfIdentity(identity) { - const pid = Number(String(identity).split(':')[0]); - return Number.isInteger(pid) && pid > 0 ? pid : undefined; -} - -function isProcessAlive(pid) { - if (pid === undefined) return false; - try { - process.kill(pid, 0); - return true; - } catch (error) { - return error.code === 'EPERM'; // exists, not signalable by us - } -} - -function tryCreateClaim(claim) { - try { - fs.symlinkSync(CLAIM_IDENTITY, claim); - return true; - } catch (error) { - if (error.code === 'EEXIST') return false; - throw error; - } -} - -// The takeover mutex is a symlink naming its holder, created in one syscall like a claim, and it -// is *never* force-reclaimed: an age-based reclamation would hand a second holder the section -// whenever the first is merely slow (a paused or SIGSTOPed process crosses any threshold), and -// two holders is exactly the split ownership the mutex exists to prevent. So there is at most one -// holder, ever, and release removes only a mutex that still names this run — it can neither -// force-remove nor release a replacement. -// -// The cost of never reclaiming is a mutex leaked by a process killed inside a critical section of -// three syscalls with no I/O between them. That wedges one cache entry, loudly and with the path -// to remove in the message, instead of silently deleting another run's live claim. -function enterTakeover(takeover) { - try { - fs.symlinkSync(CLAIM_IDENTITY, takeover); - return true; - } catch (error) { - if (error.code === 'EEXIST') return false; - throw error; - } -} - -function leaveTakeover(takeover) { - try { - if (fs.readlinkSync(takeover) !== CLAIM_IDENTITY) return; // someone else's: not ours to remove - fs.unlinkSync(takeover); - } catch { - // Already gone, or not a symlink we own: nothing this run may remove. - } -} - -/** - * Removes an abandoned claim, under the takeover mutex so it can never remove a replacement. - * Returns what it found: 'removed' | 'gone' | 'changed' | 'live' | 'busy'. - */ -export function removeAbandonedClaim(worktreeDir, observedIdentity) { - const claim = claimPath(worktreeDir); - const takeover = takeoverPath(worktreeDir); - if (!enterTakeover(takeover)) return 'busy'; - try { - let stats; - try { - stats = fs.lstatSync(claim); - } catch { - return 'gone'; - } - if (!stats.isSymbolicLink()) { - // Not a claim of this scheme at all (a stray file or directory): safe to clear here. - fs.rmSync(claim, { recursive: true, force: true }); - return 'removed'; - } - const current = fs.readlinkSync(claim); - if (current !== observedIdentity) return 'changed'; - if (isProcessAlive(pidOfIdentity(current))) return 'live'; - fs.unlinkSync(claim); - return 'removed'; - } finally { - leaveTakeover(takeover); - } -} - -function heldError(worktreeDir, holder, label) { - const pid = pidOfIdentity(holder); - return new Error( - `another \`size --base\` (pid ${pid ?? 'unknown'}) is using base ${label} in ${worktreeDir}; ` + - `wait for it, measure a different base, or remove ${claimPath(worktreeDir)} if that run is gone`, - ); -} - -/** - * Claims one cache entry for this run. Returns the release function; throws when another live - * run holds it, or when a takeover by another run keeps the claim contended. - */ -export function acquireBaseClaim(worktreeDir, label) { - for (let attempt = 0; attempt < CLAIM_ATTEMPTS; attempt += 1) { - const claim = claimPath(worktreeDir); - if (tryCreateClaim(claim) && readClaimIdentity(claim) === CLAIM_IDENTITY) { - return () => releaseBaseClaim(worktreeDir); - } - const holder = readClaimIdentity(claim); - if (holder !== undefined && isProcessAlive(pidOfIdentity(holder))) { - throw heldError(worktreeDir, holder, label); - } - const outcome = removeAbandonedClaim(worktreeDir, holder); - if (outcome === 'live') throw heldError(worktreeDir, readClaimIdentity(claim), label); - if (outcome === 'busy' && attempt === CLAIM_ATTEMPTS - 1) { - throw new Error( - `another run is taking over the abandoned claim on base ${label} in ${worktreeDir}; ` + - `retry shortly, or remove ${takeoverPath(worktreeDir)} if no other \`size --base\` is running`, - ); - } - } - throw new Error( - `could not claim base ${label} in ${worktreeDir} after ${CLAIM_ATTEMPTS} attempts`, - ); -} - -/** Releases this run's claim; a claim that has come to name someone else is left alone. */ -function releaseBaseClaim(worktreeDir) { - const claim = claimPath(worktreeDir); - const takeover = takeoverPath(worktreeDir); - if (!enterTakeover(takeover)) { - // Someone is mid-takeover of this entry; they re-verify identity, so they cannot remove ours - // while we still own it, and our claim is removed by the next run that finds it abandoned. - return; - } - try { - if (readClaimIdentity(claim) === CLAIM_IDENTITY) fs.unlinkSync(claim); - } finally { - leaveTakeover(takeover); - } -} - -function removeWorktree(root, dir) { - try { - execFileSync('git', ['worktree', 'remove', '--force', dir], { cwd: root, stdio: 'ignore' }); - } catch { - // Not a registered worktree (half-created, or hand-copied): plain removal. - } - fs.rmSync(dir, { recursive: true, force: true }); -} - -/** Creates the entry's worktree when it is missing or unregistered; otherwise leaves it. */ -function ensureBaseWorktree(root, worktreeDir, sha) { - const registered = execFileSync('git', ['worktree', 'list', '--porcelain'], { - cwd: root, - encoding: 'utf8', - }).includes(`worktree ${worktreeDir}\n`); - if (registered && fs.existsSync(worktreeDir)) return; - // Registered but gone (hand-deleted), or present but unregistered (hand-copied): start clean. - fs.rmSync(worktreeDir, { recursive: true, force: true }); - execFileSync('git', ['worktree', 'prune'], { cwd: root, stdio: 'ignore' }); - execFileSync('git', ['worktree', 'add', '--detach', worktreeDir, sha], { - cwd: root, - stdio: ['ignore', 'ignore', 'pipe'], - encoding: 'utf8', - }); -} - -/** - * Evicts every cache entry except `keep`, each under its own claim: an entry a live run holds is - * skipped, and one this run evicts cannot be adopted mid-removal. - */ -function pruneOtherBaseWorktrees(root, worktreesRoot, keep) { - const others = fs - .readdirSync(worktreesRoot, { withFileTypes: true }) - .filter((entry) => entry.isDirectory() && !entry.name.endsWith('.takeover')) - .map((entry) => path.join(worktreesRoot, entry.name)) - .filter((dir) => dir !== keep); - for (const dir of others) { - let release; - try { - release = acquireBaseClaim(dir, path.basename(dir)); - } catch { - continue; // in use by a live run, or contended: not ours to evict - } - try { - removeWorktree(root, dir); - } finally { - release(); - } - } -} - -/** - * Prepares (or reuses) the cache entry for `ref` under this run's claim and hands its worktree to - * `measure`. The install+build runs only when the entry carries no completion stamp, so an - * interrupted build is redone rather than trusted because `dist/src` happens to exist. - */ -export function withPreparedBaseWorktree(root, ref, measure) { - const sha = execFileSync('git', ['rev-parse', '--verify', `${ref}^{commit}`], { - cwd: root, - encoding: 'utf8', - }).trim(); - fs.mkdirSync(path.join(root, '.tmp', 'size-base'), { recursive: true }); - // Canonical: git lists worktrees by real path (/tmp is /private/tmp on macOS), and - // ensureBaseWorktree compares against that listing. - const worktreesRoot = fs.realpathSync(path.join(root, '.tmp', 'size-base')); - const worktreeDir = path.join(worktreesRoot, sha.slice(0, 12)); - const release = acquireBaseClaim(worktreeDir, sha.slice(0, 9)); - try { - pruneOtherBaseWorktrees(root, worktreesRoot, worktreeDir); - ensureBaseWorktree(root, worktreeDir, sha); - if (!fs.existsSync(completionStampPath(worktreeDir))) { - process.stderr.write( - `[size] measuring base ${sha.slice(0, 9)} (${ref}): install + build in ${worktreeDir}\n`, - ); - execFileSync('pnpm', ['install', '--frozen-lockfile', '--prefer-offline'], { - cwd: worktreeDir, - stdio: ['ignore', 'ignore', 'inherit'], - }); - execFileSync('pnpm', ['build'], { cwd: worktreeDir, stdio: ['ignore', 'ignore', 'inherit'] }); - fs.writeFileSync(completionStampPath(worktreeDir), `${sha}\n`); - } - return measure(worktreeDir); - } finally { - release(); - } -} diff --git a/scripts/size-report-comment.mjs b/scripts/size-report-comment.mjs deleted file mode 100644 index 863c855af..000000000 --- a/scripts/size-report-comment.mjs +++ /dev/null @@ -1,164 +0,0 @@ -// Publishing the size report as a PR comment — the convenience half of `scripts/size-report.mjs`, -// kept separate from measuring and formatting it. The same markdown always reaches the job -// summary, so this surface is best-effort by design: a GitHub outage must not fail the job, while -// a real misconfiguration still must. -// -// `scripts/__tests__/size-report-post-comment.test.ts` drives this through the real script with a -// stubbed fetch, pinning the retry, reconcile, and fatal outcomes. - -import fs from 'node:fs'; - -/** Identifies this job's comment so re-runs update one comment instead of appending new ones. */ -export const COMMENT_MARKER = ''; - -const GITHUB_REQUEST_ATTEMPTS = 4; -// Overridable so the regression tests do not sleep through real backoff. -const GITHUB_RETRY_BASE_MS = Number(process.env.SIZE_REPORT_RETRY_BASE_MS ?? 1000); - -class TransientGitHubError extends Error {} - -// The PR comment is a convenience surface: the same markdown is already in the -// job summary. A GitHub outage (5xx / 429 / network error) must not fail the -// job, but a real misconfiguration (bad token, missing permissions) still does. -export async function postGitHubCommentBestEffort(markdownPath, explicitPrNumber) { - try { - await postGitHubComment(markdownPath, explicitPrNumber); - } catch (error) { - if (!(error instanceof TransientGitHubError)) throw error; - const message = `Skipping PR size comment after transient GitHub failure: ${error.message}`; - process.stdout.write(`::warning::${message}\n`); - appendStepSummary(`> ⚠️ ${message} The size report above is authoritative.\n`); - } -} - -function appendStepSummary(text) { - const summaryPath = process.env.GITHUB_STEP_SUMMARY; - if (summaryPath) fs.appendFileSync(summaryPath, text); -} - -async function postGitHubComment(markdownPath, explicitPrNumber) { - const config = readGitHubCommentConfig(explicitPrNumber); - const body = fs.readFileSync(markdownPath, 'utf8'); - const commentsUrl = buildCommentsUrl(config.repository, config.prNumber); - await retryTransient(() => syncGitHubComment(commentsUrl, config.headers, body)); -} - -// Every attempt re-lists before writing: a create whose response was lost -// (network error / 5xx) may still have landed server-side, and re-listing turns -// that into an update of the existing marker comment instead of a duplicate. -async function syncGitHubComment(commentsUrl, headers, body) { - const comments = await listGitHubComments(commentsUrl, headers); - const existing = comments.find((comment) => comment.body?.includes(COMMENT_MARKER)); - await writeGitHubComment(commentsUrl, headers, body, existing?.url); -} - -function readGitHubCommentConfig(explicitPrNumber) { - const token = process.env.GITHUB_TOKEN; - const repository = process.env.GITHUB_REPOSITORY; - const prNumber = explicitPrNumber ?? process.env.GITHUB_PR_NUMBER; - assertGitHubCommentConfig(token, repository, prNumber); - return { - repository, - prNumber, - headers: buildGitHubHeaders(token), - }; -} - -function assertGitHubCommentConfig(token, repository, prNumber) { - for (const value of [token, repository, prNumber]) { - if (!value) { - throw new Error( - 'GITHUB_TOKEN, GITHUB_REPOSITORY, and PR number are required to post a comment.', - ); - } - } -} - -function buildGitHubHeaders(token) { - return { - accept: 'application/vnd.github+json', - authorization: `Bearer ${token}`, - 'content-type': 'application/json', - 'x-github-api-version': '2022-11-28', - }; -} - -function buildCommentsUrl(repository, prNumber) { - const [owner, repo] = repository.split('/'); - return `https://api.github.com/repos/${owner}/${repo}/issues/${prNumber}/comments`; -} - -async function listGitHubComments(commentsUrl, headers) { - const response = await githubRequest( - `${commentsUrl}?per_page=100`, - { headers }, - 'list PR comments', - ); - return await response.json(); -} - -async function writeGitHubComment(commentsUrl, headers, body, existingUrl) { - const target = commentWriteTarget(commentsUrl, existingUrl); - await githubRequest( - target.url, - { method: target.method, headers, body: JSON.stringify({ body }) }, - `${target.action} PR comment`, - ); -} - -function commentWriteTarget(commentsUrl, existingUrl) { - if (existingUrl) { - return { url: existingUrl, method: 'PATCH', action: 'update' }; - } - return { url: commentsUrl, method: 'POST', action: 'create' }; -} - -// Re-runs `operation` with exponential backoff while it throws -// TransientGitHubError; any other error (a non-transient HTTP status, i.e. a -// configuration problem) propagates immediately and fails the job. -async function retryTransient(operation) { - for (let attempt = 1; ; attempt += 1) { - try { - return await operation(); - } catch (error) { - await backoffOrRethrow(error, attempt); - } - } -} - -async function backoffOrRethrow(error, attempt) { - if (!(error instanceof TransientGitHubError)) throw error; - if (attempt >= GITHUB_REQUEST_ATTEMPTS) { - throw new TransientGitHubError(`${error.message} after ${attempt} attempts`); - } - const delayMs = GITHUB_RETRY_BASE_MS * 2 ** (attempt - 1); - process.stderr.write(`${error.message} (retrying in ${delayMs}ms)\n`); - await new Promise((resolve) => setTimeout(resolve, delayMs)); -} - -// One attempt: network errors and 5xx / 429 throw TransientGitHubError; -// any other non-OK status throws a plain (fatal) Error. -async function githubRequest(url, init, action) { - const response = await fetchOrTransient(url, init, action); - if (response.ok) return response; - throw await githubStatusError(response, action); -} - -async function fetchOrTransient(url, init, action) { - try { - return await fetch(url, init); - } catch (error) { - throw new TransientGitHubError(`Failed to ${action}: ${error?.message ?? error}`); - } -} - -async function githubStatusError(response, action) { - const failure = `Failed to ${action}: ${response.status} ${await response.text()}`; - return isTransientGitHubStatus(response.status) - ? new TransientGitHubError(failure) - : new Error(failure); -} - -function isTransientGitHubStatus(status) { - return status === 429 || status >= 500; -} diff --git a/scripts/size-report.mjs b/scripts/size-report.mjs index 04db9e9ae..08e15366a 100644 --- a/scripts/size-report.mjs +++ b/scripts/size-report.mjs @@ -4,15 +4,17 @@ import path from 'node:path'; import { execFileSync } from 'node:child_process'; import { performance } from 'node:perf_hooks'; import { gzipSync } from 'node:zlib'; -import { withPreparedBaseWorktree } from './size-base-cache.mjs'; -import { COMMENT_MARKER, postGitHubCommentBestEffort } from './size-report-comment.mjs'; +const COMMENT_MARKER = ''; +const GITHUB_REQUEST_ATTEMPTS = 4; +// Overridable so the regression tests do not sleep through real backoff. +const GITHUB_RETRY_BASE_MS = Number(process.env.SIZE_REPORT_RETRY_BASE_MS ?? 1000); +class TransientGitHubError extends Error {} const VALUE_ARGS = new Map([ ['--cwd', 'cwd'], ['--json', 'json'], ['--markdown', 'markdown'], ['--compare', 'compare'], - ['--base', 'base'], ['--post-comment', 'postComment'], ['--pr', 'pr'], ['--startup-runs', 'startupRuns'], @@ -31,24 +33,16 @@ if (args.postComment) { process.exit(0); } -if (args.compare && args.base) { - throw new Error( - '--compare and --base are exclusive: one supplies the base report, the other measures it', - ); -} -const startupRuns = parseNonNegativeInteger(args.startupRuns ?? '0', '--startup-runs'); -const report = collectReport(cwd, { startupRuns }); -const baseReport = args.compare - ? JSON.parse(fs.readFileSync(args.compare, 'utf8')) - : args.base - ? measureBaseRef(cwd, args.base, { startupRuns }) - : null; +const report = collectReport(cwd, { + startupRuns: parseNonNegativeInteger(args.startupRuns ?? '0', '--startup-runs'), +}); +const baseReport = args.compare ? JSON.parse(fs.readFileSync(args.compare, 'utf8')) : null; if (args.json) { writeFile(args.json, `${JSON.stringify(report, null, 2)}\n`); } -const markdown = formatMarkdown(report, baseReport, args.base); +const markdown = formatMarkdown(report, baseReport); if (args.markdown) { writeFile(args.markdown, markdown); @@ -86,9 +80,6 @@ Options: --json Write the raw size report JSON. --markdown Write the markdown report. --compare Compare against a previously written JSON report. - --base Measure (e.g. origin/main) in a detached worktree under - .tmp/size-base/ and compare against it: the local one-command - equivalent of the Size workflow's base/PR comparison. --startup-runs Measure startup medians for side-effect-free CLI commands. --post-comment Post or update the markdown report on the current PR. --pr Pull request number for --post-comment. @@ -153,13 +144,6 @@ function collectReport(root, options) { }; } -// The Size workflow measures the base by checking it out, installing, and building; this is the -// same recipe in a detached worktree, so the working tree is never touched. Entry reuse, claim -// ownership, and eviction belong to scripts/size-base-cache.mjs. -function measureBaseRef(root, ref, options) { - return withPreparedBaseWorktree(root, ref, (worktreeDir) => collectReport(worktreeDir, options)); -} - function prepareGeneratedPackageAssets(root) { const packageAppleRunnerScript = path.join(root, 'scripts', 'package-apple-runner-source.mjs'); if (!fs.existsSync(packageAppleRunnerScript)) { @@ -250,7 +234,7 @@ function countNpmPackEntries(pack) { return Array.isArray(pack.files) ? pack.files.length : 0; } -function formatMarkdown(report, baseReport, baseLabel) { +function formatMarkdown(report, baseReport) { const rows = [ metricRow('JS raw', baseReport?.js.rawBytes, report.js.rawBytes), metricRow('JS gzip', baseReport?.js.gzipBytes, report.js.gzipBytes), @@ -266,7 +250,7 @@ function formatMarkdown(report, baseReport, baseLabel) { return `${COMMENT_MARKER} ## Size Report -| Metric | ${baseColumnLabel(baseLabel)} | Current | Diff | +| Metric | Base | Current | Diff | |---|---:|---:|---:| ${rows.join('\n')} @@ -275,10 +259,6 @@ ${changedChunks} `; } -function baseColumnLabel(baseLabel) { - return baseLabel ? `Base (${baseLabel})` : 'Base'; -} - function metricRow(label, base, current) { return `| ${label} | ${formatMaybeBytes(base)} | ${formatBytes(current)} | ${formatDiff(base, current)} |`; } @@ -384,3 +364,149 @@ function writeFile(filePath, contents) { fs.mkdirSync(path.dirname(path.resolve(filePath)), { recursive: true }); fs.writeFileSync(filePath, contents); } + +// The PR comment is a convenience surface: the same markdown is already in the +// job summary. A GitHub outage (5xx / 429 / network error) must not fail the +// job, but a real misconfiguration (bad token, missing permissions) still does. +async function postGitHubCommentBestEffort(markdownPath, explicitPrNumber) { + try { + await postGitHubComment(markdownPath, explicitPrNumber); + } catch (error) { + if (!(error instanceof TransientGitHubError)) throw error; + const message = `Skipping PR size comment after transient GitHub failure: ${error.message}`; + process.stdout.write(`::warning::${message}\n`); + appendStepSummary(`> ⚠️ ${message} The size report above is authoritative.\n`); + } +} + +function appendStepSummary(text) { + const summaryPath = process.env.GITHUB_STEP_SUMMARY; + if (summaryPath) fs.appendFileSync(summaryPath, text); +} + +async function postGitHubComment(markdownPath, explicitPrNumber) { + const config = readGitHubCommentConfig(explicitPrNumber); + const body = fs.readFileSync(markdownPath, 'utf8'); + const commentsUrl = buildCommentsUrl(config.repository, config.prNumber); + await retryTransient(() => syncGitHubComment(commentsUrl, config.headers, body)); +} + +// Every attempt re-lists before writing: a create whose response was lost +// (network error / 5xx) may still have landed server-side, and re-listing turns +// that into an update of the existing marker comment instead of a duplicate. +async function syncGitHubComment(commentsUrl, headers, body) { + const comments = await listGitHubComments(commentsUrl, headers); + const existing = comments.find((comment) => comment.body?.includes(COMMENT_MARKER)); + await writeGitHubComment(commentsUrl, headers, body, existing?.url); +} + +function readGitHubCommentConfig(explicitPrNumber) { + const token = process.env.GITHUB_TOKEN; + const repository = process.env.GITHUB_REPOSITORY; + const prNumber = explicitPrNumber ?? process.env.GITHUB_PR_NUMBER; + assertGitHubCommentConfig(token, repository, prNumber); + return { + repository, + prNumber, + headers: buildGitHubHeaders(token), + }; +} + +function assertGitHubCommentConfig(token, repository, prNumber) { + for (const value of [token, repository, prNumber]) { + if (!value) { + throw new Error( + 'GITHUB_TOKEN, GITHUB_REPOSITORY, and PR number are required to post a comment.', + ); + } + } +} + +function buildGitHubHeaders(token) { + return { + accept: 'application/vnd.github+json', + authorization: `Bearer ${token}`, + 'content-type': 'application/json', + 'x-github-api-version': '2022-11-28', + }; +} + +function buildCommentsUrl(repository, prNumber) { + const [owner, repo] = repository.split('/'); + return `https://api.github.com/repos/${owner}/${repo}/issues/${prNumber}/comments`; +} + +async function listGitHubComments(commentsUrl, headers) { + const response = await githubRequest( + `${commentsUrl}?per_page=100`, + { headers }, + 'list PR comments', + ); + return await response.json(); +} + +async function writeGitHubComment(commentsUrl, headers, body, existingUrl) { + const target = commentWriteTarget(commentsUrl, existingUrl); + await githubRequest( + target.url, + { method: target.method, headers, body: JSON.stringify({ body }) }, + `${target.action} PR comment`, + ); +} + +function commentWriteTarget(commentsUrl, existingUrl) { + if (existingUrl) { + return { url: existingUrl, method: 'PATCH', action: 'update' }; + } + return { url: commentsUrl, method: 'POST', action: 'create' }; +} + +// Re-runs `operation` with exponential backoff while it throws +// TransientGitHubError; any other error (a non-transient HTTP status, i.e. a +// configuration problem) propagates immediately and fails the job. +async function retryTransient(operation) { + for (let attempt = 1; ; attempt += 1) { + try { + return await operation(); + } catch (error) { + await backoffOrRethrow(error, attempt); + } + } +} + +async function backoffOrRethrow(error, attempt) { + if (!(error instanceof TransientGitHubError)) throw error; + if (attempt >= GITHUB_REQUEST_ATTEMPTS) { + throw new TransientGitHubError(`${error.message} after ${attempt} attempts`); + } + const delayMs = GITHUB_RETRY_BASE_MS * 2 ** (attempt - 1); + process.stderr.write(`${error.message} (retrying in ${delayMs}ms)\n`); + await new Promise((resolve) => setTimeout(resolve, delayMs)); +} + +// One attempt: network errors and 5xx / 429 throw TransientGitHubError; +// any other non-OK status throws a plain (fatal) Error. +async function githubRequest(url, init, action) { + const response = await fetchOrTransient(url, init, action); + if (response.ok) return response; + throw await githubStatusError(response, action); +} + +async function fetchOrTransient(url, init, action) { + try { + return await fetch(url, init); + } catch (error) { + throw new TransientGitHubError(`Failed to ${action}: ${error?.message ?? error}`); + } +} + +async function githubStatusError(response, action) { + const failure = `Failed to ${action}: ${response.status} ${await response.text()}`; + return isTransientGitHubStatus(response.status) + ? new TransientGitHubError(failure) + : new Error(failure); +} + +function isTransientGitHubStatus(status) { + return status === 429 || status >= 500; +} diff --git a/vitest.config.ts b/vitest.config.ts index 627839a42..21f5a9b0e 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -15,11 +15,6 @@ export const SUBPROCESS_STUB_TESTS: readonly string[] = [ 'scripts/fuzz/harness.test.ts', // Replays the fuzz corpus through that same worker watchdog, waiting its per-case budget. 'scripts/fuzz/corpus-replay.test.ts', - // Spawns `node scripts/size-report.mjs` per case, which itself spawns git plus the shimmed - // pnpm/npm — several real subprocesses deep. Un-serialized it took 14s for the file under the - // full suite versus ~5.5s alone, and starved spawns surfaced as a vitest test timeout instead - // of the orchestration assertion the case is about (#1842). - 'scripts/__tests__/size-report-base.test.ts', ]; const SETUP_FILES = ['src/__tests__/hermetic-env-setup.ts', 'src/__tests__/process-memo-setup.ts']; @@ -83,16 +78,6 @@ export default defineConfig({ // The Bundle Size lane's PR-comment path: spawns the real script against a // stubbed fetch, so it needs no network; pins retry/reconcile/fatal outcomes. 'scripts/__tests__/size-report-post-comment.test.ts', - // The `size --base` cache's claim protocol: pure filesystem, so the dangerous - // takeover interleavings are planted directly rather than raced for. Milliseconds. - 'scripts/__tests__/size-base-cache.test.ts', - // The Size workflow copies the reporter out of the tree to measure the base with the - // PR's instrument; nothing local reproduces that copy, so this holds the step to the - // reporter's real import closure. - 'scripts/__tests__/size-report-preserved-closure.test.ts', - // `--base` orchestration (per-SHA worktree, lock, completion stamp, eviction) against - // a throwaway git repo with pnpm/npm shimmed on PATH: git + node only. - 'scripts/__tests__/size-report-base.test.ts', // Parses CI configuration only, so this action guard needs no device or subprocess lane. 'test/ci/upload-agent-device-artifacts.test.ts', // #1781 A9: pins the root-doc paths-ignore entries directly against the