From cfb799b9d63771d2533822869bd779eadd13ab0a Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 15:04:28 +0000 Subject: [PATCH] fix: pre-landing review fixes (untracked reads ineligible, tool bytes and runner imports in the key) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/test-impact.md | 6 ++++-- lib/test-impact/key.ts | 24 ++++++++++++++++++++---- lib/test-impact/trace.ts | 25 +++++++++++++++++-------- scripts/test-impact.ts | 2 +- test/test-impact-key.test.ts | 25 ++++++++++++++++++++++++- test/test-impact-trace.test.ts | 2 ++ 6 files changed, 68 insertions(+), 16 deletions(-) diff --git a/docs/test-impact.md b/docs/test-impact.md index 1d65d466b..3b441e412 100644 --- a/docs/test-impact.md +++ b/docs/test-impact.md @@ -24,7 +24,8 @@ The recorder runs `strace -f -y -qq -v` with `openat, open, execve, execveat, ge - the names in each traced directory listing, at the tree; - the test's forward import-graph closure from `lib/pregate/import-graph.ts`, up to 12 hops; - `bun.lock`, every `tsconfig*.json` and the `dependencies` in `.gstack/pregate.json`; -- the key code itself: the tracer, the key, the CLI, the free-test runner and planner (`scripts/test-free-shards.ts`), the shard engine, the import graph and `lib/pregate/lane-selection.ts`; +- the key code itself: the tracer, the key, the CLI, the free-test runner and planner (`scripts/test-free-shards.ts`) with every repo module it imports (shard engine, HOME guard, CI health, Windows curation, the paid/free split, the state root), the import graph and `lib/pregate/lane-selection.ts`; +- the bytes of each executable outside the repo the trace ran (git, bash, tar and so on), read on the machine computing the key; - the runtime (platform, architecture, Bun version) and the values of `CI`, `GITHUB_ACTIONS`, `TZ`, `LANG`, `LC_ALL`, `NODE_ENV`, `EVALS` and `EVALS_TIER`. ## What is never reused @@ -34,7 +35,8 @@ A file is ineligible, so it always runs, when: - it is on the always-run list in the base branch's `.gstack/always-run.json` or the tree's (the union, so a branch can add entries but never drop main's); - it has no trace, a trace older than 7 days, or a trace whose run failed; - its trace shows an inet connect or send to anything other than a loopback port the same run opened, any access under the real HOME other than a PATH tool lookup, shared /tmp state, or a read of the repository's own `.git`; +- its trace read a repo path the tree doesn't track, such as a build output under `dist/`. A probe of a missing path and a file the run created itself don't count; - its import-graph walk reaches the 12-hop cap. The walk fails closed: a deeper chain makes the test ineligible instead of cutting the closure short; - the tree changes any key-code file against the base. Then every file runs. -Limits: strace can't see which environment variables a test reads, so only the allowlist above is in the key, and a test that depends on another variable has to read it through a file the trace sees or stay off the reuse path. Timing assertions, flaky history and the shard's execution grouping are for PR 3 and PR 4, which have the receipts to check them against. Windows has no tracer, so the Windows lane stays full. +Limits: strace can't see which environment variables a test reads, so only the allowlist above is in the key, and a test that depends on another variable has to read it through a file the trace sees or stay off the reuse path. The recorder runs `bun test` directly, not inside the free runner's per-shard sandbox, so PR 3 should record traces through the runner. Timing assertions, flaky history and the shard's execution grouping are for PR 3 and PR 4, which have the receipts to check them against. Windows has no tracer, so the Windows lane stays full. diff --git a/lib/test-impact/key.ts b/lib/test-impact/key.ts index f2648b509..077049904 100644 --- a/lib/test-impact/key.ts +++ b/lib/test-impact/key.ts @@ -10,10 +10,12 @@ * capped at MAX_HOPS hops; a closure that reaches the cap is ineligible, never truncated; * - global: bun.lock, every tsconfig*.json and the pregate `dependencies`; * - code: KEY_CODE_FILES, the runner, planner, shard engine, graph and key code; + * - tool: the bytes of each executable outside the repo the trace ran (git, bash, tar…), read on this machine; * - runtime and env: the runtime manifest and ENV_ALLOWLIST values. * A test is ineligible (always executed) when it is always_run in the base or the tree list, * has no trace or one older than TRACE_MAX_AGE_MS, its traced run failed or touched the - * network, the real HOME, shared /tmp or .git, or its graph walk hit the cap. A tree that + * network, the real HOME, shared /tmp or .git, it read a repo path the tree does not track (a build + * output), or its graph walk hit the cap. A tree that * changes any KEY_CODE_FILES file against the base makes every test ineligible. * scripts/test-impact.ts loads this module from the base ref, so a branch cannot change the * rules that decide what it skips. @@ -31,10 +33,12 @@ import { readTrace } from './trace'; export const MAX_HOPS = 12; export const TRACE_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; -/** Tracer, key, CLI, runner/planner, shard engine and the graph and always-run code the key reads. PR 4 adds its verifier here. */ +/** Tracer, key, CLI, the runner/planner and every repo module it imports, and the graph and always-run code the key reads. PR 4 adds its verifier here. */ export const KEY_CODE_FILES = [ 'lib/test-impact/trace.ts', 'lib/test-impact/key.ts', 'scripts/test-impact.ts', - 'scripts/test-free-shards.ts', 'scripts/lib/shard-engine.ts', 'lib/pregate/import-graph.ts', 'lib/pregate/lane-selection.ts', + 'scripts/test-free-shards.ts', 'scripts/lib/shard-engine.ts', 'scripts/lib/free-home-guard.ts', 'scripts/lib/free-ci-health.ts', + 'scripts/lib/windows-curation.ts', 'test/helpers/paid-test-set.ts', 'test/helpers/touchfiles.ts', 'lib/state-root.ts', + 'lib/pregate/import-graph.ts', 'lib/pregate/lane-selection.ts', ]; /** Environment variables whose values a free test may branch on; anything else a test reads is outside the key. */ export const ENV_ALLOWLIST = ['CI', 'GITHUB_ACTIONS', 'TZ', 'LANG', 'LC_ALL', 'NODE_ENV', 'EVALS', 'EVALS_TIER']; @@ -42,7 +46,7 @@ const TSCONFIG = /(?:^|\/)tsconfig[^/]*\.json$/; export interface TreeEntry { mode: string; type: string; oid: string } export interface TreeIndex { treeish: string; entries: Map; children: Map } -export type InputKind = 'traced' | 'listed' | 'graph' | 'global' | 'code' | 'runtime' | 'env'; +export type InputKind = 'traced' | 'listed' | 'graph' | 'global' | 'code' | 'tool' | 'runtime' | 'env'; export interface KeyInput { kind: InputKind; path: string; digest: string } export interface KeyResult { test: string; eligible: boolean; reasons: string[]; key: string | null; inputs: KeyInput[]; tracedAt: string | null } export interface KeyReport { tree: string; base: string | null; keyCodeEdited: string[]; alwaysRunGlobs: { base: string[]; tree: string[] }; results: KeyResult[] } @@ -148,6 +152,15 @@ export function forwardClosure(graph: Graph, test: string, maxHops = MAX_HOPS): } const sha = (text: string) => createHash('sha256').update(text).digest('hex'); +const toolDigests = new Map(); +function toolDigest(file: string): string { + let digest = toolDigests.get(file); + if (digest === undefined) { + try { digest = createHash('sha256').update(fs.readFileSync(file)).digest('hex'); } catch { digest = 'absent'; } + toolDigests.set(file, digest); + } + return digest; +} /** Keys and eligibility for each test at `tree`, with `base` as the trusted reference for always_run and key code. */ export function computeKeys(opts: KeyOptions): KeyReport { @@ -189,6 +202,8 @@ export function computeKeys(opts: KeyOptions): KeyReport { if (!(age <= TRACE_MAX_AGE_MS)) reasons.push(`stale trace (traced ${trace.tracedAt}, limit ${TRACE_MAX_AGE_MS / 86_400_000} days)`); if (trace.exitCode !== 0) reasons.push(`the traced run failed (exit ${trace.exitCode})`); reasons.push(...trace.ineligible); + const untracked = (trace.present ?? []).filter(p => p !== '.' && !treeIdx.entries.has(p)); + if (untracked.length) reasons.push(`reads paths ${tree} does not track (${untracked.slice(0, 5).join(', ')}${untracked.length > 5 ? ` +${untracked.length - 5} more` : ''})`); } const closure = graph ? forwardClosure(graph, test) : null; if (closure?.capped) reasons.push(`import graph deeper than ${MAX_HOPS} hops: ${closure.capped.join(' → ')}`); @@ -197,6 +212,7 @@ export function computeKeys(opts: KeyOptions): KeyReport { ...[...traced].map(p => ({ kind: 'traced' as const, path: p, digest: '' })), ...(closure?.files ?? []).filter(p => !traced.has(p)).map(p => ({ kind: 'graph' as const, path: p, digest: '' })), ...Object.keys(trace?.dirs ?? {}).map(d => ({ kind: 'listed' as const, path: d, digest: '' })), + ...(trace?.tools ?? []).map(t => ({ kind: 'tool' as const, path: t, digest: toolDigest(t) })), ]; return { test, reasons, trace, inputs: [...files, ...shared.map(s => ({ ...s }))] }; }); diff --git a/lib/test-impact/trace.ts b/lib/test-impact/trace.ts index 263bc0196..bbce670c6 100644 --- a/lib/test-impact/trace.ts +++ b/lib/test-impact/trace.ts @@ -11,12 +11,14 @@ * ineligible with a reason instead: an inet connect or send that is not to a * loopback port the trace itself bound, any access under the real HOME other * than a PATH tool lookup, shared /tmp state, and reads of the repo's own .git. + * Repo paths a call reached (and the run did not create) are listed as `present`, and + * executables outside the repo as `tools`, so key.ts can refuse untracked inputs and hash tools. * lib/test-impact/key.ts hashes the read set at a tree. */ import * as fs from 'node:fs'; import * as path from 'node:path'; -export const TRACE_VERSION = 1; +export const TRACE_VERSION = 2; /** `-e trace=` set; strace 6.8 on x86_64 Linux accepts every name (stat and open are the legacy calls static binaries still make). */ export const TRACE_SYSCALLS = [ 'openat', 'open', 'execve', 'execveat', 'getdents64', 'readlink', 'readlinkat', 'stat', 'lstat', 'newfstatat', 'statx', @@ -47,7 +49,9 @@ export interface ReadSet { dirs: Record; /** Reasons the trace cannot prove the test's inputs; empty when eligible. */ ineligible: string[]; - /** Executables outside the repo the run started, sorted (for explain; the runtime manifest covers the toolchain). */ + /** Repo paths a call reached that the run did not create itself, sorted: key.ts refuses one the tree does not track. */ + present: string[]; + /** Executables outside the repo the run started, sorted; key.ts hashes each one's bytes. */ tools: string[]; } @@ -126,6 +130,8 @@ export function parseStrace(text: string, ctx: TraceContext): ReadSet { for (const p of [repo, tmp, home]) for (let d = path.posix.dirname(p); ; d = path.posix.dirname(d)) { ancestors.add(d); if (d === '/') break; } const cwd = new Map(); const paths = new Set(); + const reached = new Set(); + const created = new Set(); const dirs = new Map>(); const reasons = new Set(); const tools = new Set(); @@ -138,7 +144,7 @@ export function parseStrace(text: string, ctx: TraceContext): ReadSet { } const cwdOf = (pid: number) => cwd.get(pid) ?? ctx.startCwd ?? repo; const absolute = (pid: number, p: string, dir: string | null) => path.posix.normalize(path.posix.isAbsolute(p) ? p : path.posix.join(dir ?? cwdOf(pid), p)); - const record = (raw: string, kind: 'access' | 'exec' | 'content') => { + const record = (raw: string, kind: 'access' | 'exec' | 'content', ok = false, creates = false) => { const abs = raw.length > 1 ? raw.replace(/\/+$/, '') : raw; if (within(abs, '/proc') || within(abs, '/dev') || within(abs, tmp) || ancestors.has(abs)) return; if (within(abs, repo)) { @@ -147,6 +153,7 @@ export function parseStrace(text: string, ctx: TraceContext): ReadSet { if (rel === '.git' && kind === 'access') return; if (rel === '.git' || rel.startsWith('.git/')) { reasons.add(`reads the repository's own git state (${rel})`); return; } paths.add(rel); + if (ok) (creates ? created : reached).add(rel); return; } if (pathDirs.has(path.posix.dirname(abs))) { if (kind === 'exec') tools.add(abs); return; } @@ -168,14 +175,14 @@ export function parseStrace(text: string, ctx: TraceContext): ReadSet { if (p === null || !ok) break; const abs = absolute(c.pid, p, null); cwd.set(c.pid, abs); - record(abs, 'access'); + record(abs, 'access', true); break; } case 'fchdir': if (ok && dirfd) cwd.set(c.pid, path.posix.normalize(dirfd)); break; case 'getdents64': { if (!dirfd) break; const abs = path.posix.normalize(dirfd); - record(abs, 'content'); + record(abs, 'content', true); if (!within(abs, repo)) break; const rel = abs === repo ? '.' : abs.slice(repo.length + 1); if (/(?:^|\/)(?:node_modules|\.git)(?:\/|$)/.test(rel)) break; @@ -194,14 +201,15 @@ export function parseStrace(text: string, ctx: TraceContext): ReadSet { case 'execve': case 'open': case 'stat': case 'lstat': case 'access': case 'readlink': { const p = firstString(c.args); if (p === null) break; - record(absolute(c.pid, p, null), c.name === 'execve' ? (ok ? 'exec' : 'access') : c.name === 'open' ? 'content' : 'access'); + record(absolute(c.pid, p, null), c.name === 'execve' ? (ok ? 'exec' : 'access') : c.name === 'open' ? 'content' : 'access', ok, c.name === 'open' && /O_CREAT/.test(c.args)); break; } default: { if (file === null) break; const target = file === '' && dirfd ? path.posix.normalize(dirfd) : absolute(c.pid, file, dirfd); - record(target, c.name === 'execveat' ? (ok ? 'exec' : 'access') : c.name === 'openat' ? 'content' : 'access'); - if (ok && c.retPath && c.name === 'openat' && path.posix.isAbsolute(c.retPath)) record(path.posix.normalize(c.retPath), 'content'); + const creates = c.name === 'openat' && /O_CREAT/.test(c.args); + record(target, c.name === 'execveat' ? (ok ? 'exec' : 'access') : c.name === 'openat' ? 'content' : 'access', ok, creates); + if (ok && c.retPath && c.name === 'openat' && path.posix.isAbsolute(c.retPath)) record(path.posix.normalize(c.retPath), 'content', true, creates); } } } @@ -209,6 +217,7 @@ export function parseStrace(text: string, ctx: TraceContext): ReadSet { paths: [...paths].sort(), dirs: Object.fromEntries([...dirs].sort(([a], [b]) => a.localeCompare(b)).map(([d, n]) => [d, [...n].sort()])), ineligible: [...reasons].sort(), + present: [...reached].filter(p => !created.has(p)).sort(), tools: [...tools].sort(), }; } diff --git a/scripts/test-impact.ts b/scripts/test-impact.ts index f5bf3d487..050867277 100755 --- a/scripts/test-impact.ts +++ b/scripts/test-impact.ts @@ -110,7 +110,7 @@ async function key(root: string, argv: string[], explain: boolean): Promise i.kind === kind); if (!list.length) continue; console.log(` ${kind} (${list.length}):`); diff --git a/test/test-impact-key.test.ts b/test/test-impact-key.test.ts index 931d92f27..8e8cbea3a 100644 --- a/test/test-impact-key.test.ts +++ b/test/test-impact-key.test.ts @@ -35,7 +35,7 @@ function traceFrom(repo: FixtureRepo, test: string, opts: { reads?: string[]; li lines.push(...(opts.extra ?? [])); const dir = path.join(repo.root, 'traces'); fs.mkdirSync(dir, { recursive: true }); - const set = parseStrace(lines.join('\n'), { repoRoot: w, home: repo.home, tmpDir: path.join(repo.root, 'run') }); + const set = parseStrace(lines.join('\n'), { repoRoot: w, home: repo.home, tmpDir: path.join(repo.root, 'run'), sharedTmp: [] }); const record: TraceRecord = { version: TRACE_VERSION, test, tracedAt: opts.tracedAt ?? '2026-11-01T00:00:00Z', commit: null, exitCode: 0, runtime: RUNTIME, ...set }; fs.writeFileSync(tracePath(dir, test), JSON.stringify(record)); return dir; @@ -92,6 +92,20 @@ describe('test-impact key: what changes it', () => { expect(keyAt(repo, 'HEAD', t).key).not.toBe(before.key); }); + test('the bytes of an executable outside the repo that the test ran are in the key', () => { + const repo = fixture(); + const tool = path.join(repo.root, 'tools', 'mytool'); + fs.mkdirSync(path.dirname(tool), { recursive: true }); + fs.writeFileSync(tool, 'v1'); + traceFrom(repo, 'test/util.test.js', { extra: [`100 execve("${tool}", ["mytool"], []) = 0`] }); + const before = keyAt(repo, 'main', 'test/util.test.js'); + expect(before.inputs).toContainEqual(expect.objectContaining({ kind: 'tool', path: tool })); + fs.writeFileSync(tool, 'v2'); + const fresh = spawnSync(process.execPath, [...bunIsolationArgs(), '-e', `const k = await import(${JSON.stringify(path.join(ROOT, 'lib/test-impact/key.ts'))}); console.log(k.computeKeys({ repoRoot: ${JSON.stringify(repo.work)}, tree: 'main', base: 'main', tests: ['test/util.test.js'], traceDir: ${JSON.stringify(path.join(repo.root, 'traces'))}, now: ${NOW}, runtime: ${JSON.stringify(RUNTIME)}, env: {} }).results[0].key)`], { encoding: 'utf8', timeout: 60_000 }); + expect(fresh.stdout.trim()).toMatch(/^[0-9a-f]{64}$/); + expect(fresh.stdout.trim()).not.toBe(before.key); + }); + test('a spawned bin that sources a sibling script: the traced sibling is in the key', () => { const t = 'test/tool2.test.js'; const repo = fixture({ 'bin/tool2': '#!/bin/bash\nsource "$(dirname "$0")/tool2-lib.sh"\ngreet\n', 'bin/tool2-lib.sh': 'greet() { echo hi; }\n', [t]: `${NODE_TEST}test('greets', () => require('child_process').spawnSync('bin/tool2', { timeout: 5000 }));\n` }); @@ -131,6 +145,15 @@ describe('test-impact key: ineligible', () => { expect(keyAt(repo, 'main', 'test/config-read.test.js').eligible).toBe(true); }); + test('a successful read of a path the tree does not track (a build output) is ineligible; a probe or a file the run created is not', () => { + const repo = fixture(); + const w = repo.work; + traceFrom(repo, 'test/util.test.js', { reads: ['dist/tool'] }); + traceFrom(repo, 'test/retry.test.js', { extra: [`100 access("${w}/dist/tool", F_OK) = -1 ENOENT (No such file or directory)`, `100 openat(AT_FDCWD<${w}>, "${w}/out.json", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3<${w}/out.json>`, `100 openat(AT_FDCWD<${w}>, "${w}/out.json", O_RDONLY) = 3<${w}/out.json>`] }); + expect(keyAt(repo, 'main', 'test/util.test.js').reasons).toEqual(['reads paths main does not track (dist/tool)']); + expect(keyAt(repo, 'main', 'test/retry.test.js').reasons).toEqual([]); + }); + test('no trace, a stale trace and a failed traced run are ineligible', () => { const repo = fixture(); traceFrom(repo, 'test/retry.test.js', { tracedAt: '2026-10-20T00:00:00Z' }); diff --git a/test/test-impact-trace.test.ts b/test/test-impact-trace.test.ts index b2f9de9f0..31a4f1507 100644 --- a/test/test-impact-trace.test.ts +++ b/test/test-impact-trace.test.ts @@ -17,6 +17,8 @@ describe('parseStrace on a captured bun test', () => { for (const p of ['test/cap.test.ts', 'lib/data.txt', 'lib/missing.txt', 'bin/tool2', 'bin/tool2-lib.sh', 'package.json', 'test/data']) expect(set.paths).toContain(p); expect(set.paths.some(p => p.startsWith('.git') || p.startsWith('/') || p.includes('node_modules'))).toBe(false); expect(set.tools).toContain('/usr/local/bin/bun'); + expect(set.present).toContain('bin/tool2-lib.sh'); + expect(set.present).not.toContain('lib/missing.txt'); }); test('a directory listing records its sorted names', () => {