diff --git a/fleetd/fleetd.example.yaml b/fleetd/fleetd.example.yaml index 32b0ced..6522753 100644 --- a/fleetd/fleetd.example.yaml +++ b/fleetd/fleetd.example.yaml @@ -782,7 +782,9 @@ guard: # .claude/skills/ is never overwritten — the repo's own copy always wins. Claude Code # members only; an opencode member reads a different path (.opencode/agent) this key does not # touch. Best-effort like worktreeGroup above: a missing/unreadable directory here is logged and -# skipped, never a failed spawn. +# skipped, never a failed spawn. Every non-hidden subdirectory of this directory is copied +# wholesale, with no per-file allowlist — don't park scratch files or drafts alongside the real +# skill folders, they will be copied into every provisioned worktree too. # memberSkills: /path/to/fleetd/checkout/.claude/skills # Session lifecycle limits (CB-303). All knobs are opt-in; omit or set to null to keep diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java index 2ac430c..74cc6dd 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java @@ -115,7 +115,10 @@ import java.util.regex.PatternSyntaxException; * today's behaviour. A skill folder the target repo already carries is never * overwritten — see {@link dev.ltms.fleet.session.GitWorktrees}. Claude Code * members only; an opencode member's equivalent lives under a different path - * ({@code .opencode/agent}) and is not covered by this key. + * ({@code .opencode/agent}) and is not covered by this key. Every non-hidden + * subdirectory of this directory is copied wholesale, with no per-file + * allowlist — do not park scratch files or drafts alongside the real skill + * folders, they will be copied into every provisioned worktree too. */ @JsonIgnoreProperties(ignoreUnknown = true) public record FleetConfig( 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 0bbf464..0f232b5 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -95,6 +95,12 @@ public final class GitWorktrees implements Worktrees { /** Source directory of skill folders for {@link #seedSkills} (fleetd #362, {@code memberSkills:} * in config); {@code null} ⇒ feature off, no worktree is touched beyond today's behaviour. */ private final String memberSkillsSource; + /** Extra environment merged into every {@code git} subprocess this instance runs. Always {@code + * Map.of()} from every production constructor. Test seam only (fleetd #362 review fix): lets + * {@code GitWorktreesTest} point {@code GIT_CONFIG_GLOBAL} at an isolated temp file so it can + * drive the real {@link #add} path against a controlled "operator's global git config" and + * prove the excludesFile composition below without ever touching the real machine's config. */ + private final Map gitEnv; private final Consumer afterWorktreeAdded; /** How the initial {@code git worktree add} command runs. Package-private test seam for an * interrupted command after Git has made worktree state. */ @@ -179,10 +185,25 @@ public final class GitWorktrees implements Worktrees { GitWorktrees(String configuredRoot, String group, Consumer afterWorktreeAdded, Function shareGroupRunner, Function worktreeAddRunner, String memberSkillsSource) { + this(configuredRoot, group, afterWorktreeAdded, shareGroupRunner, worktreeAddRunner, + memberSkillsSource, Map.of()); + } + + /** + * Full test seam, plus {@code gitEnv} (fleetd #362 review fix, verification only): extra + * environment merged into every {@code git} subprocess this instance runs, so a test can isolate + * something like {@code GIT_CONFIG_GLOBAL} from the real machine while still driving the real + * {@link #add} path end to end. Every production constructor above delegates here with {@code + * Map.of()}. + */ + GitWorktrees(String configuredRoot, String group, Consumer afterWorktreeAdded, + Function shareGroupRunner, Function worktreeAddRunner, + String memberSkillsSource, Map gitEnv) { this.configuredRoot = configuredRoot; this.group = (group == null || group.isBlank()) ? null : group; this.memberSkillsSource = (memberSkillsSource == null || memberSkillsSource.isBlank()) ? null : memberSkillsSource; + this.gitEnv = gitEnv == null ? Map.of() : gitEnv; this.afterWorktreeAdded = afterWorktreeAdded == null ? _ -> {} : afterWorktreeAdded; this.shareGroupRunner = shareGroupRunner != null ? shareGroupRunner : this::exec; this.worktreeAddRunner = worktreeAddRunner != null ? worktreeAddRunner : this::exec; @@ -634,6 +655,32 @@ public final class GitWorktrees implements Worktrees { * worktree or the primary checkout. Proven with a real {@code git status --porcelain} in * {@code GitWorktreesTest}, not by reasoning. * + *

Compose, don't replace. {@code core.excludesFile} is single-valued: the first cut of + * this method pointed it at fleetd's own file with {@code --replace-all}, which SHADOWS whatever + * the operator's own (global, or repo-local) {@code core.excludesFile} was already resolving to + * inside this worktree, rather than adding to it. Measured concretely: this repo's own {@code + * .gitignore} does not ignore {@code target/} — only an operator's global excludesFile does — so + * every worker's {@code mvn clean install} would otherwise make {@code target/} appear as + * untracked, and {@link #hasUncommitted}'s deliberately-untracked-inclusive {@code git status + * --porcelain} (CB-576) would then read every such worktree as dirty forever, so it is never + * cleaned up. {@link #excludeSeededSkillsFromGitStatus} now reads whatever {@code + * core.excludesFile} resolves to BEFORE writing anything (falling back to git's own documented + * default, {@code $XDG_CONFIG_HOME/git/ignore} or {@code $HOME/.config/git/ignore}, when the key + * is unset entirely — see {@code gitignore(5)}), and writes that content into its OWN exclude + * file ahead of the seeded skill patterns, so every operator-configured pattern keeps applying + * inside the seeded worktree exactly as it did before seeding ran. + * + *

Instead of using worktree-scoped-config as an add-then-append (a second key does not exist + * for {@code core.excludesFile} — it takes exactly one value), an actual second exclude source + * was ruled out because git resolves only ONE {@code core.excludesFile}; concatenating the prior + * content into fleetd's own file is what "compose" reduces to for a single-valued key. + * + *

Proven the same way as invariant 2's own leak check: {@code + * GitWorktreesTest#seedSkillsComposesWithAnAlreadyEffectiveGlobalExcludesFile} isolates a + * synthetic "operator's global config" via {@code GIT_CONFIG_GLOBAL} (never the real machine's), + * seeds a skill, and asserts {@code git status --porcelain} is still empty for a file matching + * that global config's own ignore pattern. + * *

Invariant 3 — best-effort. A missing/unreadable {@link #memberSkillsSource}, or a * copy/exclude failure, is logged and skipped — it must never fail the spawn, the same contract * {@link #overlayParity} and {@link #isolateToolSurface} already hold. @@ -734,16 +781,25 @@ public final class GitWorktrees implements Worktrees { * --absolute-git-dir}), which lives outside the working tree, so the exclude file itself can * never be committed either. * - *

This replaces any {@code --worktree}-scoped {@code core.excludesFile} this worktree already - * had — acceptable because a freshly provisioned worktree has none, and worktree-scoped git - * config is already used exclusively for fleetd's own isolation (the credential helper, the SSH - * rewrite) rather than anything an operator sets by hand. + *

Compose, don't replace. {@code core.excludesFile} is single-valued, so pointing it at + * fleetd's own file would otherwise SHADOW whatever excludesFile this worktree was already + * resolving (an operator's global config, most commonly) rather than add to it — see the + * "Compose, don't replace" discussion on {@link #seedSkills}. {@link + * #previouslyEffectiveExcludesFileContent} is read BEFORE this method's own {@code --worktree} + * write below, so it still sees whatever was effective beforehand; that content is written into + * 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. */ private void excludeSeededSkillsFromGitStatus(String worktreePath, List seededSkillNames) { exec("git", "-C", worktreePath, "config", "extensions.worktreeConfig", "true"); + String previouslyEffective = previouslyEffectiveExcludesFileContent(worktreePath); String gitDir = exec("git", "-C", worktreePath, "rev-parse", "--absolute-git-dir").trim(); Path excludeFile = Path.of(gitDir, "fleet-seeded-skills-exclude"); StringBuilder patterns = new StringBuilder(); + if (!previouslyEffective.isEmpty()) { + patterns.append(previouslyEffective); + } for (String name : seededSkillNames) { patterns.append("/.claude/skills/").append(name).append('/').append(System.lineSeparator()); } @@ -757,6 +813,56 @@ public final class GitWorktrees implements Worktrees { excludeFile.toString()); } + /** + * The content of whatever {@code core.excludesFile} resolves to for {@code worktreePath} right + * now — BEFORE {@link #excludeSeededSkillsFromGitStatus} points that key at fleetd's own file — + * so it can be carried forward instead of shadowed. {@code --type=path} makes git itself perform + * {@code ~}/{@code ~user} expansion the same way it would when actually reading the key to build + * exclude rules, rather than handing back a raw, unexpanded config string. + * + *

When the key is unset entirely (exit code non-zero), falls back to git's own documented + * default excludes file — {@code $XDG_CONFIG_HOME/git/ignore}, or {@code + * $HOME/.config/git/ignore} when that variable is unset — per {@code gitignore(5)}: git applies + * that file even with no {@code core.excludesFile} configured at all, so skipping it here would + * silently drop patterns an operator never had to configure to get. + * + *

Never throws: a missing, unreadable, or unresolvable file is treated as "nothing to carry + * 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. + * + * @return the file's content, trailing-newline-normalized, or {@code ""} when there is nothing + * to compose with. + */ + private String previouslyEffectiveExcludesFileContent(String worktreePath) { + String resolvedPath; + if (exitCode("git", "-C", worktreePath, "config", "--get", "--type=path", "core.excludesFile") == 0) { + resolvedPath = exec("git", "-C", worktreePath, "config", "--get", "--type=path", + "core.excludesFile").trim(); + } else { + String xdgConfigHome = System.getenv("XDG_CONFIG_HOME"); + Path fallback = (xdgConfigHome != null && !xdgConfigHome.isBlank()) + ? Path.of(xdgConfigHome, "git", "ignore") + : Path.of(System.getProperty("user.home"), ".config", "git", "ignore"); + resolvedPath = fallback.toString(); + } + if (resolvedPath.isBlank()) { + return ""; + } + Path file = Path.of(resolvedPath); + if (!Files.isRegularFile(file) || !Files.isReadable(file)) { + return ""; + } + try { + String content = Files.readString(file); + return content.isBlank() ? "" : content.stripTrailing() + System.lineSeparator(); + } catch (IOException e) { + log.warn("could not read previously-effective excludesFile {} while seeding skills into " + + "{}: {} — its patterns will not carry forward into the seeded worktree", + file, worktreePath, e.getMessage()); + return ""; + } + } + /** * 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 @@ -1283,6 +1389,9 @@ public final class GitWorktrees implements Worktrees { Process p; try { ProcessBuilder pb = new ProcessBuilder(command).redirectErrorStream(true); + if (!gitEnv.isEmpty()) { + pb.environment().putAll(gitEnv); + } if (extraEnv != null && !extraEnv.isEmpty()) { pb.environment().putAll(extraEnv); } @@ -1318,7 +1427,11 @@ public final class GitWorktrees implements Worktrees { private int exitCode(String... command) { Process p; try { - p = new ProcessBuilder(command).redirectErrorStream(true).start(); + ProcessBuilder pb = new ProcessBuilder(command).redirectErrorStream(true); + if (!gitEnv.isEmpty()) { + pb.environment().putAll(gitEnv); + } + p = pb.start(); } catch (IOException e) { throw new WorktreeException("failed to start " + command[0] + ": " + e.getMessage(), e); } 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 a2cb113..b099944 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java @@ -1603,4 +1603,54 @@ class GitWorktreesTest { m.contains("seeded: implementer") && m.contains("kept the repo's own copy of: hunter")), "expected a summary naming both the seeded and kept skills, got:\n" + capturedMessages()); } + + /** + * fleetd #362 review fix — the "compose, don't replace" invariant, a third direction alongside + * {@link #seedSkillsHidesSeededPathsFromGitStatus} and + * {@link #seedSkillsExcludeDoesNotLeakIntoASiblingWorktree}. {@code core.excludesFile} is + * single-valued: the first cut of {@code excludeSeededSkillsFromGitStatus} pointed it at + * fleetd's own exclude file with {@code --replace-all}, which SHADOWS whatever excludesFile the + * worktree was already resolving (an operator's global config, most commonly) instead of adding + * to it. Concretely, this repo's own {@code .gitignore} does not ignore {@code target/} — only an + * operator's global excludesFile does — so every worker's {@code mvn clean install} would + * otherwise make {@code target/} appear as untracked, and CB-576's deliberately + * untracked-inclusive {@code hasUncommitted} would then read every such worktree as dirty + * forever, so {@code SessionManager} never cleans it up. + * + *

A synthetic "operator's global git config" is isolated via {@code GIT_CONFIG_GLOBAL} + * pointed at a throwaway temp file, passed to {@link GitWorktrees} through its {@code gitEnv} + * test seam — never the real machine's own git config. That global config ignores {@code + * target}. A skill is then seeded through the real {@link GitWorktrees#add} path, and a file + * named {@code target} is written into the worktree afterward: {@code git status --porcelain} + * must still be empty, proving the operator's own global pattern kept applying after seeding. + */ + @Test + void seedSkillsComposesWithAnAlreadyEffectiveGlobalExcludesFile(@TempDir Path tmp) throws Exception { + Path globalExcludes = tmp.resolve("operator-global-ignore"); + Files.writeString(globalExcludes, "target\n"); + Path globalConfig = tmp.resolve("operator-global.gitconfig"); + Files.writeString(globalConfig, "[core]\n\texcludesFile = " + globalExcludes + "\n"); + Map gitEnv = Map.of( + "GIT_CONFIG_GLOBAL", globalConfig.toString(), + "GIT_CONFIG_SYSTEM", "/dev/null", + "GIT_TERMINAL_PROMPT", "0"); + + 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-global-compose", "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, "target"), "build output the operator's global config ignores\n"); + + String porcelain = fullStatus(Path.of(wt)); + assertEquals("", porcelain, + "the operator's own global excludesFile pattern ('target') must still apply after " + + "skill seeding ran — got:\n" + porcelain); + } }