From 37ba160923d8e8afe15d7981a31d2535c74b41bc Mon Sep 17 00:00:00 2001 From: oratis Date: Sun, 9 Aug 2026 21:56:56 +0800 Subject: [PATCH] fix(test): stop a fixture from re-initialising the real repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `git` reads GIT_DIR, GIT_WORK_TREE and GIT_INDEX_FILE from the environment, and a git hook sets all three. The pre-commit gate runs the whole suite from inside `git commit`, so `workspace-diff.test.ts` calling `git init` on a temp directory did not initialise the temp directory — it re-initialised the developer's own checkout, as bare, and wrote `user.name = DeepCode Test` into its config. Every git command in that checkout then failed with "this operation must be run in a work tree". Repairing it means resetting core.bare and unsetting the injected identity; refs and objects are untouched. `collectWorkspaceDiff` itself already scrubs the environment. The fixture that tests it did not. Three other fixtures had each grown their own private copy of the scrub, and two of them carry a comment describing this exact failure. That is the tell: the protection existed, it was known, and it was being passed along by word of mouth rather than enforced — so a fixture written later simply did not get it. All four now share `gitSpawnEnv`, and a check fails the build if a test spawns `git` without it. Verified by reverting the fixture and watching the check name it. Not covered: `scripts/gen-release-notes.ts` also spawns git unscrubbed, but it runs only in the release workflow on a fresh checkout where GIT_DIR is never set, and it cannot import from the workspace without a build step it currently does not have. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 17 ++++++++ apps/cli/src/commands.test.ts | 10 ++--- apps/server/src/workspace-diff.test.ts | 21 +++++++--- packages/core/src/worktree/index.test.ts | 13 ++---- scripts/git-env-isolation.test.ts | 52 ++++++++++++++++++++++++ 5 files changed, 90 insertions(+), 23 deletions(-) create mode 100644 scripts/git-env-isolation.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9899a44..2342a33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,23 @@ All notable changes to DeepCode are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### 🐛 Fixed + +- **The test suite could re-initialise your own repository.** `git` reads + `GIT_DIR` from the environment and a git hook sets it, so a fixture calling + `git init` on a temp directory from inside the pre-commit gate did not + initialise the temp directory — it re-initialised the developer's checkout as + bare and wrote the test identity into its config, after which every git + command there failed with "this operation must be run in a work tree". + `apps/server/src/workspace-diff.test.ts` was the fixture; the code it tests + scrubs the environment, the fixture did not. Three other fixtures had each + grown their own copy of the scrub, two carrying a comment describing this + precise failure — a convention passed by word of mouth that had stopped being + enforced. They now share `gitSpawnEnv`, and a check fails the build if a test + spawns `git` without it. + ## [0.3.0] — 2026-08-08 A workspace-governance layer: what the agent may touch, what it changed, and how diff --git a/apps/cli/src/commands.test.ts b/apps/cli/src/commands.test.ts index 8653052..3ac1e83 100644 --- a/apps/cli/src/commands.test.ts +++ b/apps/cli/src/commands.test.ts @@ -4,19 +4,15 @@ import { promisify } from 'node:util'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; -import { SessionManager } from '@deepcode/core'; +import { SessionManager, gitSpawnEnv } from '@deepcode/core'; import { CommandRegistry, type SessionContext } from './commands.js'; const exec = promisify(execFile); // Strip inherited GIT_* so this test's `git init` can't be hijacked by a leaked // GIT_DIR when the suite runs inside a git hook (which would re-init the real -// repo as bare). Mirrors the inline scrub in commands.ts. -function gitEnv(): NodeJS.ProcessEnv { - const env: NodeJS.ProcessEnv = { ...process.env }; - for (const k of Object.keys(env)) if (k.startsWith('GIT_')) delete env[k]; - return env; -} +// repo as bare). +const gitEnv = gitSpawnEnv; function makeContext(overrides: Partial = {}): SessionContext { return { diff --git a/apps/server/src/workspace-diff.test.ts b/apps/server/src/workspace-diff.test.ts index 7a450cc..dfd696c 100644 --- a/apps/server/src/workspace-diff.test.ts +++ b/apps/server/src/workspace-diff.test.ts @@ -6,9 +6,18 @@ import { promisify } from 'node:util'; import { afterEach, describe, expect, it } from 'vitest'; +import { gitSpawnEnv } from '@deepcode/core'; + import { collectWorkspaceDiff } from './workspace-diff.js'; const exec = promisify(execFile); + +// `git init` inherits GIT_DIR. Run the suite from a git hook — which is exactly +// what the pre-commit gate does — and this fixture re-initialises the developer's +// own repository as bare and writes the test identity into its config. The code +// under test scrubs the environment; this fixture must too. +const GIT = { env: gitSpawnEnv() }; + let root: string | undefined; afterEach(async () => { @@ -18,14 +27,14 @@ afterEach(async () => { async function repository(): Promise { root = await mkdtemp(join(tmpdir(), 'deepcode-workspace-diff-')); - await exec('git', ['init', '-q'], { cwd: root }); - await exec('git', ['config', 'user.email', 'deepcode@example.invalid'], { cwd: root }); - await exec('git', ['config', 'user.name', 'DeepCode Test'], { cwd: root }); + await exec('git', ['init', '-q'], { cwd: root, ...GIT }); + await exec('git', ['config', 'user.email', 'deepcode@example.invalid'], { cwd: root, ...GIT }); + await exec('git', ['config', 'user.name', 'DeepCode Test'], { cwd: root, ...GIT }); await writeFile(join(root, 'modify me.txt'), 'one\ntwo\nthree\n'); await writeFile(join(root, 'delete.ts'), 'delete me\n'); await writeFile(join(root, 'rename-old.ts'), 'rename me\n'); - await exec('git', ['add', '.'], { cwd: root }); - await exec('git', ['commit', '-qm', 'initial'], { cwd: root }); + await exec('git', ['add', '.'], { cwd: root, ...GIT }); + await exec('git', ['commit', '-qm', 'initial'], { cwd: root, ...GIT }); return root; } @@ -36,7 +45,7 @@ describe('collectWorkspaceDiff', () => { await rm(join(cwd, 'delete.ts')); await mkdir(join(cwd, 'new dir')); await writeFile(join(cwd, 'new dir', 'new.ts'), 'export const value = 1;\n'); - await exec('git', ['mv', 'rename-old.ts', 'renamed.ts'], { cwd }); + await exec('git', ['mv', 'rename-old.ts', 'renamed.ts'], { cwd, ...GIT }); const diff = await collectWorkspaceDiff(cwd); expect(diff).toMatchObject({ repository: true, base: 'HEAD', truncated: false }); diff --git a/packages/core/src/worktree/index.test.ts b/packages/core/src/worktree/index.test.ts index f47e52f..ec7a70c 100644 --- a/packages/core/src/worktree/index.test.ts +++ b/packages/core/src/worktree/index.test.ts @@ -4,6 +4,7 @@ import { mkdtemp, realpath, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { gitSpawnEnv } from '../util/git-env.js'; import { createWorktree, removeWorktree } from './index.js'; /** @@ -26,16 +27,8 @@ async function canonicalMkdtemp(prefix: string): Promise { * and they try to operate on the outer repo's index — failing with * `.git/index: index file open failed: Not a directory`. */ -function cleanGitEnv(): NodeJS.ProcessEnv { - const env = { ...process.env }; - for (const k of Object.keys(env)) { - if (k.startsWith('GIT_')) delete env[k]; - } - return env; -} - function runOrFail(cmd: string, args: string[], cwd: string): void { - const r = spawnSync(cmd, args, { cwd, encoding: 'utf8', env: cleanGitEnv() }); + const r = spawnSync(cmd, args, { cwd, encoding: 'utf8', env: gitSpawnEnv() }); if (r.status !== 0) { throw new Error(`${cmd} ${args.join(' ')} failed (exit ${r.status}): ${r.stderr || r.stdout}`); } @@ -75,7 +68,7 @@ describe('createWorktree / removeWorktree', () => { await expect(fs.access(h.path)).rejects.toThrow(); const branch = spawnSync('git', ['-C', src, 'rev-parse', '--verify', h.branch], { encoding: 'utf8', - env: cleanGitEnv(), + env: gitSpawnEnv(), }); expect(branch.status).toBe(0); }); diff --git a/scripts/git-env-isolation.test.ts b/scripts/git-env-isolation.test.ts new file mode 100644 index 0000000..979a731 --- /dev/null +++ b/scripts/git-env-isolation.test.ts @@ -0,0 +1,52 @@ +// A test that shells out to git must not inherit the outer repository. +// +// `git` reads GIT_DIR, GIT_WORK_TREE and GIT_INDEX_FILE from the environment, +// and a git hook sets all three. The pre-commit gate runs the whole suite from +// inside `git commit`, so a fixture that calls `git init` on a temp directory +// without scrubbing the environment does not initialise the temp directory — it +// re-initialises the developer's own repository, as bare, and writes the test +// identity into its config. Every subsequent git command in that checkout then +// fails with "this operation must be run in a work tree". +// +// This is not hypothetical: apps/server/src/workspace-diff.test.ts did exactly +// that. Three separate fixtures had already grown their own copy of the scrub, +// two of them carrying a comment describing this precise failure — which is the +// tell that a convention passed by word of mouth had stopped being enforced. +// +// The rule is mechanical, so check it mechanically. + +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join, relative, resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const root = resolve(import.meta.dirname, '..'); +const roots = ['packages', 'apps', 'scripts']; +const skip = new Set(['node_modules', 'dist', 'target', '.git', 'out']); + +function testFiles(dir: string, found: string[] = []): string[] { + for (const entry of readdirSync(dir)) { + if (skip.has(entry)) continue; + const path = join(dir, entry); + if (statSync(path).isDirectory()) testFiles(path, found); + else if (entry.endsWith('.test.ts')) found.push(path); + } + return found; +} + +// Matches `exec('git', [`, `spawnSync('git', [`, `runOrFail('git', [` — any +// helper whose first argument is the git binary and second is an argv array. +const spawnsGit = /\('git',\s*\[/; + +describe('test fixtures that shell out to git', () => { + it('scrub the inherited git environment', () => { + const offenders = roots + .flatMap((dir) => testFiles(join(root, dir))) + .filter((path) => { + const body = readFileSync(path, 'utf8'); + return spawnsGit.test(body) && !body.includes('gitSpawnEnv'); + }) + .map((path) => relative(root, path)); + + expect(offenders).toEqual([]); + }); +});