diff --git a/fleetd/fleetd.example.yaml b/fleetd/fleetd.example.yaml index 5788a06..5765c04 100644 --- a/fleetd/fleetd.example.yaml +++ b/fleetd/fleetd.example.yaml @@ -774,6 +774,19 @@ guard: # so fleetd falls back to the weaker CB-596 sentinel overlay instead (a WARN names the gap). # worktreeGroup: fleet-workers +# fleetd #362: a directory of skill folders (each a subdirectory holding a SKILL.md, the same +# shape as this repo's own .claude/skills/) copied into every PROVISIONED worktree's +# .claude/skills/, so a member spawned against ANY repo — not only one that already ships its own +# copy — can load a bridge skill (e.g. implementer). Unset (the default): no worktree is touched +# beyond today's behaviour. A skill folder the target repo already carries under +# .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. 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 # the feature disabled. By default the daemon never reaps, caps, or drains sessions. # idleTtlSeconds → reap READY/DONE sessions idle longer than this (never BUSY/SPAWNING) @@ -821,10 +834,17 @@ guard: # across every daemon sharing this vhost. # prefetch → consumer basicQos, capping how many unacked messages the mailbox holds in-heap. # Default 32 when omitted. +# peers → fleetd #361: the coord-ids of the OTHER daemons on this vhost, declared by the +# operator (the daemon never guesses). fleet_list reports each one's live reachability +# (a passive queue check, never a presence protocol) alongside this daemon's own +# mailbox state. Omit, or leave empty, for a daemon with no known peers yet — an +# undeclared peer can still reach you and be reached by fleet_send, it just will not +# show up as a row in fleet_list. # coordinator: # uriEnv: LEAD_COORD_URI # selfId: mac-opus # prefetch: 32 +# peers: [fleet01-lead] # Active push-to-primary (CB-307 Stage 3). When a worker reply lands with no open fleet_send, # the ReplyPushLoop injects a *drain nudge* (never the payload) into the primary's own herdr diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 6058535..8550526 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -248,7 +248,8 @@ public final class Fleetd { contextCap = cfg.lifecycle().contextCap(); } boolean clearAfterTurn = cfg.lifecycle() != null && cfg.lifecycle().clearAfterTurn(); - SessionManager sessions = new SessionManager(workers, new GitWorktrees(cfg.worktreeRoot(), cfg.worktreeGroup()), + SessionManager sessions = new SessionManager(workers, + new GitWorktrees(cfg.worktreeRoot(), cfg.worktreeGroup(), cfg.memberSkills()), System::nanoTime, contextCap, clearAfterTurn); liveCountRef.set(profileName -> liveSessionCount(sessions.roster(), profileName)); @@ -652,7 +653,12 @@ public final class Fleetd { quarantineSource, leadMailbox, outageSource, - new FleetMcp.LeadSeatSource(leadSeatLookup(() -> config.get().profiles(), leaders, leads))); + new FleetMcp.LeadSeatSource(leadSeatLookup(() -> config.get().profiles(), leaders, leads)), + // fleetd #361: the operator-declared peers this daemon's fleet_list should try to + // reach. Read from the SAME snapshot leadMailbox itself opened from (cfg.coordinator()), + // not the live config.get() — coordinator wiring is already boot-time-fixed (see + // leadMailbox above), so peers follows the same rule rather than half hot-reloading. + cfg.coordinator() == null ? List.of() : cfg.coordinator().peers()); // CB-637: the receive half. Only constructed when a lead mailbox actually opened — with no // coordinator (or an unreachable one) there is nothing to deliver, so no scheduler is diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java index 1c37bd1..7c11ccf 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java @@ -39,10 +39,11 @@ import java.util.function.Supplier; * keeps the old value until a restart: {@code lifecycle:}, {@code leadHeartbeat:}, * {@code spawnReadyTimeoutMs} / {@code spawnReadyPollMs}, {@code quarantineCooldownSeconds} * (CB-578 stage B — baked once into the {@code BackendQuarantine} built at startup), - * {@code guard:}, {@code worktreeRoot:} and {@code worktreeGroup:} (both baked once into the - * {@code GitWorktrees} built at {@code Fleetd.java:251} and never rebuilt — fleetd #323 - * instance 2 found {@code worktreeGroup} missing from this list and from - * {@link #changedDeferredKeys}), {@code primary:} (fleetd #326 — {@code Fleetd.java:506, 519, + * {@code guard:}, {@code worktreeRoot:}, {@code worktreeGroup:} and {@code memberSkills:} + * (all three of the latter baked once into the {@code GitWorktrees} built at + * {@code Fleetd.java:251} and never rebuilt — fleetd #323 instance 2 found + * {@code worktreeGroup} missing from this list and from {@link #changedDeferredKeys}; + * {@code memberSkills} (fleetd #362) followed the same shape), {@code primary:} (fleetd #326 — {@code Fleetd.java:506, 519, * 520} read {@code cfg.primary()} only off the startup snapshot to build {@code * PrimaryRegistry} and size {@code ReplyPushLoop}'s reminder cap/backoff, and neither is * rebuilt on reload. Say the consequence exactly: {@code primary.terminal} is DEPRECATED @@ -129,9 +130,10 @@ import java.util.function.Supplier; * five of COLD_KEYS" rather than re-listing them, so prose and set cannot drift again. * * - *

The denominator, measured on 2026-09-04 (fleetd #330; recounted for fleetd #333). - * {@code FleetConfig} has 22 top-level record components: 5 cold, 11 deferred, 3 split, 3 - * hot-excluded. Three of them are named nowhere in this file, and the reason is the same for all + *

The denominator, measured on 2026-09-04 (fleetd #330; recounted for fleetd #333); + * recounted again for fleetd #362. {@code FleetConfig} has 23 top-level record components: + * 5 cold, 12 deferred, 3 split, 3 hot-excluded. Three of them are named nowhere in this file, and + * the reason is the same for all * three: {@code placement}, {@code memberCredentials} and {@code memberLoginShell} are * hot and correctly absent — all three are read live off {@code config.get()} * (placement through the {@code CompositePeerLauncher} supplier the Hot bullet names; @@ -211,7 +213,7 @@ public final class ConfigRef implements Supplier { * read {@link #COLD_KEYS} and {@link #SPLIT_KEYS}. */ static final Set DEFERRED_KEYS = Set.of( - "guard", "worktreeRoot", "worktreeGroup", "primary", "configReload", + "guard", "worktreeRoot", "worktreeGroup", "memberSkills", "primary", "configReload", "leadHeartbeat", "lifecycle", "spawnReadyTimeoutMs", "spawnReadyPollMs", "quarantineCooldownSeconds", "profiles"); @@ -402,6 +404,13 @@ public final class ConfigRef implements Supplier { if (!Objects.equals(old.worktreeGroup(), fresh.worktreeGroup())) { changed.add("worktreeGroup"); } + // fleetd #362: baked into the same GitWorktrees as worktreeRoot/worktreeGroup + // (Fleetd.java:251) and never rebuilt either — a reload that changes only memberSkills + // must be reported the same way, or a newly provisioned worktree keeps seeding from (or + // skipping) the old source directory with nothing telling the operator why. + if (!Objects.equals(old.memberSkills(), fresh.memberSkills())) { + changed.add("memberSkills"); + } // fleetd #326: Fleetd.java:506, 519, 520 read cfg.primary() only off the startup snapshot // (PrimaryRegistry's pinned terminal, ReplyPushLoop's reminder cap and backoff) — neither is // rebuilt on reload, so a changed value needs a restart. Note what it does NOT mean: 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 ef96eb1..5faca33 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java @@ -106,6 +106,19 @@ import java.util.regex.PatternSyntaxException; * When {@code memberHerdrSocket} is NOT configured this field is never * consulted at all; fleetd keeps reading its own {@code $SHELL}, exactly as * before this field existed. + * @param memberSkills fleetd #362: nullable directory of skill folders (each a subdirectory + * holding a {@code SKILL.md}, the same shape as this repo's own {@code + * .claude/skills/}) copied into every provisioned worktree's {@code + * .claude/skills/}, so a member spawned against ANY repo — not only one that + * already ships its own copy — can load a bridge skill such as {@code + * implementer}. {@code null}/blank ⇒ off: no worktree is touched beyond + * 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. 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( @@ -130,7 +143,22 @@ public record FleetConfig( MemberCredentials memberCredentials, Coordinator coordinator, String worktreeGroup, - String memberLoginShell) { + String memberLoginShell, + String memberSkills) { + + /** Back-compat form before the {@code memberSkills} key was added. */ + public FleetConfig(Bind bind, String herdrSocket, String memberHerdrSocket, Map profiles, + Guard guard, String worktreeRoot, Lifecycle lifecycle, Integer spawnReadyTimeoutMs, + Integer spawnReadyPollMs, Broker broker, Primary primary, Fleet fleet, + LeadHeartbeat leadHeartbeat, Health health, String placement, Auth auth, + ConfigReload configReload, Integer quarantineCooldownSeconds, + MemberCredentials memberCredentials, Coordinator coordinator, String worktreeGroup, + String memberLoginShell) { + this(bind, herdrSocket, memberHerdrSocket, profiles, guard, worktreeRoot, lifecycle, spawnReadyTimeoutMs, + spawnReadyPollMs, broker, primary, fleet, leadHeartbeat, health, placement, auth, + configReload, quarantineCooldownSeconds, memberCredentials, coordinator, worktreeGroup, + memberLoginShell, null); + } /** Back-compat form before the {@code memberLoginShell} key was added. */ public FleetConfig(Bind bind, String herdrSocket, String memberHerdrSocket, Map profiles, @@ -888,12 +916,18 @@ public record FleetConfig( * {@code null} ⇒ kept as {@code null} (no self id configured). * @param prefetch the consumer's {@code basicQos} prefetch count. {@code null}/non-positive ⇒ * {@link LeadMailbox#DEFAULT_PREFETCH}. + * @param peers fleetd #361: the coord-ids the operator declares as this daemon's peers — the + * daemon never guesses who else exists. {@code fleet_list} reports each one's + * live reachability. Blank entries are dropped; {@code null} ⇒ an empty list, so + * a config written before this field existed still parses unchanged. */ @JsonIgnoreProperties(ignoreUnknown = true) - public record Coordinator(String uri, String uriEnv, String selfId, Integer prefetch) { + public record Coordinator(String uri, String uriEnv, String selfId, Integer prefetch, List peers) { public Coordinator { selfId = (selfId == null || selfId.isBlank()) ? null : selfId; + peers = peers == null ? List.of() + : peers.stream().filter(p -> p != null && !p.isBlank()).toList(); } /** True when a {@code uriEnv} is configured by name, whether or not its variable resolves. */ @@ -1495,7 +1529,7 @@ public record FleetConfig( "bind", "herdrSocket", "memberHerdrSocket", "profiles", "guard", "worktreeRoot", "lifecycle", "spawnReadyTimeoutMs", "spawnReadyPollMs", "broker", "primary", "fleet", "leadHeartbeat", "health", "placement", "auth", "configReload", "quarantineCooldownSeconds", - "memberCredentials", "coordinator", "worktreeGroup", "memberLoginShell"); + "memberCredentials", "coordinator", "worktreeGroup", "memberLoginShell", "memberSkills"); /** Load and validate config from {@code path}. */ public static FleetConfig load(Path path) { @@ -2172,9 +2206,12 @@ public record FleetConfig( // memberLoginShell is left as-is (fleetd #213), like worktreeGroup: null/blank is "not // configured", and there is no sane non-null default — a member's login shell is // operator-specific and only meaningful when memberHerdrSocket is also set. + // memberSkills is left as-is (fleetd #362), like worktreeGroup/memberLoginShell: null/blank + // is "off", and there is no sane non-null default — the daemon may not even run from a + // checkout that ships its own .claude/skills/. return new FleetConfig(b, herdrSocket, memberHerdrSocket, profiles, g, worktreeRoot, l, timeout, pollMs, broker, primary, f, leadHeartbeat, health, placementOrDefault, a, configReload, - quarantineCooldown, mc, coordinator, worktreeGroup, memberLoginShell); + quarantineCooldown, mc, coordinator, worktreeGroup, memberLoginShell, memberSkills); } /** diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java index 9dda812..7b35930 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -44,6 +44,7 @@ import java.util.Objects; import java.util.Set; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; import java.util.function.BiFunction; import java.util.function.Function; import java.util.function.LongSupplier; @@ -101,6 +102,8 @@ public final class FleetMcp { private final LeadSeatSource leadSeats; /** CB-637: this daemon's lead-to-lead channel; {@code null} when no coordinator is configured. */ private final LeadChannel leadChannel; + /** fleetd #361: {@code coordinator.peers} — see {@link CoordinationSource}. Empty when unset. */ + private final List peers; /** Capacity facts used by {@code fleet_list}; production must supply the placement live count. */ public record CapacitySource(Function liveCount, Function maxLoad, @@ -164,6 +167,32 @@ public final class FleetMcp { public static LeadSeatSource none() { return new LeadSeatSource(_ -> 0); } } + /** + * fleetd #361: peer-visibility facts for {@code fleet_list}'s {@code coordinator} row — this + * daemon's own {@link LeadChannel} (for its self mailbox state and held messages) plus the + * coord-ids the operator has declared as peers ({@code coordinator.peers}). Bundled as its own + * Source, the same idiom as {@link OutageSource}/{@link QuarantineSource}/{@link LeadSeatSource}, + * so {@code listFleet}'s already-long overload chain gains exactly one new required parameter + * instead of a further bare positional argument. + * + *

Every mailbox look this triggers goes through {@link LeadChannel#inspect}, which is + * specified to run on its own disposable channel — never the channel {@link LeadChannel#publish} + * or the consume loop depends on — so a peer that happens to be down, or the coordination broker + * itself being unreachable, can never take {@code fleet_send}/{@code LeadCoordLoop}'s own path + * down with it. See {@code FleetMcp.probe} for the additional timeout bound on top of that. + * + * @param leadChannel this daemon's own channel, or {@code null} when no coordinator is configured + * @param peers the coord-ids declared under {@code coordinator.peers}, or empty + */ + public record CoordinationSource(LeadChannel leadChannel, List peers) { + public CoordinationSource { + peers = peers == null ? List.of() : List.copyOf(peers); + } + + /** Inert source — no coordinator row is ever reported. */ + public static CoordinationSource none() { return new CoordinationSource(null, List.of()); } + } + /** * @param callers resolves each call's {@link Principal}; {@code null} disables authorization. * This surface needs its own enforcement: {@code /mcp} is a raw servlet on @@ -211,19 +240,34 @@ public final class FleetMcp { } /** - * As above, with fleetd #176 lead-seat facts (see {@link LeadSeatSource}). This is what - * {@code Fleetd.main} actually wires up. - * - * @param leadSeats required — pass {@link LeadSeatSource#none()} for a caller that does not want - * the feature, never a defaulting overload (the same rule {@code quarantine} and - * {@code outage} follow). + * As above, with fleetd #176 lead-seat facts (see {@link LeadSeatSource}). */ public FleetMcp(MessageService messages, PeerLauncher workers, SessionManager sessions, ConnectionIdentity identity, MemberPresence presence, PrimaryRegistry primaryRegistry, CallerResolver callers, Metrics metrics, CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine, LeadChannel leadChannel, OutageSource outage, LeadSeatSource leadSeats) { + this(messages, workers, sessions, identity, presence, primaryRegistry, callers, metrics, capacity, + healthCoverage, quarantine, leadChannel, outage, leadSeats, List.of()); + } + + /** + * As above, with fleetd #361 {@code coordinator.peers} (see {@link CoordinationSource}). This is + * what {@code Fleetd.main} actually wires up. + * + * @param leadSeats required — pass {@link LeadSeatSource#none()} for a caller that does not want + * the feature, never a defaulting overload (the same rule {@code quarantine} and + * {@code outage} follow). + * @param peers the coord-ids declared under {@code coordinator.peers}; empty when unset or + * when {@code leadChannel} is {@code null}. + */ + public FleetMcp(MessageService messages, PeerLauncher workers, SessionManager sessions, + ConnectionIdentity identity, MemberPresence presence, PrimaryRegistry primaryRegistry, + CallerResolver callers, Metrics metrics, CapacitySource capacity, HealthCoverageSource healthCoverage, + QuarantineSource quarantine, LeadChannel leadChannel, OutageSource outage, + LeadSeatSource leadSeats, List peers) { this.leadChannel = leadChannel; + this.peers = peers == null ? List.of() : List.copyOf(peers); this.capacity = capacity; this.quarantine = Objects.requireNonNull(quarantine, "quarantine"); this.outage = Objects.requireNonNull(outage, "outage"); @@ -361,7 +405,7 @@ public final class FleetMcp { return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, outage, leadSeats, callers == null ? Map.of() : callers.leads(), callerTerminal(exchange), - leadChannel == null ? null : leadChannel.selfCoordId()); + new CoordinationSource(leadChannel, peers)); }; BiFunction stopHandler = (exchange, req) -> { @@ -697,6 +741,15 @@ public final class FleetMcp { * turned into a tool error naming the coord-id. It is never allowed to escape as a crash: an * unreachable peer is an ordinary outcome of addressing a fleet you do not control. * + *

fleetd #361: the success text is honest about what "durably confirmed" does and + * does not mean. The broker's publisher confirm proves the message is durably queued — + * it says nothing about whether the peer's pane has, or ever will, receive it. After a + * successful publish this looks at the target mailbox's consumer count (via + * {@link LeadChannel#inspect}, bounded and never allowed to fail the call — see {@link #probe}) + * and appends a warning when it is zero: that is the observable form of "nobody is reading this + * right now". A zero-consumer publish is still reported as a SUCCESS, never an error — the + * message is safely queued and will be read once a daemon owning that coord-id connects. + * * @param leadChannel this daemon's channel, or {@code null} when no coordinator is configured */ static McpSchema.CallToolResult sendToLead(LeadChannel leadChannel, String coordId, String content, @@ -724,7 +777,16 @@ public final class FleetMcp { + ". Check that a daemon is running with coordinator.selfId=\"" + coordId + "\" and is connected to the same coordination broker."); } - return text("delivered to peer lead " + coordId + " (msgId " + msg.msgId() + ")"); + String result = "published to peer lead \"" + coordId + "\"'s mailbox and durably confirmed " + + "by the broker (msgId " + msg.msgId() + ")."; + LeadChannel.MailboxState state = probe(leadChannel, coordId); + if (state.exists() && state.consumers() == 0) { + result += " Warning: that mailbox currently has NO consumers attached — nobody is reading " + + "it right now. The message is safely queued and will be delivered once a daemon " + + "with coordinator.selfId=\"" + coordId + "\" is running and connected; until then " + + "it will not reach that lead's pane."; + } + return text(result); } /** @@ -1110,7 +1172,8 @@ public final class FleetMcp { static McpSchema.CallToolResult listFleet(PeerLauncher workers, SessionManager sessions, MessageService messages, CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine, Map leads, String selfTerm) { - return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, leads, selfTerm, null); + return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, leads, selfTerm, + CoordinationSource.none()); } /** As above, plus fleetd #201 Unit 5 cool-off facts (see {@link OutageSource}). */ @@ -1119,33 +1182,33 @@ public final class FleetMcp { QuarantineSource quarantine, OutageSource outage, Map leads, String selfTerm) { return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, outage, - LeadSeatSource.none(), leads, selfTerm, null); + LeadSeatSource.none(), leads, selfTerm, CoordinationSource.none()); } /** - * As above, additionally reporting this daemon's own lead coordination id (CB-637) when one is - * configured and its channel opened. There is no peer-discovery surface yet — a lead addresses a - * peer by a coord-id it was told — so this row exists to answer the one question the operator - * cannot answer any other way: what is MY coord-id, the one a peer must use to reach me. It is - * omitted entirely when no coordinator is configured, so an ordinary fleet's output is unchanged. + * As above, additionally reporting this daemon's own lead coordination state (CB-637, fleetd + * #361) when a coordinator is configured and its channel opened — see {@link #coordinatorView} + * for the shape. Omitted entirely when no coordinator is configured, so an ordinary fleet's + * output is unchanged. * - * @param selfCoordId this daemon's coord-id, or {@code null} when lead coordination is off + * @param coordination this daemon's lead channel plus its declared peers, or + * {@link CoordinationSource#none()} when lead coordination is off */ static McpSchema.CallToolResult listFleet(PeerLauncher workers, SessionManager sessions, MessageService messages, CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine, Map leads, String selfTerm, - String selfCoordId) { + CoordinationSource coordination) { return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, OutageSource.none(), - LeadSeatSource.none(), leads, selfTerm, selfCoordId); + LeadSeatSource.none(), leads, selfTerm, coordination); } /** As above, plus fleetd #201 Unit 5 cool-off facts (see {@link OutageSource}). */ static McpSchema.CallToolResult listFleet(PeerLauncher workers, SessionManager sessions, MessageService messages, CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine, OutageSource outage, - Map leads, String selfTerm, String selfCoordId) { + Map leads, String selfTerm, CoordinationSource coordination) { return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, outage, - LeadSeatSource.none(), leads, selfTerm, selfCoordId); + LeadSeatSource.none(), leads, selfTerm, coordination); } /** As above, plus fleetd #176 lead-seat facts (see {@link LeadSeatSource}). */ @@ -1153,7 +1216,7 @@ public final class FleetMcp { CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine, OutageSource outage, LeadSeatSource leadSeats, Map leads, String selfTerm, - String selfCoordId) { + CoordinationSource coordination) { try { Map live = workers.list().stream() .map(Agent.class::cast) @@ -1175,8 +1238,9 @@ public final class FleetMcp { Map result = new LinkedHashMap<>(); result.put("leads", leadRows); result.put("members", out); result.put("healthCoverage", healthCoverage.value().get()); - if (selfCoordId != null && !selfCoordId.isBlank()) { - result.put("coordinator", Map.of("selfId", selfCoordId, "configured", true)); + Map coordinatorRow = coordinatorView(coordination); + if (coordinatorRow != null) { + result.put("coordinator", coordinatorRow); } if (capacity.available()) result.put("capacity", profiles.stream() .map(profile -> capacityView(profile, capacity.liveCount(), capacity.maxLoad(), roster, messages, @@ -1187,6 +1251,140 @@ public final class FleetMcp { } } + /** + * fleetd #361: the {@code coordinator} row — this daemon's own coord-id and mailbox state, the + * messages currently held for it, and the live reachability of every operator-declared peer. + * {@code null} (the row is then omitted entirely) when lead coordination is off, so an ordinary + * fleet's {@code fleet_list} output is byte-identical to before this feature existed. + * + *

Every peer/self mailbox look goes through {@link #probe}, which bounds each + * {@link LeadChannel#inspect} call to {@link #PEER_PROBE_TIMEOUT_MS} and never lets it throw — + * a coordination broker that is down or slow degrades this row toward "unreachable"/"unknown" + * counts, it can never make {@code fleet_list} itself slow or fail. {@code held} comes from + * {@link LeadChannel#peek}, a pure in-memory read with no broker round trip, so it is never + * subject to that bound. + */ + private static Map coordinatorView(CoordinationSource coordination) { + LeadChannel channel = coordination.leadChannel(); + if (channel == null) { + return null; + } + String selfId = channel.selfCoordId(); + Map row = new LinkedHashMap<>(); + row.put("selfId", selfId); + row.put("configured", true); + row.put("mailbox", mailboxView(probe(channel, selfId))); + row.put("held", channel.peek().stream().map(FleetMcp::heldView).toList()); + row.put("peers", coordination.peers().stream().map(p -> peerView(channel, p)).toList()); + return row; + } + + /** + * fleetd #361: render a {@link LeadChannel.MailboxState} without ever presenting an unmeasured + * fact as a measured one. {@code status} is the tri-state itself — {@code "exists"}, + * {@code "absent"} (the broker positively confirmed no such queue), or {@code "unknown"} (the + * probe could not determine either way: down, unreachable, or timed out). {@code pending}/ + * {@code consumers} are included ONLY when {@code status == "exists"} — a reader must never see + * them default to {@code 0} for a mailbox this call never actually measured. This is the fix for + * the review finding that a collapsed {@code absent()} rendered a self-probe timeout as + * "pending: 0, consumers: 0", indistinguishable from an actually-empty, actually-unread mailbox. + */ + private static Map mailboxView(LeadChannel.MailboxState state) { + Map row = new LinkedHashMap<>(); + row.put("status", state.exists() ? "exists" : state.known() ? "absent" : "unknown"); + if (state.exists()) { + row.put("pending", state.pending()); + row.put("consumers", state.consumers()); + } + return row; + } + + /** One held-for-me message: enough to identify it and see roughly what it says, never the whole body. */ + private static Map heldView(LeadMessage m) { + Map row = new LinkedHashMap<>(); + row.put("msgId", m.msgId()); + row.put("from", m.from()); + row.put("preview", preview(m.content())); + return row; + } + + /** Cap a held message's content to a short preview — {@code fleet_list} must never dump a full body. */ + private static final int HELD_PREVIEW_MAX_CHARS = 80; + + private static String preview(String content) { + if (content == null) { + return ""; + } + return content.length() <= HELD_PREVIEW_MAX_CHARS + ? content + : content.substring(0, HELD_PREVIEW_MAX_CHARS) + "…"; + } + + /** + * One declared peer's row: its coord-id, then the same tri-state {@link #mailboxView} shape. + * Deliberately no boolean "reachable" field — that collapsed "confirmed gone" and "could not + * check" into the same {@code false}, which is exactly the review finding this row now avoids: + * an operator reading {@code status} can tell "fleet01 is down" (a {@code coordinator.selfId} + * nobody has ever run) apart from "my own broker is slow or unreachable right now". + */ + private static Map peerView(LeadChannel channel, String coordId) { + Map row = new LinkedHashMap<>(); + row.put("coordId", coordId); + row.putAll(mailboxView(probe(channel, coordId))); + return row; + } + + /** + * fleetd #361: how long {@code fleet_list} waits on any single {@link LeadChannel#inspect} call + * before giving up on it — see {@link #probe}. + */ + private static final long PEER_PROBE_TIMEOUT_MS = 1_500L; + + /** + * Dedicated pool for {@link LeadChannel#inspect} calls so a slow one blocks only its own virtual + * thread, never the MCP request thread calling {@code fleet_list}. Not a bounded pool — + * {@code newThreadPerTaskExecutor} starts a fresh virtual thread per call with no cap on how many + * run at once; virtual threads make that cheap, not bounded. What actually keeps a hung probe + * from accumulating forever is the {@link Future#cancel} in {@link #probe}, not a pool limit. + */ + private static final java.util.concurrent.ExecutorService PEER_PROBE_POOL = + java.util.concurrent.Executors.newThreadPerTaskExecutor(Thread.ofVirtual().name("fleet-peer-probe-", 0).factory()); + + /** + * fleetd #361: {@link LeadChannel#inspect}, bounded to {@link #PEER_PROBE_TIMEOUT_MS} and never + * allowed to throw or hang the caller — a coordination broker that is unreachable or slow + * degrades to {@link LeadChannel.MailboxState#unknown} (never {@code absent}: a timeout proves + * nothing about whether the mailbox exists) rather than making {@code fleet_list} slow or + * failing it. {@code inspect} itself is already specified to never throw, but this is the seam + * that also survives an implementation that does, or one that blocks indefinitely on a dead + * connection. + * + *

A timeout cancels the orphaned task rather than abandoning it. Before this, + * {@code get(timeout)} on a hung {@code inspect} left the submitted task running forever on its + * own virtual thread, holding the AMQP channel it had already opened — against a broker that + * hangs rather than fails fast, every {@code fleet_list} call would orphan one more channel until + * the connection's channel-max (2047 by default) was exhausted, which would break {@link + * LeadChannel#publish} too. {@link Future#cancel(boolean) cancel(true)} interrupts the orphaned + * task's thread; {@link LeadMailbox#inspect} has no interruptible wait of its own to catch that, + * but the underlying AMQP RPC continuation does block on one, so the interrupt reaches it and the + * task's {@code finally} still closes the probe channel it opened rather than leaking it forever. + */ + private static LeadChannel.MailboxState probe(LeadChannel channel, String coordId) { + return probe(channel, coordId, PEER_PROBE_TIMEOUT_MS); + } + + /** As {@link #probe(LeadChannel, String)}, with an explicit timeout — a seam for tests. */ + static LeadChannel.MailboxState probe(LeadChannel channel, String coordId, long timeoutMs) { + java.util.concurrent.Future future = + PEER_PROBE_POOL.submit(() -> channel.inspect(coordId)); + try { + return future.get(timeoutMs, TimeUnit.MILLISECONDS); + } catch (Exception e) { + future.cancel(true); // best-effort: don't leave a hung probe (and its channel) running forever + return LeadChannel.MailboxState.unknown(coordId); + } + } + /** * Capacity is advisory only. {@code reclaimable} says there is no bridge work, not that fleetd * may stop the member: the bridge has capacity facts but no work list, and choosing work needs @@ -1377,8 +1575,12 @@ public final class FleetMcp { + "routes your answer back into the same turn (omit for a normal delegation)"), "coordId", stringProp("A peer LEAD's coordination id — delivers content to that " + "lead's durable mailbox on the shared coordination broker, which works " - + "across hosts. Mutually exclusive with sessionId and turnId. Your own " - + "coordId is reported by fleet_list.")), + + "across hosts. Mutually exclusive with sessionId and turnId. Success means " + + "the message is durably queued and confirmed by the broker, with a warning " + + "if that mailbox has no consumers attached right now (queued, but nobody is " + + "reading it yet) — it does not mean the peer's pane has seen it. Your own " + + "coordId, this daemon's mailbox state, and every coordinator.peers entry's " + + "live reachability are reported by fleet_list's coordinator row.")), List.of("content"))); } @@ -1486,7 +1688,13 @@ public final class FleetMcp { + "for a quarantined profile's credential (see fleet_profiles), whatever its " + "maxLoad/live — with credentialId and quarantinedForSeconds naming the " + "quarantine, so 'free: 0, busy' can be told apart from 'free: 0, refusing " - + "for N seconds'.", + + "for N seconds'. When lead-to-lead coordination is configured, a 'coordinator' " + + "object reports this daemon's own coord-id ('selfId') and mailbox state " + + "('mailbox': pending/consumers), the messages currently held for it ('held': " + + "msgId/from/preview, never the full body), and one row per coordinator.peers " + + "coord-id ('peers': coordId/reachable, plus pending/consumers when reachable) — " + + "this is peer DISCOVERY for cross-host leads, distinct from the local 'leads' " + + "array above. It is omitted entirely when no coordinator is configured.", objectSchema(Map.of(), List.of())); } diff --git a/fleetd/src/main/java/dev/ltms/fleet/msg/LeadChannel.java b/fleetd/src/main/java/dev/ltms/fleet/msg/LeadChannel.java index 07664a9..db16aa3 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/msg/LeadChannel.java +++ b/fleetd/src/main/java/dev/ltms/fleet/msg/LeadChannel.java @@ -4,8 +4,8 @@ import java.util.List; /** * The lead-to-lead message channel this daemon speaks, as its callers need it — one lead's own - * mailbox: publish to a peer's coord-id, look at what has arrived for me, and ack what I have - * delivered. + * mailbox: publish to a peer's coord-id, look at what has arrived for me, ack what I have + * delivered, and (fleetd #361) inspect any coord-id's mailbox from the outside without owning it. * *

Extracted from {@link LeadMailbox} purely as a seam. {@code LeadMailbox} is the one production * implementation and owns a live AMQP connection, so a test that wanted to exercise the routing in @@ -37,4 +37,78 @@ public interface LeadChannel { /** This daemon's own lead coordination id — the mailbox it owns, and the {@code from} it sends as. */ String selfCoordId(); + + /** + * A non-destructive look at {@code coordId}'s mailbox — does it exist, how many messages are + * waiting on it, and how many consumers are attached — without owning, consuming, or otherwise + * changing it. {@code consumers == 0} on an existing mailbox is the observable form of "nobody + * is reading this right now": a publish to it will sit queued rather than reach a pane. + * + *

Never throws — this is a best-effort fact-finding call, not an operation a + * caller must handle failing. But it must never turn "I could not check" into a false negative: + * {@link MailboxState#absent(String)} means the broker positively confirmed there is no such + * queue, and {@link MailboxState#unknown(String)} — a distinct value — means the look could not + * be completed at all (broker unreachable, timed out, connection closed). A caller that + * collapses those two into one, as fleetd #361 initially did, cannot tell "that peer is down" + * from "I could not check", and a reader of {@code pending}/{@code consumers} cannot tell a + * measured zero from a zero standing in for "not measured". + * + *

Must never share fate with {@link #publish} or {@link #peek}/{@link #ack}. + * fleetd #361: in AMQP 0-9-1 a passive queue declare of a queue that does not exist closes the + * channel it was declared on with a 404. An implementation backed by a real broker connection + * must inspect on a channel it can afford to lose — never the channel {@link #publish} or the + * consume loop depends on — so that looking at a peer that happens to be down can never break + * this daemon's own send or receive path. + */ + MailboxState inspect(String coordId); + + /** + * The result of {@link #inspect}. {@code presence} tells apart three states a caller must not + * conflate: a confirmed-existing mailbox ({@link Presence#EXISTS}, the only case where + * {@code pending}/{@code consumers} are measured facts), a confirmed-absent one + * ({@link Presence#ABSENT} — the broker positively said "no such queue"), and one this call + * simply could not determine ({@link Presence#UNKNOWN} — broker unreachable, timed out, + * connection closed). {@code pending}/{@code consumers} are always {@code 0} and meaningless + * outside {@link Presence#EXISTS}; a renderer must gate on {@link #exists()} (or {@code + * presence} directly), never present them as measured otherwise. + * + * @param coordId the coord-id inspected + * @param presence whether the mailbox is confirmed to exist, confirmed absent, or unknown + * @param pending messages ready for delivery but not yet in a consumer's hands (0 unless EXISTS) + * @param consumers how many consumers are attached (0 unless EXISTS) + */ + record MailboxState(String coordId, Presence presence, int pending, int consumers) { + + /** Whether {@link #inspect} was able to reach a definite answer, of either kind. */ + public enum Presence { EXISTS, ABSENT, UNKNOWN } + + /** {@code true} only when the broker confirmed this exact queue is currently declared. */ + public boolean exists() { + return presence == Presence.EXISTS; + } + + /** + * {@code true} when {@link #inspect} reached a definite answer (exists or confirmed + * absent); {@code false} when it could not determine either way. A caller must never treat + * {@code !known()} the same as a confirmed absence — the mailbox may well exist. + */ + public boolean known() { + return presence != Presence.UNKNOWN; + } + + /** The broker confirmed this queue exists, with these measured counts. */ + public static MailboxState exists(String coordId, int pending, int consumers) { + return new MailboxState(coordId, Presence.EXISTS, pending, consumers); + } + + /** The broker positively confirmed there is no such queue (e.g. a 404 on passive declare). */ + public static MailboxState absent(String coordId) { + return new MailboxState(coordId, Presence.ABSENT, 0, 0); + } + + /** The look could not be completed — broker unreachable, timed out, or connection closed. */ + public static MailboxState unknown(String coordId) { + return new MailboxState(coordId, Presence.UNKNOWN, 0, 0); + } + } } diff --git a/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java b/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java index 78ee0da..a2f3b64 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java +++ b/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java @@ -10,6 +10,7 @@ import com.rabbitmq.client.DeliverCallback; import com.rabbitmq.client.Recoverable; import com.rabbitmq.client.RecoveryListener; import com.rabbitmq.client.Return; +import com.rabbitmq.client.ShutdownSignalException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -259,6 +260,89 @@ public final class LeadMailbox implements LeadChannel, AutoCloseable { } } + /** + * fleetd #361: look at {@code coordId}'s mailbox on a fresh, immediately-closed throwaway + * channel — never {@link #channel} (consume/ack) or {@link #publishChannel} (publish). A + * passive queue declare of a queue that does not exist closes the channel it was declared on + * with a 404; using a disposable probe channel means that closure can never touch either + * long-lived channel this instance depends on for {@link #publish} or the consume loop. + * + *

Classifies failures rather than collapsing them, both measured against a real broker in + * {@code LeadMailboxTest} rather than assumed from the AMQP 0-9-1 spec text: + *

+ * + *

Honesty about which catch is measured and which is defensive: the + * {@code createChannel()} catch above is exercised end-to-end against a real broker by + * {@code LeadMailboxTest.inspectReportsUnknownRatherThanThrowingWhenTheConnectionIsAlreadyClosed}. + * The second {@code catch (RuntimeException e)}, around the passive declare itself — for the + * narrower race where the connection drops between {@code createChannel()} succeeding + * and the declare landing — has no such test; reaching it needs a connection that dies at that + * exact instant, which is not a scenario this suite drives on purpose. It stays purely + * defensive: correct by the same reasoning as the first catch, but unproven the way the first + * one is proven. + */ + @Override + public MailboxState inspect(String coordId) { + String queue = queueName(coordId); + Channel probe; + try { + probe = connection.createChannel(); + } catch (IOException | RuntimeException e) { + log.debug("lead mailbox inspect: cannot open a probe channel for {}: {}", coordId, e.toString()); + return MailboxState.unknown(coordId); + } + try { + AMQP.Queue.DeclareOk declared = probe.queueDeclarePassive(queue); + return MailboxState.exists(coordId, declared.getMessageCount(), declared.getConsumerCount()); + } catch (IOException e) { + // The broker (or the client library) has already closed `probe` for us either way; only + // a confirmed 404 means "no such queue" — anything else (a different declare failure) is + // "could not determine", never silently reported as the same value as a genuine absence. + return isMissingQueue(e) ? MailboxState.absent(coordId) : MailboxState.unknown(coordId); + } catch (RuntimeException e) { + // E.g. the connection dropped between createChannel() and the declare landing. + log.debug("lead mailbox inspect: declare failed unexpectedly for {}: {}", coordId, e.toString()); + return MailboxState.unknown(coordId); + } finally { + try { + if (probe.isOpen()) { + probe.close(); + } + } catch (Exception e) { + log.debug("lead mailbox inspect: probe channel close for {}: {}", coordId, e.toString()); + } + } + } + + /** + * {@code true} only for the specific shape a missing-queue passive declare actually produces — + * measured against a real broker, not assumed from the spec text (see {@code + * LeadMailboxTest.passiveDeclareOfAMissingQueueThrowsAnIOExceptionWrappingA404ShutdownSignal}): + * an {@link IOException} whose cause is a {@link ShutdownSignalException} carrying an + * {@link AMQP.Channel.Close} reason with {@code replyCode == 404}. Any other shape (a different + * reply code, a {@code ShutdownSignalException} cause whose reason is not a + * {@code Channel.Close}, or no cause at all) is a declare failure of some other kind and must + * not be read as "confirmed absent" — pinned hermetically, with no broker needed, by + * {@code LeadMailboxIsMissingQueueTest} for exactly those three false shapes. Package-private + * (not {@code private}) so that test can call it directly. + */ + static boolean isMissingQueue(IOException e) { + if (!(e.getCause() instanceof ShutdownSignalException sse)) { + return false; + } + return sse.getReason() instanceof AMQP.Channel.Close close && close.getReplyCode() == AMQP.NOT_FOUND; + } + /** Convenience: {@link #peek} the current snapshot, then {@link #ack} every message in it. */ public List drain() { List snapshot = peek(); 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 1da0f95..af80e13 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -92,6 +92,15 @@ public final class GitWorktrees implements Worktrees { private final String configuredRoot; /** OS group name for {@link #shareWithGroup} (fleetd #185 stage 3); {@code null} ⇒ feature off. */ private final String group; + /** 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. */ @@ -125,7 +134,20 @@ public final class GitWorktrees implements Worktrees { * config); null/blank ⇒ {@link #shareWithGroup} is a no-op. */ public GitWorktrees(String configuredRoot, String group) { - this(configuredRoot, group, _ -> {}); + this(configuredRoot, group, (String) null); + } + + /** + * @param configuredRoot nullable absolute or relative path; null/blank derives a sibling of + * the repo root. + * @param group optional OS group name (fleetd #185 stage 3, {@code worktreeGroup:} + * in config); null/blank ⇒ {@link #shareWithGroup} is a no-op. + * @param memberSkillsSource fleetd #362: optional directory of skill folders ({@code + * memberSkills:} in config) copied into every provisioned worktree's + * {@code .claude/skills/}; null/blank ⇒ {@link #seedSkills} is a no-op. + */ + public GitWorktrees(String configuredRoot, String group, String memberSkillsSource) { + this(configuredRoot, group, _ -> {}, null, null, memberSkillsSource); } /** Test seam for changing a real worktree between its creation and its security check. */ @@ -135,13 +157,20 @@ public final class GitWorktrees implements Worktrees { /** Test seam combining a configurable {@code group} with {@link #afterWorktreeAdded}. */ GitWorktrees(String configuredRoot, String group, Consumer afterWorktreeAdded) { - this(configuredRoot, group, afterWorktreeAdded, null, null); + this(configuredRoot, group, afterWorktreeAdded, null, null, null); } /** Test seam for changing how {@link #shareWithGroup}'s processes run. */ GitWorktrees(String configuredRoot, String group, Consumer afterWorktreeAdded, Function shareGroupRunner) { - this(configuredRoot, group, afterWorktreeAdded, shareGroupRunner, null); + this(configuredRoot, group, afterWorktreeAdded, shareGroupRunner, null, null); + } + + /** Test seam for changing how the initial {@code git worktree add} command runs, with no + * {@code memberSkillsSource} configured. */ + GitWorktrees(String configuredRoot, String group, Consumer afterWorktreeAdded, + Function shareGroupRunner, Function worktreeAddRunner) { + this(configuredRoot, group, afterWorktreeAdded, shareGroupRunner, worktreeAddRunner, null); } /** @@ -151,11 +180,30 @@ public final class GitWorktrees implements Worktrees { * * @param shareGroupRunner {@code null} ⇒ the real {@link #exec(String...)}. * @param worktreeAddRunner {@code null} ⇒ the real {@link #exec(String...)}. + * @param memberSkillsSource {@code null}/blank ⇒ {@link #seedSkills} is a no-op. */ GitWorktrees(String configuredRoot, String group, Consumer afterWorktreeAdded, - Function shareGroupRunner, Function worktreeAddRunner) { + 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; @@ -184,6 +232,7 @@ public final class GitWorktrees implements Worktrees { configureEnvironmentCredentialHelper(repoRoot, wt); configureHttpsUrlRewriteForSshOrigin(repoRoot, wt); isolateToolSurface(wt); + seedSkills(wt); } catch (RuntimeException e) { cleanupAfterAddFailure(repoRoot, wt, branch, e); throw e; @@ -574,6 +623,310 @@ public final class GitWorktrees implements Worktrees { return true; } + /** + * fleetd #362: copy each skill folder from the configured {@link #memberSkillsSource} directory + * into {@code /.claude/skills/}, so a member spawned against ANY repo — not only + * one that already ships its own {@code .claude/skills/} — can load a bridge skill such as + * {@code implementer}. Every brief this fleet sends starts with {@code "Load the + * skill."}; outside a repo carrying its own copy that line was previously a no-op. + * + *

No-op — nothing read, nothing written, nothing logged — when {@link + * #memberSkillsSource} is null/blank (today's default), the same off-switch shape as + * {@link #shareWithGroup}. + * + *

Invariant 1 — a repo's own skill wins. A skill folder already present at + * {@code /.claude/skills/} — because the just-checked-out branch commits its + * own copy — is left completely untouched: never overwritten, and never even opened. + * + *

Invariant 2 — a seeded skill can never end up in a worker's commit. Every path this + * writes is untracked in the target repo (that is the whole reason it is being seeded), so + * {@code git status} would otherwise show each one as a new, addable, committable path. The + * repo-wide {@code .git/info/exclude} is NOT used for this: measured against a real linked + * worktree, that file resolves to the repository's COMMON git dir even from a worktree (the + * same file {@link dev.ltms.fleet.member.ClaudeCodeLauncher#writeIdeOverlay writeIdeOverlay} + * appends {@code CLAUDE.local.md} to), so an entry written there would hide the seeded skill + * from {@code git status} in the PRIMARY's own checkout and every sibling worktree too — not + * only this one. Instead, {@link #excludeSeededSkillsFromGitStatus} points {@code + * core.excludesFile} at a file scoped {@code --worktree} (the same {@code + * extensions.worktreeConfig} mechanism {@link #configureEnvironmentCredentialHelper} already + * relies on) that itself lives under this worktree's own private git dir ({@code + * .git/worktrees//}, OUTSIDE the working tree) — invisible to this worktree's {@code git + * status} and structurally impossible for this worktree to commit, with no effect on any other + * 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. + * + *

Recorded for the worker itself the same way fleetd #134 records {@code + * fleet.neutralizedConfig}: {@code fleet.seededSkills} (one value per seeded skill folder) and + * {@code fleet.seededSkillsNote}, readable with {@code git config --worktree --get-all + * fleet.seededSkills}. + * + *

Claude Code specific by construction, not by a backend check here. Only {@code + * .claude/skills//SKILL.md} is a path any launcher reads today (opencode's equivalent is a + * different shape under {@code .opencode/agent}, out of scope — see issue #362). This method + * only copies files; like {@link #isolateToolSurface} — which neutralizes BOTH {@code .mcp.json} + * and {@code opencode.json} unconditionally — it runs the same for every worktree regardless of + * which backend ultimately spawns into it, because the backend is not yet chosen at {@link #add} + * time. A seeded {@code .claude/skills/} directory in an opencode member's worktree is simply + * never read by that launcher. + */ + private void seedSkills(String worktreePath) { + if (memberSkillsSource == null) { + return; + } + Path source = Path.of(memberSkillsSource).toAbsolutePath().normalize(); + if (!Files.isDirectory(source)) { + log.warn("memberSkills source '{}' is not a directory — skipping skill seeding for worktree {}", + source, worktreePath); + return; + } + Path skillsRoot = Path.of(worktreePath).resolve(".claude").resolve("skills"); + List seeded = new ArrayList<>(); + List kept = new ArrayList<>(); + try (var candidates = Files.list(source)) { + for (Path candidate : candidates + .filter(Files::isDirectory) + .filter(p -> !p.getFileName().toString().startsWith(".")) + .sorted() + .toList()) { + String name = candidate.getFileName().toString(); + Path dst = skillsRoot.resolve(name); + if (Files.exists(dst)) { + kept.add(name); + continue; + } + copySkillDirectory(candidate, dst); + seeded.add(name); + } + } catch (IOException | RuntimeException e) { + log.warn("failed to seed skills into worktree {} from memberSkills source '{}': {}", + worktreePath, source, e.getMessage()); + return; + } + String detail = seeded.isEmpty() ? "" : "seeded: " + String.join(", ", seeded); + if (!kept.isEmpty()) { + detail += (detail.isEmpty() ? "" : "; ") + "kept the repo's own copy of: " + String.join(", ", kept); + } + if (detail.isEmpty()) { + detail = "no skill folders found under " + source; + } + log.info("skill seeding: {} of {} candidate(s) from {} into {}/.claude/skills — {}", + seeded.size(), seeded.size() + kept.size(), source, worktreePath, detail); + if (seeded.isEmpty()) { + return; + } + try { + excludeSeededSkillsFromGitStatus(worktreePath, seeded); + recordSeededSkillsForWorker(worktreePath, seeded); + } catch (RuntimeException e) { + log.warn("seeded skill(s) {} into {} but could not hide them from git status: {} — " + + "they may show as untracked; never commit them", seeded, worktreePath, e.getMessage()); + } + } + + /** Recursively copy a skill folder ({@code src}) into a fresh destination ({@code dst}) that + * {@link #seedSkills} has already confirmed does not exist, preserving the directory structure + * (e.g. {@code implementer/SKILL.md}, {@code implementer/references/...}). */ + private static void copySkillDirectory(Path src, Path dst) { + try (var walk = Files.walk(src)) { + for (Path path : walk.sorted().toList()) { + Path target = dst.resolve(src.relativize(path).toString()); + if (Files.isDirectory(path)) { + Files.createDirectories(target); + } else { + Files.createDirectories(target.getParent()); + Files.copy(path, target, StandardCopyOption.COPY_ATTRIBUTES); + } + } + } catch (IOException e) { + throw new WorktreeException("cannot copy skill directory " + src + " -> " + dst + ": " + + e.getMessage(), e); + } + } + + /** + * Make every path in {@code seededSkillNames} (each a name under {@code .claude/skills/}) + * invisible to {@code git status} in THIS worktree only — see the invariant-2 discussion on + * {@link #seedSkills}. Sets {@code core.excludesFile} scoped {@code --worktree} to a file + * written under this worktree's own private git dir ({@code git rev-parse + * --absolute-git-dir}), which lives outside the working tree, so the exclude file itself can + * never be committed either. + * + *

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. + * + *

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"); + 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()); + } + try { + Files.writeString(excludeFile, patterns.toString()); + } catch (IOException e) { + throw new WorktreeException("cannot write skills exclude file " + excludeFile + ": " + + e.getMessage(), e); + } + exec("git", "-C", worktreePath, "config", "--worktree", "--replace-all", "core.excludesFile", + 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. + * + *

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. + */ + 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 = resolveEnv("XDG_CONFIG_HOME"); + Path fallback = (xdgConfigHome != null && !xdgConfigHome.isBlank()) + ? Path.of(xdgConfigHome, "git", "ignore") + : Path.of(resolveHome(), ".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 ""; + } + } + + /** + * 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 + * working-tree file that would show up in {@code git status}. + */ + private void recordSeededSkillsForWorker(String worktreePath, List seeded) { + exec("git", "-C", worktreePath, "config", "extensions.worktreeConfig", "true"); + for (String name : seeded) { + exec("git", "-C", worktreePath, "config", "--worktree", "--add", "fleet.seededSkills", name); + } + exec("git", "-C", worktreePath, "config", "--worktree", "fleet.seededSkillsNote", + "each fleet.seededSkills value names a skill folder fleetd copied into " + + ".claude/skills/ because this repo did not already ship it; it is excluded " + + "from git status (core.excludesFile, worktree-scoped) and must never be committed"); + } + @Override public void remove(String repoRoot, String worktreePath) { Path p = Path.of(worktreePath); @@ -1084,6 +1437,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); } @@ -1119,7 +1475,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/FleetdLeadMailboxSelectionTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadMailboxSelectionTest.java index 82cc333..f681bfa 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadMailboxSelectionTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadMailboxSelectionTest.java @@ -80,7 +80,7 @@ class FleetdLeadMailboxSelectionTest { @Test void opensTheMailboxWhenAUriAndSelfIdAreConfigured() { var opener = new RecordingOpener(); - var coordinator = new FleetConfig.Coordinator(RESOLVED_URI, null, "mac-opus", null); + var coordinator = new FleetConfig.Coordinator(RESOLVED_URI, null, "mac-opus", null, null); Fleetd.openLeadMailbox(coordinator, Map.of(), opener); @@ -94,7 +94,7 @@ class FleetdLeadMailboxSelectionTest { void honoursUriEnvOverALiteralUri() { var opener = new RecordingOpener(); var coordinator = new FleetConfig.Coordinator("amqp://stale:stale@old:5672/x", "COORD_URI", - "mac-opus", 8); + "mac-opus", 8, null); Fleetd.openLeadMailbox(coordinator, Map.of("COORD_URI", RESOLVED_URI), opener); @@ -105,7 +105,7 @@ class FleetdLeadMailboxSelectionTest { @Test void turnsOffWhenUriEnvDoesNotResolve() { var opener = new RecordingOpener(); - var coordinator = new FleetConfig.Coordinator(null, "COORD_URI", "mac-opus", null); + var coordinator = new FleetConfig.Coordinator(null, "COORD_URI", "mac-opus", null, null); assertNull(Fleetd.openLeadMailbox(coordinator, Map.of(), opener)); @@ -116,7 +116,7 @@ class FleetdLeadMailboxSelectionTest { void warnsAndStaysOffWhenSelfIdIsMissing() { var appender = captureFleetdLogs(); var opener = new RecordingOpener(); - var coordinator = new FleetConfig.Coordinator(RESOLVED_URI, null, null, null); + var coordinator = new FleetConfig.Coordinator(RESOLVED_URI, null, null, null, null); assertNull(Fleetd.openLeadMailbox(coordinator, Map.of(), opener)); @@ -131,7 +131,7 @@ class FleetdLeadMailboxSelectionTest { var appender = captureFleetdLogs(); var opener = new RecordingOpener(); opener.unreachable = true; - var coordinator = new FleetConfig.Coordinator(RESOLVED_URI, null, "mac-opus", null); + var coordinator = new FleetConfig.Coordinator(RESOLVED_URI, null, "mac-opus", null, null); assertNull(Fleetd.openLeadMailbox(coordinator, Map.of(), opener), "a down coordination broker turns the feature off; it must never take the daemon down"); diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java index aea7bba..8bf2cf1 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java @@ -104,9 +104,10 @@ class ConfigRefTopLevelReportingCoverageTest { v.put("configReload", new FleetConfig.ConfigReload(true, 10)); v.put("quarantineCooldownSeconds", 1800); v.put("memberCredentials", null); - v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-a", null, "self-a", 1)); + v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-a", null, "self-a", 1, null)); v.put("worktreeGroup", "group-a"); v.put("memberLoginShell", null); + v.put("memberSkills", "/skills/a"); assertNamesMatchComponents(v); return v; } @@ -144,9 +145,10 @@ class ConfigRefTopLevelReportingCoverageTest { v.put("configReload", new FleetConfig.ConfigReload(false, 20)); v.put("quarantineCooldownSeconds", 3600); v.put("memberCredentials", null); - v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-b", null, "self-b", 2)); + v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-b", null, "self-b", 2, null)); v.put("worktreeGroup", "group-b"); v.put("memberLoginShell", null); + v.put("memberSkills", "/skills/b"); assertNamesMatchComponents(v); return v; } diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java index 4292139..df9331b 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java @@ -1189,7 +1189,7 @@ class FleetConfigTest { @Test void coordinatorEffectiveUriHonorsUriEnv() { FleetConfig.Coordinator withEnv = new FleetConfig.Coordinator( - "amqp://stale-clear-text@127.0.0.1:5672/coord", "LEAD_COORD_URI", "fleet01-lead", null); + "amqp://stale-clear-text@127.0.0.1:5672/coord", "LEAD_COORD_URI", "fleet01-lead", null, null); assertEquals("amqp://from-env@127.0.0.1:5672/coord", withEnv.effectiveUri(Map.of("LEAD_COORD_URI", "amqp://from-env@127.0.0.1:5672/coord")), @@ -1200,12 +1200,61 @@ class FleetConfigTest { "a blank uriEnv variable must not fall back to the literal uri"); FleetConfig.Coordinator noEnv = new FleetConfig.Coordinator( - "amqp://guest:guest@127.0.0.1:5672/coord", null, null, null); + "amqp://guest:guest@127.0.0.1:5672/coord", null, null, null, null); assertEquals("amqp://guest:guest@127.0.0.1:5672/coord", noEnv.effectiveUri(Map.of()), "the literal uri is used when no uriEnv is configured"); assertEquals(LeadMailbox.DEFAULT_PREFETCH, noEnv.prefetchOrDefault()); } + /** + * fleetd #361: the live block ships as just {@code uriEnv} + {@code selfId} (see + * {@code Fleetd.example.yaml} / the operator's real {@code fleetd.yaml}, gitignored). That exact + * shape, with no {@code peers:} key at all, must keep parsing unchanged after this field is added. + */ + @Test + void coordinatorBlockWithNoPeersKeyStillParses(@TempDir Path dir) throws Exception { + Path f = dir.resolve("coordinator-no-peers.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + coordinator: + uriEnv: COORD_AMQP_URI + selfId: mac + """); + + FleetConfig cfg = FleetConfig.load(f); + assertNotNull(cfg.coordinator()); + assertEquals("mac", cfg.coordinator().selfId()); + assertEquals(List.of(), cfg.coordinator().peers(), "no peers: key means no configured peers, never null"); + } + + @Test + void coordinatorPeersParsesAndDropsBlankEntries(@TempDir Path dir) throws Exception { + Path f = dir.resolve("coordinator-peers.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + coordinator: + selfId: mac + uri: amqp://guest:guest@127.0.0.1:5672/coord + peers: + - fleet01 + - "" + - fleet02 + """); + + FleetConfig cfg = FleetConfig.load(f); + assertEquals(List.of("fleet01", "fleet02"), cfg.coordinator().peers(), + "a blank peer entry must be dropped, never kept as an empty coord-id"); + } + + @Test + void coordinatorPeersDefaultsToEmptyWhenConstructedWithNull() { + FleetConfig.Coordinator c = new FleetConfig.Coordinator( + "amqp://guest:guest@127.0.0.1:5672/coord", null, "mac", null, null); + assertEquals(List.of(), c.peers(), "a null peers list must default to empty, never NPE downstream"); + } + @Test void absentWorktreeGroupLeavesItNull(@TempDir Path dir) throws Exception { Path f = dir.resolve("no-worktree-group.yaml"); diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigWithDefaultsPreservesEveryComponentTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigWithDefaultsPreservesEveryComponentTest.java index 1f710a0..5faefb4 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigWithDefaultsPreservesEveryComponentTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigWithDefaultsPreservesEveryComponentTest.java @@ -45,8 +45,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; *

Why this is a valid check for every component, not just some: {@link #withDefaults()}'s own * comments document that it only ever REPLACES a component when the incoming value is {@code null} * (or blank, for {@code placement}) — {@code broker}/{@code primary}/{@code leadHeartbeat}/ - * {@code configReload}/{@code coordinator}/{@code worktreeGroup}/{@code memberLoginShell} are left - * as-is unconditionally, and {@code bind}/{@code guard}/{@code lifecycle}/{@code auth}/ + * {@code configReload}/{@code coordinator}/{@code worktreeGroup}/{@code memberLoginShell}/ + * {@code memberSkills} are left as-is unconditionally, and {@code bind}/{@code guard}/{@code lifecycle}/{@code auth}/ * {@code fleet}/{@code quarantineCooldownSeconds}/{@code memberCredentials}/{@code placement} are * replaced only on null/blank input. A value that is never null or blank going in must therefore * never change coming out, for every current component. No exclusion is needed today. @@ -92,9 +92,10 @@ class FleetConfigWithDefaultsPreservesEveryComponentTest { v.put("memberCredentials", new FleetConfig.MemberCredentials( FleetConfig.MemberCredentials.POLICY_DENY_BY_DEFAULT, List.of("git"), List.of("git", "ssh"), null)); - v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-guard", null, "self-guard", 3)); + v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-guard", null, "self-guard", 3, null)); v.put("worktreeGroup", "group-guard"); v.put("memberLoginShell", "/bin/zsh"); + v.put("memberSkills", "/skills/guard"); assertNamesMatchComponents(v); return v; } diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpLeadCoordTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpLeadCoordTest.java index 6a3b66c..2384c1c 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpLeadCoordTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpLeadCoordTest.java @@ -1,6 +1,7 @@ package dev.ltms.fleet.mcp; import dev.ltms.fleet.msg.FakeLeadChannel; +import dev.ltms.fleet.msg.LeadChannel; import dev.ltms.fleet.msg.LeadMessage; import io.modelcontextprotocol.spec.McpSchema; import org.junit.jupiter.api.Test; @@ -93,4 +94,56 @@ class FleetMcpLeadCoordTest { assertTrue(FleetMcp.sendToLead(channel, PEER, " ", null, null).isError()); assertEquals(0, channel.published().size()); } + + /** + * fleetd #361: "delivered" overstated what publish actually proves — the broker's confirm means + * durably queued, not read. A zero-consumer target is the observable form of "this will not + * reach a pane right now", so the (still-successful) result must say so. + */ + @Test + void warnsWhenThePeerMailboxHasNoConsumersButStillReportsSuccess() { + var channel = new FakeLeadChannel(SELF) + .withMailbox(PEER, LeadChannel.MailboxState.exists(PEER, 0, 0)); + + McpSchema.CallToolResult res = FleetMcp.sendToLead(channel, PEER, "hi", null, null); + + assertFalse(res.isError(), "a zero-consumer mailbox is still a successful, durably-queued publish"); + String out = textOf(res); + assertTrue(out.contains("durably confirmed"), out); + assertTrue(out.toLowerCase().contains("no consumers"), () -> "must warn nobody is reading it: " + out); + } + + @Test + void staysQuietAboutConsumersWhenThePeerMailboxHasOne() { + var channel = new FakeLeadChannel(SELF) + .withMailbox(PEER, LeadChannel.MailboxState.exists(PEER, 0, 1)); + + McpSchema.CallToolResult res = FleetMcp.sendToLead(channel, PEER, "hi", null, null); + + assertFalse(res.isError()); + String out = textOf(res); + assertTrue(out.contains("durably confirmed"), out); + assertFalse(out.toLowerCase().contains("no consumers"), () -> "a consumer IS attached: " + out); + } + + /** + * fleetd #361 review finding 1: an unmeasured fact must never render as a definite one. When + * the post-publish probe could not determine the mailbox's consumer count at all (broker slow, + * unreachable, or the probe timed out — {@link LeadChannel.MailboxState#unknown}), the result + * must stay just as quiet as the has-a-consumer case — never assert "no consumers" for a mailbox + * this call never actually measured. + */ + @Test + void staysQuietAboutConsumersWhenThePeerMailboxStateIsUnknown() { + var channel = new FakeLeadChannel(SELF) + .withMailbox(PEER, LeadChannel.MailboxState.unknown(PEER)); + + McpSchema.CallToolResult res = FleetMcp.sendToLead(channel, PEER, "hi", null, null); + + assertFalse(res.isError(), "an unresolved post-publish probe must never turn a durably-confirmed publish into an error"); + String out = textOf(res); + assertTrue(out.contains("durably confirmed"), out); + assertFalse(out.toLowerCase().contains("no consumers"), + () -> "an unmeasured fact must never be reported as a definite zero-consumer mailbox: " + out); + } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java index 0e08820..554d767 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java @@ -10,6 +10,9 @@ import dev.ltms.fleet.herdr.FakeHerdr; import dev.ltms.fleet.herdr.PaneLocator; import dev.ltms.fleet.herdr.WorkspaceControl; import dev.ltms.fleet.inject.Injector; +import dev.ltms.fleet.msg.FakeLeadChannel; +import dev.ltms.fleet.msg.LeadChannel; +import dev.ltms.fleet.msg.LeadMessage; import dev.ltms.fleet.msg.MessageService; import dev.ltms.fleet.msg.Rendezvous; import dev.ltms.fleet.session.FakeWorktrees; @@ -34,7 +37,9 @@ import java.util.Map; import java.util.EnumSet; import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Function; import static org.junit.jupiter.api.Assertions.*; @@ -576,17 +581,48 @@ class FleetMcpTest { void listReportsThisDaemonsOwnCoordIdWhenLeadCoordinationIsOn() { FakeHerdr h = new FakeHerdr(); SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + FakeLeadChannel channel = new FakeLeadChannel("mac-opus") + .withMailbox("mac-opus", LeadChannel.MailboxState.exists("mac-opus", 0, 1)); McpSchema.CallToolResult res = FleetMcp.listFleet( workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), sessions, null, FleetMcp.CapacitySource.none(), new FleetMcp.HealthCoverageSource(() -> "off"), - FleetMcp.QuarantineSource.none(), Map.of(), "", "mac-opus"); + FleetMcp.QuarantineSource.none(), Map.of(), "", + new FleetMcp.CoordinationSource(channel, List.of())); String out = textOf(res); - // There is no peer-discovery surface yet, so this row answers the one question an operator - // cannot answer any other way: which coord-id a peer must use to reach ME. + // fleetd #361: reports both which coord-id a peer must use to reach ME, and this daemon's + // own mailbox state (a self-diagnosis: is my own consumer actually attached?). assertTrue(out.contains("\"coordinator\""), out); assertTrue(out.contains("\"selfId\":\"mac-opus\""), out); + assertTrue(out.contains("\"mailbox\":{\"status\":\"exists\",\"pending\":0,\"consumers\":1}"), out); + assertTrue(out.contains("\"held\":[]"), out); + assertTrue(out.contains("\"peers\":[]"), out); + } + + /** + * fleetd #361 review finding 1: a self-probe that could not complete (broker unreachable, timed + * out) must never render the same as a measured "0 pending, 0 consumers" — that was exactly the + * bug: a reader could not tell "my mailbox is empty and idle" from "I could not check", and the + * second one is the far more alarming state. + */ + @Test + void listReportsAnUnresolvedSelfProbeAsUnknownNeverAsAMeasuredZero() { + FakeHerdr h = new FakeHerdr(); + SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + FakeLeadChannel channel = new FakeLeadChannel("mac-opus") + .withMailbox("mac-opus", LeadChannel.MailboxState.unknown("mac-opus")); + + McpSchema.CallToolResult res = FleetMcp.listFleet( + workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), sessions, null, + FleetMcp.CapacitySource.none(), new FleetMcp.HealthCoverageSource(() -> "off"), + FleetMcp.QuarantineSource.none(), Map.of(), "", + new FleetMcp.CoordinationSource(channel, List.of())); + + String out = textOf(res); + assertTrue(out.contains("\"mailbox\":{\"status\":\"unknown\"}"), out); + assertFalse(out.contains("\"pending\""), "an unresolved probe must never carry a pending count at all: " + out); + assertFalse(out.contains("\"consumers\""), "an unresolved probe must never carry a consumers count at all: " + out); } @Test @@ -601,6 +637,110 @@ class FleetMcpTest { "an ordinary fleet's output must be unchanged by this feature"); } + @Test + void listReportsHeldMessagesWithATruncatedPreviewNeverTheFullBody() { + FakeHerdr h = new FakeHerdr(); + SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + String longContent = "x".repeat(200); + FakeLeadChannel channel = new FakeLeadChannel("mac-opus") + .hold(new LeadMessage("m1", "fleet01-lead", "mac-opus", longContent)); + + McpSchema.CallToolResult res = FleetMcp.listFleet( + workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), sessions, null, + FleetMcp.CapacitySource.none(), new FleetMcp.HealthCoverageSource(() -> "off"), + FleetMcp.QuarantineSource.none(), Map.of(), "", + new FleetMcp.CoordinationSource(channel, List.of())); + + String out = textOf(res); + assertTrue(out.contains("\"msgId\":\"m1\""), out); + assertTrue(out.contains("\"from\":\"fleet01-lead\""), out); + assertFalse(out.contains(longContent), "fleet_list must never dump a held message's full body: " + out); + assertTrue(out.contains("x".repeat(80) + "…"), "expected an 80-char preview with an ellipsis: " + out); + } + + @Test + void listReportsEachDeclaredPeersLiveReachability() { + FakeHerdr h = new FakeHerdr(); + SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + FakeLeadChannel channel = new FakeLeadChannel("mac-opus") + .withMailbox("fleet01-lead", LeadChannel.MailboxState.exists("fleet01-lead", 2, 1)) + .withMailbox("fleet03-lead", LeadChannel.MailboxState.unknown("fleet03-lead")); + // "fleet02-lead" is declared as a peer but never configured on the fake — inspect() falls + // back to MailboxState.absent, exactly as a real down (never-run) peer would report. + // "fleet03-lead" IS configured, as unknown — a broker that could not be reached in time, + // which review finding 1 says must render distinctly from "fleet02-lead"'s confirmed absence. + + McpSchema.CallToolResult res = FleetMcp.listFleet( + workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), sessions, null, + FleetMcp.CapacitySource.none(), new FleetMcp.HealthCoverageSource(() -> "off"), + FleetMcp.QuarantineSource.none(), Map.of(), "", + new FleetMcp.CoordinationSource(channel, List.of("fleet01-lead", "fleet02-lead", "fleet03-lead"))); + + String out = textOf(res); + assertTrue(out.contains("\"coordId\":\"fleet01-lead\",\"status\":\"exists\",\"pending\":2,\"consumers\":1"), out); + assertTrue(out.contains("\"coordId\":\"fleet02-lead\",\"status\":\"absent\""), out); + assertTrue(out.contains("\"coordId\":\"fleet03-lead\",\"status\":\"unknown\""), out); + assertFalse(out.contains("\"coordId\":\"fleet02-lead\",\"status\":\"absent\",\"pending\""), + "pending/consumers must be omitted, not faked as zero, for a confirmed-absent peer: " + out); + assertFalse(out.contains("\"coordId\":\"fleet03-lead\",\"status\":\"unknown\",\"pending\""), + "pending/consumers must be omitted, not faked as zero, for an unresolved peer probe: " + out); + } + + /** + * fleetd #361 review finding 2: {@code get(timeout)} alone times out the CALLER but leaves the + * submitted {@link LeadChannel#inspect} task running forever on its own virtual thread — against + * a hung (not down) broker every probe would orphan one more thread holding an AMQP channel + * until the connection's channel-max is exhausted, which would break {@code publish} too. This + * proves {@link FleetMcp#probe(LeadChannel, String, long)} does not merely give up on a slow + * task: it interrupts it, so the task does not go on running unbounded after the caller has + * already moved on. No hung broker needed — a {@link LeadChannel} fake that blocks until + * interrupted is enough to observe the same mechanism. + */ + @Test + void aTimedOutProbeInterruptsTheOrphanedTaskRatherThanAbandoningIt() throws Exception { + CountDownLatch started = new CountDownLatch(1); + AtomicBoolean wasInterrupted = new AtomicBoolean(false); + LeadChannel hangs = new LeadChannel() { + @Override + public void publish(String toCoordId, LeadMessage m) { } + + @Override + public List peek() { return List.of(); } + + @Override + public void ack(String msgId) { } + + @Override + public String selfCoordId() { return "mac-opus"; } + + @Override + public MailboxState inspect(String coordId) { + started.countDown(); + try { + Thread.sleep(60_000); + } catch (InterruptedException e) { + wasInterrupted.set(true); + Thread.currentThread().interrupt(); + } + return MailboxState.unknown(coordId); + } + }; + + LeadChannel.MailboxState result = FleetMcp.probe(hangs, "fleet01-lead", 100L); + + assertFalse(result.exists(), "a timed-out probe must never claim the mailbox exists"); + assertFalse(result.known(), "a timed-out probe proves nothing either way — it must report unknown"); + assertTrue(started.await(2, TimeUnit.SECONDS), "the probe task must actually have started"); + // The interrupt is delivered asynchronously to the orphaned task's own thread — poll briefly + // rather than assume it has already landed the instant probe() returns. + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(2); + while (!wasInterrupted.get() && System.nanoTime() < deadline) { + Thread.sleep(20); + } + assertTrue(wasInterrupted.get(), + "probe() must cancel the orphaned task (interrupt it) instead of leaving it to run forever"); + } + @Test void capacityUsesThePlacementLiveCount() { FakeHerdr h = new FakeHerdr(); @@ -911,7 +1051,8 @@ class FleetMcpTest { String out = textOf(FleetMcp.listFleet(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), sessions, null, new FleetMcp.CapacitySource(profile -> 2, profile -> 3, () -> Set.of("sonnet"), () -> 0), new FleetMcp.HealthCoverageSource(() -> "off"), - FleetMcp.QuarantineSource.none(), FleetMcp.OutageSource.none(), leadSeats, Map.of(), "", null)); + FleetMcp.QuarantineSource.none(), FleetMcp.OutageSource.none(), leadSeats, Map.of(), "", + FleetMcp.CoordinationSource.none())); assertTrue(out.contains("\"maxLoad\":3"), "maxLoad itself must be left untouched: " + out); assertTrue(out.contains("\"live\":2"), out); @@ -934,7 +1075,8 @@ class FleetMcpTest { String out = textOf(FleetMcp.listFleet(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), sessions, null, new FleetMcp.CapacitySource(profile -> 0, profile -> 3, () -> Set.of("sonnet"), () -> 0), new FleetMcp.HealthCoverageSource(() -> "off"), - FleetMcp.QuarantineSource.none(), FleetMcp.OutageSource.none(), leadSeats, Map.of(), "", null)); + FleetMcp.QuarantineSource.none(), FleetMcp.OutageSource.none(), leadSeats, Map.of(), "", + FleetMcp.CoordinationSource.none())); assertTrue(out.contains("\"live\":0"), out); assertTrue(out.contains("\"free\":3"), "the real gate never subtracts the lead's seat: " + out); @@ -988,7 +1130,7 @@ class FleetMcpTest { String out = textOf(FleetMcp.listFleet(composite, sm, null, new FleetMcp.CapacitySource(liveCount, p -> profiles.get(p).maxLoad(), profiles::keySet, () -> 0), new FleetMcp.HealthCoverageSource(() -> "off"), FleetMcp.QuarantineSource.none(), - FleetMcp.OutageSource.none(), leadSeats, Map.of(), "", null)); + FleetMcp.OutageSource.none(), leadSeats, Map.of(), "", FleetMcp.CoordinationSource.none())); int reportedFree = extractInt(out, "free"); for (int i = 0; i < reportedFree; i++) { @@ -1017,7 +1159,7 @@ class FleetMcpTest { sessions, null, new FleetMcp.CapacitySource(profile -> 0, profile -> 2, () -> Set.of("terra"), () -> 0), new FleetMcp.HealthCoverageSource(() -> "off"), FleetMcp.QuarantineSource.none(), FleetMcp.OutageSource.none(), FleetMcp.LeadSeatSource.none(), - Map.of(), "", null)); + Map.of(), "", FleetMcp.CoordinationSource.none())); assertTrue(out.contains("\"free\":2"), out); assertFalse(out.contains("leadSeats"), "no lead shares this profile's credential: " + out); diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/MemberEnvAllowListTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/MemberEnvAllowListTest.java index d579419..ae8d64b 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/MemberEnvAllowListTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/MemberEnvAllowListTest.java @@ -146,7 +146,7 @@ class MemberEnvAllowListTest { return new FleetConfig(null, null, null, Map.of(), null, null, null, null, null, new FleetConfig.Broker(null, brokerUriEnv, null), null, null, null, null, null, null, null, null, null, - new FleetConfig.Coordinator(null, coordinatorUriEnv, null, null)).withDefaults(); + new FleetConfig.Coordinator(null, coordinatorUriEnv, null, null, null)).withDefaults(); } /** {@code LC_*} categories are infrastructure by prefix; everything else needs an exact match. */ diff --git a/fleetd/src/test/java/dev/ltms/fleet/msg/FakeLeadChannel.java b/fleetd/src/test/java/dev/ltms/fleet/msg/FakeLeadChannel.java index 38ee2e9..b603a79 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/msg/FakeLeadChannel.java +++ b/fleetd/src/test/java/dev/ltms/fleet/msg/FakeLeadChannel.java @@ -3,6 +3,8 @@ package dev.ltms.fleet.msg; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; /** * Hermetic stand-in for {@link LeadChannel}: an in-memory mailbox that records what was published @@ -24,11 +26,19 @@ public final class FakeLeadChannel implements LeadChannel { private final List acked = Collections.synchronizedList(new ArrayList<>()); /** When set, every {@link #publish} throws it — the unroutable/nacked/timed-out peer. */ private volatile IllegalStateException publishFailure; + /** Canned {@link #inspect} results by coord-id — absent for any coord-id not configured here. */ + private final Map mailboxes = new ConcurrentHashMap<>(); public FakeLeadChannel(String selfCoordId) { this.selfCoordId = selfCoordId; } + /** Make {@link #inspect(String)} return {@code state} for {@code coordId} instead of "absent". */ + public FakeLeadChannel withMailbox(String coordId, MailboxState state) { + mailboxes.put(coordId, state); + return this; + } + /** Make every publish fail as an unreachable peer would. */ public FakeLeadChannel failPublishWith(String message) { this.publishFailure = new IllegalStateException(message); @@ -65,6 +75,11 @@ public final class FakeLeadChannel implements LeadChannel { return selfCoordId; } + @Override + public MailboxState inspect(String coordId) { + return mailboxes.getOrDefault(coordId, MailboxState.absent(coordId)); + } + public List published() { return List.copyOf(published); } diff --git a/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxIsMissingQueueTest.java b/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxIsMissingQueueTest.java new file mode 100644 index 0000000..aa2f50e --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxIsMissingQueueTest.java @@ -0,0 +1,77 @@ +package dev.ltms.fleet.msg; + +import com.rabbitmq.client.ShutdownSignalException; +import com.rabbitmq.client.impl.AMQImpl; +import org.junit.jupiter.api.Test; + +import java.io.IOException; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * fleetd #361 review round 2: a mutation that made {@link LeadMailbox#isMissingQueue} return + * {@code true} unconditionally still left {@code mvn clean install} green — 1389 tests, 0 + * failures — because nothing exercised its false branch. That branch is the whole discriminator + * between {@link LeadChannel.MailboxState#absent} and {@link LeadChannel.MailboxState#unknown}; + * without a test pinning it, a future refactor that widens it back to "always true" (restoring the + * exact overstatement fleetd #361 exists to fix) would pass this suite. + * + *

Hermetic — no broker needed, per the review's own suggestion. {@code isMissingQueue} takes a + * plain {@link IOException}, so every input here is constructed directly rather than provoked from + * a live connection. The real 404 shape itself is still pinned against a real broker, in + * {@code LeadMailboxTest.passiveDeclareOfAMissingQueueThrowsAnIOExceptionWrappingA404ShutdownSignal} + * — this class covers the three false shapes {@link LeadMailbox#isMissingQueue}'s own javadoc + * lists, so both directions of the discriminator are proven somewhere. + */ +class LeadMailboxIsMissingQueueTest { + + @Test + void aConfirmedMissingQueueIsRecognized() { + ShutdownSignalException sse = new ShutdownSignalException(true, false, + new AMQImpl.Channel.Close(404, "NOT_FOUND - no queue 'lead.x.inbox' in vhost '/'", 50, 10), null); + IOException e = new IOException("channel error", sse); + + assertTrue(LeadMailbox.isMissingQueue(e), "a genuine 404 Channel.Close must be recognized as a missing queue"); + } + + @Test + void aDifferentReplyCodeIsNotAMissingQueue() { + // E.g. 403 ACCESS_REFUSED — the queue may well exist; this call was simply refused. + ShutdownSignalException sse = new ShutdownSignalException(true, false, + new AMQImpl.Channel.Close(403, "ACCESS_REFUSED", 50, 10), null); + IOException e = new IOException("channel error", sse); + + assertFalse(LeadMailbox.isMissingQueue(e), + "a non-404 reply code must never be read as a confirmed absence — the mailbox's real state is unknown"); + } + + @Test + void aShutdownSignalWhoseReasonIsNotAChannelCloseIsNotAMissingQueue() { + // A Connection.Close (a whole different broker-level shutdown) is still a ShutdownSignalException, + // but its reason is not a Channel.Close at all — must not be misread as "no such queue". + ShutdownSignalException sse = new ShutdownSignalException(true, false, + new AMQImpl.Connection.Close(404, "coincidentally 404, but this is a CONNECTION close", 10, 50), null); + IOException e = new IOException("connection error", sse); + + assertFalse(LeadMailbox.isMissingQueue(e), + "a ShutdownSignalException whose reason is not a Channel.Close must never be read as a missing queue," + + " even if its reply code happens to be 404"); + } + + @Test + void anIOExceptionWithNoCauseAtAllIsNotAMissingQueue() { + IOException e = new IOException("some other declare failure, no cause attached"); + + assertFalse(LeadMailbox.isMissingQueue(e), + "an IOException with no ShutdownSignalException cause must never be read as a confirmed absence"); + } + + @Test + void anIOExceptionWithAnUnrelatedCauseIsNotAMissingQueue() { + IOException e = new IOException("wrapped something else entirely", new RuntimeException("boom")); + + assertFalse(LeadMailbox.isMissingQueue(e), + "a cause that isn't even a ShutdownSignalException must never be read as a confirmed absence"); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxTest.java b/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxTest.java index bc015ac..ccc5144 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxTest.java @@ -1,5 +1,9 @@ package dev.ltms.fleet.msg; +import com.rabbitmq.client.AMQP; +import com.rabbitmq.client.Channel; +import com.rabbitmq.client.Connection; +import com.rabbitmq.client.ShutdownSignalException; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; @@ -7,11 +11,14 @@ import org.testcontainers.containers.RabbitMQContainer; import org.testcontainers.junit.jupiter.Testcontainers; import org.testcontainers.utility.DockerImageName; +import java.io.IOException; import java.util.List; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -159,6 +166,145 @@ class LeadMailboxTest { } } + @Test + void inspectReportsAnOwnedMailboxAsExistingWithItsOwnConsumer() throws Exception { + // A LeadMailbox declares AND consumes its own queue the moment open() returns (see own()), + // so inspecting a coord-id this same process owns must always find exactly one consumer. + String self = coordId("lead-inspect-self"); + try (LeadMailbox mailbox = LeadMailbox.open(uri(), self)) { + LeadChannel.MailboxState state = mailbox.inspect(self); + assertEquals(self, state.coordId()); + assertTrue(state.exists(), "this daemon owns and has declared this exact queue"); + assertEquals(0, state.pending(), "nothing has been published to it yet"); + assertEquals(1, state.consumers(), "the mailbox's own constructor already attached a consumer"); + } + } + + @Test + void inspectReportsAMissingMailboxAsAbsentRatherThanThrowing() throws Exception { + String nobody = coordId("lead-inspect-nobody"); + try (LeadMailbox mailbox = LeadMailbox.open(uri(), coordId("lead-inspect-caller"))) { + LeadChannel.MailboxState state = mailbox.inspect(nobody); + assertEquals(LeadChannel.MailboxState.absent(nobody), state, + "a queue nobody has ever declared must report absent, never throw"); + assertTrue(state.known(), "a confirmed 404 IS a definite answer — this is not the unknown case"); + } + } + + /** + * fleetd #361 review finding 3: {@code inspect} is specified to never throw, but the original + * implementation caught only {@link IOException} — and {@link Connection#createChannel()} on an + * already-closed connection throws {@link com.rabbitmq.client.AlreadyClosedException}, an + * unchecked {@link RuntimeException} (pinned by {@code + * createChannelOnAnAlreadyClosedConnectionThrowsAnUncheckedException} above). This drives that + * exact scenario through the real {@link LeadMailbox#inspect} — not the raw client call — and + * checks both halves of finding 1 and finding 3 at once: no exception escapes, and the result is + * {@code UNKNOWN} rather than the wrong-but-plausible-looking {@code ABSENT}. + */ + @Test + void inspectReportsUnknownRatherThanThrowingWhenTheConnectionIsAlreadyClosed() throws Exception { + LeadMailbox mailbox = LeadMailbox.open(uri(), coordId("lead-inspect-dead-connection")); + mailbox.close(); // tears down the connection `inspect` will try to open a probe channel on + + LeadChannel.MailboxState state = mailbox.inspect(coordId("lead-inspect-irrelevant-target")); + + assertFalse(state.exists()); + assertFalse(state.known(), "a dead connection proves nothing about the target mailbox — it must be unknown, not absent"); + assertEquals(LeadChannel.MailboxState.Presence.UNKNOWN, state.presence()); + } + + @Test + void inspectReportsPendingMessagesAndZeroConsumersWhenNobodyIsReadingAnymore() throws Exception { + // Publish into a mailbox this test owns, then never consume from it, to prove `pending` and + // `consumers` really come off the broker rather than off this process's own in-memory state. + String to = coordId("lead-inspect-pending"); + String observerId = coordId("lead-inspect-observer"); + try (LeadMailbox owner = LeadMailbox.open(uri(), to); + LeadMailbox observer = LeadMailbox.open(uri(), observerId)) { + owner.publish(to, new LeadMessage("m1", "lead-from", to, "sitting in the queue")); + awaitPeek(owner); // make sure the broker has actually enqueued it before inspecting + } // `owner` closes here: its consumer disconnects, but the durable, unacked message stays queued. + + try (LeadMailbox observer = LeadMailbox.open(uri(), coordId("lead-inspect-observer-2"))) { + // The broker requeues `owner`'s unacked delivery asynchronously once its connection drops, + // so poll rather than assume the very first passive declare already sees the settled state. + LeadChannel.MailboxState state = awaitInspect(observer, to, s -> s.consumers() == 0); + assertTrue(state.exists()); + assertEquals(0, state.consumers(), "the only owner just closed — nobody is reading this anymore"); + assertEquals(1, state.pending(), "the unacked message must be requeued, never dropped"); + } + } + + /** + * fleetd #361's central invariant, proved rather than assumed: a passive queue declare of a + * missing queue closes ITS channel with a 404 in AMQP 0-9-1. {@link LeadMailbox#inspect} is + * specified to run on its own disposable channel for exactly this reason — this test is the one + * that actually exercises the failure mode and shows {@link LeadMailbox#publish} on the SAME + * instance is unaffected by it. + */ + @Test + void inspectingAMissingMailboxNeverBreaksPublishOnTheSameInstance() throws Exception { + String self = coordId("lead-invariant-self"); + try (LeadMailbox mailbox = LeadMailbox.open(uri(), self)) { + // Miss on a queue that has never existed — this is exactly the 404-closes-the-channel case. + LeadChannel.MailboxState missed = mailbox.inspect(coordId("lead-invariant-nobody-home")); + assertFalse(missed.exists()); + assertTrue(missed.known(), "a genuine 404 on a queue that never existed is a confirmed fact, not an unknown"); + + // publish() must still work on THIS SAME instance: if inspect() had reused `publishChannel` + // (or `channel`), the broker's 404 would have closed it out from underneath publish(). + LeadMessage sent = new LeadMessage("after-miss", "lead-from", self, "still alive"); + mailbox.publish(self, sent); + List got = awaitPeek(mailbox); + assertEquals(1, got.size(), "publish must still reach this mailbox's own queue after a missed inspect"); + assertEquals("after-miss", got.getFirst().msgId()); + + // And a second inspect() — of a mailbox that DOES exist this time — must also still work, + // proving the miss did not wedge inspect() itself either. + LeadChannel.MailboxState self2 = mailbox.inspect(self); + assertTrue(self2.exists()); + } + } + + /** + * Pins the exact exception shape {@link LeadMailbox#inspect} relies on to tell a genuine 404 + * (mailbox confirmed absent) apart from everything else (mailbox state unknown) — measured + * against a real broker rather than assumed from the AMQP 0-9-1 spec text. If this ever fails, + * the classification in {@code inspect} is reading the wrong shape and must be revisited. + */ + @Test + void passiveDeclareOfAMissingQueueThrowsAnIOExceptionWrappingA404ShutdownSignal() throws Exception { + try (Connection conn = LeadMailbox.connectionFactory(uri()).newConnection()) { + Channel probe = conn.createChannel(); + String missing = LeadMailbox.queueName(coordId("lead-404-shape")); + IOException thrown = assertThrows(IOException.class, () -> probe.queueDeclarePassive(missing)); + assertInstanceOf(ShutdownSignalException.class, thrown.getCause(), + () -> "expected the IOException to wrap a ShutdownSignalException, got: " + thrown); + ShutdownSignalException sse = (ShutdownSignalException) thrown.getCause(); + assertInstanceOf(AMQP.Channel.Close.class, sse.getReason(), + () -> "expected a Channel.Close reason: " + sse); + AMQP.Channel.Close close = (AMQP.Channel.Close) sse.getReason(); + assertEquals(404, close.getReplyCode(), () -> "expected AMQP NOT_FOUND (404): " + close); + assertFalse(probe.isOpen(), "the 404 must have closed the channel the declare ran on"); + } + } + + /** + * The other half of the same measurement: calling {@code createChannel()} on an + * already-closed connection — the shape {@link LeadMailbox#inspect} hits when the broker + * connection itself is gone — throws {@link com.rabbitmq.client.AlreadyClosedException}, an + * unchecked {@link RuntimeException}, not an {@link IOException}. An {@code inspect} that only + * caught {@code IOException} here would let this escape instead of reporting "unknown". + */ + @Test + void createChannelOnAnAlreadyClosedConnectionThrowsAnUncheckedException() throws Exception { + Connection conn = LeadMailbox.connectionFactory(uri()).newConnection(); + conn.close(); + RuntimeException thrown = assertThrows(RuntimeException.class, conn::createChannel); + assertInstanceOf(com.rabbitmq.client.AlreadyClosedException.class, thrown, + () -> "expected AlreadyClosedException, got: " + thrown); + } + /** Poll peek until at least one message is held, or ~10s elapse (broker delivery is async). */ @SuppressWarnings("BusyWait") private static List awaitPeek(LeadMailbox inbox) throws InterruptedException { @@ -170,4 +316,18 @@ class LeadMailboxTest { } return msgs; } + + /** Poll inspect(coordId) until it satisfies {@code done}, or ~10s elapse (broker state settles async). */ + @SuppressWarnings("BusyWait") + private static LeadChannel.MailboxState awaitInspect( + LeadMailbox observer, String coordId, java.util.function.Predicate done) + throws InterruptedException { + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); + LeadChannel.MailboxState state = observer.inspect(coordId); + while (!done.test(state) && System.nanoTime() < deadline) { + Thread.sleep(50); + state = observer.inspect(coordId); + } + return state; + } } 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 971c4f8..a08938f 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java @@ -12,6 +12,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.slf4j.LoggerFactory; +import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -1469,4 +1470,273 @@ class GitWorktreesTest { assertTrue(reportingAppender.list.isEmpty(), "a null/empty overlay must log nothing, got:\n" + capturedMessages()); } + + // ---- fleetd #362: seedSkills. Drives GitWorktrees#add end-to-end (not a bare worktree) so the + // real memberSkillsSource wiring is exercised, exactly like the credential-helper/origin tests + // above do for their own seams. ---- + + /** Write {@code content} as {@code

//SKILL.md}, creating {@code dir} first. */ + private static void writeSkill(Path dir, String skillName, String content) throws IOException { + Path skillFile = dir.resolve(skillName).resolve("SKILL.md"); + Files.createDirectories(skillFile.getParent()); + 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 + void seedSkillsCopiesIntoAWorktreeWithNoClaudeDirAtAll(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + Path skillsSource = tmp.resolve("skills-src"); + writeSkill(skillsSource, "implementer", "IMPLEMENTER SKILL\n"); + + String wt = seedingGitWorktrees(tmp.resolve("wts"), skillsSource.toString(), tmp) + .add(repo.toString(), "cb-362-fresh", "HEAD"); + + assertEquals("IMPLEMENTER SKILL\n", + Files.readString(Path.of(wt, ".claude", "skills", "implementer", "SKILL.md"))); + } + + /** Acceptance criterion 2 (part 2) / invariant 1: a repo that already ships its own {@code + * implementer} skill keeps it byte-for-byte — fleetd's copy is never written over it, even + * though the configured source also carries a same-named skill with different content. */ + @Test + void seedSkillsNeverOverwritesAReposOwnSkill(@TempDir Path tmp) throws Exception { + Path repo = tmp.resolve("repo"); + Files.createDirectories(repo); + git(repo, "init", "-q", "-b", "main"); + git(repo, "config", "user.email", "test@example.invalid"); + git(repo, "config", "user.name", "Test"); + writeSkill(repo.resolve(".claude/skills"), "implementer", "REPO OWN SKILL\n"); + git(repo, "add", ".claude"); + git(repo, "commit", "-q", "-m", "repo ships its own implementer skill"); + Path skillsSource = tmp.resolve("skills-src"); + writeSkill(skillsSource, "implementer", "FLEETD SKILL — must never land here\n"); + + String wt = new GitWorktrees(tmp.resolve("wts").toString(), null, skillsSource.toString()) + .add(repo.toString(), "cb-362-repo-own", "HEAD"); + + assertEquals("REPO OWN SKILL\n", + Files.readString(Path.of(wt, ".claude", "skills", "implementer", "SKILL.md")), + "the repo's own committed skill must survive untouched"); + } + + /** Acceptance criterion 2 (part 3) / invariant 3: a misconfigured or missing {@code + * memberSkillsSource} must never fail the spawn — the worktree is still created. */ + @Test + void seedSkillsIsBestEffortWhenSourceDoesNotExist(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + String missingSource = tmp.resolve("does-not-exist").toString(); + + String wt = new GitWorktrees(tmp.resolve("wts").toString(), null, missingSource) + .add(repo.toString(), "cb-362-missing-src", "HEAD"); + + assertTrue(Files.isDirectory(Path.of(wt)), "the spawn must still produce a worktree"); + assertFalse(Files.exists(Path.of(wt, ".claude", "skills")), + "nothing should be seeded when the source directory does not exist"); + assertTrue(capturedMessages().stream().anyMatch(m -> m.contains("is not a directory")), + "expected a warning naming the bad memberSkills source, got:\n" + capturedMessages()); + } + + /** Acceptance criterion 3: prove invariant 2 with a real git command — a freshly seeded skill + * must not appear in {@code git status --porcelain} for the worktree it was seeded into. */ + @Test + void seedSkillsHidesSeededPathsFromGitStatus(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + Path skillsSource = tmp.resolve("skills-src"); + writeSkill(skillsSource, "implementer", "IMPLEMENTER SKILL\n"); + + String wt = seedingGitWorktrees(tmp.resolve("wts"), skillsSource.toString(), tmp) + .add(repo.toString(), "cb-362-status", "HEAD"); + + assertEquals("", fullStatus(Path.of(wt)), + "a seeded skill must be invisible to git status, so it can never be staged or committed"); + } + + /** Invariant 2, the other direction: the exclude {@link #seedSkillsHidesSeededPathsFromGitStatus} + * proves is scoped to ONE worktree, not the whole repo. A second worktree of the same repo, + * provisioned with no {@code memberSkillsSource}, still reports an untracked {@code + * .claude/skills/} the ordinary way — proving the exclude did not leak in via the shared + * {@code .git/info/exclude} (which a linked worktree resolves to the repo's COMMON git dir). */ + @Test + void seedSkillsExcludeDoesNotLeakIntoASiblingWorktree(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + Path skillsSource = tmp.resolve("skills-src"); + writeSkill(skillsSource, "implementer", "IMPLEMENTER SKILL\n"); + 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"); + String plainWt = plain.add(repo.toString(), "cb-362-scope-b", "HEAD"); + // Simulate the same untracked shape landing in the sibling worktree by hand, since `plain` + // was never configured with a memberSkillsSource to seed it itself. + writeSkill(Path.of(plainWt, ".claude", "skills"), "implementer", "unrelated untracked content\n"); + + assertEquals("", fullStatus(Path.of(seededWt)), "seeded worktree stays clean"); + assertTrue(porcelainPaths(fullStatus(Path.of(plainWt))).contains(".claude/"), + "an unrelated worktree's own untracked .claude/ must still show up in its status — " + + "the seeded worktree's exclude must not have leaked into it, got:\n" + + fullStatus(Path.of(plainWt))); + } + + /** Criterion 2's log shape, mirroring the {@code overlayParity} log assertions above: the + * denominator, what was seeded, and what was kept because the repo already had it. */ + @Test + void seedSkillsLogsSeededAndKept(@TempDir Path tmp) throws Exception { + reportingLogger.setLevel(Level.INFO); + Path repo = tmp.resolve("repo"); + Files.createDirectories(repo); + git(repo, "init", "-q", "-b", "main"); + git(repo, "config", "user.email", "test@example.invalid"); + git(repo, "config", "user.name", "Test"); + writeSkill(repo.resolve(".claude/skills"), "hunter", "REPO OWN HUNTER\n"); + git(repo, "add", "."); + git(repo, "commit", "-q", "-m", "repo ships hunter only"); + Path skillsSource = tmp.resolve("skills-src"); + writeSkill(skillsSource, "hunter", "FLEETD HUNTER\n"); + writeSkill(skillsSource, "implementer", "FLEETD IMPLEMENTER\n"); + + seedingGitWorktrees(tmp.resolve("wts"), skillsSource.toString(), tmp) + .add(repo.toString(), "cb-362-log", "HEAD"); + + assertTrue(capturedMessages().stream().anyMatch(m -> + 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", + // 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"); + 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); + } + + /** + * 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); + } }