fleetd #362 review fix: compose skill-seeding excludes with the operator's own excludesFile
core.excludesFile is single-valued, so pointing it at fleetd's own seeded-skill exclude file with --replace-all at worktree scope was SHADOWING whatever excludesFile the worktree already resolved (an operator's global config, most commonly) instead of adding to it. This repo's own .gitignore does not ignore target/ — only an operator's global excludesFile does — so every worker's `mvn clean install` would make target/ show up as untracked, and CB-576's deliberately untracked-inclusive hasUncommitted would then read every such worktree as dirty forever, so it is never cleaned up. excludeSeededSkillsFromGitStatus now reads whatever core.excludesFile resolves to BEFORE writing anything (falling back to git's own $XDG_CONFIG_HOME/git/ignore default when the key is unset entirely, per gitignore(5)), and writes that content into fleetd's own exclude file ahead of the seeded skill patterns, so every operator-configured pattern keeps applying inside the seeded worktree. Proven with a new test, seedSkillsComposesWithAnAlreadyEffectiveGlobalExcludesFile, which isolates a synthetic "operator's global config" via a new gitEnv test seam on GitWorktrees (GIT_CONFIG_GLOBAL pointed at a throwaway temp file, never the real machine's config) and drives the real add() path end to end. Also documents (FleetConfig javadoc + fleetd.example.yaml) that memberSkills copies every non-hidden subdirectory of its source wholesale, with no per-file allowlist.
This commit is contained in:
@@ -782,7 +782,9 @@ guard:
|
||||
# .claude/skills/<name> 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
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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<String, String> gitEnv;
|
||||
private final Consumer<String> 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<String> afterWorktreeAdded,
|
||||
Function<String[], String> shareGroupRunner, Function<String[], String> 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<String> afterWorktreeAdded,
|
||||
Function<String[], String> shareGroupRunner, Function<String[], String> worktreeAddRunner,
|
||||
String memberSkillsSource, Map<String, String> 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.
|
||||
*
|
||||
* <p><b>Compose, don't replace.</b> {@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.
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p><b>Invariant 3 — best-effort.</b> 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.
|
||||
*
|
||||
* <p>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.
|
||||
* <p><b>Compose, don't replace.</b> {@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<String> 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.
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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);
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p>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<String, String> 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);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user