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

fleetd #369: make GitWorktreesTest hermetic against the machine’s real git config #372

Closed
agent wants to merge 0 commits from worker/fleetd-369-hermetic-git-tests-e8b19a-3 into main
pull from: worker/fleetd-369-hermetic-git-tests-e8b19a-3
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-09-06 14:55:17 +02:00
Member
Copy Link
Copy Source

fleetd #369 — GitWorktreesTest was reading the machine's real git config, so it could pass or fail for reasons outside the repo.

The defect, measured 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

Where the leak was: the test's own gitOutput (used by git()) set GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM/GIT_TERMINAL_PROMPT but not XDG_CONFIG_HOME. status(Path, String), fullStatus(Path), and every other raw git subprocess this test class started (revParse, lsTree, diffNameOnly, forEachRef, blobOf, treeOf, commitTree, exitCode) set NO isolation at all, inheriting the JVM's whole real environment — including the operator's real ~/.gitconfig and default excludes file ($XDG_CONFIG_HOME/git/ignore / $HOME/.config/git/ignore, applied by git with no core.excludesFile configured at all — gitignore(5)).

Fix (test-only, no production code touched): centralized every git subprocess this test class starts through one factory, gitProcessBuilder(Path cwd, String... args), which always applies hermeticGitEnv (the isolation map #366 already introduced for seedingGitWorktrees's production GitWorktrees instances) including XDG_CONFIG_HOME pointed at a class-scoped throwaway @TempDir.

Criterion 4 — made hard to undo by accident: added everyGitSubprocessGoesThroughTheHermeticFactory, a self-check test that counts literal new ProcessBuilder( constructions in this file's own source and asserts there are exactly 2 (the factory itself, and the one documented exception below). A future helper that shells out to git directly instead of going through the factory changes that count and fails the test — the omission that caused this ticket is now caught by name instead of rediscovered on a poisoned machine.

Deliberately left alone:

  • worktreeCredentialHelperCompletesWithoutUsingAnInheritedHelper — its whole point is that git must resolve a synthetic "operator's global config" and then have credential.helper NOT read it via the environment credential helper; it cannot use the shared hermetic env (which points GIT_CONFIG_GLOBAL at /dev/null) without defeating the test. It never runs git status, so it needs no XDG_CONFIG_HOME isolation either. Documented in a comment at the call site and in everyGitSubprocessGoesThroughTheHermeticFactory's javadoc.
  • seedSkillsExcludeDoesNotLeakIntoASiblingWorktree's plain GitWorktrees instance — already commented in-file: memberSkillsSource is null, so seedSkills no-ops before touching core.excludesFile, so it does not need the hermetic gitEnv seam.
  • seedSkillsIsBestEffortWhenSourceDoesNotExist and seedSkillsNeverOverwritesAReposOwnSkill — checked both: the first never seeds anything past a missing-directory check, the second seeds nothing new (the skill is "kept", not "seeded") so excludeSeededSkillsFromGitStatus (the method that reads the XDG fallback) is never reached in either case.

Criterion 5 — machine dependencies found elsewhere in the test tree (reported, not changed):

  • EnvAllowListScrubTest (src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java) deliberately starts a real login zsh that sources this operator's real ~/.zshenv/~/.zshrc/~/.zprofile/~/.zlogin chain — documented in the class javadoc as intentional, and gated with assumeTrue so it skips cleanly on a machine with no /bin/zsh or no real shell rc files.
  • AmqpReplyInboxContractTest and LeadMailboxTest (src/test/java/dev/ltms/fleet/msg/) read AMQP_URI from the real environment to opt into an externally-provisioned broker in CI; locally (unset) Testcontainers spins a container instead. Documented, tagged contract.
  • ClaudeCodeLauncherTest already carries a dedicated, well-designed guard for real-~/.claude.json writes (fleetd #258): a @BeforeAll/@AfterAll differential check plus per-test user.home redirection to a @TempDir. Checked it — no violation found, and no test in this class writes the real file.
  • No other real-network calls or System.getenv/user.home reliance found in the sweep (ClaudeCodeLauncherTest's own comment references aside).

Build results:

  • Poisoned, class only: Tests run: 60, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS (60 = the original 59 plus the new self-check test).
  • Poisoned, whole suite: Tests run: 1413, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.
  • Clean (no poison), mvn clean install, run unpiped: Tests run: 1413, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

No assertion was weakened — every test still asserts exactly what it asserted before; only the environment each git subprocess runs in changed.

Review round 2 — criterion 4 fix

The reviewer correctly showed everyGitSubprocessGoesThroughTheHermeticFactory was a proxy, not the property itself: it counts new ProcessBuilder( call sites, so it catches a new helper built the old way, but not gitProcessBuilder itself being gutted. Proof given: delete pb.environment().putAll(hermeticEnv()) from inside the factory, every call site is unchanged, the count stays 2, and mvn clean install (no poison) stays green at 1413 tests.

Fix: added gitProcessBuilderCarriesTheFullHermeticEnvironment(@TempDir Path tmp), which calls gitProcessBuilder directly and asserts on pb.environment():

  • GIT_CONFIG_GLOBAL == /dev/null
  • GIT_CONFIG_SYSTEM == /dev/null
  • GIT_TERMINAL_PROMPT == 0
  • XDG_CONFIG_HOME is set, non-blank, resolves inside the class's own CLASS_TMP (never left unset — which would inherit whatever the JVM's real environment carries), and the resolved <xdg>/git/ignore provably does not exist

This inspects the actual environment the factory hands to ProcessBuilder#start(), so it fails the moment the hermetic environment stops being applied — on any machine, no poison required. Kept the existing call-site count check as well; the two catch different regressions (a bypassing call site vs. a gutted factory).

Proof, all four numbers requested:

  1. Removed pb.environment().putAll(hermeticEnv()) from the factory, no poison, ran only the new test:
    Tests run: 1, Failures: 1 — GIT_CONFIG_GLOBAL must be neutralized... ==> expected: </dev/null> but was: <null> — new test FAILS, confirmed.
  2. Restored the factory; added a third raw new ProcessBuilder("git", "-C", cwd.toString(), "status").start() in a rogue helper (factory untouched); ran only the count-check test:
    Tests run: 1, Failures: 1 — expected: <2> but was: <3> — existing count check FAILS, confirmed still working.
  3. Reverted both mutations (diffed byte-identical against the pre-mutation file), then mvn clean install, unpiped, no poison:
    Tests run: 1414, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS (1414 = prior 1413 + 1 new test). Zero [ERROR] lines in the full log.
  4. XDG_CONFIG_HOME=/tmp/poison mvn test (whole suite):
    Tests run: 1414, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

The smaller point (shared CLASS_TMP): left as is. Every call to hermeticEnv() goes through hermeticGitEnv(CLASS_TMP), which appends a fresh hermetic-xdg-config-home-<System.nanoTime()> subdirectory per call — so no two git subprocesses in this class ever share the same XDG_CONFIG_HOME path, and nothing ever writes into any of them (git only reads a git/ignore file there, and none is ever created) — so the shared-parent-directory concern doesn't have a write hazard to land on. Not changing it.

Pushed as a37acd5 on the same branch.

fleetd #369 — `GitWorktreesTest` was reading the machine's real git config, so it could pass or fail for reasons outside the repo. **The defect, measured 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 ``` **Where the leak was:** the test's own `gitOutput` (used by `git()`) set `GIT_CONFIG_GLOBAL`/`GIT_CONFIG_SYSTEM`/`GIT_TERMINAL_PROMPT` but not `XDG_CONFIG_HOME`. `status(Path, String)`, `fullStatus(Path)`, and every other raw `git` subprocess this test class started (`revParse`, `lsTree`, `diffNameOnly`, `forEachRef`, `blobOf`, `treeOf`, `commitTree`, `exitCode`) set NO isolation at all, inheriting the JVM's whole real environment — including the operator's real `~/.gitconfig` and default excludes file (`$XDG_CONFIG_HOME/git/ignore` / `$HOME/.config/git/ignore`, applied by git with no `core.excludesFile` configured at all — `gitignore(5)`). **Fix (test-only, no production code touched):** centralized every git subprocess this test class starts through one factory, `gitProcessBuilder(Path cwd, String... args)`, which always applies `hermeticGitEnv` (the isolation map #366 already introduced for `seedingGitWorktrees`'s production `GitWorktrees` instances) including `XDG_CONFIG_HOME` pointed at a class-scoped throwaway `@TempDir`. **Criterion 4 — made hard to undo by accident:** added `everyGitSubprocessGoesThroughTheHermeticFactory`, a self-check test that counts literal `new ProcessBuilder(` constructions in this file's own source and asserts there are exactly 2 (the factory itself, and the one documented exception below). A future helper that shells out to `git` directly instead of going through the factory changes that count and fails the test — the omission that caused this ticket is now caught by name instead of rediscovered on a poisoned machine. **Deliberately left alone:** - `worktreeCredentialHelperCompletesWithoutUsingAnInheritedHelper` — its whole point is that git must resolve a synthetic "operator's global config" and then have `credential.helper` NOT read it via the environment credential helper; it cannot use the shared hermetic env (which points `GIT_CONFIG_GLOBAL` at `/dev/null`) without defeating the test. It never runs `git status`, so it needs no `XDG_CONFIG_HOME` isolation either. Documented in a comment at the call site and in `everyGitSubprocessGoesThroughTheHermeticFactory`'s javadoc. - `seedSkillsExcludeDoesNotLeakIntoASiblingWorktree`'s `plain` `GitWorktrees` instance — already commented in-file: `memberSkillsSource` is null, so `seedSkills` no-ops before touching `core.excludesFile`, so it does not need the hermetic `gitEnv` seam. - `seedSkillsIsBestEffortWhenSourceDoesNotExist` and `seedSkillsNeverOverwritesAReposOwnSkill` — checked both: the first never seeds anything past a missing-directory check, the second seeds nothing new (the skill is "kept", not "seeded") so `excludeSeededSkillsFromGitStatus` (the method that reads the XDG fallback) is never reached in either case. **Criterion 5 — machine dependencies found elsewhere in the test tree (reported, not changed):** - `EnvAllowListScrubTest` (`src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java`) deliberately starts a real login zsh that sources this operator's real `~/.zshenv`/`~/.zshrc`/`~/.zprofile`/`~/.zlogin` chain — documented in the class javadoc as intentional, and gated with `assumeTrue` so it skips cleanly on a machine with no `/bin/zsh` or no real shell rc files. - `AmqpReplyInboxContractTest` and `LeadMailboxTest` (`src/test/java/dev/ltms/fleet/msg/`) read `AMQP_URI` from the real environment to opt into an externally-provisioned broker in CI; locally (unset) Testcontainers spins a container instead. Documented, tagged `contract`. - `ClaudeCodeLauncherTest` already carries a dedicated, well-designed guard for real-`~/.claude.json` writes (fleetd #258): a `@BeforeAll`/`@AfterAll` differential check plus per-test `user.home` redirection to a `@TempDir`. Checked it — no violation found, and no test in this class writes the real file. - No other real-network calls or `System.getenv`/`user.home` reliance found in the sweep (`ClaudeCodeLauncherTest`'s own comment references aside). **Build results:** - Poisoned, class only: `Tests run: 60, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS` (60 = the original 59 plus the new self-check test). - Poisoned, whole suite: `Tests run: 1413, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. - Clean (no poison), `mvn clean install`, run unpiped: `Tests run: 1413, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. No assertion was weakened — every test still asserts exactly what it asserted before; only the environment each git subprocess runs in changed. --- ## Review round 2 — criterion 4 fix The reviewer correctly showed `everyGitSubprocessGoesThroughTheHermeticFactory` was a proxy, not the property itself: it counts `new ProcessBuilder(` call sites, so it catches a *new* helper built the old way, but not `gitProcessBuilder` itself being gutted. Proof given: delete `pb.environment().putAll(hermeticEnv())` from inside the factory, every call site is unchanged, the count stays 2, and `mvn clean install` (no poison) stays green at 1413 tests. **Fix:** added `gitProcessBuilderCarriesTheFullHermeticEnvironment(@TempDir Path tmp)`, which calls `gitProcessBuilder` directly and asserts on `pb.environment()`: - `GIT_CONFIG_GLOBAL` == `/dev/null` - `GIT_CONFIG_SYSTEM` == `/dev/null` - `GIT_TERMINAL_PROMPT` == `0` - `XDG_CONFIG_HOME` is set, non-blank, resolves inside the class's own `CLASS_TMP` (never left unset — which would inherit whatever the JVM's real environment carries), and the resolved `<xdg>/git/ignore` provably does not exist This inspects the actual environment the factory hands to `ProcessBuilder#start()`, so it fails the moment the hermetic environment stops being applied — on any machine, no poison required. Kept the existing call-site count check as well; the two catch different regressions (a bypassing call site vs. a gutted factory). **Proof, all four numbers requested:** 1. Removed `pb.environment().putAll(hermeticEnv())` from the factory, no poison, ran only the new test: `Tests run: 1, Failures: 1` — `GIT_CONFIG_GLOBAL must be neutralized... ==> expected: </dev/null> but was: <null>` — **new test FAILS**, confirmed. 2. Restored the factory; added a third raw `new ProcessBuilder("git", "-C", cwd.toString(), "status").start()` in a rogue helper (factory untouched); ran only the count-check test: `Tests run: 1, Failures: 1` — `expected: <2> but was: <3>` — **existing count check FAILS**, confirmed still working. 3. Reverted both mutations (diffed byte-identical against the pre-mutation file), then `mvn clean install`, unpiped, no poison: `Tests run: 1414, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS` (1414 = prior 1413 + 1 new test). Zero `[ERROR]` lines in the full log. 4. `XDG_CONFIG_HOME=/tmp/poison mvn test` (whole suite): `Tests run: 1414, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. **The smaller point (shared `CLASS_TMP`):** left as is. Every call to `hermeticEnv()` goes through `hermeticGitEnv(CLASS_TMP)`, which appends a fresh `hermetic-xdg-config-home-<System.nanoTime()>` subdirectory per call — so no two git subprocesses in this class ever share the same `XDG_CONFIG_HOME` path, and nothing ever writes into any of them (git only reads a `git/ignore` file there, and none is ever created) — so the shared-parent-directory concern doesn't have a write hazard to land on. Not changing it. Pushed as `a37acd5` on the same branch.
agent added 1 commit 2026-09-06 14:55:18 +02:00
fleetd #369: make GitWorktreesTest hermetic against the machine's real git config
CI / contract (pull_request) Successful in 47s
Details
CI / build (pull_request) Failing after 1m28s
Details
dd2efd8541 
gitOutput set GIT_CONFIG_GLOBAL/SYSTEM/TERMINAL_PROMPT but not XDG_CONFIG_HOME,
and status/fullStatus (plus every other raw git subprocess in this class) set no
isolation at all — inheriting the JVM's real environment, including the
operator's real ~/.gitconfig and default excludes file. Measured: with a
poisoned XDG_CONFIG_HOME, 56 of 59 tests failed.

Centralize every git subprocess this test starts through one factory,
gitProcessBuilder, which always applies the existing hermeticGitEnv isolation
(extended with XDG_CONFIG_HOME, the same fix #366 already applied to the
production-instance seam). Add a self-check test that counts direct
ProcessBuilder("git", ...) constructions in this file's own source and fails
if a future helper bypasses the factory, so the omission that caused this
ticket is caught by name instead of rediscovered on a poisoned machine.
agent added 1 commit 2026-09-06 15:09:36 +02:00
fleetd #369 review round 2: pin the factory's behaviour, not its call count
CI / contract (pull_request) Successful in 41s
Details
CI / build (pull_request) Successful in 1m26s
Details
a37acd5ee3 
everyGitSubprocessGoesThroughTheHermeticFactory counts ProcessBuilder("git",
...) call sites, so it catches a new helper built the old way, but a
reviewer proved it does not catch gitProcessBuilder itself being gutted:
removing pb.environment().putAll(hermeticEnv()) from inside the factory
leaves every call site unchanged, the count stays 2, and the whole
unpoisoned suite stays green.

Add gitProcessBuilderCarriesTheFullHermeticEnvironment, which inspects what
the factory actually hands to ProcessBuilder#start(): every hermetic key
present with the isolating value, and XDG_CONFIG_HOME pointed inside the
class's own throwaway directory rather than left unset or pointing at the
operator's real one. This fails the moment the hermetic environment stops
being applied, on any machine, with no poison needed. Keep the call-site
count check too — the two catch different regressions.
ltms commented 2026-09-06 15:33:31 +02:00
Owner
Copy Link
Copy Source

Merged into main as 154971c (merge commit, not via the forge button), so closing this PR.

Round 2 (a37acd5) is what made it mergeable: gitProcessBuilderCarriesTheFullHermeticEnvironment asserts on the factory's actual output instead of relying on the call-site count alone. Full reasoning, the poison control, and the merge-time mutation are in #369; the gap that mutation found is #373.

Post-merge on main: mvn clean install → Tests run: 1425, Failures: 0, Errors: 0 — BUILD SUCCESS.

Merged into `main` as `154971c` (merge commit, not via the forge button), so closing this PR. Round 2 (`a37acd5`) is what made it mergeable: `gitProcessBuilderCarriesTheFullHermeticEnvironment` asserts on the factory's actual output instead of relying on the call-site count alone. Full reasoning, the poison control, and the merge-time mutation are in #369; the gap that mutation found is #373. Post-merge on `main`: `mvn clean install` → Tests run: 1425, Failures: 0, Errors: 0 — BUILD SUCCESS.
ltms closed this pull request 2026-09-06 15:33:37 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 41s
Details
CI / build (pull_request) Successful in 1m26s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#372
Delete Branch "worker/fleetd-369-hermetic-git-tests-e8b19a-3"

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?