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

The hermetic env on production GitWorktrees instances is unpinned — stripping it stays green, poisoned or not #373

New Issue
Closed
opened 2026-09-06 15:32:56 +02:00 by ltms · 1 comment
ltms commented 2026-09-06 15:32:56 +02:00
Owner
Copy Link
Copy Source

Found by a mutation run while merging #372 (fleetd #369). This is not a defect in #369, and #369's own fix is properly pinned. This is the other half — fleetd #362's protection — which no test observes.

What is unpinned

GitWorktreesTest.seedingGitWorktrees builds a production GitWorktrees with hermeticGitEnv(tmp) as its env override. That override exists because of fleetd #362 review finding 2: GitWorktrees.previouslyEffectiveExcludesFileContent resolves core.excludesFile's XDG fallback in Java, not through a git subprocess, so it reads XDG_CONFIG_HOME/HOME directly and no subprocess-level isolation can reach it.

Mutation: change seedingGitWorktrees to pass null instead of hermeticGitEnv(tmp).

unpoisoned   mvn test -Dtest=GitWorktreesTest                      tests=61 failures=0
poisoned     XDG_CONFIG_HOME=<dir with a '*' git/ignore> ...       tests=61 failures=0

Both green. Nothing notices.

The poison is not weak — I checked

Same poison, against the pre-fix version of the file (origin/main~1):

XDG_CONFIG_HOME=<dir with a '*' git/ignore> mvn test -Dtest=GitWorktreesTest
tests=59 errors=0 skipped=0 failures=56

That reproduces the number recorded in #369 exactly, so the poison does reach these tests. It simply no longer reaches this path once the test-side subprocesses are hermetic: the 56 failures were the test's own git status calls seeing the poisoned excludes file, and those are fixed. The production Java-side read is a different path, and no assertion looks at it.

Why this matters

The protection is real but invisible. Delete it and every signal stays green, so the next person who finds the extra constructor argument noisy will remove it and be right to think nothing broke. #369 solved exactly this problem for the test-side half by adding gitProcessBuilderCarriesTheFullHermeticEnvironment, which asserts on the environment the factory produces rather than on a proxy. The same shape of test is missing for the production seam.

Also measured, same run

58 new GitWorktrees(...) constructions in GitWorktreesTest; 5 go through seedingGitWorktrees. The other 53 pass no env override, so they inherit the JVM's real environment. That is not currently observable — the seeding branch is the only one that reads XDG in Java — but it means the class-wide claim in hermeticGitEnv's javadoc was wrong. I corrected that sentence in 22cdebb rather than leaving a false comment excusing the gap.

Suggested direction (candidate, not decided)

Pin the property, not the call site — a test that asserts a GitWorktrees built for seeding actually resolves its excludes-file fallback inside a throwaway directory, and fails with no poison command needed. Whether the 53 override-free constructions should also be routed through a factory is a separate question, and probably not worth it unless a second Java-side environment read appears.

Related: #369 (the test-side half, fixed), #362 (where this override came from).

Found by a mutation run while merging #372 (fleetd #369). This is **not** a defect in #369, and #369's own fix is properly pinned. This is the *other* half — fleetd #362's protection — which no test observes. ## What is unpinned `GitWorktreesTest.seedingGitWorktrees` builds a production `GitWorktrees` with `hermeticGitEnv(tmp)` as its env override. That override exists because of fleetd #362 review finding 2: `GitWorktrees.previouslyEffectiveExcludesFileContent` resolves `core.excludesFile`'s XDG fallback **in Java**, not through a `git` subprocess, so it reads `XDG_CONFIG_HOME`/`HOME` directly and no subprocess-level isolation can reach it. Mutation: change `seedingGitWorktrees` to pass `null` instead of `hermeticGitEnv(tmp)`. ``` unpoisoned mvn test -Dtest=GitWorktreesTest tests=61 failures=0 poisoned XDG_CONFIG_HOME=<dir with a '*' git/ignore> ... tests=61 failures=0 ``` Both green. Nothing notices. ## The poison is not weak — I checked Same poison, against the **pre-fix** version of the file (`origin/main~1`): ``` XDG_CONFIG_HOME=<dir with a '*' git/ignore> mvn test -Dtest=GitWorktreesTest tests=59 errors=0 skipped=0 failures=56 ``` That reproduces the number recorded in #369 exactly, so the poison does reach these tests. It simply no longer reaches *this* path once the test-side subprocesses are hermetic: the 56 failures were the test's own `git status` calls seeing the poisoned excludes file, and those are fixed. The production Java-side read is a different path, and no assertion looks at it. ## Why this matters The protection is real but invisible. Delete it and every signal stays green, so the next person who finds the extra constructor argument noisy will remove it and be right to think nothing broke. #369 solved exactly this problem for the test-side half by adding `gitProcessBuilderCarriesTheFullHermeticEnvironment`, which asserts on the environment the factory produces rather than on a proxy. The same shape of test is missing for the production seam. ## Also measured, same run 58 `new GitWorktrees(...)` constructions in `GitWorktreesTest`; 5 go through `seedingGitWorktrees`. The other 53 pass **no** env override, so they inherit the JVM's real environment. That is not currently observable — the seeding branch is the only one that reads XDG in Java — but it means the class-wide claim in `hermeticGitEnv`'s javadoc was wrong. I corrected that sentence in `22cdebb` rather than leaving a false comment excusing the gap. ## Suggested direction (candidate, not decided) Pin the property, not the call site — a test that asserts a `GitWorktrees` built for seeding actually resolves its excludes-file fallback inside a throwaway directory, and fails with no poison command needed. Whether the 53 override-free constructions should also be routed through a factory is a separate question, and probably not worth it unless a second Java-side environment read appears. Related: #369 (the test-side half, fixed), #362 (where this override came from).
ltms commented 2026-09-09 02:39:30 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main in 145a8c8 (pushed as part of 6f828b8). PR #380.

What I verified myself

Full mvn clean install on the merged tree: MVN_EXIT=0, Tests run: 1450, Failures: 0, 0 compile errors.

Then I ran the mutation myself rather than trusting the worker's paste, because this test had a specific way of being fake and I wanted to rule it out.

The vacuity risk I checked for, and disproved

The new test writes an ignore pattern into a throwaway XDG_CONFIG_HOME, drops a matching file into the worktree, and asserts git status --porcelain is empty.

My worry: the test's own git status subprocess runs through gitProcessBuilder. If that subprocess also inherited this test's throwaway XDG_CONFIG_HOME, then git itself would honour the ignore pattern directly — and the assertion would pass whether or not the production code ever read the seam. The test would be measuring its own fixture.

So I stripped the seam from the new test's own construction and ran it:

Tests run: 1, Failures: 1
the marker pattern lives only in this test's throwaway XDG_CONFIG_HOME; git status must
still be empty, proving the production seam resolved the excludes-file fallback through
the gitEnv seam rather than the JVM's real environment — got:
?? cb373-xdg-fallback-marker
 ==> expected: <> but was: <?? cb373-xdg-fallback-marker>

It fails. So the test's own git subprocess does not see the throwaway ignore file — the only path by which that marker becomes invisible runs through the production code reading the seam. The test is load-bearing. Restored and confirmed byte-identical.

It also needs no poison command and no special machine, which is what makes it survivable.

A number in this ticket was wrong

This ticket said "58 constructions, 5 through seedingGitWorktrees". Both came from a comment already in the file that had never been re-run. The real counts at the time of the work were 59 and 4. The worker corrected the "4" in the file's comment and added a note that the class-wide total moves whenever a test is added, so it should not be cited without recounting. It left the "53" alone as outside what this ticket asked to re-verify, and said so.

I am recording this here because this ticket is itself about a claim that went stale without anyone re-running it, and it repeated one.

Closing.

Merged to `main` in 145a8c8 (pushed as part of 6f828b8). PR #380. ## What I verified myself Full `mvn clean install` on the merged tree: `MVN_EXIT=0`, `Tests run: 1450, Failures: 0`, 0 compile errors. Then I ran the mutation myself rather than trusting the worker's paste, because this test had a specific way of being fake and I wanted to rule it out. ## The vacuity risk I checked for, and disproved The new test writes an ignore pattern into a throwaway `XDG_CONFIG_HOME`, drops a matching file into the worktree, and asserts `git status --porcelain` is empty. My worry: the test's **own** `git status` subprocess runs through `gitProcessBuilder`. If that subprocess also inherited this test's throwaway `XDG_CONFIG_HOME`, then git itself would honour the ignore pattern directly — and the assertion would pass whether or not the production code ever read the seam. The test would be measuring its own fixture. So I stripped the seam from the new test's own construction and ran it: ``` Tests run: 1, Failures: 1 the marker pattern lives only in this test's throwaway XDG_CONFIG_HOME; git status must still be empty, proving the production seam resolved the excludes-file fallback through the gitEnv seam rather than the JVM's real environment — got: ?? cb373-xdg-fallback-marker ==> expected: <> but was: <?? cb373-xdg-fallback-marker> ``` It fails. So the test's own git subprocess does **not** see the throwaway ignore file — the only path by which that marker becomes invisible runs through the production code reading the seam. The test is load-bearing. Restored and confirmed byte-identical. It also needs no poison command and no special machine, which is what makes it survivable. ## A number in this ticket was wrong This ticket said "58 constructions, 5 through `seedingGitWorktrees`". Both came from a comment already in the file that had never been re-run. The real counts at the time of the work were **59 and 4**. The worker corrected the "4" in the file's comment and added a note that the class-wide total moves whenever a test is added, so it should not be cited without recounting. It left the "53" alone as outside what this ticket asked to re-verify, and said so. I am recording this here because this ticket is itself about a claim that went stale without anyone re-running it, and it repeated one. Closing.
ltms closed this issue 2026-09-09 02:39:35 +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#373
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?