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

fleetd #362 (item 3): seed .claude/skills/ into provisioned worktrees #366

Closed
agent wants to merge 0 commits from worker/362-worktree-skills-c03e51-3 into main
pull from: worker/362-worktree-skills-c03e51-3
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-09-05 07:55:22 +02:00
Member
Copy Link
Copy Source

Review fix round 2 (lead-caught, two findings on the same root cause): the XDG fallback

The lead re-verified round 1's fix live (merged origin/main — which had picked up the
concurrent #363/items-1-2 PR — into this branch, clean merge, mvn clean install →
Tests run: 1386, BUILD SUCCESS, matching my number exactly) and confirmed the
--absolute-git-dir placement and the composition itself are correct. Two findings came back,
both about the XDG fallback branch I added in round 1 (the one that fires when
core.excludesFile is unset entirely, not just the "already configured" case round 1's test
covers).

Finding 1 — the fallback was unpinned. The lead ran a mutation I hadn't: delete the fallback
so an unset key composes with "". Green both ways (Tests run: 1386 before and after). My round
1 test only ever exercised the "already set" branch. Fixed by adding
seedSkillsComposesWithTheXdgDefaultExcludesFileWhenNoneIsConfigured — isolates
XDG_CONFIG_HOME via the gitEnv seam at a temp dir carrying a synthetic git/ignore, points
GIT_CONFIG_GLOBAL at an empty file (so core.excludesFile is genuinely unset, forcing the
fallback branch rather than the "already configured" one), seeds a skill, and asserts a file
matching the XDG-default pattern is still invisible to git status. I re-ran the lead's exact
mutation (this time against the new test) and captured the real red output before reverting:

org.opentest4j.AssertionFailedError:
the XDG default excludesFile pattern ('xdg-fallback-marker') must still apply after skill seeding ran — got:
?? xdg-fallback-marker
 ==> expected: <> but was: <?? xdg-fallback-marker
>
	at dev.ltms.fleet.session.GitWorktreesTest.seedSkillsComposesWithTheXdgDefaultExcludesFileWhenNoneIsConfigured(GitWorktreesTest.java:1738)
[ERROR] Tests run: 59, Failures: 1, Errors: 0, Skipped: 0

Finding 2 — the actual code bug: the fallback bypassed the gitEnv seam entirely. It read
XDG_CONFIG_HOME/HOME straight from the JVM's own environment (System.getenv/user.home)
instead of through git, so no test could isolate it — and on any machine carrying a real
~/.config/git/ignore (this dev machine measurably does: **/.claude/settings.local.json),
every seeding test in the suite was silently composing with that real file, machine-dependently.
Fixed: added resolveEnv(String)/resolveHome(), which check the gitEnv seam first and
fall back to the real JVM environment only when the seam doesn't supply a value — production
behaviour (gitEnv is always Map.of() there) is byte-identical to before. Also added a
hermeticGitEnv(Path) test helper and routed every skill-seeding test in GitWorktreesTest
through it, so nothing in the class can reach the real machine's home directory for this
fallback anymore.

Two non-defect javadoc notes added, per the lead's request:

  • previouslyEffectiveExcludesFileContent copies a snapshot at seed time, not a live
    reference — a later edit to the operator's own excludesFile does not propagate into an
    already-seeded worktree.
  • excludeSeededSkillsFromGitStatus assumes a fresh worktree and is not idempotent if ever
    called twice on the same one — not reachable today (only add() calls seedSkills, and add()
    always creates a fresh worktree), so no guard was added for a path nothing takes; documented so
    a future second caller is warned instead.

Build after round 2, unpiped: Tests run: 1387, Failures: 0, Errors: 0, Skipped: 0,
BUILD SUCCESS. (GitWorktreesTest: 59 tests, all green.)

wiki/11-Features.md entry (per CLAUDE.md's "the prompt is part of the product" rule — I
cannot commit to wiki/ myself, this is text for the lead to add):

Seed bridge skills into any provisioned worktree — memberSkills: <dir> in fleetd.yaml ·
exists because a member spawned against a repo that does not ship its own .claude/skills/
(i.e. any repo but this one) previously could not load implementer/reviewer/hunter at
all — every brief starts with "Load the <name> skill", and outside this repo that line was
silently a no-op · gotcha: the directory's contents are copied wholesale (every non-hidden
subdirectory, no per-file allowlist) into every provisioned worktree's .claude/skills/, so
don't park scratch files there, and a skill folder the target repo already ships under that
name is never overwritten. Claude Code members only; an opencode member's equivalent
(.opencode/agent) is a different file shape this key does not touch.

Review fix (lead-caught bug): compose, don't replace — added after first review

The lead found a serious bug in the mechanism below: core.excludesFile is single-valued, so
setting it --worktree (as the first cut of this PR did, via --replace-all) replaces —
does not add to — whatever excludesFile the worktree was already resolving, most commonly an
operator's own global config. Concretely: 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 GitWorktrees#hasUncommitted's deliberately
untracked-inclusive git status --porcelain (CB-576) would then read every worktree that
builds as dirty forever, so SessionManager never releases it. The lead proved this live before
I fixed it (isolated global excludesFile ignoring target; before seeding git status --porcelain was empty; after seeding it showed ?? target/).

Fix: excludeSeededSkillsFromGitStatus now reads whatever core.excludesFile resolves to
before writing anything — via git config --get --type=path core.excludesFile (so ~
expansion happens exactly the way git itself would apply it), falling back to git's own documented
default ($XDG_CONFIG_HOME/git/ignore, or $HOME/.config/git/ignore) when the key is unset
entirely, per gitignore(5). That content is written into fleetd's own exclude file ahead of
the seeded skill patterns, and only then does the worktree-scoped override point at the combined
file. Every operator-configured pattern keeps applying inside the seeded worktree, plus the seeded
skill paths.

New test, seedSkillsComposesWithAnAlreadyEffectiveGlobalExcludesFile — the third invariant-2
direction, alongside the two that already existed (seeded worktree stays clean; a sibling
worktree's own untracked files don't get hidden by the leaking exclude). It isolates a synthetic
"operator's global git config" via a new gitEnv test seam on GitWorktrees (GIT_CONFIG_GLOBAL
pointed at a throwaway temp file — never the real machine's config), drives the real add() path
end to end, then writes a target file into the seeded worktree and asserts git status --porcelain is still empty. Reproduced the same shape with plain git commands outside the test
suite for a concrete before/after:

== BEFORE fix: old --replace-all with ONLY the seeded pattern (discards the global excludesFile) ==
git status --porcelain (OLD, buggy behavior):
?? target

== AFTER fix: compose — read the previously-effective excludesFile content first, then append the seeded pattern ==
resolved previously-effective excludesFile (before fleetd's own write): /.../operator-global-ignore
composed exclude file content:
target
/.claude/skills/implementer/
git status --porcelain (NEW, composed behavior):
(empty output above = clean: the operator's own global 'target' pattern still applies after seeding)

Also documented in FleetConfig's javadoc and fleetd.example.yaml, per the lead's request: the
memberSkills directory's non-hidden subdirectories are copied wholesale, with no per-file
allowlist — don't park scratch files there.

This is a NEW commit on top of the original one; everything below this section describes the
original PR as first submitted. The one place it is now stale is the "Caveat" paragraph inside
"Invariant 2" below, which is corrected in place rather than deleted, so the review history stays
readable.

Scope

fleetd #362, scope item 3 only: "Seed .claude/skills/ into provisioned worktrees." Items 1
and 2 of that issue (the plugin fixes, plugin/, README.md, CLAUDE.md) are out of scope for
this PR and were not touched — verified with git diff --stat before committing.

What changed

  • FleetConfig: new optional top-level key memberSkills: <dir> — a directory of skill
    folders (each holding a SKILL.md, the same shape as this repo's own .claude/skills/).
    null/blank = off (today's behaviour, unchanged).
  • GitWorktrees#add: after the existing isolateToolSurface(wt) step, calls a new
    seedSkills(wt). For each subdirectory of the configured source, if
    <worktree>/.claude/skills/<name> does not already exist (i.e. the target repo doesn't commit
    its own copy), it is copied in recursively. A name that already exists is left completely
    untouched — never opened, never overwritten (invariant 1).
  • Invariant 2 (never end up in a commit): proven with git status --porcelain, not by
    reasoning — see "Invariant 2" below.
  • Fleetd.java: passes cfg.memberSkills() into the GitWorktrees constructor at the one
    construction site (Fleetd.java:251).
  • ConfigRef: memberSkills is triaged as a DEFERRED key — baked once into the
    GitWorktrees built at startup and never rebuilt, exactly like worktreeGroup and
    worktreeRoot. Added a changedDeferredKeys branch and updated the coverage-test value maps
    (ConfigRefTopLevelReportingCoverageTest, FleetConfigWithDefaultsPreservesEveryComponentTest)
    so the new key is proven, not just claimed, to have real reporting behind it — this repo has a
    purpose-built test (ConfigRefTopLevelCoverageTest) that fails the build if a new FleetConfig
    component is left untriaged.
  • fleetd.example.yaml: documented memberSkills: (commented out), matching the doc-coverage
    test FleetConfigTest.everyKnownTopLevelKeyIsDocumentedInTheExample.

Where the code goes

Right beside the existing worktree-provisioning steps in GitWorktrees.java — isolateToolSurface
(.mcp.json/opencode.json/.autoenv neutralization) and overlayParity — in the same
lifecycle (add()), not in a new place, per the brief.

Copy vs symlink — copy, and why

A symlink into the fleetd daemon's own directory would dangle the moment that checkout moves, is
archived, or the daemon runs from a different jar/checkout than the one that provisioned a given
worktree (a real risk once a worktree can outlive a daemon restart or redeploy). A copy is
self-contained and survives all of that; the tradeoff (drift if the source skill changes after
seeding) is the same one overlayParity already accepts for its own copies, and is far less
harmful than a dangling link that silently makes Load the <skill> skill. fail.

Invariant 2 — proof, not reasoning

The ticket suggested the shared .git/info/exclude (the same mechanism
fleet.neutralizedConfig/ClaudeCodeLauncher#writeIdeOverlay use for CLAUDE.local.md). I
checked this before using it, with a real throwaway repo:

$ git -C wt1 rev-parse --git-path info/exclude
/private/tmp/giteattest/main/.git/info/exclude    # <- resolves to the COMMON dir, not the worktree

.git/info/exclude is shared across every linked worktree and the primary checkout — not
per-worktree. Using it here would make a seeded skill's path invisible to git status in the
primary's own checkout and every sibling worktree too, not just the one it was seeded
into. That's a bigger footprint than the ticket's own invariant 2 asks for ("it is per-worktree
and local").

Instead I set core.excludesFile scoped --worktree (the same extensions.worktreeConfig
mechanism this file already uses for the credential helper and the SSH→HTTPS rewrite) to a file
written under the worktree's own private git dir (git rev-parse --absolute-git-dir, e.g.
.git/worktrees/<nonce>/fleet-seeded-skills-exclude) — outside the working tree, so the exclude
file itself can never be committed either, and scoped so it affects only that one worktree.
Verified with a real git command in a throwaway repo before writing any code:

$ git -C wt1 config --worktree --replace-all core.excludesFile <path-under-.git/worktrees/wt1>
$ git -C wt1 status --porcelain     # empty — the seeded file is invisible
$ git status --porcelain            # unaffected in the primary checkout (different worktree)

GitWorktreesTest now proves both directions with real git commands (not the recording fake):
seedSkillsHidesSeededPathsFromGitStatus proves the seeded worktree stays clean, and
seedSkillsExcludeDoesNotLeakIntoASiblingWorktree proves a second worktree of the same repo
(provisioned with memberSkills unset) still reports an untracked .claude/skills/ the ordinary
way — proving the exclude did not leak in via the shared common dir.

Recorded for the worker itself, mirroring fleetd #134's fleet.neutralizedConfig /
fleet.neutralizedConfigNote: fleet.seededSkills (one value per seeded skill) and
fleet.seededSkillsNote, readable with git config --worktree --get-all fleet.seededSkills.

Caveat: this sets core.excludesFile at worktree scope unconditionally when at least one
skill is seeded, which would override an operator-configured global excludesFile's effect
within that one worktree — FIXED, see "Review fix" at the top. This was wrong: it does not
merely affect that one worktree's exclude behavior as a stylistic choice, it actively discards
whatever the operator's own excludesFile was already doing there, with a concrete failure mode
(worker mvn build artifacts like target/ reading as untracked forever, so
SessionManager/CB-576 never releases the worktree). excludeSeededSkillsFromGitStatus now reads
and carries forward whatever was previously effective before pointing the worktree-scoped key at
its own composed file — see the top of this PR body for the fix and its proof.

Invariant 3 — best-effort

seedSkills never throws out of add(): a missing/unreadable memberSkills source, or a
copy/exclude failure, is caught, logged with log.warn, and the spawn proceeds — same contract as
overlayParity/isolateToolSurface. Covered by
seedSkillsIsBestEffortWhenSourceDoesNotExist.

Invariant 4 — teardown

No new cleanup code was needed. GitWorktrees#remove already runs git worktree remove --force,
which deletes the whole working tree (the seeded .claude/skills/<name> files included) AND the
worktree's private git dir (.git/worktrees/<nonce>/, which is where the exclude file and the
fleet.seededSkills* config live) in one step. Nothing this PR adds lives outside that boundary,
so there is nothing extra to clean up — I read the teardown path to confirm this rather than
assuming it.

Scope limit — Claude Code only

seedSkills itself is backend-agnostic — it runs inside GitWorktrees#add, which is shared by
every launcher, the same way isolateToolSurface unconditionally neutralizes BOTH .mcp.json
(Claude) and opencode.json (opencode) regardless of which backend ultimately spawns into a given
worktree, because the backend isn't chosen yet at add() time. What makes this "Claude Code
members only" is that .claude/skills/<name>/SKILL.md is a path only the Claude Code launcher
ever reads (ClaudeCodeLauncher/HerdrPeerLauncher#agentDefinitionFile for .claude/agents, and
the skill-loading convention on top of it) — a seeded .claude/skills/ in an opencode member's
worktree is just an inert, unused directory.

What opencode would still need (not implemented, per the brief): opencode's skill-equivalent
lives under .opencode/agent, a different file shape (opencode agent-definition format, not
SKILL.md), so this would need either a source-format adapter (fleetd translates each skill into
an opencode agent file) or the source directory itself carrying both shapes side by side. Out of
scope here.

FleetConfig.java — exact lines touched (per the brief: another worker is editing this file on

a different ticket right now)

All edits are additive and localized to the memberSkills-specific spots, matching the existing
pattern used for worktreeGroup/memberLoginShell:

  • Javadoc: one new @param memberSkills block, appended after the existing @param memberLoginShell block.
  • Record header: String memberSkills appended as the last component (after memberLoginShell).
  • One new back-compat constructor: "Back-compat form before the memberSkills key was added"
    (22→23-arg forwarding), placed immediately above the existing "before memberLoginShell"
    back-compat constructor — no other constructor in the chain was touched.
  • KNOWN_TOP_LEVEL_KEYS: appended "memberSkills".
  • withDefaults(): one new comment + memberSkills passed through unchanged (left-as-is, no
    default — same treatment as worktreeGroup/memberLoginShell) in the final return new FleetConfig(...) call.

No existing line was modified in place except the two call-sites above that had to grow one more
trailing argument (the back-compat constructor's delegation, and withDefaults()'s own
constructor call) — both are pure append-at-the-end edits, chosen specifically to minimize merge
risk against the concurrent FleetConfig.java edit on the other ticket.

Build

Ran unpiped inside the worktree:

cd fleetd && mvn clean install

Real result (after the review fix, current HEAD of this branch):
Tests run: 1386, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS. (The original
submission's run, before the fix, was Tests run: 1385.)

This run includes 7 new tests in GitWorktreesTest (58 total there now, up from 51) covering:
fresh seed with no .claude/ at all, invariant 1 (repo's own skill survives byte-for-byte even
when the source carries a same-named skill with different content), invariant 3 (missing source
directory never fails the spawn), invariant 2 in three directions (seeded worktree's git status
is clean; a sibling worktree's own unrelated untracked .claude/skills/ still shows up normally;
and — added in the review fix — an operator's own global excludesFile pattern still applies after
seeding, via the new seedSkillsComposesWithAnAlreadyEffectiveGlobalExcludesFile), and the
seeded/kept log-line shape. It also includes updates to two existing config coverage tests
(ConfigRefTopLevelReportingCoverageTest, FleetConfigWithDefaultsPreservesEveryComponentTest)
required to keep the new memberSkills key's BASE/ALT value maps in sync with the record shape —
those tests fail loudly (by design) if a new top-level FleetConfig component isn't given a real
value and triaged.

Along the way, adding memberSkills initially broke three pre-existing tests that check every
FleetConfig component is triaged into a reload class and given a real value
(ConfigRefTopLevelCoverageTest, ConfigRefTopLevelReportingCoverageTest,
FleetConfigWithDefaultsPreservesEveryComponentTest) until memberSkills was added to
ConfigRef.DEFERRED_KEYS with its own changedDeferredKeys branch and to those tests' value
maps — all now green.

I have no IDE MCP tools as a worker; the above mvn clean install output is the only check I ran
or am claiming.

Anything else in this repo with the same shape (not fixed, per the brief)

  • plugin/skills/setup/SKILL.md and the rest of plugin/ — a member-facing onboarding asset that
    only a plugin install delivers, never a provisioned worktree; issue #362 items 1–2 (someone
    else's scope) cover it.
  • .claude/agents/*.md — already worktree-seeded via agentDefinitionFile, so not itself a gap,
    but any new agent role added only to this repo's .claude/agents/ (not to whatever directory a
    future memberSkills-style config seeds) would be invisible in another repo's worktree the same
    way skills were before this PR.
  • docs/Worker-Git-Workflow.md and docs/MCP-Contract.md — referenced by the implementer skill
    and by this repo's own CLAUDE.md, but neither is seeded into a worktree of a different repo;
    a worker there following a seeded implementer skill that links to ../../docs/... would find
    nothing.
  • wiki/ — already known to be a submodule workers see as months-stale (per team memory); not this
    ticket's shape exactly, but the same underlying pattern ("an asset a member is expected to have
    that only exists because the member happens to be in this one repo").

Caveats for review

  • The core.excludesFile worktree-scope caveat above — fixed, see "Review fix" at the top of
    this PR body.
  • seedSkills does not restrict candidate skill names to a known allowlist (implementer,
    reviewer, hunter) — it copies every immediate, non-hidden subdirectory of the configured
    source. The lead reviewed this and said to leave it as-is; per that review I documented in
    FleetConfig's javadoc and fleetd.example.yaml that the directory's contents are copied
    wholesale, so an operator knows not to park scratch files there.
  • I did not verify this against a live fleetd.yaml (gitignored, and I cannot see it as a worker)
    — all proof is from throwaway repos exercising GitWorktrees#add end-to-end, per the brief's
    own instruction to reproduce the shape rather than reason about the live config.
## Review fix round 2 (lead-caught, two findings on the same root cause): the XDG fallback The lead re-verified round 1's fix live (merged `origin/main` — which had picked up the concurrent #363/items-1-2 PR — into this branch, clean merge, `mvn clean install` → `Tests run: 1386, BUILD SUCCESS`, matching my number exactly) and confirmed the `--absolute-git-dir` placement and the composition itself are correct. Two findings came back, both about the **XDG fallback branch** I added in round 1 (the one that fires when `core.excludesFile` is unset entirely, not just the "already configured" case round 1's test covers). **Finding 1 — the fallback was unpinned.** The lead ran a mutation I hadn't: delete the fallback so an unset key composes with `""`. Green both ways (`Tests run: 1386` before and after). My round 1 test only ever exercised the "already set" branch. **Fixed** by adding `seedSkillsComposesWithTheXdgDefaultExcludesFileWhenNoneIsConfigured` — isolates `XDG_CONFIG_HOME` via the `gitEnv` seam at a temp dir carrying a synthetic `git/ignore`, points `GIT_CONFIG_GLOBAL` at an *empty* file (so `core.excludesFile` is genuinely unset, forcing the fallback branch rather than the "already configured" one), seeds a skill, and asserts a file matching the XDG-default pattern is still invisible to `git status`. I re-ran the lead's exact mutation (this time against the new test) and captured the real red output before reverting: ``` org.opentest4j.AssertionFailedError: the XDG default excludesFile pattern ('xdg-fallback-marker') must still apply after skill seeding ran — got: ?? xdg-fallback-marker ==> expected: <> but was: <?? xdg-fallback-marker > at dev.ltms.fleet.session.GitWorktreesTest.seedSkillsComposesWithTheXdgDefaultExcludesFileWhenNoneIsConfigured(GitWorktreesTest.java:1738) [ERROR] Tests run: 59, Failures: 1, Errors: 0, Skipped: 0 ``` **Finding 2 — the actual code bug: the fallback bypassed the `gitEnv` seam entirely.** It read `XDG_CONFIG_HOME`/`HOME` straight from the JVM's own environment (`System.getenv`/`user.home`) instead of through `git`, so no test could isolate it — and on any machine carrying a real `~/.config/git/ignore` (this dev machine measurably does: `**/.claude/settings.local.json`), every seeding test in the suite was silently composing with that real file, machine-dependently. **Fixed**: added `resolveEnv(String)`/`resolveHome()`, which check the `gitEnv` seam first and fall back to the real JVM environment only when the seam doesn't supply a value — production behaviour (`gitEnv` is always `Map.of()` there) is byte-identical to before. Also added a `hermeticGitEnv(Path)` test helper and routed every skill-seeding test in `GitWorktreesTest` through it, so nothing in the class can reach the real machine's home directory for this fallback anymore. Two non-defect javadoc notes added, per the lead's request: - `previouslyEffectiveExcludesFileContent` copies a **snapshot** at seed time, not a live reference — a later edit to the operator's own excludesFile does not propagate into an already-seeded worktree. - `excludeSeededSkillsFromGitStatus` **assumes a fresh worktree** and is not idempotent if ever called twice on the same one — not reachable today (only `add()` calls `seedSkills`, and `add()` always creates a fresh worktree), so no guard was added for a path nothing takes; documented so a future second caller is warned instead. **Build after round 2, unpiped**: `Tests run: 1387, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`. (`GitWorktreesTest`: 59 tests, all green.) **wiki/11-Features.md entry** (per CLAUDE.md's "the prompt is part of the product" rule — I cannot commit to `wiki/` myself, this is text for the lead to add): > **Seed bridge skills into any provisioned worktree** — `memberSkills: <dir>` in `fleetd.yaml` · > exists because a member spawned against a repo that does not ship its own `.claude/skills/` > (i.e. any repo but this one) previously could not load `implementer`/`reviewer`/`hunter` at > all — every brief starts with "Load the `<name>` skill", and outside this repo that line was > silently a no-op · gotcha: the directory's contents are copied **wholesale** (every non-hidden > subdirectory, no per-file allowlist) into every provisioned worktree's `.claude/skills/`, so > don't park scratch files there, and a skill folder the target repo already ships under that > name is never overwritten. Claude Code members only; an opencode member's equivalent > (`.opencode/agent`) is a different file shape this key does not touch. ## Review fix (lead-caught bug): compose, don't replace — added after first review The lead found a serious bug in the mechanism below: `core.excludesFile` is **single-valued**, so setting it `--worktree` (as the first cut of this PR did, via `--replace-all`) **replaces** — does not add to — whatever excludesFile the worktree was already resolving, most commonly an operator's own global config. Concretely: 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 `GitWorktrees#hasUncommitted`'s deliberately untracked-inclusive `git status --porcelain` (CB-576) would then read **every worktree that builds** as dirty forever, so `SessionManager` never releases it. The lead proved this live before I fixed it (isolated global excludesFile ignoring `target`; before seeding `git status --porcelain` was empty; after seeding it showed `?? target/`). **Fix**: `excludeSeededSkillsFromGitStatus` now reads whatever `core.excludesFile` resolves to **before** writing anything — via `git config --get --type=path core.excludesFile` (so `~` expansion happens exactly the way git itself would apply it), falling back to git's own documented default (`$XDG_CONFIG_HOME/git/ignore`, or `$HOME/.config/git/ignore`) when the key is unset entirely, per `gitignore(5)`. That content is written into fleetd's own exclude file **ahead of** the seeded skill patterns, and only then does the worktree-scoped override point at the combined file. Every operator-configured pattern keeps applying inside the seeded worktree, plus the seeded skill paths. New test, `seedSkillsComposesWithAnAlreadyEffectiveGlobalExcludesFile` — the third invariant-2 direction, alongside the two that already existed (seeded worktree stays clean; a sibling worktree's own untracked files don't get hidden by the leaking exclude). It isolates a synthetic "operator's global git config" via a new `gitEnv` test seam on `GitWorktrees` (`GIT_CONFIG_GLOBAL` pointed at a throwaway temp file — never the real machine's config), drives the real `add()` path end to end, then writes a `target` file into the seeded worktree and asserts `git status --porcelain` is still empty. Reproduced the same shape with plain `git` commands outside the test suite for a concrete before/after: ``` == BEFORE fix: old --replace-all with ONLY the seeded pattern (discards the global excludesFile) == git status --porcelain (OLD, buggy behavior): ?? target == AFTER fix: compose — read the previously-effective excludesFile content first, then append the seeded pattern == resolved previously-effective excludesFile (before fleetd's own write): /.../operator-global-ignore composed exclude file content: target /.claude/skills/implementer/ git status --porcelain (NEW, composed behavior): (empty output above = clean: the operator's own global 'target' pattern still applies after seeding) ``` Also documented in `FleetConfig`'s javadoc and `fleetd.example.yaml`, per the lead's request: the `memberSkills` directory's non-hidden subdirectories are copied **wholesale**, with no per-file allowlist — don't park scratch files there. This is a NEW commit on top of the original one; everything below this section describes the original PR as first submitted. The one place it is now stale is the "Caveat" paragraph inside "Invariant 2" below, which is corrected in place rather than deleted, so the review history stays readable. ## Scope fleetd #362, **scope item 3 only**: "Seed `.claude/skills/` into provisioned worktrees." Items 1 and 2 of that issue (the plugin fixes, `plugin/`, `README.md`, `CLAUDE.md`) are out of scope for this PR and were not touched — verified with `git diff --stat` before committing. ## What changed - **`FleetConfig`**: new optional top-level key `memberSkills: <dir>` — a directory of skill folders (each holding a `SKILL.md`, the same shape as this repo's own `.claude/skills/`). `null`/blank = off (today's behaviour, unchanged). - **`GitWorktrees#add`**: after the existing `isolateToolSurface(wt)` step, calls a new `seedSkills(wt)`. For each subdirectory of the configured source, if `<worktree>/.claude/skills/<name>` does not already exist (i.e. the target repo doesn't commit its own copy), it is copied in recursively. A name that already exists is left completely untouched — never opened, never overwritten (invariant 1). - **Invariant 2 (never end up in a commit)**: proven with `git status --porcelain`, not by reasoning — see "Invariant 2" below. - **`Fleetd.java`**: passes `cfg.memberSkills()` into the `GitWorktrees` constructor at the one construction site (`Fleetd.java:251`). - **`ConfigRef`**: `memberSkills` is triaged as a `DEFERRED` key — baked once into the `GitWorktrees` built at startup and never rebuilt, exactly like `worktreeGroup` and `worktreeRoot`. Added a `changedDeferredKeys` branch and updated the coverage-test value maps (`ConfigRefTopLevelReportingCoverageTest`, `FleetConfigWithDefaultsPreservesEveryComponentTest`) so the new key is proven, not just claimed, to have real reporting behind it — this repo has a purpose-built test (`ConfigRefTopLevelCoverageTest`) that fails the build if a new `FleetConfig` component is left untriaged. - **`fleetd.example.yaml`**: documented `memberSkills:` (commented out), matching the doc-coverage test `FleetConfigTest.everyKnownTopLevelKeyIsDocumentedInTheExample`. ## Where the code goes Right beside the existing worktree-provisioning steps in `GitWorktrees.java` — `isolateToolSurface` (`.mcp.json`/`opencode.json`/`.autoenv` neutralization) and `overlayParity` — in the same lifecycle (`add()`), not in a new place, per the brief. ## Copy vs symlink — copy, and why A symlink into the fleetd daemon's own directory would dangle the moment that checkout moves, is archived, or the daemon runs from a different jar/checkout than the one that provisioned a given worktree (a real risk once a worktree can outlive a daemon restart or redeploy). A copy is self-contained and survives all of that; the tradeoff (drift if the source skill changes after seeding) is the same one `overlayParity` already accepts for its own copies, and is far less harmful than a dangling link that silently makes `Load the <skill> skill.` fail. ## Invariant 2 — proof, not reasoning The ticket suggested the shared `.git/info/exclude` (the same mechanism `fleet.neutralizedConfig`/`ClaudeCodeLauncher#writeIdeOverlay` use for `CLAUDE.local.md`). I checked this **before** using it, with a real throwaway repo: ``` $ git -C wt1 rev-parse --git-path info/exclude /private/tmp/giteattest/main/.git/info/exclude # <- resolves to the COMMON dir, not the worktree ``` `.git/info/exclude` is **shared** across every linked worktree and the primary checkout — not per-worktree. Using it here would make a seeded skill's path invisible to `git status` in the **primary's own checkout** and **every sibling worktree** too, not just the one it was seeded into. That's a bigger footprint than the ticket's own invariant 2 asks for ("it is per-worktree and local"). Instead I set `core.excludesFile` **scoped `--worktree`** (the same `extensions.worktreeConfig` mechanism this file already uses for the credential helper and the SSH→HTTPS rewrite) to a file written under the worktree's own **private** git dir (`git rev-parse --absolute-git-dir`, e.g. `.git/worktrees/<nonce>/fleet-seeded-skills-exclude`) — outside the working tree, so the exclude file itself can never be committed either, and scoped so it affects only that one worktree. Verified with a real git command in a throwaway repo before writing any code: ``` $ git -C wt1 config --worktree --replace-all core.excludesFile <path-under-.git/worktrees/wt1> $ git -C wt1 status --porcelain # empty — the seeded file is invisible $ git status --porcelain # unaffected in the primary checkout (different worktree) ``` `GitWorktreesTest` now proves both directions with real git commands (not the recording fake): `seedSkillsHidesSeededPathsFromGitStatus` proves the seeded worktree stays clean, and `seedSkillsExcludeDoesNotLeakIntoASiblingWorktree` proves a **second** worktree of the same repo (provisioned with `memberSkills` unset) still reports an untracked `.claude/skills/` the ordinary way — proving the exclude did not leak in via the shared common dir. Recorded for the worker itself, mirroring fleetd #134's `fleet.neutralizedConfig` / `fleet.neutralizedConfigNote`: `fleet.seededSkills` (one value per seeded skill) and `fleet.seededSkillsNote`, readable with `git config --worktree --get-all fleet.seededSkills`. ~~**Caveat**: this sets `core.excludesFile` at worktree scope unconditionally when at least one skill is seeded, which would override an operator-configured global `excludesFile`'s effect *within that one worktree*~~ — **FIXED, see "Review fix" at the top.** This was wrong: it does not merely affect that one worktree's exclude behavior as a stylistic choice, it actively *discards* whatever the operator's own excludesFile was already doing there, with a concrete failure mode (worker `mvn` build artifacts like `target/` reading as untracked forever, so `SessionManager`/CB-576 never releases the worktree). `excludeSeededSkillsFromGitStatus` now reads and carries forward whatever was previously effective before pointing the worktree-scoped key at its own composed file — see the top of this PR body for the fix and its proof. ## Invariant 3 — best-effort `seedSkills` never throws out of `add()`: a missing/unreadable `memberSkills` source, or a copy/exclude failure, is caught, logged with `log.warn`, and the spawn proceeds — same contract as `overlayParity`/`isolateToolSurface`. Covered by `seedSkillsIsBestEffortWhenSourceDoesNotExist`. ## Invariant 4 — teardown No new cleanup code was needed. `GitWorktrees#remove` already runs `git worktree remove --force`, which deletes the whole working tree (the seeded `.claude/skills/<name>` files included) AND the worktree's private git dir (`.git/worktrees/<nonce>/`, which is where the exclude file and the `fleet.seededSkills*` config live) in one step. Nothing this PR adds lives outside that boundary, so there is nothing extra to clean up — I read the teardown path to confirm this rather than assuming it. ## Scope limit — Claude Code only `seedSkills` itself is backend-agnostic — it runs inside `GitWorktrees#add`, which is shared by every launcher, the same way `isolateToolSurface` unconditionally neutralizes BOTH `.mcp.json` (Claude) and `opencode.json` (opencode) regardless of which backend ultimately spawns into a given worktree, because the backend isn't chosen yet at `add()` time. What makes this "Claude Code members only" is that `.claude/skills/<name>/SKILL.md` is a path only the Claude Code launcher ever reads (`ClaudeCodeLauncher`/`HerdrPeerLauncher#agentDefinitionFile` for `.claude/agents`, and the skill-loading convention on top of it) — a seeded `.claude/skills/` in an opencode member's worktree is just an inert, unused directory. **What opencode would still need** (not implemented, per the brief): opencode's skill-equivalent lives under `.opencode/agent`, a different file shape (opencode agent-definition format, not `SKILL.md`), so this would need either a source-format adapter (fleetd translates each skill into an opencode agent file) or the source directory itself carrying both shapes side by side. Out of scope here. ## `FleetConfig.java` — exact lines touched (per the brief: another worker is editing this file on a different ticket right now) All edits are additive and localized to the `memberSkills`-specific spots, matching the existing pattern used for `worktreeGroup`/`memberLoginShell`: - Javadoc: one new `@param memberSkills` block, appended after the existing `@param memberLoginShell` block. - Record header: `String memberSkills` appended as the last component (after `memberLoginShell`). - One new back-compat constructor: "Back-compat form before the `memberSkills` key was added" (22→23-arg forwarding), placed immediately above the existing "before `memberLoginShell`" back-compat constructor — no other constructor in the chain was touched. - `KNOWN_TOP_LEVEL_KEYS`: appended `"memberSkills"`. - `withDefaults()`: one new comment + `memberSkills` passed through unchanged (left-as-is, no default — same treatment as `worktreeGroup`/`memberLoginShell`) in the final `return new FleetConfig(...)` call. No existing line was modified in place except the two call-sites above that had to grow one more trailing argument (the back-compat constructor's delegation, and `withDefaults()`'s own constructor call) — both are pure append-at-the-end edits, chosen specifically to minimize merge risk against the concurrent `FleetConfig.java` edit on the other ticket. ## Build Ran **unpiped** inside the worktree: ``` cd fleetd && mvn clean install ``` Real result (after the review fix, current HEAD of this branch): **`Tests run: 1386, Failures: 0, Errors: 0, Skipped: 0`**, **`BUILD SUCCESS`**. (The original submission's run, before the fix, was `Tests run: 1385`.) This run includes 7 new tests in `GitWorktreesTest` (58 total there now, up from 51) covering: fresh seed with no `.claude/` at all, invariant 1 (repo's own skill survives byte-for-byte even when the source carries a same-named skill with different content), invariant 3 (missing source directory never fails the spawn), invariant 2 in three directions (seeded worktree's `git status` is clean; a sibling worktree's own unrelated untracked `.claude/skills/` still shows up normally; and — added in the review fix — an operator's own global excludesFile pattern still applies after seeding, via the new `seedSkillsComposesWithAnAlreadyEffectiveGlobalExcludesFile`), and the seeded/kept log-line shape. It also includes updates to two existing config coverage tests (`ConfigRefTopLevelReportingCoverageTest`, `FleetConfigWithDefaultsPreservesEveryComponentTest`) required to keep the new `memberSkills` key's BASE/ALT value maps in sync with the record shape — those tests fail loudly (by design) if a new top-level `FleetConfig` component isn't given a real value and triaged. Along the way, adding `memberSkills` initially broke three pre-existing tests that check every `FleetConfig` component is triaged into a reload class and given a real value (`ConfigRefTopLevelCoverageTest`, `ConfigRefTopLevelReportingCoverageTest`, `FleetConfigWithDefaultsPreservesEveryComponentTest`) until `memberSkills` was added to `ConfigRef.DEFERRED_KEYS` with its own `changedDeferredKeys` branch and to those tests' value maps — all now green. I have no IDE MCP tools as a worker; the above `mvn clean install` output is the only check I ran or am claiming. ## Anything else in this repo with the same shape (not fixed, per the brief) - `plugin/skills/setup/SKILL.md` and the rest of `plugin/` — a member-facing onboarding asset that only a plugin *install* delivers, never a provisioned worktree; issue #362 items 1–2 (someone else's scope) cover it. - `.claude/agents/*.md` — already worktree-seeded via `agentDefinitionFile`, so not itself a gap, but any *new* agent role added only to this repo's `.claude/agents/` (not to whatever directory a future `memberSkills`-style config seeds) would be invisible in another repo's worktree the same way skills were before this PR. - `docs/Worker-Git-Workflow.md` and `docs/MCP-Contract.md` — referenced by the `implementer` skill and by this repo's own `CLAUDE.md`, but neither is seeded into a worktree of a *different* repo; a worker there following a seeded `implementer` skill that links to `../../docs/...` would find nothing. - `wiki/` — already known to be a submodule workers see as months-stale (per team memory); not this ticket's shape exactly, but the same underlying pattern ("an asset a member is expected to have that only exists because the member happens to be in this one repo"). ## Caveats for review - ~~The `core.excludesFile` worktree-scope caveat above~~ — fixed, see "Review fix" at the top of this PR body. - `seedSkills` does not restrict candidate skill names to a known allowlist (`implementer`, `reviewer`, `hunter`) — it copies every immediate, non-hidden subdirectory of the configured source. The lead reviewed this and said to leave it as-is; per that review I documented in `FleetConfig`'s javadoc and `fleetd.example.yaml` that the directory's contents are copied wholesale, so an operator knows not to park scratch files there. - I did not verify this against a live `fleetd.yaml` (gitignored, and I cannot see it as a worker) — all proof is from throwaway repos exercising `GitWorktrees#add` end-to-end, per the brief's own instruction to reproduce the shape rather than reason about the live config.
agent added 1 commit 2026-09-05 07:55:22 +02:00
fleetd #362 (item 3): seed .claude/skills/ into provisioned worktrees
CI / contract (pull_request) Successful in 1m20s
Details
CI / build (pull_request) Successful in 1m46s
Details
7c684e40d3 
Add memberSkills: <dir> to FleetConfig. GitWorktrees#add copies each
skill folder from that directory into <worktree>/.claude/skills/ so a
member spawned against ANY repo — not only one that already ships its
own skills — can load a bridge skill (e.g. implementer). A skill the
target repo already carries is never overwritten.

Every seeded path is hidden from `git status` in that worktree ONLY,
via a --worktree-scoped core.excludesFile pointing at a file under the
worktree's own private git dir (outside the working tree, so it can
never be committed) — not the shared .git/info/exclude, which a linked
worktree resolves to the repo's common git dir and would otherwise leak
visibility changes into the primary checkout and every sibling
worktree. Proven with a real `git status --porcelain` in
GitWorktreesTest, not by reasoning.

Seeding is best-effort like the existing overlayParity/isolateToolSurface
steps: a missing/unreadable source or a copy/exclude failure is logged
and skipped, never fails the spawn. memberSkills is triaged as a
DEFERRED config key in ConfigRef (baked once into GitWorktrees at
startup, like worktreeGroup), with its own changedDeferredKeys branch
and coverage-test entries.
agent added 1 commit 2026-09-05 08:10:36 +02:00
fleetd #362 review fix: compose skill-seeding excludes with the operator's own excludesFile
CI / contract (pull_request) Successful in 44s
Details
CI / build (pull_request) Successful in 1m54s
Details
f84824ee29 
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.
agent added 2 commits 2026-09-05 08:23:48 +02:00
Merge origin/main into worker/362-worktree-skills-c03e51-3 (picks up #363) 105c065615
fleetd #362 review fix 2: route the XDG excludesFile fallback through gitEnv too
CI / contract (pull_request) Successful in 1m16s
Details
CI / build (pull_request) Successful in 1m24s
Details
9e813ec179 
Finding 1 (lead): the XDG fallback branch of previouslyEffectiveExcludesFileContent was
unpinned — deleting it left the suite green (Tests run: 1386, Failures: 0). Added
seedSkillsComposesWithTheXdgDefaultExcludesFileWhenNoneIsConfigured to pin it: isolates
XDG_CONFIG_HOME via the gitEnv seam at a temp dir carrying a synthetic git/ignore, points
GIT_CONFIG_GLOBAL at an empty file so core.excludesFile is genuinely unset (forcing the
fallback branch), seeds a skill, and asserts a file matching the XDG-default pattern still
reads as clean. Reverting the fix (mutating the fallback to resolve to "") turns this test
red with a real pasted failure (see PR body): "expected: <> but was: <?? xdg-fallback-marker>".

Finding 2 (lead, the one that actually needed a code fix): the fallback read XDG_CONFIG_HOME
and HOME straight from the JVM's own environment, not through the gitEnv seam every git
subprocess in this class already honours — so no test could isolate it, and on a machine
carrying a real ~/.config/git/ignore (this dev machine does), every seeding test silently
composed with that real file. Added resolveEnv/resolveHome, which check gitEnv first and
fall back to the JVM's real environment only when the seam doesn't supply a value (production
behaviour, where gitEnv is always Map.of(), is unchanged). Added a hermeticGitEnv() test
helper and routed every seeding test in GitWorktreesTest through it, so no test in the class
can reach the real machine's home directory for this fallback.

Also documents two non-defects the lead asked for one javadoc line each on: the composed
excludesFile is a snapshot taken at seed time, not a live reference to the operator's file;
and excludeSeededSkillsFromGitStatus assumes a fresh worktree (not idempotent, but the
double-seed path does not exist today, so no guard was added for it).
ltms commented 2026-09-05 08:33:33 +02:00
Owner
Copy Link
Copy Source

Merged to main as 92c0f16.

Verified on the merge itself:

mvn clean install  ->  Tests run: 1402, Failures: 0, Errors: 0, Skipped: 0
                   ->  BUILD SUCCESS

Two mutations run on the merge, both red:

drop the XDG fallback           ->  1 failure   BUILD FAILURE
remove the composition itself   ->  2 failures  BUILD FAILURE

Both directions of the compose behaviour are pinned now. Before round 2 the first of those was green.

One hypothesis of mine that measuring disproved, recorded so nobody re-raises it. I suspected the two compose tests could pass for the wrong reason: their assertion helper fullStatus builds its own ProcessBuilder and inherits the JVM environment, so it runs git status against the operator's real global config — which on this machine has a core.excludesFile containing target. That looked like it would hide a broken composition.

It does not. The worktree-scoped core.excludesFile the fix writes shadows the operator's global one in that process too, so with composition removed target really does show up. That is exactly what the mutation above measured: 2 failures, not 0. The tests pin what they claim to pin.

One real note, pre-existing, not from this PR. The test class is sensitive to the ambient environment. Running it with XDG_CONFIG_HOME pointed at a directory whose git/ignore is * turns 56 of 59 tests red. The cause is the helpers, not the fix: status() and fullStatus() set no git-isolation variables at all, and gitOutput() sets GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM but not XDG_CONFIG_HOME. Both helpers are on main in that shape and predate this work. Filed separately rather than held against this PR.

The hermeticGitEnv helper this PR adds is the right pattern to extend to them.

Merged to `main` as `92c0f16`. Verified on the merge itself: ``` mvn clean install -> Tests run: 1402, Failures: 0, Errors: 0, Skipped: 0 -> BUILD SUCCESS ``` Two mutations run on the merge, both red: ``` drop the XDG fallback -> 1 failure BUILD FAILURE remove the composition itself -> 2 failures BUILD FAILURE ``` Both directions of the compose behaviour are pinned now. Before round 2 the first of those was green. **One hypothesis of mine that measuring disproved, recorded so nobody re-raises it.** I suspected the two compose tests could pass for the wrong reason: their assertion helper `fullStatus` builds its own `ProcessBuilder` and inherits the JVM environment, so it runs `git status` against the operator's **real** global config — which on this machine has a `core.excludesFile` containing `target`. That looked like it would hide a broken composition. It does not. The worktree-scoped `core.excludesFile` the fix writes **shadows** the operator's global one in that process too, so with composition removed `target` really does show up. That is exactly what the mutation above measured: 2 failures, not 0. The tests pin what they claim to pin. **One real note, pre-existing, not from this PR.** The test class is sensitive to the ambient environment. Running it with `XDG_CONFIG_HOME` pointed at a directory whose `git/ignore` is `*` turns 56 of 59 tests red. The cause is the helpers, not the fix: `status()` and `fullStatus()` set no git-isolation variables at all, and `gitOutput()` sets `GIT_CONFIG_GLOBAL`/`GIT_CONFIG_SYSTEM` but not `XDG_CONFIG_HOME`. Both helpers are on `main` in that shape and predate this work. Filed separately rather than held against this PR. The `hermeticGitEnv` helper this PR adds is the right pattern to extend to them.
ltms closed this pull request 2026-09-05 08:33:38 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m16s
Details
CI / build (pull_request) Successful in 1m24s
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#366
Delete Branch "worker/362-worktree-skills-c03e51-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?