test: the tests that shell out to git bring their own config #49

thisilike · 2026-08-21T16:34:13+02:00

thisilike commented

2026-08-21 16:34:13 +02:00

Two test files shell out to real git — packages/server/tests/external_modules_test.ts (seeds a repo so a real clone has something to find) and packages/modules/docker/backend/commit_test.ts (runs commitLines against real repositories). Both ran under whatever the developer's machine happened to say, and several of those settings decide whether a test passes.

With commit.gpgsign=true, the seed commit goes to pinentry:

error: Error: git commit: error: gpg failed to sign the data:
gpg: signing failed: Operation cancelled

That is the two clone tests in external_modules_test.ts failing. commit_test.ts had already been bitten — its repo() set commit.gpgsign=false per repo, with a comment recording four tests and five minutes of pinentry timeouts. The same reach-in works through core.hooksPath, init.templateDir and url.*.insteadOf, none of which either file had a line for. CI sets none of them, so the whole class only ever fires on a developer machine — the worse failure, because it reads as a bug in the branch under test. It has already cost review time on #48 twice, where both rounds had to say "not this branch, my git config".

What this does

git takes its configuration from two places, so isolating it takes two:

The config files. tools/test-gitconfig is now what those tests run under, passed as GIT_CONFIG_GLOBAL (plus GIT_CONFIG_NOSYSTEM=1) so it replaces the global and system files rather than layering on top of them. It carries the identity and gpgsign=false both callers were setting per repo, so those lines are gone — one file read by both, instead of a line per repo covering only the setting somebody already got bitten by.

The environment, which outranks the files. GIT_CONFIG_COUNT with its GIT_CONFIG_KEY_<n>/GIT_CONFIG_VALUE_<n> pairs sets any config key at all, GIT_AUTHOR_*/GIT_COMMITTER_* set the identity, GIT_DIR/GIT_WORK_TREE move the repository out from under -C — and Deno.Command's env option merges into the parent environment instead of replacing it, so a config file alone closes half the hole. tools/test-git-env.ts owns the spawn options both call sites now spread:

export const TEST_GIT_SPAWN = {
  env: testGitEnv(), // the inherited environment, every GIT_* stripped out
  clearEnv: true,
} as const;

A strip rather than an allow-list of the variables that matter, because an allow-list written against today's git would not have GIT_CONFIG_COUNT in it (2.31). This is what git's own test suite does — t/test-lib.sh unsets GIT_* wholesale before setting the few it wants. env and clearEnv ship as one object because env on its own is the merge that caused this, so a call site cannot take half of it. Everything that is not GIT_* is inherited on purpose: PATH, HOME, TMPDIR, SYSTEMROOT and whatever else a platform needs to start a process — naming those breaks on the first machine nobody tried.

The two duplicated TEST_GIT_ENV blocks are gone with it, and with them the ../../../ depth each was counting to the same file. In commit_test.ts the options go on sh(), so the commitLines code under test runs under them too; it shells out through that same helper and its commits were the ones stalling.

tools/ rather than beside either caller, and a file plus a module rather than something in a package, because both callers live in different workspace packages and neither should reach into the other's. Not the product: the real clone in modules/external.ts still inherits the environment whole, which is where an operator's credentials and insteadOf rules live.

Verification

deno task test → 484 passed | 0 failed | 1 ignored, with commit.gpgsign=true globally. No pinentry prompt. (external_modules_test.ts alone: 9 passed, was 7 passed / 2 failed.)
Isolation, positive control, both layers hostile at once. A hostile global config file — commit.gpgsign, tag.gpgsign, a pre-commit hook via core.hooksPath that exits 1, url."https://evil.invalid/".insteadOf, init.defaultBranch=hostile — and a hostile environment: GIT_AUTHOR_NAME/GIT_AUTHOR_EMAIL, GIT_COMMITTER_*, and a GIT_CONFIG_COUNT pair setting commit.gpgsign=true and core.hooksPath. Still 484 passed | 0 failed | 1 ignored. The same environment against the previous commit (config file only): 8 passed | 8 failed, which is the hole this second commit closes.
Negative control. Move tools/test-gitconfig aside and the git tests fail loudly (Author identity unknown) — proof the file is read rather than the suite passing for some other reason.
No --allow-env. The helper inherits nothing (nothing to inherit is also nothing that can bleed in), git gets the two config variables alone, and commit_test.ts still passes 7 | 0 — so a narrower single-file invocation than deno task test does not break on the env read.
deno task check, deno lint, deno fmt --check clean.
GIT_CONFIG_GLOBAL needs git ≥ 2.32 (June 2021); verified against 2.55.0.

Two test files shell out to real git — `packages/server/tests/external_modules_test.ts` (seeds a repo so a real clone has something to find) and `packages/modules/docker/backend/commit_test.ts` (runs `commitLines` against real repositories). Both ran under whatever the developer's machine happened to say, and several of those settings decide whether a test passes. With `commit.gpgsign=true`, the seed commit goes to pinentry: ``` error: Error: git commit: error: gpg failed to sign the data: gpg: signing failed: Operation cancelled ``` That is the two clone tests in `external_modules_test.ts` failing. `commit_test.ts` had already been bitten — its `repo()` set `commit.gpgsign=false` per repo, with a comment recording four tests and five minutes of pinentry timeouts. The same reach-in works through `core.hooksPath`, `init.templateDir` and `url.*.insteadOf`, none of which either file had a line for. CI sets none of them, so the whole class only ever fires on a developer machine — the worse failure, because it reads as a bug in the branch under test. It has already cost review time on #48 twice, where both rounds had to say "not this branch, my git config". ### What this does git takes its configuration from two places, so isolating it takes two: **The config files.** `tools/test-gitconfig` is now what those tests run under, passed as `GIT_CONFIG_GLOBAL` (plus `GIT_CONFIG_NOSYSTEM=1`) so it **replaces** the global and system files rather than layering on top of them. It carries the identity and `gpgsign=false` both callers were setting per repo, so those lines are gone — one file read by both, instead of a line per repo covering only the setting somebody already got bitten by. **The environment, which outranks the files.** `GIT_CONFIG_COUNT` with its `GIT_CONFIG_KEY_<n>`/`GIT_CONFIG_VALUE_<n>` pairs sets any config key at all, `GIT_AUTHOR_*`/`GIT_COMMITTER_*` set the identity, `GIT_DIR`/`GIT_WORK_TREE` move the repository out from under `-C` — and `Deno.Command`'s `env` option merges into the parent environment instead of replacing it, so a config file alone closes half the hole. `tools/test-git-env.ts` owns the spawn options both call sites now spread: ```ts export const TEST_GIT_SPAWN = { env: testGitEnv(), // the inherited environment, every GIT_* stripped out clearEnv: true, } as const; ``` A strip rather than an allow-list of the variables that matter, because an allow-list written against today's git would not have `GIT_CONFIG_COUNT` in it (2.31). This is what git's own test suite does — `t/test-lib.sh` unsets `GIT_*` wholesale before setting the few it wants. `env` and `clearEnv` ship as one object because `env` on its own is the merge that caused this, so a call site cannot take half of it. Everything that is not `GIT_*` is inherited on purpose: PATH, HOME, TMPDIR, SYSTEMROOT and whatever else a platform needs to start a process — naming those breaks on the first machine nobody tried. The two duplicated `TEST_GIT_ENV` blocks are gone with it, and with them the `../../../` depth each was counting to the same file. In `commit_test.ts` the options go on `sh()`, so the `commitLines` code under test runs under them too; it shells out through that same helper and its commits were the ones stalling. `tools/` rather than beside either caller, and a file plus a module rather than something in a package, because both callers live in different workspace packages and neither should reach into the other's. Not the product: the real clone in `modules/external.ts` still inherits the environment whole, which is where an operator's credentials and `insteadOf` rules live. ### Verification - `deno task test` → **484 passed | 0 failed | 1 ignored**, with `commit.gpgsign=true` globally. No pinentry prompt. (`external_modules_test.ts` alone: 9 passed, was 7 passed / 2 failed.) - **Isolation, positive control, both layers hostile at once.** A hostile global config file — `commit.gpgsign`, `tag.gpgsign`, a `pre-commit` hook via `core.hooksPath` that exits 1, `url."https://evil.invalid/".insteadOf`, `init.defaultBranch=hostile` — *and* a hostile environment: `GIT_AUTHOR_NAME`/`GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_*`, and a `GIT_CONFIG_COUNT` pair setting `commit.gpgsign=true` and `core.hooksPath`. Still **484 passed | 0 failed | 1 ignored**. The same environment against the previous commit (config file only): **8 passed | 8 failed**, which is the hole this second commit closes. - **Negative control.** Move `tools/test-gitconfig` aside and the git tests fail loudly (`Author identity unknown`) — proof the file is read rather than the suite passing for some other reason. - **No `--allow-env`.** The helper inherits nothing (nothing to inherit is also nothing that can bleed in), git gets the two config variables alone, and `commit_test.ts` still passes 7 | 0 — so a narrower single-file invocation than `deno task test` does not break on the env read. - `deno task check`, `deno lint`, `deno fmt --check` clean. - `GIT_CONFIG_GLOBAL` needs git ≥ 2.32 (June 2021); verified against 2.55.0.

thisilike added 1 commit

2026-08-21 16:34:13 +02:00

test: a seeded temp repo turns commit signing off

Build and Deploy / verify (pull_request) Successful in 1m19s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m11s

Details

0f32049add

seedRepo inherits the developer's global git config, so with
commit.gpgsign=true its `git commit -q -m init` goes to pinentry and the two
tests that clone that repo fail with "gpg failed to sign the data" on a
machine where signing is the default. CI has no signing key configured, so
this only ever breaks locally — which is the worse failure, because it looks
like the branch under test.

commit_test.ts's repo() already disables it for exactly this reason; this is
the same two lines in the one other place that commits.

julian was assigned by thisilike

2026-08-21 16:34:22 +02:00

thisilike force-pushed fix/test-repo-no-signing from 0f32049add

Build and Deploy / verify (pull_request) Successful in 1m19s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m11s

Details

to d0f7df8f1f

Build and Deploy / verify (pull_request) Successful in 1m20s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m10s

Details

2026-08-21 16:44:45 +02:00

Compare

thisilike changed title from ~~test: a seeded temp repo turns commit signing off~~ to test: the tests that shell out to git bring their own config

2026-08-21 16:45:03 +02:00

julian requested changes

2026-08-21 19:59:33 +02:00

Dismissed

julian left a comment

The config-file isolation is solid and everything the PR body claims checked out on verification: path depths are correct from both files, every git invocation in both test files goes through the env-wrapped helper, the removed per-repo lines are all covered by tools/test-gitconfig, and the product clone in external.ts is rightly untouched.

One hole remains, same disease one layer up: Deno.Command's env option merges with the parent environment (no clearEnv), so environment-variable git configuration still bleeds through — GIT_AUTHOR_NAME/GIT_AUTHOR_EMAIL, GIT_COMMITTER_*, and especially GIT_CONFIG_COUNT/GIT_CONFIG_KEY_*/GIT_CONFIG_VALUE_*, all of which override the config file. A machine that exports any of these reproduces exactly the class of failure this PR closes for ~/.gitconfig. Two inline comments, one per call site.

Suggested fix: either clearEnv: true (then check git still finds HOME/TEMP/PATH-dependent bits on all platforms), or explicitly unset the known GIT_* overrides in TEST_GIT_ENV.

The config-file isolation is solid and everything the PR body claims checked out on verification: path depths are correct from both files, every git invocation in both test files goes through the env-wrapped helper, the removed per-repo lines are all covered by tools/test-gitconfig, and the product clone in external.ts is rightly untouched. One hole remains, same disease one layer up: `Deno.Command`'s `env` option merges with the parent environment (no `clearEnv`), so environment-variable git configuration still bleeds through — `GIT_AUTHOR_NAME`/`GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_*`, and especially `GIT_CONFIG_COUNT`/`GIT_CONFIG_KEY_*`/`GIT_CONFIG_VALUE_*`, all of which override the config file. A machine that exports any of these reproduces exactly the class of failure this PR closes for `~/.gitconfig`. Two inline comments, one per call site. Suggested fix: either `clearEnv: true` (then check git still finds HOME/TEMP/PATH-dependent bits on all platforms), or explicitly unset the known `GIT_*` overrides in TEST_GIT_ENV.

packages/modules/docker/backend/commit_test.ts Outdated

					
				@ -27,3 +41,4 @@

				async function sh(cmd: string, args: string[]) {

				  const r = await new Deno.Command(cmd, {

				    args,

				    env: TEST_GIT_ENV,

env here merges with the parent environment — Deno.Command only replaces it with clearEnv: true. So GIT_AUTHOR_NAME, GIT_COMMITTER_*, and GIT_CONFIG_COUNT/GIT_CONFIG_KEY_* from the developer's shell still reach every git run and override tools/test-gitconfig. Same machine-state bleed this PR fixes for the config file, one layer up. Either clearEnv: true plus whatever git needs re-added, or explicitly unset the known GIT_* overrides in TEST_GIT_ENV.

`env` here merges with the parent environment — Deno.Command only replaces it with `clearEnv: true`. So `GIT_AUTHOR_NAME`, `GIT_COMMITTER_*`, and `GIT_CONFIG_COUNT`/`GIT_CONFIG_KEY_*` from the developer's shell still reach every git run and override tools/test-gitconfig. Same machine-state bleed this PR fixes for the config file, one layer up. Either `clearEnv: true` plus whatever git needs re-added, or explicitly unset the known `GIT_*` overrides in TEST_GIT_ENV.

packages/server/tests/external_modules_test.ts Outdated

					
				@ -222,3 +235,4 @@

				  const run = async (...args: string[]) => {

				    const out = await new Deno.Command("git", {

				      args: ["-C", dir, ...args],

				      env: TEST_GIT_ENV,

Same env-merge hole as commit_test.ts sh(): parent GIT_* environment variables (author/committer identity, GIT_CONFIG_COUNT family) merge in and override the config file. Whatever fix lands there belongs here too — the two TEST_GIT_ENV blocks should stay identical.

Same env-merge hole as commit_test.ts `sh()`: parent `GIT_*` environment variables (author/committer identity, `GIT_CONFIG_COUNT` family) merge in and override the config file. Whatever fix lands there belongs here too — the two TEST_GIT_ENV blocks should stay identical.

thisilike added 1 commit

2026-08-21 20:10:59 +02:00

test: the git tests bring their own environment, not just their own config

Build and Deploy / verify (pull_request) Successful in 1m22s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m15s

Details

652252791b

GIT_CONFIG_GLOBAL replaces the config FILES only, and `Deno.Command`'s `env`
option MERGES into the parent environment instead of replacing it, so the
machine's GIT_* variables still reached every git these tests spawn — and they
outrank the file: GIT_CONFIG_COUNT with its KEY/VALUE pairs sets any config key
at all, GIT_AUTHOR_*/GIT_COMMITTER_* the identity, GIT_DIR/GIT_WORK_TREE the
repository out from under `-C`. Same machine-state bleed one layer up.

tools/test-git-env.ts now owns the spawn options both call sites use: the
inherited environment with every GIT_* stripped out of it — what git's own
t/test-lib.sh does, and a strip rather than an allow-list because an allow-list
written against today's git would not have GIT_CONFIG_COUNT in it — plus
clearEnv, so what git gets IS the environment rather than an overlay on the
developer's. Those two ship as one object: `env` on its own merges, which is
the bug, so a call site cannot take half of it. Everything not GIT_* is
inherited on purpose (PATH, HOME, TMPDIR, SYSTEMROOT); naming those breaks on
the first platform nobody tried.

The two duplicated TEST_GIT_ENV blocks go with it, and with them the
../../../ depth each was counting to the same file.

Verified with commit.gpgsign, core.hooksPath, GIT_AUTHOR_*/GIT_COMMITTER_* and
a GIT_CONFIG_COUNT pair all set hostile in the environment, on top of the
hostile global config file: 484 passed | 0 failed | 1 ignored, where the same
environment fails 8 tests on the previous commit. Config file moved aside still
fails loudly (`Author identity unknown`), and without --allow-env the helper
inherits nothing, git gets the two config variables alone, and the git tests
pass.

thisilike commented

2026-08-21 20:11:45 +02:00

Right on both counts, and thanks for chasing it one layer up — env without clearEnv is a merge, and the environment beats the config file, so the file alone closed half the hole. Fixed in 6522527.

Neither of the two suggested shapes on its own: clearEnv: true with a re-added allow-list is the portability bug (PATH, HOME, TMPDIR, SYSTEMROOT, and whatever the next platform needs), and explicitly unsetting the known GIT_* names rots — an allow-list written against today's git would not have GIT_CONFIG_COUNT in it, which only exists since 2.31. Also GIT_AUTHOR_NAME="" is not "unset": git dies with empty ident name (for <…>) not allowed.

So: clearEnv: true with the inherited environment minus every GIT_*, which is what git's own t/test-lib.sh does (unsets GIT_* wholesale, then sets the few it wants). Case-insensitive strip, because Windows environment names are.

tools/test-git-env.ts now owns both, and the two TEST_GIT_ENV blocks are gone rather than kept in sync — your "should stay identical" is a maintenance requirement I would rather not have:

export const TEST_GIT_SPAWN = {
  env: testGitEnv(),
  clearEnv: true,
} as const;

One object, because env on its own merges — that being the bug, a call site cannot take half of it. Both call sites are now { args, ...TEST_GIT_SPAWN, stdout, stderr }, and the ../../../../tools/test-gitconfig depth each file was counting is gone too; the helper resolves it from its own import.meta.dirname.

Verification, with the hostile config file from the original round and a hostile environment on top (GIT_AUTHOR_*, GIT_COMMITTER_*, GIT_CONFIG_COUNT=2 setting commit.gpgsign=true and core.hooksPath):

484 passed | 0 failed | 1 ignored. The same environment on d0f7df8: 8 passed | 8 failed — the hole, reproduced and then closed.
Config file moved aside still fails loudly (Author identity unknown).
Without --allow-env the helper inherits nothing and git gets the two config variables alone; commit_test.ts passes 7 | 0. That path exists because the env read is at module load, and a single-file deno test invocation narrower than deno task test would otherwise die before the git gate.
deno task check, deno lint, deno fmt --check clean.

PR body updated for both layers, including the "a file crosses no package boundary" line, which no longer describes what is there.

Right on both counts, and thanks for chasing it one layer up — `env` without `clearEnv` is a merge, and the environment beats the config file, so the file alone closed half the hole. Fixed in 6522527. Neither of the two suggested shapes on its own: `clearEnv: true` with a re-added allow-list is the portability bug (PATH, HOME, TMPDIR, SYSTEMROOT, and whatever the next platform needs), and explicitly unsetting the known `GIT_*` names rots — an allow-list written against today's git would not have `GIT_CONFIG_COUNT` in it, which only exists since 2.31. Also `GIT_AUTHOR_NAME=""` is not "unset": git dies with `empty ident name (for <…>) not allowed`. So: `clearEnv: true` with the **inherited environment minus every `GIT_*`**, which is what git's own `t/test-lib.sh` does (unsets `GIT_*` wholesale, then sets the few it wants). Case-insensitive strip, because Windows environment names are. `tools/test-git-env.ts` now owns both, and the two `TEST_GIT_ENV` blocks are gone rather than kept in sync — your "should stay identical" is a maintenance requirement I would rather not have: ```ts export const TEST_GIT_SPAWN = { env: testGitEnv(), clearEnv: true, } as const; ``` One object, because `env` on its own merges — that being the bug, a call site cannot take half of it. Both call sites are now `{ args, ...TEST_GIT_SPAWN, stdout, stderr }`, and the `../../../../tools/test-gitconfig` depth each file was counting is gone too; the helper resolves it from its own `import.meta.dirname`. Verification, with the hostile config file from the original round **and** a hostile environment on top (`GIT_AUTHOR_*`, `GIT_COMMITTER_*`, `GIT_CONFIG_COUNT=2` setting `commit.gpgsign=true` and `core.hooksPath`): - **484 passed | 0 failed | 1 ignored.** The same environment on d0f7df8: **8 passed | 8 failed** — the hole, reproduced and then closed. - Config file moved aside still fails loudly (`Author identity unknown`). - Without `--allow-env` the helper inherits nothing and git gets the two config variables alone; `commit_test.ts` passes 7 | 0. That path exists because the env read is at module load, and a single-file `deno test` invocation narrower than `deno task test` would otherwise die before the git gate. - `deno task check`, `deno lint`, `deno fmt --check` clean. PR body updated for both layers, including the "a file crosses no package boundary" line, which no longer describes what is there.

thisilike requested review from julian

2026-08-21 20:11:50 +02:00

julian approved these changes

2026-08-21 20:15:46 +02:00

julian left a comment

Both requested changes are closed by the rework, verified against the tree rather than the diff text:

tools/test-git-env.ts strips every GIT_* variable from the inherited environment (case-insensitive, which Windows needs), re-adds GIT_CONFIG_GLOBAL + GIT_CONFIG_NOSYSTEM, and ships the result as one TEST_GIT_SPAWN object with clearEnv: true — a call site cannot take the env without the clear, which was the failure mode of the previous shape.
Both call sites spread it correctly; stdout/stderr after the spread touch nothing it sets.
Import path depths from both packages and the test-gitconfig reference from tools/ itself all resolve.
tools/ sits outside the packages/* workspace, so the shared module crosses no package boundary; lint covers it, and nothing in deno.json breaks.
GIT_CONFIG_GLOBAL replaces the entire global scope including $XDG_CONFIG_HOME/git/config, so no lookup path is left uncovered.
Wholesale strip rather than an allow-list is the right call for the GIT_CONFIG_COUNT reason the comment gives; matches what git's own test suite does.

The module-load-time env snapshot means a test mutating Deno.env after import won't reach spawned git — fine for what these tests do, just worth knowing it's frozen.

Both requested changes are closed by the rework, verified against the tree rather than the diff text: - `tools/test-git-env.ts` strips every `GIT_*` variable from the inherited environment (case-insensitive, which Windows needs), re-adds `GIT_CONFIG_GLOBAL` + `GIT_CONFIG_NOSYSTEM`, and ships the result as one `TEST_GIT_SPAWN` object with `clearEnv: true` — a call site cannot take the env without the clear, which was the failure mode of the previous shape. - Both call sites spread it correctly; `stdout`/`stderr` after the spread touch nothing it sets. - Import path depths from both packages and the `test-gitconfig` reference from `tools/` itself all resolve. - `tools/` sits outside the `packages/*` workspace, so the shared module crosses no package boundary; lint covers it, and nothing in deno.json breaks. - `GIT_CONFIG_GLOBAL` replaces the entire global scope including `$XDG_CONFIG_HOME/git/config`, so no lookup path is left uncovered. - Wholesale strip rather than an allow-list is the right call for the `GIT_CONFIG_COUNT` reason the comment gives; matches what git's own test suite does. The module-load-time env snapshot means a test mutating `Deno.env` after import won't reach spawned git — fine for what these tests do, just worth knowing it's frozen.

thisilike merged commit cc65b8c261 into main

2026-08-21 20:19:23 +02:00

thisilike referenced this pull request from a commit

2026-08-21 20:19:24 +02:00

Merge pull request 'test: the tests that shell out to git bring their own config' (#49) from fix/test-repo-no-signing into main

julian referenced this pull request

2026-08-21 21:42:00 +02:00

feat(storage): see what a module is holding, and drop what outlived it #46

julian referenced this pull request

2026-08-21 21:42:10 +02:00

feat(storage): see what a module is holding, and drop what outlived it #46

Sign in to join this conversation.