diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java index 0f232b5..af80e13 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -790,6 +790,15 @@ public final class GitWorktrees implements Worktrees { * fleetd's own exclude file ahead of the seeded skill patterns, and the worktree-scoped override * then points at that combined file — so every pattern the operator's own configuration already * applied keeps applying, plus the seeded skill paths. + * + *

Assumes a fresh worktree — not idempotent. {@link #seedSkills} only ever calls this + * from {@link #add}, which always creates a brand-new worktree, so {@code core.excludesFile} is + * never already worktree-scoped-set to fleetd's own file when this runs. A hypothetical second + * call on the SAME worktree would read fleetd's own already-composed file back as "previously + * effective" (worktree scope now wins) and append the seeded patterns a second time — harmless + * to {@code git status} (duplicate exclude lines are a no-op), but not something to rely on. No + * guard is added for this because the path does not exist today; if a future caller ever seeds + * the same worktree twice, it will need one. */ private void excludeSeededSkillsFromGitStatus(String worktreePath, List seededSkillNames) { exec("git", "-C", worktreePath, "config", "extensions.worktreeConfig", "true"); @@ -830,6 +839,25 @@ public final class GitWorktrees implements Worktrees { * forward" (empty string) — this is a best-effort read in service of {@link #seedSkills}'s own * invariant 3, not a new way for skill seeding to fail a spawn. * + *

Review fix, finding 2. The XDG-fallback branch below does not go through {@code git} + * at all, so a first cut of it read {@code XDG_CONFIG_HOME}/{@code HOME} straight from the JVM's + * own environment ({@link System#getenv} / {@code user.home}) — unlike every other value this + * class resolves, which goes through a {@code git} subprocess and therefore already honours + * {@link #gitEnv}. That meant no test could make this branch hermetic, and on any machine + * carrying a real {@code ~/.config/git/ignore} (this repo's own dev machine does), every + * skill-seeding test silently composed with that real file — correct in production, but + * machine-dependent in the test suite, and a future broader pattern in that real file could + * silently change what a seeded worktree's {@code git status} reports depending on whose home + * directory ran the test. {@link #resolveEnv} now checks {@link #gitEnv} first for both + * variables, falling back to the JVM's real environment only when the seam does not supply + * them — production behaviour (empty {@link #gitEnv}) is unchanged, and a test can now isolate + * this branch exactly as it already isolates every {@code git} subprocess call. + * + *

Snapshot, not a reference. The content below is read once, at seeding time, and + * copied into fleetd's own exclude file. If the operator edits their global excludesFile + * afterward, an already-seeded worktree keeps the old copy — acceptable for a worktree's + * expected lifetime, but worth knowing before reading a stale pattern as a bug. + * * @return the file's content, trailing-newline-normalized, or {@code ""} when there is nothing * to compose with. */ @@ -839,10 +867,10 @@ public final class GitWorktrees implements Worktrees { resolvedPath = exec("git", "-C", worktreePath, "config", "--get", "--type=path", "core.excludesFile").trim(); } else { - String xdgConfigHome = System.getenv("XDG_CONFIG_HOME"); + String xdgConfigHome = resolveEnv("XDG_CONFIG_HOME"); Path fallback = (xdgConfigHome != null && !xdgConfigHome.isBlank()) ? Path.of(xdgConfigHome, "git", "ignore") - : Path.of(System.getProperty("user.home"), ".config", "git", "ignore"); + : Path.of(resolveHome(), ".config", "git", "ignore"); resolvedPath = fallback.toString(); } if (resolvedPath.isBlank()) { @@ -863,6 +891,26 @@ public final class GitWorktrees implements Worktrees { } } + /** + * Resolve environment variable {@code name} for {@link #previouslyEffectiveExcludesFileContent}'s + * XDG fallback, checking {@link #gitEnv} FIRST so a test can isolate this the same way it + * already isolates every {@code git} subprocess this class runs, and falling back to the JVM's + * real environment only when the seam does not supply it (always the case in production, where + * {@link #gitEnv} is {@code Map.of()}). + */ + private String resolveEnv(String name) { + String fromSeam = gitEnv.get(name); + return fromSeam != null ? fromSeam : System.getenv(name); + } + + /** Same as {@link #resolveEnv(String)}, for {@code HOME} — falls back to {@code user.home} + * (rather than {@code System.getenv("HOME")}) when the seam does not supply it, matching this + * class's pre-existing behaviour for every other home-directory resolution. */ + private String resolveHome() { + String fromSeam = gitEnv.get("HOME"); + return fromSeam != null ? fromSeam : System.getProperty("user.home"); + } + /** * The worker-readable half of fleetd #362, mirroring {@link #recordNeutralizedConfigForWorker}: * record which skill folders were seeded where the worker itself can read it, without a diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java index b099944..a08938f 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java @@ -1482,6 +1482,34 @@ class GitWorktreesTest { Files.writeString(skillFile, content); } + /** + * fleetd #362 review fix, finding 2. {@code core.excludesFile}'s XDG-fallback branch + * ({@link GitWorktrees#previouslyEffectiveExcludesFileContent}) does not go through a {@code + * git} subprocess, so a first cut of it read {@code XDG_CONFIG_HOME}/{@code HOME} straight from + * the JVM's real environment — no test could isolate it, and on any machine carrying a real + * {@code ~/.config/git/ignore} (this repo's own dev machine does — measured, not assumed), every + * seeding test below silently composed with that real file instead of a controlled fixture. + * Every test that seeds at least one skill now constructs its {@link GitWorktrees} with this — + * an empty, machine-independent {@code XDG_CONFIG_HOME} (so the fallback resolves to a file that + * provably does not exist) plus the same {@code GIT_CONFIG_GLOBAL}/{@code GIT_CONFIG_SYSTEM}/ + * {@code GIT_TERMINAL_PROMPT} isolation the {@link #git}/{@link #gitOutput} helpers already use + * for repo setup — so no test in this class can reach the real machine's home directory. + */ + private static Map 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()); + } + + /** {@link GitWorktrees}'s full test seam, with a {@code memberSkillsSource} and no other + * overrides — the shape every seeding test below needs, isolated via {@link #hermeticGitEnv}. */ + private static GitWorktrees seedingGitWorktrees(Path root, String memberSkillsSource, Path tmp) { + return new GitWorktrees(root.toString(), null, _ -> {}, null, null, memberSkillsSource, + hermeticGitEnv(tmp)); + } + /** Acceptance criterion 2 (part 1): a worktree with no {@code .claude/} at all gets the skill * copied in from the configured {@code memberSkillsSource}, structure and content intact. */ @Test @@ -1490,7 +1518,7 @@ class GitWorktreesTest { Path skillsSource = tmp.resolve("skills-src"); writeSkill(skillsSource, "implementer", "IMPLEMENTER SKILL\n"); - String wt = new GitWorktrees(tmp.resolve("wts").toString(), null, skillsSource.toString()) + String wt = seedingGitWorktrees(tmp.resolve("wts"), skillsSource.toString(), tmp) .add(repo.toString(), "cb-362-fresh", "HEAD"); assertEquals("IMPLEMENTER SKILL\n", @@ -1546,7 +1574,7 @@ class GitWorktreesTest { Path skillsSource = tmp.resolve("skills-src"); writeSkill(skillsSource, "implementer", "IMPLEMENTER SKILL\n"); - String wt = new GitWorktrees(tmp.resolve("wts").toString(), null, skillsSource.toString()) + String wt = seedingGitWorktrees(tmp.resolve("wts"), skillsSource.toString(), tmp) .add(repo.toString(), "cb-362-status", "HEAD"); assertEquals("", fullStatus(Path.of(wt)), @@ -1563,7 +1591,9 @@ class GitWorktreesTest { Path repo = initRepo(tmp.resolve("repo")); Path skillsSource = tmp.resolve("skills-src"); writeSkill(skillsSource, "implementer", "IMPLEMENTER SKILL\n"); - GitWorktrees seeding = new GitWorktrees(tmp.resolve("wts").toString(), null, skillsSource.toString()); + GitWorktrees seeding = seedingGitWorktrees(tmp.resolve("wts"), skillsSource.toString(), tmp); + // `plain` never seeds anything (memberSkillsSource is null, so seedSkills no-ops before it + // ever touches core.excludesFile), so it does not need the hermetic gitEnv seam. GitWorktrees plain = new GitWorktrees(tmp.resolve("wts").toString()); String seededWt = seeding.add(repo.toString(), "cb-362-scope-a", "HEAD"); @@ -1596,7 +1626,7 @@ class GitWorktreesTest { writeSkill(skillsSource, "hunter", "FLEETD HUNTER\n"); writeSkill(skillsSource, "implementer", "FLEETD IMPLEMENTER\n"); - new GitWorktrees(tmp.resolve("wts").toString(), null, skillsSource.toString()) + seedingGitWorktrees(tmp.resolve("wts"), skillsSource.toString(), tmp) .add(repo.toString(), "cb-362-log", "HEAD"); assertTrue(capturedMessages().stream().anyMatch(m -> @@ -1633,7 +1663,11 @@ class GitWorktreesTest { Map gitEnv = Map.of( "GIT_CONFIG_GLOBAL", globalConfig.toString(), "GIT_CONFIG_SYSTEM", "/dev/null", - "GIT_TERMINAL_PROMPT", "0"); + "GIT_TERMINAL_PROMPT", "0", + // core.excludesFile is explicitly set above, so the XDG fallback branch is never + // reached here — this is belt-and-braces so the test stays hermetic even if that + // ever changes, matching every other seeding test in this file. + "XDG_CONFIG_HOME", tmp.resolve("unused-xdg-config-home").toString()); Path repo = initRepo(tmp.resolve("repo")); Path skillsSource = tmp.resolve("skills-src"); @@ -1653,4 +1687,56 @@ class GitWorktreesTest { "the operator's own global excludesFile pattern ('target') must still apply after " + "skill seeding ran — got:\n" + porcelain); } + + /** + * fleetd #362 review fix, finding 2: pins the XDG-fallback branch of {@link + * GitWorktrees#previouslyEffectiveExcludesFileContent}, exercised when {@code core.excludesFile} + * is unset entirely (no global, local, or worktree-scoped value at all) — the branch that used to + * read {@code XDG_CONFIG_HOME} straight from the JVM's own environment, unreachable by any test + * seam, and would silently compose with whatever real {@code ~/.config/git/ignore} happened to + * exist on the machine running the suite. {@code GIT_CONFIG_GLOBAL} points at an empty file (so + * {@code core.excludesFile} is genuinely unset, forcing the fallback branch to fire — not the + * "already configured" branch {@link #seedSkillsComposesWithAnAlreadyEffectiveGlobalExcludesFile} + * covers), and {@code XDG_CONFIG_HOME} is isolated through the {@code gitEnv} seam at a throwaway + * temp dir carrying a synthetic {@code git/ignore} that ignores {@code xdg-fallback-marker}. A + * skill is seeded through the real {@link GitWorktrees#add} path, and a file named {@code + * xdg-fallback-marker} is written into the worktree afterward: {@code git status --porcelain} + * must still be empty, proving the XDG-default pattern kept applying after seeding. + * + *

Deleting the fallback (so an unset key composes with {@code ""}) turns this test red with: + * {@code expected: <> but was: } — see the PR body for the pasted + * failure from actually running that mutation. + */ + @Test + void seedSkillsComposesWithTheXdgDefaultExcludesFileWhenNoneIsConfigured(@TempDir Path tmp) throws Exception { + Path xdgConfigHome = tmp.resolve("xdg-config-home"); + Files.createDirectories(xdgConfigHome.resolve("git")); + Files.writeString(xdgConfigHome.resolve("git").resolve("ignore"), "xdg-fallback-marker\n"); + Path emptyGlobalConfig = tmp.resolve("empty-global.gitconfig"); + Files.writeString(emptyGlobalConfig, ""); + Map gitEnv = Map.of( + "GIT_CONFIG_GLOBAL", emptyGlobalConfig.toString(), + "GIT_CONFIG_SYSTEM", "/dev/null", + "GIT_TERMINAL_PROMPT", "0", + "XDG_CONFIG_HOME", xdgConfigHome.toString()); + + Path repo = initRepo(tmp.resolve("repo")); + Path skillsSource = tmp.resolve("skills-src"); + writeSkill(skillsSource, "implementer", "IMPLEMENTER SKILL\n"); + GitWorktrees gitWorktrees = new GitWorktrees(tmp.resolve("wts").toString(), null, _ -> {}, + null, null, skillsSource.toString(), gitEnv); + + String wt = gitWorktrees.add(repo.toString(), "cb-362-xdg-fallback", "HEAD"); + assertEquals("IMPLEMENTER SKILL\n", + Files.readString(Path.of(wt, ".claude", "skills", "implementer", "SKILL.md")), + "fixture check — the skill really was seeded"); + + Files.writeString(Path.of(wt, "xdg-fallback-marker"), + "build output the XDG default ignore file (not core.excludesFile) covers\n"); + + String porcelain = fullStatus(Path.of(wt)); + assertEquals("", porcelain, + "the XDG default excludesFile pattern ('xdg-fallback-marker') must still apply " + + "after skill seeding ran — got:\n" + porcelain); + } }