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>
This commit is contained in:
garrytanandcapy-ai[bot] committed 2026-11-01 15:04:28 +00:00
1 parent 32c8a85005
commit cfb799b9d6
6 files changed
+68 -16

No files matched your search

+4 -2
View File
@@ -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.
+20 -4
View File
@@ -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<string, TreeEntry>; children: Map<string, string[]> }
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<string, string>();
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 }))] };
});
+17 -8
View File
@@ -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<string, string[]>;
/** 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<number, string>();
const paths = new Set<string>();
const reached = new Set<string>();
const created = new Set<string>();
const dirs = new Map<string, Set<string>>();
const reasons = new Set<string>();
const tools = new Set<string>();
@@ -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(),
};
}
+1 -1
View File
@@ -110,7 +110,7 @@ async function key(root: string, argv: string[], explain: boolean): Promise<numb
if (!explain) continue;
console.log(` traced at: ${r.tracedAt ?? 'never'}${r.key ? `\n key: ${r.key}` : ''}`);
console.log(` always_run globs: base [${report.alwaysRunGlobs.base.join(', ')}], tree [${report.alwaysRunGlobs.tree.join(', ')}]`);
for (const kind of ['traced', 'listed', 'graph', 'global', 'code', 'runtime', 'env'] as const) {
for (const kind of ['traced', 'listed', 'graph', 'global', 'code', 'tool', 'runtime', 'env'] as const) {
const list = r.inputs.filter(i => i.kind === kind);
if (!list.length) continue;
console.log(` ${kind} (${list.length}):`);
+24 -1
View File
@@ -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' });
+2
View File
@@ -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', () => {