mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-01 06:24:41 +01:00
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 <name>' 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>
This commit is contained in:
1 parent
d96425a419
commit
fe99b200dc
4 files changed
+33
-10
No files matched your search
+2
-2
@@ -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/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 <name>` 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`** (vilano93): selected Rust tests run per crate as `cargo test -p <name>` 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/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 <name>` 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/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.
|
- **`lib/pregate/init-draft.ts`** (vilano93): `init` notes that Rust tests run per crate.
|
||||||
|
|
||||||
### Tests
|
### 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`.
|
- 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.
|
- `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.
|
||||||
|
|
||||||
|
|||||||
@@ -177,8 +177,8 @@ export function planRuns(ctx: PregateContext, files: readonly string[]): RunPlan
|
|||||||
if (f.endsWith('.rs')) {
|
if (f.endsWith('.rs')) {
|
||||||
model ??= cargoModel(listRepoFiles(ctx.repoRoot), rel => readRepoFile(ctx.repoRoot, rel));
|
model ??= cargoModel(listRepoFiles(ctx.repoRoot), rel => readRepoFile(ctx.repoRoot, rel));
|
||||||
const crate = model.crateOf(f);
|
const crate = model.crateOf(f);
|
||||||
if (!crate) { unclaimed.push(f); continue; }
|
const cwd = crate?.workspace ?? null;
|
||||||
const cwd = crate.workspace;
|
if (!crate || cwd === null) { unclaimed.push(f); continue; }
|
||||||
const key = `${cwd}\0${crate.name}`;
|
const key = `${cwd}\0${crate.name}`;
|
||||||
if (!cargo.has(key)) cargo.set(key, { name: crate.name, cwd, files: [] });
|
if (!cargo.has(key)) cargo.set(key, { name: crate.name, cwd, files: [] });
|
||||||
cargo.get(key)!.files.push(under(cwd, f));
|
cargo.get(key)!.files.push(under(cwd, f));
|
||||||
|
|||||||
+12
-5
@@ -33,8 +33,8 @@ interface DepSpec { path: string | null; name: string }
|
|||||||
export interface Crate {
|
export interface Crate {
|
||||||
root: string;
|
root: string;
|
||||||
name: string;
|
name: string;
|
||||||
/** Where `cargo test -p <name>` runs: the root of the workspace it is a member of, else the crate directory. */
|
/** Where `cargo test -p <name>` runs: the root of the workspace it is a member of, else the crate directory; null when membership is undecided. */
|
||||||
workspace: string;
|
workspace: string | null;
|
||||||
/** Roots of the tracked crates it depends on. */
|
/** Roots of the tracked crates it depends on. */
|
||||||
deps: string[];
|
deps: string[];
|
||||||
}
|
}
|
||||||
@@ -99,7 +99,11 @@ function memberMatch(glob: string, rel: string): boolean {
|
|||||||
}
|
}
|
||||||
return row[p.length]!;
|
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 MAX_MEMBER_GLOB_CHARS = 4096;
|
||||||
const inside = (dir: string, root: string) => root === '' || dir === root || dir.startsWith(`${root}/`);
|
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));
|
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;
|
return workspaces.filter(w => inside(root, w.dir)).sort((a, b) => b.dir.length - a.dir.length)[0] ?? null;
|
||||||
};
|
};
|
||||||
const memberCache = new Map<string, Set<string>>();
|
const memberCache = new Map<string, Set<string>>();
|
||||||
|
const undecided = new Set<string>();
|
||||||
const membersOf = (ws: Manifest): Set<string> => {
|
const membersOf = (ws: Manifest): Set<string> => {
|
||||||
const cached = memberCache.get(ws.dir);
|
const cached = memberCache.get(ws.dir);
|
||||||
if (cached) return cached;
|
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
|
// 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.
|
// 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 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);
|
memberCache.set(ws.dir, members);
|
||||||
const queue = [...members];
|
const queue = [...members];
|
||||||
while (queue.length) {
|
while (queue.length) {
|
||||||
@@ -176,7 +183,7 @@ export function cargoModel(files: readonly string[], read: ReadFile): CargoModel
|
|||||||
for (const root of roots) {
|
for (const root of roots) {
|
||||||
const ws = candidate(root);
|
const ws = candidate(root);
|
||||||
const member = ws !== null && membersOf(ws).has(root) ? ws : null;
|
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 => {
|
const crateOf = (file: string): Crate | null => {
|
||||||
for (let d = path.posix.dirname(file); ; d = path.posix.dirname(d)) {
|
for (let d = path.posix.dirname(file); ; d = path.posix.dirname(d)) {
|
||||||
|
|||||||
@@ -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']);
|
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 <name>` 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
|
// 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
|
// 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
|
// 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;
|
const brackets = run(['['.repeat(1000000)], Array.from({ length: 10 }, (_, i) => 'c' + i)).crates.get('c0').workspace;
|
||||||
console.log(JSON.stringify([exp, brackets]));`);
|
console.log(JSON.stringify([exp, brackets]));`);
|
||||||
expect(r.error).toBeUndefined();
|
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);
|
}, 60_000);
|
||||||
|
|
||||||
test('a run of unclosed cfg attributes is scanned once and counts as a test', () => {
|
test('a run of unclosed cfg attributes is scanned once and counts as a test', () => {
|
||||||
|
|||||||
Reference in new issue
Block a user