fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

GitWorktreesTest reads the machine's real git config, so it can pass or fail for reasons outside the repo #369

New Issue
Closed
opened 2026-09-05 08:34:33 +02:00 by ltms · 1 comment
ltms commented 2026-09-05 08:34:33 +02:00
Owner
Copy Link
Copy Source

GitWorktreesTest shells out to real git in several helpers. Two of them isolate nothing, and a third isolates only part of what git reads. The result is a test class whose behaviour depends on the machine it runs on.

Measured on 2026-09-05, on main at 92c0f16:

$ mkdir -p /tmp/poison/git && printf '*\n' > /tmp/poison/git/ignore
$ cd fleetd && XDG_CONFIG_HOME=/tmp/poison mvn -q test -Dtest=GitWorktreesTest
[ERROR] Tests run: 59, Failures: 56, Errors: 0, Skipped: 0

56 of 59 fail. Without the poisoned variable the same command is green.

Where the leak is

gitOutput (used by git()) sets three variables but not XDG_CONFIG_HOME:

pb.environment().put("GIT_CONFIG_GLOBAL", "/dev/null");
pb.environment().put("GIT_CONFIG_SYSTEM", "/dev/null");
pb.environment().put("GIT_TERMINAL_PROMPT", "0");

status(Path, String) and fullStatus(Path) set nothing at all — they inherit the JVM's whole environment, so they run git status against the operator's real ~/.gitconfig and real default ignore file:

private static String fullStatus(Path cwd) throws Exception {
    Process p = new ProcessBuilder("git", "status", "--porcelain")
            .directory(cwd.toFile()).redirectErrorStream(true).start();

GIT_CONFIG_GLOBAL=/dev/null does not stop git applying $XDG_CONFIG_HOME/git/ignore (or $HOME/.config/git/ignore) — that is git's documented default excludes file and it applies with no core.excludesFile configured at all, per gitignore(5).

Why this is worth fixing even though the suite is green today

The class asserts on git status output. Any pattern in the operator's own configuration that happens to match a path a test writes changes what those assertions see. Nothing warns; the test simply means something different on a different machine, and CI (where ~/.config/git/ignore usually does not exist) is a different machine from every developer's.

One thing this is NOT

I first suspected the two memberSkills compose tests could pass for the wrong reason, because fullStatus runs against a real global config whose core.excludesFile contains target — the very pattern one of them asserts is hidden. That hypothesis is wrong, and I measured it rather than reasoning about it. The worktree-scoped core.excludesFile those tests write shadows the operator's global one in fullStatus's process too, so removing the composition really does make the file appear:

remove the composition  ->  Tests run: 2, Failures: 2, BUILD FAILURE

Those tests pin what they claim to pin. This issue is about the class's general sensitivity to the environment, not about a specific false pass.

Suggested direction (candidate, not a decided fix)

GitWorktreesTest already gained the right pattern in #366:

private static Map<String, String> hermeticGitEnv(Path tmp) {
    return Map.of(
            "GIT_CONFIG_GLOBAL", "/dev/null",
            "GIT_CONFIG_SYSTEM", "/dev/null",
            "GIT_TERMINAL_PROMPT", "0",
            "XDG_CONFIG_HOME", tmp.resolve("hermetic-xdg-config-home-" + System.nanoTime()).toString());
}

Extending that same set to gitOutput, status and fullStatus looks like the smallest change. Whatever the mechanism, the check that it worked is the command at the top of this issue: with XDG_CONFIG_HOME poisoned, the suite should stay green.

Worth a wider look too — GitWorktreesTest is where this was found, not necessarily the only test class that shells out to git without isolating it.

Notes

  • Both unisolated helpers are on main in that shape and predate #366; this is not a regression from that PR.
  • Scope is tests only. No production code reads these helpers.
`GitWorktreesTest` shells out to real `git` in several helpers. Two of them isolate nothing, and a third isolates only part of what git reads. The result is a test class whose behaviour depends on the machine it runs on. Measured on 2026-09-05, on `main` at `92c0f16`: ``` $ mkdir -p /tmp/poison/git && printf '*\n' > /tmp/poison/git/ignore $ cd fleetd && XDG_CONFIG_HOME=/tmp/poison mvn -q test -Dtest=GitWorktreesTest [ERROR] Tests run: 59, Failures: 56, Errors: 0, Skipped: 0 ``` 56 of 59 fail. Without the poisoned variable the same command is green. ## Where the leak is `gitOutput` (used by `git()`) sets three variables but not `XDG_CONFIG_HOME`: ```java pb.environment().put("GIT_CONFIG_GLOBAL", "/dev/null"); pb.environment().put("GIT_CONFIG_SYSTEM", "/dev/null"); pb.environment().put("GIT_TERMINAL_PROMPT", "0"); ``` `status(Path, String)` and `fullStatus(Path)` set **nothing at all** — they inherit the JVM's whole environment, so they run `git status` against the operator's real `~/.gitconfig` and real default ignore file: ```java private static String fullStatus(Path cwd) throws Exception { Process p = new ProcessBuilder("git", "status", "--porcelain") .directory(cwd.toFile()).redirectErrorStream(true).start(); ``` `GIT_CONFIG_GLOBAL=/dev/null` does not stop git applying `$XDG_CONFIG_HOME/git/ignore` (or `$HOME/.config/git/ignore`) — that is git's documented default excludes file and it applies with no `core.excludesFile` configured at all, per `gitignore(5)`. ## Why this is worth fixing even though the suite is green today The class asserts on `git status` output. Any pattern in the operator's own configuration that happens to match a path a test writes changes what those assertions see. Nothing warns; the test simply means something different on a different machine, and CI (where `~/.config/git/ignore` usually does not exist) is a different machine from every developer's. ## One thing this is NOT I first suspected the two `memberSkills` compose tests could pass for the wrong reason, because `fullStatus` runs against a real global config whose `core.excludesFile` contains `target` — the very pattern one of them asserts is hidden. **That hypothesis is wrong, and I measured it rather than reasoning about it.** The worktree-scoped `core.excludesFile` those tests write shadows the operator's global one in `fullStatus`'s process too, so removing the composition really does make the file appear: ``` remove the composition -> Tests run: 2, Failures: 2, BUILD FAILURE ``` Those tests pin what they claim to pin. This issue is about the class's general sensitivity to the environment, not about a specific false pass. ## Suggested direction (candidate, not a decided fix) `GitWorktreesTest` already gained the right pattern in #366: ```java private static Map<String, String> hermeticGitEnv(Path tmp) { return Map.of( "GIT_CONFIG_GLOBAL", "/dev/null", "GIT_CONFIG_SYSTEM", "/dev/null", "GIT_TERMINAL_PROMPT", "0", "XDG_CONFIG_HOME", tmp.resolve("hermetic-xdg-config-home-" + System.nanoTime()).toString()); } ``` Extending that same set to `gitOutput`, `status` and `fullStatus` looks like the smallest change. Whatever the mechanism, the check that it worked is the command at the top of this issue: with `XDG_CONFIG_HOME` poisoned, the suite should stay green. Worth a wider look too — `GitWorktreesTest` is where this was found, not necessarily the only test class that shells out to `git` without isolating it. ## Notes - Both unisolated helpers are on `main` in that shape and predate #366; this is not a regression from that PR. - Scope is tests only. No production code reads these helpers.
ltms commented 2026-09-06 15:33:11 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 154971c (PR #372), plus 22cdebb for one javadoc correction.

Every git subprocess this test class starts now goes through a single gitProcessBuilder factory that applies the hermetic environment, including XDG_CONFIG_HOME — the key the old code left out, and the only one that stops git's default excludes file from applying.

Round 1 pinned this with a call-site count (exactly 2 literal new ProcessBuilder( occurrences). That is a proxy, not the property: I measured that deleting pb.environment().putAll(hermeticEnv()) from inside the factory left every call site unchanged, the count at 2, and 1413 tests green. Round 2 added gitProcessBuilderCarriesTheFullHermeticEnvironment, which asserts on the environment the factory actually hands to ProcessBuilder#start(). Both checks are kept — they catch different regressions.

Verified on the merge, not taken from the worker's report:

mvn clean install                                Tests run: 1425, Failures: 0, Errors: 0 — BUILD SUCCESS
poison control, PRE-FIX file (origin/main~1):
  XDG_CONFIG_HOME=<dir with a '*' git/ignore> mvn test -Dtest=GitWorktreesTest
                                                 tests=59 errors=0 failures=56

That control reproduces the 56/59 recorded in this ticket, so the poison genuinely reaches these tests and the fix is what closes it.

Mutation on merge, on a half neither the worker nor the reviewer touched. I made seedingGitWorktrees pass null instead of hermeticGitEnv(tmp) — stripping the hermetic environment from the production GitWorktrees instances rather than from the test's own subprocesses. Result: tests=61 failures=0, unpoisoned and poisoned. That half is unpinned. It is fleetd #362's protection and guards a different path (the Java-side XDG read in previouslyEffectiveExcludesFileContent), so it is out of scope here — filed as #373 rather than held against this PR.

Same run showed hermeticGitEnv's javadoc claim that "no test in this class can reach the real machine's home directory" is too broad: 53 of the 58 new GitWorktrees(...) constructions in the file pass no env override. Corrected in 22cdebb.

No wiki/11-Features.md entry — this is test hardening with no operator-visible surface, so it belongs on the Roadmap line, not in Features.

Merged as `154971c` (PR #372), plus `22cdebb` for one javadoc correction. Every `git` subprocess this test class starts now goes through a single `gitProcessBuilder` factory that applies the hermetic environment, including `XDG_CONFIG_HOME` — the key the old code left out, and the only one that stops git's default excludes file from applying. Round 1 pinned this with a call-site count (exactly 2 literal `new ProcessBuilder(` occurrences). That is a proxy, not the property: I measured that deleting `pb.environment().putAll(hermeticEnv())` from inside the factory left every call site unchanged, the count at 2, and 1413 tests green. Round 2 added `gitProcessBuilderCarriesTheFullHermeticEnvironment`, which asserts on the environment the factory actually hands to `ProcessBuilder#start()`. Both checks are kept — they catch different regressions. Verified on the merge, not taken from the worker's report: ``` mvn clean install Tests run: 1425, Failures: 0, Errors: 0 — BUILD SUCCESS poison control, PRE-FIX file (origin/main~1): XDG_CONFIG_HOME=<dir with a '*' git/ignore> mvn test -Dtest=GitWorktreesTest tests=59 errors=0 failures=56 ``` That control reproduces the 56/59 recorded in this ticket, so the poison genuinely reaches these tests and the fix is what closes it. **Mutation on merge, on a half neither the worker nor the reviewer touched.** I made `seedingGitWorktrees` pass `null` instead of `hermeticGitEnv(tmp)` — stripping the hermetic environment from the *production* `GitWorktrees` instances rather than from the test's own subprocesses. Result: `tests=61 failures=0`, unpoisoned **and** poisoned. That half is unpinned. It is fleetd #362's protection and guards a different path (the Java-side XDG read in `previouslyEffectiveExcludesFileContent`), so it is out of scope here — filed as #373 rather than held against this PR. Same run showed `hermeticGitEnv`'s javadoc claim that "no test in this class can reach the real machine's home directory" is too broad: 53 of the 58 `new GitWorktrees(...)` constructions in the file pass no env override. Corrected in `22cdebb`. No `wiki/11-Features.md` entry — this is test hardening with no operator-visible surface, so it belongs on the Roadmap line, not in Features.
ltms closed this issue 2026-09-06 15:33:22 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#369
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?