From fe99b200dcc52c702bf5518f6893c517c8ad6737 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 24 Oct 2026 20:43:29 +0000 Subject: [PATCH] fix(pregate): past the members size cap, a crate's run root is undecided and its tests unclaimed Third port review round (outside voice): admitting every crate past the cap made a non-member run as 'cargo test -p ' from the workspace root, where a same-named member answered and its passing tests stood in for the orphan's failing one (a false green). Keep the conservative membership for dependency edges, but leave such a crate's workspace undecided (null): planRuns leaves its files unclaimed and the lane incomplete. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 4 ++-- lib/pregate/checks/lanes.ts | 4 ++-- lib/pregate/rust.ts | 17 ++++++++++++----- test/pregate-rust.test.ts | 18 +++++++++++++++++- 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21a197393..c21f4f0c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,12 +8,12 @@ - **`lib/pregate/rust.ts`** (new, vilano93): a crate is the nearest Cargo.toml with `[package]`. Its `.rs` files under `tests/` or holding a `#[cfg(test)]` are tests, and every `.rs` file of a crate and its Cargo.toml select that crate's tests and those of every crate depending on it. Manifests are parsed as TOML, so renamed, inherited, dotted, table-form and target-specific dependencies all count, and a crate runs from the workspace it is a member of under Cargo's rules (members, exclude, `package.workspace`). Nothing runs cargo to build the graph. - **`lib/pregate/checks/lanes.ts`** (vilano93): selected Rust tests run per crate as `cargo test -p ` from the workspace root and are judged on the summed `test result:` lines; zero tests is ZERO-RUN and a missing cargo is incomplete. A JS/TS test owned by a nested package.json that declares vitest or jest runs from that package through the package manager its `packageManager` field or nearest lockfile names (`pnpm exec`, `yarn`, `bun x`, else `npx`). - **`lib/pregate/checks/lanes.ts`**: the crate name comes from the repo's Cargo.toml and reaches the lane's shell. Before, a crate named `evil;touch pwned` ran `touch pwned` in the workspace, and a name containing `{files}` spliced the file list into the command. A name outside Cargo's grammar (letters, digits, `_`, `-`) now gets no runner, so its files are unclaimed and the lane is incomplete, as Cargo would refuse the manifest anyway; a valid name is quoted like every file and cwd. The lane only runs with `--allow-repo-commands`, under which the repo's tests run anyway, but no other repo input reached the shell unquoted. -- **`lib/pregate/rust.ts`**: Cargo `members` globs are matched per path component and by Unicode code point, in one pass. The first cut used the policy glob matcher, which matched `[ab]` literally (a member listed that way lost its inherited workspace dependencies, so its tests were never selected) and backtracked for seconds on a hostile glob. Past 4096 characters of `members` globs, every crate under the workspace counts as a member: a wrong member fails loudly in cargo, while a dropped one would be a silent false green. +- **`lib/pregate/rust.ts`**: Cargo `members` globs are matched per path component and by Unicode code point, in one pass. The first cut used the policy glob matcher, which matched `[ab]` literally (a member listed that way lost its inherited workspace dependencies, so its tests were never selected) and backtracked for seconds on a hostile glob. Past 4096 characters of `members` globs, membership is undecided: every crate under the workspace counts as a member for dependency edges (more tests, never fewer), but its own tests are unclaimed and the lane is incomplete, because running `cargo test -p ` from the wrong root can test a same-named member instead. - **`lib/pregate/rust.ts`**: a plain `#[test]` or `#[tokio::test]` function in `src/` makes the file a test, so a dependent crate tested that way runs when its dependency changes. The `#[cfg(...)]` scan reads each attribute once, up to its closing `]`, so a file of unclosed attributes no longer takes seconds, and an attribute it cannot close counts as a test (more tests selected, never fewer). - **`lib/pregate/init-draft.ts`** (vilano93): `init` notes that Rust tests run per crate. ### Tests -- `test/pregate-rust.test.ts` (new, vilano93) covers the crate model, the dependency forms, workspace membership, the cargo summary parser and three end-to-end lane runs. New cases: crates named `evil;touch pwned-by-crate-name` and `evil{files}` never reach the shell (nothing is created, both are unclaimed) while `naïve_crate-2` still runs quoted; `crates/[ab]`, `nested/**/leaf` and `unicode/?` (a 📦 directory) members inherit workspace dependencies; a plain `#[test]` dependent and a `cfg(any(test, feature = "<300 chars>"))` are tests; and a hostile members glob, a million unmatched `[` and 100,000 unclosed `#[cfg(` run in a supervised Bun that kills a super-linear version at 30 s. Each failed before its fix. +- `test/pregate-rust.test.ts` (new, vilano93) covers the crate model, the dependency forms, workspace membership, the cargo summary parser and three end-to-end lane runs. New cases: crates named `evil;touch pwned-by-crate-name` and `evil{files}` never reach the shell (nothing is created, both are unclaimed) while `naïve_crate-2` still runs quoted; `crates/[ab]`, `nested/**/leaf` and `unicode/?` (a 📦 directory) members inherit workspace dependencies; a plain `#[test]` dependent and a `cfg(any(test, feature = "<300 chars>"))` are tests; and a same-named non-member past the size cap is unclaimed rather than run as the member; a hostile members glob, a million unmatched `[` and 100,000 unclosed `#[cfg(` run in a supervised Bun that kills a super-linear version at 30 s. Each failed before its fix. - The cargo stand-in now wins PATH lookup everywhere. On windows-free-shard (5) two end-to-end cases failed deterministically because the shim PATH was joined with `:` (Git Bash never saw it) and the runner's real cargo ran against the fixture. On hosts where Bun and cargo share a directory, the lane puts Bun's directory first and the real cargo won the same way. The shim PATH now uses the platform delimiter, adds `cargo.cmd` for Windows lookup, and on POSIX the lane's bash puts the shim first again through `BASH_ENV`. - `test/pregate-selection.test.ts` (vilano93) covers nested pnpm/yarn/vitest/jest packages. Its `pnpm` stand-in gets the same `BASH_ENV` fix: on a host with a real pnpm beside Bun, the end-to-end case installed vitest from the network and came back incomplete. diff --git a/lib/pregate/checks/lanes.ts b/lib/pregate/checks/lanes.ts index b2c3aaca1..df83154fa 100644 --- a/lib/pregate/checks/lanes.ts +++ b/lib/pregate/checks/lanes.ts @@ -177,8 +177,8 @@ export function planRuns(ctx: PregateContext, files: readonly string[]): RunPlan if (f.endsWith('.rs')) { model ??= cargoModel(listRepoFiles(ctx.repoRoot), rel => readRepoFile(ctx.repoRoot, rel)); const crate = model.crateOf(f); - if (!crate) { unclaimed.push(f); continue; } - const cwd = crate.workspace; + const cwd = crate?.workspace ?? null; + if (!crate || cwd === null) { unclaimed.push(f); continue; } const key = `${cwd}\0${crate.name}`; if (!cargo.has(key)) cargo.set(key, { name: crate.name, cwd, files: [] }); cargo.get(key)!.files.push(under(cwd, f)); diff --git a/lib/pregate/rust.ts b/lib/pregate/rust.ts index d72d1f5d7..6e0028229 100644 --- a/lib/pregate/rust.ts +++ b/lib/pregate/rust.ts @@ -33,8 +33,8 @@ interface DepSpec { path: string | null; name: string } export interface Crate { root: string; name: string; - /** Where `cargo test -p ` runs: the root of the workspace it is a member of, else the crate directory. */ - workspace: string; + /** Where `cargo test -p ` runs: the root of the workspace it is a member of, else the crate directory; null when membership is undecided. */ + workspace: string | null; /** Roots of the tracked crates it depends on. */ deps: string[]; } @@ -99,7 +99,11 @@ function memberMatch(glob: string, rel: string): boolean { } return row[p.length]!; } -/** Past this many characters of `members` globs a workspace admits every crate under it: a wrong member fails loudly in cargo, a dropped one is a silent false green. */ +/** + * Past this many characters of `members` globs a workspace's membership is undecided: every crate under it counts as a + * member for dependency edges (more tests, never fewer), but where its tests run is unknown, so they are unclaimed and the + * lane is incomplete (running from the wrong root can test a same-named member instead). + */ const MAX_MEMBER_GLOB_CHARS = 4096; const inside = (dir: string, root: string) => root === '' || dir === root || dir.startsWith(`${root}/`); const relTo = (dir: string, root: string) => (root === '' ? dir : dir === root ? '' : dir.slice(root.length + 1)); @@ -145,6 +149,7 @@ export function cargoModel(files: readonly string[], read: ReadFile): CargoModel return workspaces.filter(w => inside(root, w.dir)).sort((a, b) => b.dir.length - a.dir.length)[0] ?? null; }; const memberCache = new Map>(); + const undecided = new Set(); const membersOf = (ws: Manifest): Set => { const cached = memberCache.get(ws.dir); if (cached) return cached; @@ -157,7 +162,9 @@ export function cargoModel(files: readonly string[], read: ReadFile): CargoModel // Cargo's rules: the root package and a literally listed member always belong; a glob match or a path dependency of a // member inside the workspace belongs unless `exclude` names it or a directory above it. const literal = (r: string) => globs.some(g => !/[*?[]/.test(g) && relTo(r, ws.dir) === g); - const members = new Set([...own].filter(r => r === ws.dir || literal(r) || ((globs.join('').length > MAX_MEMBER_GLOB_CHARS || globs.some(g => memberMatch(g, relTo(r, ws.dir)))) && !excluded(r)))); + const oversized = globs.join('').length > MAX_MEMBER_GLOB_CHARS; + const members = new Set([...own].filter(r => r === ws.dir || literal(r) || ((oversized || globs.some(g => memberMatch(g, relTo(r, ws.dir)))) && !excluded(r)))); + if (oversized) for (const r of members) if (r !== ws.dir && !literal(r)) undecided.add(r); memberCache.set(ws.dir, members); const queue = [...members]; while (queue.length) { @@ -176,7 +183,7 @@ export function cargoModel(files: readonly string[], read: ReadFile): CargoModel for (const root of roots) { const ws = candidate(root); const member = ws !== null && membersOf(ws).has(root) ? ws : null; - crates.set(root, { root, name: nameOf(root)!, workspace: member?.dir ?? root, deps: resolve(depSpecs(manifests.get(root)!, member)).filter(d => d !== root) }); + crates.set(root, { root, name: nameOf(root)!, workspace: undecided.has(root) ? null : member?.dir ?? root, deps: resolve(depSpecs(manifests.get(root)!, member)).filter(d => d !== root) }); } const crateOf = (file: string): Crate | null => { for (let d = path.posix.dirname(file); ; d = path.posix.dirname(d)) { diff --git a/test/pregate-rust.test.ts b/test/pregate-rust.test.ts index c57298ac0..4085f3a44 100644 --- a/test/pregate-rust.test.ts +++ b/test/pregate-rust.test.ts @@ -251,6 +251,22 @@ describe('Cargo membership and test detection (port review)', () => { expect(dependentsOf(files, 'a/Cargo.toml')).toEqual(['a/src/lib.rs', 'b/src/lib.rs']); }); + // Value: protects=past the members-glob size cap, where a crate's tests run is undecided, so its files are unclaimed (the + // lane incomplete) instead of running `cargo test -p ` from a root where a same-named member answers; fails_when=an + // oversized workspace admits every crate as runnable and the orphan's failing test is replaced by the member's passing + // ones (a false green); why_new=third port review round; seam=none; no_claim=a real cargo (plan only) + test('past the members size cap a same-named non-member is unclaimed, not run as the member', () => { + const repo = makeFixtureRepo({ pregate: null }); + repos.push(repo); + commit(repo, { + 'Cargo.toml': `[workspace]\nmembers = ["[${'m'.repeat(4100)}]ember"]\n`, + 'member/Cargo.toml': pkg('same'), 'member/tests/m.rs': '#[test]\nfn m() {}\n', + 'orphan/Cargo.toml': pkg('same'), 'orphan/tests/t.rs': '#[test]\nfn t() { panic!() }\n', + }, 'oversized members'); + const ctx = { repoRoot: repo.work, config: DEFAULT_CONFIG, env: cargoShim(repo) } as unknown as PregateContext; + expect(planRuns(ctx, ['orphan/tests/t.rs'])).toEqual({ groups: [], unclaimed: ['orphan/tests/t.rs'], missing: [], noPubspec: [] }); + }); + // Value: protects=a hostile members glob or a file of unclosed cfg attributes cannot stall lane selection, which runs before // any runner deadline; each probe runs in its own supervised Bun so a regression is killed and fails instead of hanging the // file; fails_when=member globs backtrack again (an exponential '*a*a…b', or a rescan per unmatched '[') or the cfg scan @@ -265,7 +281,7 @@ describe('Cargo membership and test detection (port review)', () => { const brackets = run(['['.repeat(1000000)], Array.from({ length: 10 }, (_, i) => 'c' + i)).crates.get('c0').workspace; console.log(JSON.stringify([exp, brackets]));`); expect(r.error).toBeUndefined(); - expect(r.stdout.trim()).toBe(JSON.stringify(['a'.repeat(40), ''])); + expect(r.stdout.trim()).toBe(JSON.stringify(['a'.repeat(40), null])); }, 60_000); test('a run of unclosed cfg attributes is scanned once and counts as a test', () => {