From d5d058c656a5035829698b3089e882c215248026 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 19 Sep 2026 14:45:24 -0700 Subject: [PATCH 1/2] fix(gate): route test fixtures and .cargo/ into the rust gate; tsx fallback for fresh worktrees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The gate's trigger and the CI changes-filter both missed test/fixtures/ (cross-crate rust-test infrastructure — a fixture-only PR would skip cargo test everywhere) and the trigger missed .cargo/ (the plan routed it workspace but the trigger never fired). Both now route in; fixtures and .cargo/ widen to the workspace lane. - tsx resolution now falls back to the owning checkout (derived from the git common dir), so fresh worktrees without node_modules still run the rust test lane instead of silently skipping (#803's own worktree had node_modules and masked this). - Hermetic hook tests no longer depend on the worktree's install state: a stub tsx in the fixture repo delegates to the owning checkout's real tsx. --- .github/workflows/rust-tests.yml | 2 +- docs/development/pre-push-gate.md | 4 +- scripts/hooks/pre-push | 13 ++++- scripts/hooks/rust-test-targets.ts | 7 ++- test/unit/scripts/rust-test-targets.test.ts | 59 +++++++++++++++++++++ 5 files changed, 80 insertions(+), 5 deletions(-) diff --git a/.github/workflows/rust-tests.yml b/.github/workflows/rust-tests.yml index 028a56e3f..a59e5f191 100644 --- a/.github/workflows/rust-tests.yml +++ b/.github/workflows/rust-tests.yml @@ -29,7 +29,7 @@ jobs: id: filter run: | if git diff --name-only "origin/${{ github.base_ref }}...${{ github.sha }}" \ - | grep -Eq '\.rs$|(^|/)Cargo\.(toml|lock)$|^rust-toolchain|^\.cargo/'; then + | grep -Eq '\.rs$|(^|/)Cargo\.(toml|lock)$|^rust-toolchain|^\.cargo/|^test/fixtures/'; then echo "rust=true" >> "$GITHUB_OUTPUT" else echo "rust=false" >> "$GITHUB_OUTPUT" diff --git a/docs/development/pre-push-gate.md b/docs/development/pre-push-gate.md index 0827b4ddb..89ac8d9e6 100644 --- a/docs/development/pre-push-gate.md +++ b/docs/development/pre-push-gate.md @@ -8,7 +8,7 @@ Filtered by what the push changes (diff between the remote refs being updated an | Push contains | Checks | |---|---| -| Any `.rs`, `Cargo.toml`/`Cargo.lock`, `rust-toolchain*` | `cargo fmt --all --check`, then `cargo clippy --workspace --exclude freshell-tauri --all-targets -- -D warnings`, then **targeted `cargo test`**: the changed crates plus every workspace crate that transitively depends on them (full `--workspace --exclude freshell-tauri` when the change is root-level — `Cargo.lock`, toolchain, `.cargo/` — or the base is unknown). `freshell-tauri` is excluded per clippy parity. | +| Any `.rs`, `Cargo.toml`/`Cargo.lock`, `rust-toolchain*`, `test/fixtures/**` | `cargo fmt --all --check`, then `cargo clippy --workspace --exclude freshell-tauri --all-targets -- -D warnings`, then **targeted `cargo test`**: the changed crates plus every workspace crate that transitively depends on them (full `--workspace --exclude freshell-tauri` when the change is root-level — `Cargo.lock`, toolchain, `.cargo/` — or the base is unknown). `freshell-tauri` is excluded per clippy parity. Test fixtures are cross-crate rust-test infrastructure, so a `test/fixtures/**` change routes to the whole workspace. | | Any `.ts`/`.tsx`, `package.json`/`package-lock.json`, `tsconfig*` | `npm run typecheck` (client + server, tsc `--noEmit`) | | Only docs/config/other files | Nothing | | New branch with no merge-base with origin/main | Both gates (full) | @@ -27,7 +27,7 @@ Vitest/e2e/electron lanes and the real-transport clippy lanes stay with the norm - Disable for one push: `FRESHELL_PREPUSH=0 git push ...` - See routing without running checks: `FRESHELL_PREPUSH_DEBUG=1 git push --dry-run ...` - Bypass the server-side `rust-gate` (merge-time, PRs only): the owner account is a `pull_request`-mode bypass actor on the "Protect Main - No Direct Push" ruleset, so merging with a red or missing `rust-gate` is just `gh pr merge --merge` from that account — GitHub records it as a ruleset bypass with an audit entry. This is the explicit escape hatch for landing on a red base; it does NOT unlock direct pushes to main. -- If a lane's tooling is unavailable it is skipped with an accurate warning (not a failure). The hook self-heals stripped-environment contexts (ssh/agents/IDEs/cron): it sources `~/.nvm/nvm.sh` when `npm` is missing from PATH, and adds `~/.cargo/bin` when `cargo` is missing. +- If a lane's tooling is unavailable it is skipped with an accurate warning (not a failure). The hook self-heals stripped-environment contexts (ssh/agents/IDEs/cron): it sources `~/.nvm/nvm.sh` when `npm` is missing from PATH, and adds `~/.cargo/bin` when `cargo` is missing. The rust-test lane's `tsx` resolves from the pushing worktree's `node_modules`, the hook's own checkout, or the owning checkout (derived from the git common dir) — so fresh worktrees without `node_modules` still run the full lane. ## Setup diff --git a/scripts/hooks/pre-push b/scripts/hooks/pre-push index 40aafc071..29452f392 100755 --- a/scripts/hooks/pre-push +++ b/scripts/hooks/pre-push @@ -38,7 +38,7 @@ if [ "$full_gate" = 1 ]; then run_rust=1 run_ts=1 elif [ -n "$changed" ]; then - printf '%s' "$changed" | grep -Eq '\.rs$|Cargo\.(toml|lock)$|rust-toolchain' && run_rust=1 + printf '%s' "$changed" | grep -Eq '\.rs$|Cargo\.(toml|lock)$|rust-toolchain|^\.cargo/|^test/fixtures/' && run_rust=1 printf '%s' "$changed" | grep -Eq '\.(ts|tsx)$|package(-lock)?\.json$|tsconfig' && run_ts=1 fi @@ -51,6 +51,17 @@ if [ -x node_modules/.bin/tsx ]; then TSX="node_modules/.bin/tsx" elif [ -x "$HOOK_DIR/../../node_modules/.bin/tsx" ]; then TSX="$HOOK_DIR/../../node_modules/.bin/tsx" +else + # Fresh worktrees often lack node_modules; the owning checkout (the one + # whose .git dir this worktree shares) has it. Derive the owning root + # from the git common dir so worktree pushes still get the full gate. + common_dir=$(git rev-parse --path-format=absolute --git-common-dir 2>/dev/null) + if [ -n "$common_dir" ] && [ -d "$common_dir" ]; then + owning_root="${common_dir%/.git}" + if [ "$owning_root" != "$common_dir" ] && [ -x "$owning_root/node_modules/.bin/tsx" ]; then + TSX="$owning_root/node_modules/.bin/tsx" + fi + fi fi # Targeted cargo-test plan: computed only when the rust gate fired, so diff --git a/scripts/hooks/rust-test-targets.ts b/scripts/hooks/rust-test-targets.ts index 253aa1674..a4ae2bca9 100644 --- a/scripts/hooks/rust-test-targets.ts +++ b/scripts/hooks/rust-test-targets.ts @@ -30,7 +30,12 @@ function isRustRelevant(p: string): boolean { /(^|\/)Cargo\.toml$/.test(p) || /(^|\/)Cargo\.lock$/.test(p) || /^rust-toolchain/.test(p) || - /^\.cargo\//.test(p) + /^\.cargo\//.test(p) || + // Test fixtures are cross-crate rust-test infrastructure (e.g. the + // codex fake app-server consumed by freshell-codex/freshell-ws tests): + // a fixture change can break any crate's tests, and no single crate + // owns the path, so it routes to the whole workspace. + /^test\/fixtures\//.test(p) ) } diff --git a/test/unit/scripts/rust-test-targets.test.ts b/test/unit/scripts/rust-test-targets.test.ts index ac8c367f7..9adbbae08 100644 --- a/test/unit/scripts/rust-test-targets.test.ts +++ b/test/unit/scripts/rust-test-targets.test.ts @@ -96,6 +96,18 @@ describe('computeRustTestPlan', () => { expect(computeRustTestPlan(['tools/foo.rs'], graph)).toEqual({ mode: 'workspace' }) }) + it('treats test fixtures consumed by rust tests as workspace-wide', () => { + expect( + computeRustTestPlan(['test/fixtures/coding-cli/codex-app-server/fake-app-server.mjs'], graph), + ).toEqual({ mode: 'workspace' }) + }) + + it('widens to the workspace when a fixture change mixes with a crate change', () => { + expect( + computeRustTestPlan(['test/fixtures/x.mjs', 'crates/freshell-terminal/src/x.rs'], graph), + ).toEqual({ mode: 'workspace' }) + }) + it('treats crate-local manifests as crate changes', () => { expect(computeRustTestPlan(['crates/freshell-ws/Cargo.toml'], graph)).toEqual({ mode: 'packages', @@ -197,11 +209,30 @@ describe('planToInvocation', () => { describe('pre-push hook routing (hermetic fixture repo)', () => { const hookPath = path.resolve(import.meta.dirname, '../../../scripts/hooks/pre-push') + + // The hook resolves tsx from cwd, the script dir, or the owning checkout. + // A fresh worktree has no node_modules, so pin the real tsx from the + // owning checkout and stub it into the fixture repo — the hermetic tests + // then exercise the hook's full chain regardless of the worktree's + // install state (the hook passes the real rust-test-targets.ts path as + // the script argument, so the stub only supplies the runtime). + const owningRoot = (() => { + const commonDir = spawnSync( + 'git', + ['rev-parse', '--path-format=absolute', '--git-common-dir'], + { cwd: import.meta.dirname, encoding: 'utf8' }, + ) + return (commonDir.stdout ?? '').trim().replace(/\/\.git$/, '') + })() + const realTsx = path.join(owningRoot, 'node_modules', '.bin', 'tsx') + let fixtureRoot: string let baseSha: string let rustSha: string let docsSha: string let tauriSha: string + let fixtureSha: string + let cargoConfigSha: string function git(args: string[], opts: { cwd: string; stdin?: string } = { cwd: '' }): string { const res = spawnSync('git', args, { cwd: opts.cwd, encoding: 'utf8' }) @@ -229,7 +260,12 @@ describe('pre-push hook routing (hermetic fixture repo)', () => { } beforeAll(() => { + if (!fs.existsSync(realTsx)) throw new Error(`owning checkout tsx missing: ${realTsx}`) fixtureRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'prepush-routing-')) + const stubDir = path.join(fixtureRoot, 'node_modules/.bin') + fs.mkdirSync(stubDir, { recursive: true }) + fs.writeFileSync(path.join(stubDir, 'tsx'), `#!/bin/sh\nexec "${realTsx}" "$@"\n`) + fs.chmodSync(path.join(stubDir, 'tsx'), 0o755) git(['init', '-q', '-b', 'main'], { cwd: fixtureRoot }) writeFixture( path.join(fixtureRoot, 'Cargo.toml'), @@ -275,6 +311,15 @@ describe('pre-push hook routing (hermetic fixture repo)', () => { writeFixture(path.join(fixtureRoot, 'crates/freshell-tauri/src/main.rs'), 'fn main() {}\n') tauriSha = commit('rust: tauri-only change') + + writeFixture( + path.join(fixtureRoot, 'test/fixtures/coding-cli/codex-app-server/fake-app-server.mjs'), + '// fixture change\n', + ) + fixtureSha = commit('test fixture change') + + writeFixture(path.join(fixtureRoot, '.cargo/config.toml'), '[build]\n') + cargoConfigSha = commit('cargo config change') }) afterAll(() => { @@ -311,6 +356,20 @@ describe('pre-push hook routing (hermetic fixture repo)', () => { expect(out.stderr).toContain('test_mode=skip') }) + it('runs the workspace test lane for test-fixture changes', () => { + const out = runHook(fixtureSha, tauriSha) + expect(out.status).toBe(0) + expect(out.stderr).toContain('run_rust=1') + expect(out.stderr).toContain('test_mode=workspace') + }) + + it('runs the rust gate for cargo-config changes', () => { + const out = runHook(cargoConfigSha, fixtureSha) + expect(out.status).toBe(0) + expect(out.stderr).toContain('run_rust=1') + expect(out.stderr).toContain('test_mode=workspace') + }) + it('runs the full gate when the merge base is unknown', () => { const out = runHook('0000000000000000000000000000000000000001', '0000000000000000000000000000000000000002') expect(out.status).toBe(0) From 680bd5e51ebd6450b9bafa3b87bd6e13f4c4bfb4 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 19 Sep 2026 15:13:42 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(gate):=20strip=20git's=20hook-env=20ove?= =?UTF-8?q?rrides=20before=20lanes=20=E2=80=94=20GIT=5FDIR=20leak=20corrup?= =?UTF-8?q?ted=20the=20shared=20repo?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git exports GIT_DIR/GIT_PREFIX (the pushing worktree's gitdir) into the pre-push hook. Unstripped, they leak through cargo test into every test-side git child: freshell-server's git-touching tests then resolved against the SHARED repository instead of their tempdirs — 'git init' re-initialized the main checkout's config (observed writing core.bare=true and colliding on config.lock across 96 parallel tests), and one test's add/commit landed on main's HEAD. Reproduced solo with GIT_DIR= (exact panic) and repaired the damage twice. The hook's own git calls use cwd discovery (equivalent in a worktree), so unsetting the overrides is behavior-neutral for routing. Hermetic red: a polluted GIT_DIR broke the hook's routing git calls pre-fix. --- docs/development/pre-push-gate.md | 2 +- scripts/hooks/pre-push | 11 +++++++++++ test/unit/scripts/rust-test-targets.test.ts | 20 ++++++++++++++++++-- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/docs/development/pre-push-gate.md b/docs/development/pre-push-gate.md index 89ac8d9e6..07e69221c 100644 --- a/docs/development/pre-push-gate.md +++ b/docs/development/pre-push-gate.md @@ -27,7 +27,7 @@ Vitest/e2e/electron lanes and the real-transport clippy lanes stay with the norm - Disable for one push: `FRESHELL_PREPUSH=0 git push ...` - See routing without running checks: `FRESHELL_PREPUSH_DEBUG=1 git push --dry-run ...` - Bypass the server-side `rust-gate` (merge-time, PRs only): the owner account is a `pull_request`-mode bypass actor on the "Protect Main - No Direct Push" ruleset, so merging with a red or missing `rust-gate` is just `gh pr merge --merge` from that account — GitHub records it as a ruleset bypass with an audit entry. This is the explicit escape hatch for landing on a red base; it does NOT unlock direct pushes to main. -- If a lane's tooling is unavailable it is skipped with an accurate warning (not a failure). The hook self-heals stripped-environment contexts (ssh/agents/IDEs/cron): it sources `~/.nvm/nvm.sh` when `npm` is missing from PATH, and adds `~/.cargo/bin` when `cargo` is missing. The rust-test lane's `tsx` resolves from the pushing worktree's `node_modules`, the hook's own checkout, or the owning checkout (derived from the git common dir) — so fresh worktrees without `node_modules` still run the full lane. +- If a lane's tooling is unavailable it is skipped with an accurate warning (not a failure). The hook self-heals stripped-environment contexts (ssh/agents/IDEs/cron): it sources `~/.nvm/nvm.sh` when `npm` is missing from PATH, and adds `~/.cargo/bin` when `cargo` is missing. The rust-test lane's `tsx` resolves from the pushing worktree's `node_modules`, the hook's own checkout, or the owning checkout (derived from the git common dir) — so fresh worktrees without `node_modules` still run the full lane. The hook also strips git's hook-env overrides (`GIT_DIR` and friends) before anything runs: unstripped, they leak into cargo-test children and every test-side `git` call resolves against the shared repository instead of the test's tempdir (observed re-initializing the main checkout's config and committing test fixtures onto main's HEAD). ## Setup diff --git a/scripts/hooks/pre-push b/scripts/hooks/pre-push index 29452f392..865462824 100755 --- a/scripts/hooks/pre-push +++ b/scripts/hooks/pre-push @@ -12,6 +12,17 @@ if [ "${FRESHELL_PREPUSH:-1}" = "0" ]; then exit 0 fi +# git exports GIT_DIR/GIT_PREFIX (the pushing worktree's gitdir) into this +# hook. If they leak into the lane processes, every test-side `git` child +# resolves against the SHARED repository instead of the test's own tempdir: +# a `git init` inside a freshell-server test re-initialized the main +# checkout's config (observed writing core.bare=true and colliding on +# config.lock), and test add/commit landed on the main HEAD. The hook's own +# git calls use cwd discovery (equivalent in a worktree), so strip the +# overrides before anything runs. +unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_COMMON_DIR GIT_PREFIX \ + GIT_CONFIG_PARAMETERS GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES + ZERO=0000000000000000000000000000000000000000 changed="" full_gate=0 diff --git a/test/unit/scripts/rust-test-targets.test.ts b/test/unit/scripts/rust-test-targets.test.ts index 9adbbae08..ca2402836 100644 --- a/test/unit/scripts/rust-test-targets.test.ts +++ b/test/unit/scripts/rust-test-targets.test.ts @@ -326,10 +326,14 @@ describe('pre-push hook routing (hermetic fixture repo)', () => { fs.rmSync(fixtureRoot, { recursive: true, force: true }) }) - function runHook(localSha: string, remoteSha: string): { status: number; stderr: string } { + function runHook( + localSha: string, + remoteSha: string, + extraEnv: Record = {}, + ): { status: number; stderr: string } { const res = spawnSync('bash', [hookPath], { input: `refs/heads/x ${localSha} refs/heads/x ${remoteSha}\n`, - env: { ...process.env, FRESHELL_PREPUSH_DEBUG: '1' }, + env: { ...process.env, ...extraEnv, FRESHELL_PREPUSH_DEBUG: '1' }, encoding: 'utf8', cwd: fixtureRoot, }) @@ -349,6 +353,18 @@ describe('pre-push hook routing (hermetic fixture repo)', () => { expect(out.stderr).toContain('test_mode=packages test_pkgs=freshell-freshagent freshell-server freshell-ws') }) + it('strips git hook env overrides (GIT_DIR et al) before any lane runs', () => { + // git exports GIT_DIR/GIT_PREFIX into real pre-push invocations (the + // pushing worktree's gitdir). Unstripped, a bogus or stale GIT_DIR breaks + // the hook's own routing git calls — and in non-debug mode the leaked + // vars redirect every test-side git child at the SHARED repository + // (observed: cargo-test children re-initialized the main checkout's + // config). The hook must resolve everything via cwd discovery instead. + const out = runHook(rustSha, baseSha, { GIT_DIR: '/nonexistent-prepush-hook-env' }) + expect(out.status).toBe(0) + expect(out.stderr).toContain('test_mode=packages test_pkgs=freshell-freshagent freshell-server freshell-ws') + }) + it('skips the test lane for tauri-only changes (clippy parity)', () => { const out = runHook(tauriSha, docsSha) expect(out.status).toBe(0)