From 6b4e6c5f05cfb27a534c2f418e95096ac96f578e Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 15:16:05 +0000 Subject: [PATCH] fix: adversarial review fixes (creation proof, PATH reads, bound loopback endpoints, unix sockets, env and runtime binding, future traces) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/test-impact.md | 10 ++--- lib/test-impact/key.ts | 26 +++++++---- lib/test-impact/trace.ts | 55 ++++++++++++++++-------- scripts/test-impact.ts | 4 +- test/fixtures/test-impact/capture.strace | 2 +- test/test-impact-key.test.ts | 32 ++++++++++---- test/test-impact-trace.test.ts | 31 +++++++++++++ 7 files changed, 118 insertions(+), 42 deletions(-) diff --git a/docs/test-impact.md b/docs/test-impact.md index 3b441e412..c65a05807 100644 --- a/docs/test-impact.md +++ b/docs/test-impact.md @@ -10,11 +10,11 @@ GST-167 is the plan to skip a free test on a pull request only when main already - `key --tree [--base ] [--traces ] [--json] ...` prints each file's key at ``, or why it has none. - `explain [--tree ] [--base ] [--traces ] ` prints every input of one file's key and why it is or isn't eligible. -With `--base`, the CLI loads `lib/test-impact/key.ts` from that ref and runs it over the tree, so a branch can't change the rules that decide what it skips. Without `--base`, it uses this checkout's code and says so. A base ref with no key code makes every file ineligible. +With `--base`, the CLI loads `lib/test-impact/key.ts` from that ref and runs it over the tree, so a branch can't change the rules that decide what it skips. The CLI that does the loading comes from the checkout, so CI wiring (PR 3 and 4) has to run the base ref's copy of `scripts/test-impact.ts`, never the branch's. Without `--base`, it uses this checkout's code and says so. A base ref with no key code makes every file ineligible. ## What the trace records -The recorder runs `strace -f -y -qq -v` with `openat, open, execve, execveat, getdents64, readlink, readlinkat, stat, lstat, newfstatat, statx, access, faccessat, faccessat2, chdir, fchdir, clone, clone3, fork, vfork, connect, bind, sendto, getsockname` (strace 6.8 on x86_64 Linux accepts the whole set) and gives the run a private TMPDIR. `lib/test-impact/trace.ts` keeps every repo path the test or any child process opened, ran, checked or probed. A failed open counts, because a file that shows up later changes what the test sees. For each repo directory it listed, the trace keeps the sorted names. Paths outside the repo, in /proc and /dev, under node_modules and in the private TMPDIR are dropped. +The recorder runs `strace -f -y -qq -v` with `openat, open, execve, execveat, getdents64, readlink, readlinkat, stat, lstat, newfstatat, statx, access, faccessat, faccessat2, chdir, fchdir, clone, clone3, fork, vfork, connect, bind, sendto, sendmsg, sendmmsg, getsockname` (strace 6.8 on x86_64 Linux accepts the whole set) and gives the run a private TMPDIR. `lib/test-impact/trace.ts` keeps every repo path the test or any child process opened, ran, checked or probed. A failed open counts, because a file that shows up later changes what the test sees. For each repo directory it listed, the trace keeps the sorted names. Paths outside the repo, in /proc and /dev, under node_modules and in the private TMPDIR are dropped. ## What the key covers @@ -33,9 +33,9 @@ The recorder runs `strace -f -y -qq -v` with `openat, open, execve, execveat, ge 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; +- it has no trace, a trace older than 7 days or dated more than 5 minutes in the future, a trace whose run failed, or a trace made under other values of the allowlisted variables or on another runtime; +- its trace shows an inet connect or send to anything other than a loopback endpoint a socket in the same run bound, a unix socket outside the repo and the private TMPDIR (or an abstract one), any access under the real HOME other than a PATH tool lookup or the tool itself, a listing of HOME, 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 or truncated 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. diff --git a/lib/test-impact/key.ts b/lib/test-impact/key.ts index 077049904..a17fafcdd 100644 --- a/lib/test-impact/key.ts +++ b/lib/test-impact/key.ts @@ -13,7 +13,8 @@ * - 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 + * has no trace, one older than TRACE_MAX_AGE_MS or dated in the future, one made under other + * allowlisted env values or another runtime, its traced run failed or touched the * 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. @@ -29,19 +30,21 @@ import { matchesAny } from '../ship-policy'; import { PREGATE_CONFIG_PATH } from '../pregate/config'; import { buildGraph, type Graph } from '../pregate/import-graph'; import { ALWAYS_RUN_PATH, parseAlwaysRun } from '../pregate/lane-selection'; -import { readTrace } from './trace'; +import { ENV_ALLOWLIST, envDigests, readTrace } from './trace'; + +export { ENV_ALLOWLIST }; export const MAX_HOPS = 12; export const TRACE_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; +/** How far in the future a trace's timestamp may sit (clock skew) before it counts as stale. */ +export const TRACE_CLOCK_SKEW_MS = 5 * 60 * 1000; /** 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', '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', + 'scripts/lib/windows-curation.ts', 'test/helpers/paid-test-set.ts', 'test/helpers/touchfiles.ts', 'test/helpers/test-selection.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']; const TSCONFIG = /(?:^|\/)tsconfig[^/]*\.json$/; export interface TreeEntry { mode: string; type: string; oid: string } @@ -181,13 +184,14 @@ export function computeKeys(opts: KeyOptions): KeyReport { const dir = materializeTree(repoRoot, tree); try { graph = buildGraph(dir, { declarations: dependencies }); } finally { fs.rmSync(dir, { recursive: true, force: true }); } } - const env = opts.env ?? process.env; + const envNow = envDigests(opts.env ?? process.env); + const runtime = opts.runtime ?? runtimeManifest(); const shared: KeyInput[] = [ ...[...treeIdx.entries.keys()].filter(p => p === 'bun.lock' || TSCONFIG.test(p)).map(p => ({ kind: 'global' as const, path: p, digest: '' })), { kind: 'global', path: `${PREGATE_CONFIG_PATH}#dependencies`, digest: sha(JSON.stringify(dependencies)) }, ...KEY_CODE_FILES.map(p => ({ kind: 'code' as const, path: p, digest: '' })), - ...Object.entries(opts.runtime ?? runtimeManifest()).sort().map(([k, v]) => ({ kind: 'runtime' as const, path: k, digest: v })), - ...ENV_ALLOWLIST.map(k => ({ kind: 'env' as const, path: k, digest: env[k] === undefined ? 'unset' : sha(env[k]!) })), + ...Object.entries(runtime).sort().map(([k, v]) => ({ kind: 'runtime' as const, path: k, digest: v })), + ...Object.entries(envNow).map(([k, digest]) => ({ kind: 'env' as const, path: k, digest })), ]; const pending = tests.map(test => { const reasons: string[] = []; @@ -199,8 +203,12 @@ export function computeKeys(opts: KeyOptions): KeyReport { if (!trace) reasons.push('no trace'); else { const age = now - Date.parse(trace.tracedAt); - if (!(age <= TRACE_MAX_AGE_MS)) reasons.push(`stale trace (traced ${trace.tracedAt}, limit ${TRACE_MAX_AGE_MS / 86_400_000} days)`); + if (!(age >= -TRACE_CLOCK_SKEW_MS && 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})`); + const envDiff = ENV_ALLOWLIST.filter(k => trace.env?.[k] !== envNow[k]); + if (envDiff.length) reasons.push(`traced under other values of ${envDiff.join(', ')}`); + const runtimeDiff = Object.keys(runtime).filter(k => trace.runtime?.[k] !== runtime[k]); + if (runtimeDiff.length) reasons.push(`traced on another runtime (${runtimeDiff.map(k => `${k} ${trace.runtime?.[k]} vs ${runtime[k]}`).join(', ')})`); 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` : ''})`); diff --git a/lib/test-impact/trace.ts b/lib/test-impact/trace.ts index bbce670c6..173e413ca 100644 --- a/lib/test-impact/trace.ts +++ b/lib/test-impact/trace.ts @@ -15,6 +15,7 @@ * 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 { createHash } from 'node:crypto'; import * as fs from 'node:fs'; import * as path from 'node:path'; @@ -22,9 +23,17 @@ 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', - 'access', 'faccessat', 'faccessat2', 'chdir', 'fchdir', 'clone', 'clone3', 'fork', 'vfork', 'connect', 'bind', 'sendto', 'getsockname', + 'access', 'faccessat', 'faccessat2', 'chdir', 'fchdir', 'clone', 'clone3', 'fork', 'vfork', 'connect', 'bind', 'sendto', 'sendmsg', 'sendmmsg', 'getsockname', ] as const; /** strace flags: follow children, decode fd paths (gives every *at call its directory and every pid its cwd), full dirents, long strings. */ +/** Environment variables whose values a free test may branch on; a trace records them and a key refuses a trace made under other values. */ +export const ENV_ALLOWLIST = ['CI', 'GITHUB_ACTIONS', 'TZ', 'LANG', 'LC_ALL', 'NODE_ENV', 'EVALS', 'EVALS_TIER']; + +/** Each ENV_ALLOWLIST variable as `unset` or a sha256 of its value. */ +export function envDigests(env: Record): Record { + return Object.fromEntries(ENV_ALLOWLIST.map(k => [k, env[k] === undefined ? 'unset' : createHash('sha256').update(env[k]!).digest('hex')])); +} + export const STRACE_FLAGS = ['-f', '-y', '-qq', '-v', '-s', '65535', '-e', `trace=${TRACE_SYSCALLS.join(',')}`]; export interface TraceContext { @@ -63,6 +72,8 @@ export interface TraceRecord extends ReadSet { commit: string | null; exitCode: number | null; runtime: Record; + /** envDigests of the traced run. */ + env: Record; } interface Call { pid: number; name: string; args: string; ret: string; retPath: string | null } @@ -109,14 +120,16 @@ function dirfdAndPath(args: string): { dirfd: string | null; file: string | null const firstString = (args: string): string | null => { const s = /^"((?:[^"\\]|\\.)*)"/.exec(args); return s ? unescapeStrace(s[1]!) : null; }; const within = (p: string, root: string) => p === root || p.startsWith(root === '/' ? '/' : `${root}/`); -interface Peer { family: 'inet' | 'unix'; host: string; port: number; path?: string } +interface Peer { family: 'inet' | 'unix'; host: string; port: number; path?: string; abstract?: boolean } function sockaddr(args: string): Peer | null { - if (/sa_family=AF_UNIX/.test(args)) { const p = /sun_path=@?"((?:[^"\\]|\\.)*)"/.exec(args); return p ? { family: 'unix', host: '', port: 0, path: unescapeStrace(p[1]!) } : null; } + if (/sa_family=AF_UNIX/.test(args)) { const p = /sun_path=(@?)"((?:[^"\\]|\\.)*)"/.exec(args); return p ? { family: 'unix', host: '', port: 0, path: unescapeStrace(p[2]!), abstract: p[1] === '@' } : null; } if (!/sa_family=AF_INET6?\b/.test(args)) return null; const port = /sin6?_port=htons\((\d+)\)/.exec(args); const host = /inet_addr\("([^"]+)"\)/.exec(args) ?? /inet_pton\(AF_INET6?,\s*"([^"]+)"/.exec(args); return { family: 'inet', host: host?.[1] ?? '?', port: port ? Number(port[1]) : 0 }; } +/** An open that creates the file or discards its old bytes (O_CREAT with O_EXCL or O_TRUNC); plain O_CREAT may open an existing file as is. */ +const replacesFile = (args: string) => /O_CREAT/.test(args) && /O_EXCL|O_TRUNC/.test(args); const loopback = (host: string) => /^127\./.test(host) || host === '::1' || host === '::ffff:127.0.0.1'; /** A per-test read set from one strace log. */ @@ -126,37 +139,42 @@ export function parseStrace(text: string, ctx: TraceContext): ReadSet { const tmp = path.posix.normalize(ctx.tmpDir); const shared = (ctx.sharedTmp ?? ['/tmp', '/var/tmp']).map(p => path.posix.normalize(p)); const pathDirs = new Set((ctx.pathDirs ?? []).filter(Boolean).map(p => path.posix.normalize(p))); - const ancestors = new Set([home]); + const ancestors = new Set(); 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 firstReach = new Map(); const dirs = new Map>(); const reasons = new Set(); const tools = new Set(); const calls = straceCalls(text); - const ownPorts = new Set(); + const bound = new Set(); + const ownEndpoints = new Set(); for (const c of calls) { if ((c.name !== 'bind' && c.name !== 'getsockname') || c.ret !== '0') continue; + const inode = /^-?\d+/.exec(c.args)?.[1] ?? ''; + if (c.name === 'bind') bound.add(`${c.pid}:${inode}`); + else if (!bound.has(`${c.pid}:${inode}`)) continue; const a = sockaddr(c.args); - if (a?.family === 'inet' && a.port && loopback(a.host)) ownPorts.add(a.port); + if (a?.family === 'inet' && a.port) ownEndpoints.add(`${a.host}:${a.port}`); } + const ownLoopback = (p: Peer) => loopback(p.host) && [p.host, '0.0.0.0', '::'].some(h => ownEndpoints.has(`${h}:${p.port}`)); 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', 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, '/proc') || within(abs, '/dev') || within(abs, tmp) || ancestors.has(abs) || (abs === home && kind === 'access')) return; if (within(abs, repo)) { const rel = abs === repo ? '.' : abs.slice(repo.length + 1); if (/(?:^|\/)node_modules(?:\/|$)/.test(rel)) return; 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); + if (ok && !firstReach.has(rel)) firstReach.set(rel, creates ? 'created' : 'reached'); return; } - if (pathDirs.has(path.posix.dirname(abs))) { if (kind === 'exec') tools.add(abs); return; } + if (tools.has(abs) && kind === 'content') return; + if (pathDirs.has(path.posix.dirname(abs)) && kind !== 'content') { if (kind === 'exec') tools.add(abs); return; } if (within(abs, home)) { reasons.add(`reads the real HOME (${abs})`); return; } const sharedRoot = shared.find(s => within(abs, s)); if (sharedRoot && abs !== sharedRoot) { reasons.add(`reads shared temp state (${abs})`); return; } @@ -191,23 +209,26 @@ export function parseStrace(text: string, ctx: TraceContext): ReadSet { dirs.set(rel, names); break; } - case 'connect': case 'sendto': { + case 'connect': case 'sendto': case 'sendmsg': case 'sendmmsg': { const peer = sockaddr(c.args); - if (peer?.family === 'unix' && peer.path) record(absolute(c.pid, peer.path, null), 'access'); - if (peer?.family === 'inet' && !(loopback(peer.host) && ownPorts.has(peer.port))) reasons.add(`network: ${c.name} ${peer.host}:${peer.port}`); + if (peer?.family === 'inet' && !ownLoopback(peer)) reasons.add(`network: ${c.name} ${peer.host}:${peer.port}`); + if (peer?.family !== 'unix' || !peer.path || !ok) break; + const abs = peer.abstract ? null : absolute(c.pid, peer.path, null); + if (abs && (within(abs, repo) || within(abs, tmp))) record(abs, 'access', true); + else reasons.add(`external socket: ${c.name} ${peer.abstract ? '@' : ''}${peer.path}`); break; } case 'bind': case 'getsockname': break; 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', ok, c.name === 'open' && /O_CREAT/.test(c.args)); + record(absolute(c.pid, p, null), c.name === 'execve' ? (ok ? 'exec' : 'access') : c.name === 'open' ? 'content' : 'access', ok, c.name === 'open' && replacesFile(c.args)); break; } default: { if (file === null) break; const target = file === '' && dirfd ? path.posix.normalize(dirfd) : absolute(c.pid, file, dirfd); - const creates = c.name === 'openat' && /O_CREAT/.test(c.args); + const creates = c.name === 'openat' && replacesFile(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); } @@ -217,7 +238,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(), + present: [...firstReach].filter(([, how]) => how === 'reached').map(([p]) => p).sort(), tools: [...tools].sort(), }; } diff --git a/scripts/test-impact.ts b/scripts/test-impact.ts index 050867277..eedee008d 100755 --- a/scripts/test-impact.ts +++ b/scripts/test-impact.ts @@ -20,7 +20,7 @@ import * as path from 'node:path'; import { bunIsolationArgs } from '../lib/bun-isolation'; import * as localKey from '../lib/test-impact/key'; import type { KeyOptions, KeyReport } from '../lib/test-impact/key'; -import { STRACE_FLAGS, TRACE_VERSION, parseStrace, tracePath, type TraceRecord } from '../lib/test-impact/trace'; +import { STRACE_FLAGS, TRACE_VERSION, envDigests, parseStrace, tracePath, type TraceRecord } from '../lib/test-impact/trace'; const USAGE = 'usage: test-impact.ts trace [--out ] ... | key --tree [--base ] [--traces ] [--json] ... | explain [--tree ] [--base ] [--traces ] '; const DEFAULT_TRACES = path.join('.context', 'test-impact', 'traces'); @@ -62,7 +62,7 @@ function trace(root: string, argv: string[]): number { const r = spawnSync('strace', [...STRACE_FLAGS, '-o', log, '--', process.execPath, ...bunIsolationArgs(), 'test', path.join(root, file), '--timeout=30000', '--max-concurrency=1'], { cwd: root, env: { ...process.env, TMPDIR: tmp, TMP: tmp, TEMP: tmp, BUN_RUNTIME_TRANSPILER_CACHE_PATH: '0' }, timeout: 600_000, maxBuffer: 256 * 1024 * 1024, stdio: ['ignore', 'pipe', 'pipe'] }); const set = parseStrace(fs.existsSync(log) ? fs.readFileSync(log, 'utf8') : '', { repoRoot: root, home: os.homedir(), tmpDir: tmp, pathDirs: (process.env.PATH ?? '').split(path.delimiter) }); - const record: TraceRecord = { version: TRACE_VERSION, test: file, tracedAt: new Date().toISOString(), commit: head.status === 0 ? head.stdout.trim() : null, exitCode: r.status, runtime: localKey.runtimeManifest(), ...set }; + const record: TraceRecord = { version: TRACE_VERSION, test: file, tracedAt: new Date().toISOString(), commit: head.status === 0 ? head.stdout.trim() : null, exitCode: r.status, runtime: localKey.runtimeManifest(), env: envDigests(process.env), ...set }; fs.writeFileSync(tracePath(out, file), JSON.stringify(record, null, 2) + '\n'); if (r.status !== 0) failed++; const verdict = set.ineligible.length ? `ineligible: ${set.ineligible.join('; ')}` : 'eligible'; diff --git a/test/fixtures/test-impact/capture.strace b/test/fixtures/test-impact/capture.strace index 8a49b21bb..1d6bcb68b 100644 --- a/test/fixtures/test-impact/capture.strace +++ b/test/fixtures/test-impact/capture.strace @@ -1,4 +1,4 @@ -# strace 6.8 capture (x86_64 Linux, bun 1.4.4) of test/cap.test.ts under env -i, made with scripts/test-impact.ts flags. +# strace 6.8 capture (x86_64 Linux, bun 1.4.4) of test/cap.test.ts under env -i. # repo=/tmp/ti-capture/repo home=/tmp/ti-capture/home tmpdir=/tmp/ti-capture/run PATH=/usr/local/bin:/usr/bin:/bin # The test reads lib/data.txt by a computed path, lists test/data, spawns bin/tool2 (cd to bin, source ./tool2-lib.sh), # reads $HOME/.gitconfig, /tmp/ti-shared-state and .git/HEAD, probes lib/missing.txt, talks to its own loopback server, diff --git a/test/test-impact-key.test.ts b/test/test-impact-key.test.ts index 8e8cbea3a..58b7ae45c 100644 --- a/test/test-impact-key.test.ts +++ b/test/test-impact-key.test.ts @@ -10,7 +10,7 @@ import * as path from 'node:path'; import { bunIsolationArgs } from '../lib/bun-isolation'; import { ALWAYS_RUN_PATH } from '../lib/pregate/lane-selection'; import { computeKeys, MAX_HOPS, type KeyResult } from '../lib/test-impact/key'; -import { TRACE_VERSION, parseStrace, readTrace, tracePath, type TraceRecord } from '../lib/test-impact/trace'; +import { TRACE_VERSION, envDigests, parseStrace, readTrace, tracePath, type TraceRecord } from '../lib/test-impact/trace'; import { cleanup, commit, makeFixtureRepo, type FixtureRepo } from './helpers/pregate-fixture'; const ROOT = path.resolve(import.meta.dir, '..'); @@ -28,7 +28,7 @@ function fixture(baseFiles: Record = {}): FixtureRepo { } /** Write `test`'s trace from strace lines that open `reads`, list `lists` and add `extra` raw calls, through parseStrace. */ -function traceFrom(repo: FixtureRepo, test: string, opts: { reads?: string[]; lists?: Record; extra?: string[]; tracedAt?: string } = {}): string { +function traceFrom(repo: FixtureRepo, test: string, opts: { reads?: string[]; lists?: Record; extra?: string[]; tracedAt?: string; env?: Record } = {}): string { const w = repo.work; const lines = [test, ...(opts.reads ?? [])].map(p => `100 openat(AT_FDCWD<${w}>, "${w}/${p}", O_RDONLY|O_CLOEXEC) = 3<${w}/${p}>`); for (const [dir, names] of Object.entries(opts.lists ?? {})) lines.push(`100 getdents64(4<${w}/${dir}>, [${['.', '..', ...names].map(n => `{d_ino=1, d_off=1, d_reclen=24, d_type=DT_REG, d_name="${n}"}`).join(', ')}], 32768) = 96`); @@ -36,13 +36,13 @@ function traceFrom(repo: FixtureRepo, test: string, opts: { reads?: string[]; li 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'), sharedTmp: [] }); - const record: TraceRecord = { version: TRACE_VERSION, test, tracedAt: opts.tracedAt ?? '2026-11-01T00:00:00Z', commit: null, exitCode: 0, runtime: RUNTIME, ...set }; + const record: TraceRecord = { version: TRACE_VERSION, test, tracedAt: opts.tracedAt ?? '2026-11-01T00:00:00Z', commit: null, exitCode: 0, runtime: RUNTIME, env: envDigests(opts.env ?? {}), ...set }; fs.writeFileSync(tracePath(dir, test), JSON.stringify(record)); return dir; } -function keyAt(repo: FixtureRepo, tree: string, test: string, base: string | null = 'main'): KeyResult { - return computeKeys({ repoRoot: repo.work, tree, base, tests: [test], traceDir: path.join(repo.root, 'traces'), now: NOW, runtime: RUNTIME, env: {} }).results[0]!; +function keyAt(repo: FixtureRepo, tree: string, test: string, base: string | null = 'main', env: Record = {}, now = NOW): KeyResult { + return computeKeys({ repoRoot: repo.work, tree, base, tests: [test], traceDir: path.join(repo.root, 'traces'), now, runtime: RUNTIME, env }).results[0]!; } function cli(repo: FixtureRepo, args: string[]) { @@ -165,6 +165,18 @@ describe('test-impact key: ineligible', () => { expect(keyAt(repo, 'main', 'test/config-read.test.js').reasons).toEqual(['the traced run failed (exit 1)']); }); + test('a future-dated trace, or one made under other allowlisted env values or another runtime, is ineligible', () => { + const repo = fixture(); + traceFrom(repo, 'test/util.test.js', { tracedAt: '2026-11-02T00:00:00Z' }); + traceFrom(repo, 'test/retry.test.js', { env: { NODE_ENV: 'development' } }); + expect(keyAt(repo, 'main', 'test/util.test.js').reasons[0]).toStartWith('stale trace (traced 2026-11-02T00:00:00Z'); + expect(keyAt(repo, 'main', 'test/util.test.js', 'main', {}, Date.parse('2026-11-02T00:01:00Z')).reasons).toEqual([]); + expect(keyAt(repo, 'main', 'test/retry.test.js', 'main', { NODE_ENV: 'production' }).reasons).toEqual(['traced under other values of NODE_ENV']); + expect(keyAt(repo, 'main', 'test/retry.test.js', 'main', { NODE_ENV: 'development' }).reasons).toEqual([]); + const other = computeKeys({ repoRoot: repo.work, tree: 'main', base: 'main', tests: ['test/retry.test.js'], traceDir: path.join(repo.root, 'traces'), now: NOW, runtime: { ...RUNTIME, bun: '1.5.0' }, env: { NODE_ENV: 'development' } }).results[0]!; + expect(other.reasons).toEqual(['traced on another runtime (bun 1.4.4 vs 1.5.0)']); + }); + test('a tree that edits key code makes every test ineligible', () => { const repo = fixture({ 'lib/test-impact/key.ts': 'export const v = 1;\n' }); traceFrom(repo, 'test/util.test.js'); @@ -201,6 +213,7 @@ describe('scripts/test-impact.ts', () => { expect(trusted.out).toContain('key code from main:lib/test-impact/key.ts'); expect(trusted.out).toContain('INELIGIBLE test/util.test.js: stub key code from the base ref'); + traceFrom(repo, 'test/util.test.js', { env: { ...process.env, ...repo.env } }); const local = cli(repo, ['explain', '--tree', 'HEAD', '--traces', path.join(repo.root, 'traces'), 'test/util.test.js']); expect(local.status).toBe(0); expect(local.out).toMatch(/KEY test\/util\.test\.js [0-9a-f]{12}/); @@ -226,9 +239,12 @@ describe('scripts/test-impact.ts', () => { const rec = readTrace(traces, t)!; expect(rec.paths).toEqual(expect.arrayContaining([t, 'bin/tool2', 'bin/tool2-lib.sh'])); expect(rec.ineligible).toEqual([]); - const before = keyAt(repo, 'HEAD', t); - expect(before.eligible).toBe(true); + const env = { ...process.env, ...repo.env }; + const runtime = { platform: process.platform, arch: process.arch, bun: Bun.version }; + const keyNow = (tree: string) => computeKeys({ repoRoot: repo.work, tree, base: 'main', tests: [t], traceDir: traces, runtime, env }).results[0]!; + const before = keyNow('HEAD'); + expect(before.reasons).toEqual([]); commit(repo, { 'bin/tool2-lib.sh': 'greet() { echo hi; }\n# edited\n' }); - expect(keyAt(repo, 'HEAD', t).key).not.toBe(before.key); + expect(keyNow('HEAD').key).not.toBe(before.key); }); }); diff --git a/test/test-impact-trace.test.ts b/test/test-impact-trace.test.ts index 31a4f1507..5599cabd1 100644 --- a/test/test-impact-trace.test.ts +++ b/test/test-impact-trace.test.ts @@ -57,3 +57,34 @@ describe('parseStrace on a captured bun test', () => { expect(r.paths).toEqual(['lib/a b.txt', 'sub', 'sub/x.json']); }); }); + +describe('parseStrace soundness edges (ship review, #3801)', () => { + const ctx = { repoRoot: '/r', home: '/h', tmpDir: '/t', pathDirs: ['/h/bin', '/usr/bin'] }; + const parse = (...lines: string[]) => parseStrace(lines.join('\n'), ctx); + + test('only an open that creates or truncates a file makes it the run\'s own; reading an existing file first, or plain O_CREAT, keeps it an input', () => { + expect(parse('1 openat(AT_FDCWD, "/r/dist/x", O_RDONLY) = 3', '1 openat(AT_FDCWD, "/r/dist/x", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3').present).toEqual(['dist/x']); + expect(parse('1 openat(AT_FDCWD, "/r/dist/s.json", O_RDWR|O_CREAT, 0666) = 3').present).toEqual(['dist/s.json']); + expect(parse('1 openat(AT_FDCWD, "/r/out.json", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3', '1 openat(AT_FDCWD, "/r/out.json", O_RDONLY) = 3').present).toEqual([]); + }); + + test('a PATH directory under HOME exempts tool lookups and the tool itself, not other reads there; HOME itself may be stat\'ed but not listed', () => { + expect(parse('1 stat("/h/bin/git", 0x1) = -1 ENOENT (No such file or directory)', '1 execve("/h/bin/tool", ["tool"], []) = 0', '1 openat(AT_FDCWD, "/h/bin/tool", O_RDONLY) = 3', '1 newfstatat(AT_FDCWD, "/h", {...}, 0) = 0').ineligible).toEqual([]); + expect(parse('1 openat(AT_FDCWD, "/h/bin/settings.json", O_RDONLY) = 3').ineligible).toEqual(['reads the real HOME (/h/bin/settings.json)']); + expect(parse('1 getdents64(3, [{d_ino=1, d_off=1, d_reclen=24, d_type=DT_REG, d_name="x"}], 32768) = 24').ineligible).toEqual(['reads the real HOME (/h)']); + }); + + test('loopback counts as the run\'s own only for an endpoint a socket it bound reports; an outside service on a shared port number does not', () => { + const external = parse('1 connect(5, {sa_family=AF_INET, sin_port=htons(9000), sin_addr=inet_addr("127.0.0.1")}, 16) = 0', '1 getsockname(5, {sa_family=AF_INET, sin_port=htons(9000), sin_addr=inet_addr("127.0.0.2")}, [16]) = 0'); + expect(external.ineligible).toEqual(['network: connect 127.0.0.1:9000']); + const own = parse('1 bind(4, {sa_family=AF_INET, sin_port=htons(0), sin_addr=inet_addr("0.0.0.0")}, 16) = 0', '1 getsockname(4, {sa_family=AF_INET, sin_port=htons(4100), sin_addr=inet_addr("0.0.0.0")}, [16]) = 0', '1 connect(6, {sa_family=AF_INET, sin_port=htons(4100), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRESS (Operation now in progress)'); + expect(own.ineligible).toEqual([]); + expect(parse('1 sendmsg(7, {msg_name={sa_family=AF_INET, sin_port=htons(53), sin_addr=inet_addr("8.8.8.8")}, msg_namelen=16}, 0) = 40').ineligible).toEqual(['network: sendmsg 8.8.8.8:53']); + }); + + test('a unix socket outside the repo and private TMPDIR, or an abstract one, is an external dependency; a failed lookup is not', () => { + expect(parse('1 connect(3, {sa_family=AF_UNIX, sun_path="/var/run/docker.sock"}, 110) = 0').ineligible).toEqual(['external socket: connect /var/run/docker.sock']); + expect(parse('1 connect(3, {sa_family=AF_UNIX, sun_path=@"dbus-x"}, 110) = 0').ineligible).toEqual(['external socket: connect @dbus-x']); + expect(parse('1 connect(3, {sa_family=AF_UNIX, sun_path="/var/run/nscd/socket"}, 110) = -1 ENOENT (No such file or directory)', '1 connect(3, {sa_family=AF_UNIX, sun_path="/t/browse.sock"}, 110) = 0').ineligible).toEqual([]); + }); +});