Skip to content

fix(test): stop a fixture from re-initialising the real repository - #252

Merged
oratis merged 1 commit into
mainfrom
fix/git-env-test-isolation
Aug 9, 2026
Merged

fix(test): stop a fixture from re-initialising the real repository#252
oratis merged 1 commit into
mainfrom
fix/git-env-test-isolation

Conversation

@oratis

@oratis oratis commented Aug 9, 2026

Copy link
Copy Markdown
Owner

What happened

The pre-commit gate runs the full suite from inside git commit, which exports GIT_DIR, GIT_WORK_TREE and GIT_INDEX_FILE. apps/server/src/workspace-diff.test.ts calls git init on a temp directory without scrubbing them — so it did not initialise the temp directory. It re-initialised the developer's own checkout, as bare, and wrote the test identity into its config:

core.bare=true
user.email=deepcode@example.invalid
user.name=DeepCode Test

After which every git command in that checkout fails with fatal: this operation must be run in a work tree. Refs and objects are untouched; the repair is resetting core.bare and unsetting the injected identity.

This reproduces on main for anyone who commits anything — the hook runs pnpm typecheck && pnpm test, so the corruption is triggered by the act of committing, not by touching this file.

Why it survived

collectWorkspaceDiff — the code under test — already calls gitSpawnEnv(). The fixture did not.

Three other fixtures had each grown their own private copy of the same scrub, and two carry a comment describing this precise failure:

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).

The protection existed, the failure mode was understood, and it was being passed along by word of mouth instead of enforced. A fixture written later simply did not get it.

Change

  • workspace-diff.test.ts passes gitSpawnEnv() at every git call site.
  • The two ad-hoc copies (gitEnv in commands.test.ts, cleanGitEnv in worktree/index.test.ts) collapse onto the canonical helper. Four call sites, one implementation.
  • scripts/git-env-isolation.test.ts fails the build if any *.test.ts spawns git without referencing gitSpawnEnv.

Verification

  • Reverted the fixture; the new check names it and fails. Restored; passes.
  • pnpm typecheck, pnpm lint, pnpm format:check clean.
  • Full suite passes through the pre-commit hook — the path that previously did the damage.

Not covered

scripts/gen-release-notes.ts also spawns git unscrubbed. 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 that job does not have. Left for the release-notes work.

🤖 Generated with Claude Code

`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 <noreply@anthropic.com>
oratis added a commit that referenced this pull request Aug 9, 2026
`gen-release-notes.ts` walked a commit range, and with no preceding tag it fell
back to the root commit. That is how v0.3.0's release page came to read
"0 commits." — #250 fixed the shallow clone that produced the empty range, but
the underlying choice was still to describe a release by its commit subjects.

CHANGELOG.md already says what shipped, written for people, grouped by what the
changes mean rather than by the verb the commit happened to start with. A list
of commit subjects is what you write when nobody wrote anything better.

So `--version` makes that entry the release body. The commit walk remains the
fallback and announces itself, in the body and on stderr: notes generated
because nobody wrote a changelog entry should not look like notes somebody
wrote.

Repo-relative links are rewritten to absolute URLs pinned at the tag. A release
body is not rendered inside the repository, so `docs/file-contract.md` resolves
against nothing and 404s; pinning at the tag rather than the default branch also
keeps a v0.3.0 link pointing at the v0.3.0 document after the file moves.

`[Unreleased]` cannot satisfy the lookup — a release that shipped whatever
happened to be sitting under that heading would be lying about its contents.

Also passes a scrubbed environment to the git calls, for the reason in #252. It
duplicates six lines rather than importing `gitSpawnEnv`, because the release job
runs this with `npx tsx` after install but before any build, so core's `dist/`
does not exist yet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@oratis

oratis commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Review — approve

The diagnosis is the valuable part here: three fixtures had independently grown the same scrub and two carried a comment describing this exact failure. That is the tell that the convention was being transmitted by reading neighbouring code, and the fix is right to make it mechanical rather than write a fourth comment.

Verified gitSpawnEnv strips every GIT_*, and that scripts/git-env-isolation.test.ts is actually reachable — vitest.scripts.config.ts includes scripts/**/*.test.ts and root test runs it after pnpm -r test. A guard nobody runs would have been the same bug one level up.

Two limits worth knowing about, neither blocking

The check is file-level, not call-site-level. body.includes('gitSpawnEnv') passes a file that imports the helper and uses it at one of five call sites. That is a finer-grained version of the failure being fixed — the protection is present, just not everywhere. I could not find a regex-shaped way to make it call-site precise without false positives, and a check that cries wolf gets deleted, so I think file-level is the right trade. Worth stating in the comment so the next reader does not assume more than it delivers.

/\('git',\s*\[/ only matches a single-quoted literal. exec("git", [...]) and a variable holding the binary name both slip through. In practice prettier's singleQuote: true is a CI gate, so the quote-style half cannot drift — I checked, and there are currently no call sites in any form the regex misses. Mentioning it because the regex reads as more general than it is.

scripts/gen-release-notes.ts is correctly listed as not covered; #255 adds the scrub there.

@oratis
oratis merged commit d3f62c9 into main Aug 9, 2026
5 checks passed
oratis added a commit that referenced this pull request Aug 9, 2026
`gen-release-notes.ts` walked a commit range, and with no preceding tag it fell
back to the root commit. That is how v0.3.0's release page came to read
"0 commits." — #250 fixed the shallow clone that produced the empty range, but
the underlying choice was still to describe a release by its commit subjects.

CHANGELOG.md already says what shipped, written for people, grouped by what the
changes mean rather than by the verb the commit happened to start with. A list
of commit subjects is what you write when nobody wrote anything better.

So `--version` makes that entry the release body. The commit walk remains the
fallback and announces itself, in the body and on stderr: notes generated
because nobody wrote a changelog entry should not look like notes somebody
wrote.

Repo-relative links are rewritten to absolute URLs pinned at the tag. A release
body is not rendered inside the repository, so `docs/file-contract.md` resolves
against nothing and 404s; pinning at the tag rather than the default branch also
keeps a v0.3.0 link pointing at the v0.3.0 document after the file moves.

`[Unreleased]` cannot satisfy the lookup — a release that shipped whatever
happened to be sitting under that heading would be lying about its contents.

Also passes a scrubbed environment to the git calls, for the reason in #252. It
duplicates six lines rather than importing `gitSpawnEnv`, because the release job
runs this with `npx tsx` after install but before any build, so core's `dist/`
does not exist yet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant