From 0df34f32204c9e5dfbb1fd8e0074ac1c338470b1 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 5 Sep 2026 12:44:29 +0700 Subject: [PATCH 1/6] fleetd #361: close the lead-coordination visibility gap Lead-to-lead AMQP coordination had a send half with tools and a receive half without. This closes three blind spots: - LeadChannel gains inspect(coordId) -> MailboxState(exists, pending, consumers), implemented in LeadMailbox with a throwaway probe channel (never the long-lived publish/consume channels) so a passive-declare 404 on a missing queue can never take down publish() on the same instance. - FleetConfig.Coordinator gains peers: List (defaults to empty, blank entries dropped) so a daemon can declare which peer coord-ids it expects to reach. - fleet_list reports coordination state via a new CoordinationSource (own coord-id, own mailbox state, held messages as msgId/from/preview only, and one row per configured peer with reachability/pending/ consumers), following the existing OutageSource/QuarantineSource "Source record with none()" idiom instead of growing listFleet's overload chain by another positional parameter. Every peer probe is bounded by a 1.5s timeout on a virtual-thread pool and degrades to absent rather than ever slowing or failing fleet_list. - fleet_send{coordId}'s success text now says "durably confirmed by the broker" instead of "delivered", and warns (while still reporting success) when the target mailbox has zero consumers attached. Tests: hermetic unit tests for exists/absent/zero-consumer/old-config- no-peers-key/new fleet_list shape using FakeLeadChannel, plus a @Tag("contract") LeadMailboxTest.inspectingAMissingMailboxNeverBreaks PublishOnTheSameInstance proving the invariant against a real broker. --- fleetd/fleetd.example.yaml | 7 + .../src/main/java/dev/ltms/fleet/Fleetd.java | 7 +- .../dev/ltms/fleet/config/FleetConfig.java | 8 +- .../java/dev/ltms/fleet/mcp/FleetMcp.java | 222 ++++++++++++++++-- .../java/dev/ltms/fleet/msg/LeadChannel.java | 40 +++- .../java/dev/ltms/fleet/msg/LeadMailbox.java | 35 +++ .../fleet/FleetdLeadMailboxSelectionTest.java | 10 +- ...onfigRefTopLevelReportingCoverageTest.java | 4 +- .../ltms/fleet/config/FleetConfigTest.java | 53 ++++- ...thDefaultsPreservesEveryComponentTest.java | 2 +- .../ltms/fleet/mcp/FleetMcpLeadCoordTest.java | 32 +++ .../java/dev/ltms/fleet/mcp/FleetMcpTest.java | 68 +++++- .../fleet/member/MemberEnvAllowListTest.java | 2 +- .../dev/ltms/fleet/msg/FakeLeadChannel.java | 15 ++ .../dev/ltms/fleet/msg/LeadMailboxTest.java | 91 +++++++ 15 files changed, 548 insertions(+), 48 deletions(-) diff --git a/fleetd/fleetd.example.yaml b/fleetd/fleetd.example.yaml index 5788a06..9295804 100644 --- a/fleetd/fleetd.example.yaml +++ b/fleetd/fleetd.example.yaml @@ -821,10 +821,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..b8b7283 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -652,7 +652,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/FleetConfig.java b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java index ef96eb1..452743c 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java @@ -888,12 +888,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. */ 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..8609276 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,102 @@ 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); + LeadChannel.MailboxState self = probe(channel, selfId); + Map mailbox = new LinkedHashMap<>(); + mailbox.put("pending", self.pending()); + mailbox.put("consumers", self.consumers()); + row.put("mailbox", mailbox); + row.put("held", channel.peek().stream().map(FleetMcp::heldView).toList()); + row.put("peers", coordination.peers().stream().map(p -> peerView(channel, p)).toList()); + 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, whether its mailbox exists, and — only then — its counts. */ + private static Map peerView(LeadChannel channel, String coordId) { + LeadChannel.MailboxState state = probe(channel, coordId); + Map row = new LinkedHashMap<>(); + row.put("coordId", coordId); + row.put("reachable", state.exists()); + if (state.exists()) { + row.put("pending", state.pending()); + row.put("consumers", state.consumers()); + } + 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}. + */ + 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#absent} 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. + */ + private static LeadChannel.MailboxState probe(LeadChannel channel, String coordId) { + try { + return PEER_PROBE_POOL.submit(() -> channel.inspect(coordId)) + .get(PEER_PROBE_TIMEOUT_MS, TimeUnit.MILLISECONDS); + } catch (Exception e) { + return LeadChannel.MailboxState.absent(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 +1537,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 +1650,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..7bdef7b 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,40 @@ 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. + * + *

Returns {@link MailboxState#absent(String)}, never throws, when {@code coordId}'s mailbox + * does not exist or the look otherwise fails (broker unreachable, timed out) — this is a + * best-effort fact-finding call, not an operation a caller must handle failing. + * + *

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 exists} is {@code false} for a mailbox nobody has ever + * declared (or when the look could not be completed) — {@code pending}/{@code consumers} are + * meaningless in that case and always read {@code 0}. + * + * @param coordId the coord-id inspected + * @param exists whether the mailbox's queue is currently declared on the broker + * @param pending messages ready for delivery but not yet in a consumer's hands (0 when absent) + * @param consumers how many consumers are attached (0 when absent, or when nobody is reading it) + */ + record MailboxState(String coordId, boolean exists, int pending, int consumers) { + /** The mailbox does not exist, or the look could not be completed. */ + public static MailboxState absent(String coordId) { + return new MailboxState(coordId, false, 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..335c3e2 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java +++ b/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java @@ -259,6 +259,41 @@ 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. + */ + @Override + public MailboxState inspect(String coordId) { + String queue = queueName(coordId); + Channel probe; + try { + probe = connection.createChannel(); + } catch (IOException e) { + log.debug("lead mailbox inspect: cannot open a probe channel for {}: {}", coordId, e.toString()); + return MailboxState.absent(coordId); + } + try { + AMQP.Queue.DeclareOk declared = probe.queueDeclarePassive(queue); + return new MailboxState(coordId, true, declared.getMessageCount(), declared.getConsumerCount()); + } catch (IOException e) { + // Missing queue (404) or any other declare failure: the broker (or the client library) + // has already closed `probe` for us — report "does not exist" rather than throw. + return MailboxState.absent(coordId); + } finally { + try { + if (probe.isOpen()) { + probe.close(); + } + } catch (Exception e) { + log.debug("lead mailbox inspect: probe channel close for {}: {}", coordId, e.toString()); + } + } + } + /** Convenience: {@link #peek} the current snapshot, then {@link #ack} every message in it. */ public List drain() { List snapshot = peek(); 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..030258e 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java @@ -104,7 +104,7 @@ 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); assertNamesMatchComponents(v); @@ -144,7 +144,7 @@ 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); assertNamesMatchComponents(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..7bbfc3a 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigWithDefaultsPreservesEveryComponentTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigWithDefaultsPreservesEveryComponentTest.java @@ -92,7 +92,7 @@ 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"); assertNamesMatchComponents(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..d0986ab 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,35 @@ 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, new LeadChannel.MailboxState(PEER, true, 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, new LeadChannel.MailboxState(PEER, true, 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); + } } 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..1c90883 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; @@ -576,17 +579,23 @@ 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", new LeadChannel.MailboxState("mac-opus", true, 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\":{\"pending\":0,\"consumers\":1}"), out); + assertTrue(out.contains("\"held\":[]"), out); + assertTrue(out.contains("\"peers\":[]"), out); } @Test @@ -601,6 +610,49 @@ 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", new LeadChannel.MailboxState("fleet01-lead", true, 2, 1)); + // "fleet02-lead" is declared as a peer but never configured on the fake — inspect() falls + // back to MailboxState.absent, exactly as a real down peer would report. + + 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"))); + + String out = textOf(res); + assertTrue(out.contains("\"coordId\":\"fleet01-lead\",\"reachable\":true,\"pending\":2,\"consumers\":1"), out); + assertTrue(out.contains("\"coordId\":\"fleet02-lead\",\"reachable\":false"), out); + assertFalse(out.contains("\"coordId\":\"fleet02-lead\",\"reachable\":false,\"pending\""), + "pending/consumers must be omitted, not faked as zero, for an unreachable peer: " + out); + } + @Test void capacityUsesThePlacementLiveCount() { FakeHerdr h = new FakeHerdr(); @@ -911,7 +963,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 +987,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 +1042,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 +1071,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/LeadMailboxTest.java b/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxTest.java index bc015ac..6bf4915 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxTest.java @@ -12,6 +12,7 @@ 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.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -159,6 +160,82 @@ 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"); + } + } + + @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()); + + // 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()); + } + } + /** 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 +247,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; + } } From 7c684e40d33fafb99e36c8daa9d606b421164251 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 5 Sep 2026 12:54:01 +0700 Subject: [PATCH 2/6] fleetd #362 (item 3): seed .claude/skills/ into provisioned worktrees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add memberSkills:

to FleetConfig. GitWorktrees#add copies each skill folder from that directory into /.claude/skills/ so a member spawned against ANY repo — not only one that already ships its own skills — can load a bridge skill (e.g. implementer). A skill the target repo already carries is never overwritten. Every seeded path is hidden from `git status` in that worktree ONLY, via a --worktree-scoped core.excludesFile pointing at a file under the worktree's own private git dir (outside the working tree, so it can never be committed) — not the shared .git/info/exclude, which a linked worktree resolves to the repo's common git dir and would otherwise leak visibility changes into the primary checkout and every sibling worktree. Proven with a real `git status --porcelain` in GitWorktreesTest, not by reasoning. Seeding is best-effort like the existing overlayParity/isolateToolSurface steps: a missing/unreadable source or a copy/exclude failure is logged and skipped, never fails the spawn. memberSkills is triaged as a DEFERRED config key in ConfigRef (baked once into GitWorktrees at startup, like worktreeGroup), with its own changedDeferredKeys branch and coverage-test entries. --- fleetd/fleetd.example.yaml | 11 + .../src/main/java/dev/ltms/fleet/Fleetd.java | 3 +- .../java/dev/ltms/fleet/config/ConfigRef.java | 25 ++- .../dev/ltms/fleet/config/FleetConfig.java | 34 ++- .../dev/ltms/fleet/session/GitWorktrees.java | 207 +++++++++++++++++- ...onfigRefTopLevelReportingCoverageTest.java | 2 + ...thDefaultsPreservesEveryComponentTest.java | 5 +- .../ltms/fleet/session/GitWorktreesTest.java | 134 ++++++++++++ 8 files changed, 403 insertions(+), 18 deletions(-) diff --git a/fleetd/fleetd.example.yaml b/fleetd/fleetd.example.yaml index 5788a06..32b0ced 100644 --- a/fleetd/fleetd.example.yaml +++ b/fleetd/fleetd.example.yaml @@ -774,6 +774,17 @@ 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. +# 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) diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 6058535..669bda6 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)); 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..2ac430c 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,16 @@ 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. */ @JsonIgnoreProperties(ignoreUnknown = true) public record FleetConfig( @@ -130,7 +140,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, @@ -1495,7 +1520,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 +2197,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/session/GitWorktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java index 1da0f95..0bbf464 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,9 @@ 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; 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 +128,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 +151,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 +174,15 @@ 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 = configuredRoot; this.group = (group == null || group.isBlank()) ? null : group; + this.memberSkillsSource = (memberSkillsSource == null || memberSkillsSource.isBlank()) + ? null : memberSkillsSource; this.afterWorktreeAdded = afterWorktreeAdded == null ? _ -> {} : afterWorktreeAdded; this.shareGroupRunner = shareGroupRunner != null ? shareGroupRunner : this::exec; this.worktreeAddRunner = worktreeAddRunner != null ? worktreeAddRunner : this::exec; @@ -184,6 +211,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 +602,177 @@ 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. + * + *

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

This replaces any {@code --worktree}-scoped {@code core.excludesFile} this worktree already + * had — acceptable because a freshly provisioned worktree has none, and worktree-scoped git + * config is already used exclusively for fleetd's own isolation (the credential helper, the SSH + * rewrite) rather than anything an operator sets by hand. + */ + private void excludeSeededSkillsFromGitStatus(String worktreePath, List seededSkillNames) { + exec("git", "-C", worktreePath, "config", "extensions.worktreeConfig", "true"); + 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(); + 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 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); 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..f08ce0f 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java @@ -107,6 +107,7 @@ class ConfigRefTopLevelReportingCoverageTest { v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-a", null, "self-a", 1)); v.put("worktreeGroup", "group-a"); v.put("memberLoginShell", null); + v.put("memberSkills", "/skills/a"); assertNamesMatchComponents(v); return v; } @@ -147,6 +148,7 @@ class ConfigRefTopLevelReportingCoverageTest { v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-b", null, "self-b", 2)); 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/FleetConfigWithDefaultsPreservesEveryComponentTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigWithDefaultsPreservesEveryComponentTest.java index 1f710a0..9f03f08 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. @@ -95,6 +95,7 @@ class FleetConfigWithDefaultsPreservesEveryComponentTest { v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-guard", null, "self-guard", 3)); 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/session/GitWorktreesTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java index 971c4f8..a2cb113 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,137 @@ 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); + } + + /** 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 = new GitWorktrees(tmp.resolve("wts").toString(), null, skillsSource.toString()) + .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 = new GitWorktrees(tmp.resolve("wts").toString(), null, skillsSource.toString()) + .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 = new GitWorktrees(tmp.resolve("wts").toString(), null, skillsSource.toString()); + 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"); + + new GitWorktrees(tmp.resolve("wts").toString(), null, skillsSource.toString()) + .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()); + } } From c4d40fbc2b15f46df20c3b3862f899bd81d693b1 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 5 Sep 2026 13:01:48 +0700 Subject: [PATCH 3/6] fleetd #361 review: fix false-negative absent, uncancelled probes, and a throw contract gap Three findings from review of #364, fixed on the same branch: 1. LeadChannel.MailboxState.absent() was returned both for a genuinely absent mailbox AND for "the probe could not determine anything" (timeout, unreachable broker, other declare failure) -- exactly the overstatement #361 exists to fix, one level down. MailboxState now carries a Presence enum (EXISTS/ABSENT/UNKNOWN) with exists()/known() accessors; LeadMailbox.inspect classifies a real AMQP 404 (measured against a live broker, not assumed: an IOException wrapping a ShutdownSignalException whose Channel.Close reply code is 404) as ABSENT and everything else as UNKNOWN. fleet_list's mailbox/peer rows now render a "status" of exists/absent/unknown and only include pending/consumers when status is "exists", so an unresolved self- or peer-probe can never render as a measured zero. 2. FleetMcp.probe's get(timeoutMs) left a timed-out inspect() task running forever on its own virtual thread, holding the AMQP channel it had already opened -- against a hung (not down) broker this would orphan one channel per fleet_list call until the connection's channel-max was exhausted, breaking publish() too. probe() now holds the Future and calls cancel(true) on timeout/failure so the orphaned task is interrupted instead of abandoned, and now returns MailboxState.unknown() (never absent()) on timeout/exception. 3. LeadMailbox.inspect only caught IOException, but createChannel() on an already-closed connection throws AlreadyClosedException, an unchecked RuntimeException (measured against a live broker) -- so it could escape the "never throws" contract. Both places in inspect now also catch RuntimeException and report unknown(). Tests: MailboxState.exists()/absent()/unknown() call sites updated across FleetMcpTest/FleetMcpLeadCoordTest; new hermetic tests cover the tri-state fleet_list rendering (self-probe unknown, a peer that's absent vs. one that's unknown) and probe cancellation (a LeadChannel fake that blocks until interrupted, proving probe() doesn't just give up on it); new @Tag("contract") LeadMailboxTest cases pin the real exception shapes for both the 404 and the already-closed-connection paths and prove inspect() reports unknown (never throws) when the connection is already closed. --- .../java/dev/ltms/fleet/mcp/FleetMcp.java | 78 +++++++++---- .../java/dev/ltms/fleet/msg/LeadChannel.java | 62 ++++++++-- .../java/dev/ltms/fleet/msg/LeadMailbox.java | 48 +++++++- .../ltms/fleet/mcp/FleetMcpLeadCoordTest.java | 25 ++++- .../java/dev/ltms/fleet/mcp/FleetMcpTest.java | 106 ++++++++++++++++-- .../dev/ltms/fleet/msg/LeadMailboxTest.java | 69 ++++++++++++ 6 files changed, 339 insertions(+), 49 deletions(-) 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 8609276..7b35930 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -1273,16 +1273,32 @@ public final class FleetMcp { Map row = new LinkedHashMap<>(); row.put("selfId", selfId); row.put("configured", true); - LeadChannel.MailboxState self = probe(channel, selfId); - Map mailbox = new LinkedHashMap<>(); - mailbox.put("pending", self.pending()); - mailbox.put("consumers", self.consumers()); - row.put("mailbox", mailbox); + 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<>(); @@ -1304,16 +1320,17 @@ public final class FleetMcp { : content.substring(0, HELD_PREVIEW_MAX_CHARS) + "…"; } - /** One declared peer's row: its coord-id, whether its mailbox exists, and — only then — its counts. */ + /** + * 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) { - LeadChannel.MailboxState state = probe(channel, coordId); Map row = new LinkedHashMap<>(); row.put("coordId", coordId); - row.put("reachable", state.exists()); - if (state.exists()) { - row.put("pending", state.pending()); - row.put("consumers", state.consumers()); - } + row.putAll(mailboxView(probe(channel, coordId))); return row; } @@ -1325,7 +1342,10 @@ public final class FleetMcp { /** * 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}. + * 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()); @@ -1333,17 +1353,35 @@ public final class FleetMcp { /** * 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#absent} 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. + * 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 PEER_PROBE_POOL.submit(() -> channel.inspect(coordId)) - .get(PEER_PROBE_TIMEOUT_MS, TimeUnit.MILLISECONDS); + return future.get(timeoutMs, TimeUnit.MILLISECONDS); } catch (Exception e) { - return LeadChannel.MailboxState.absent(coordId); + future.cancel(true); // best-effort: don't leave a hung probe (and its channel) running forever + return LeadChannel.MailboxState.unknown(coordId); } } 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 7bdef7b..db16aa3 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/msg/LeadChannel.java +++ b/fleetd/src/main/java/dev/ltms/fleet/msg/LeadChannel.java @@ -44,9 +44,14 @@ public interface LeadChannel { * 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. * - *

Returns {@link MailboxState#absent(String)}, never throws, when {@code coordId}'s mailbox - * does not exist or the look otherwise fails (broker unreachable, timed out) — this is a - * best-effort fact-finding call, not an operation a caller must handle failing. + *

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 @@ -58,19 +63,52 @@ public interface LeadChannel { MailboxState inspect(String coordId); /** - * The result of {@link #inspect}. {@code exists} is {@code false} for a mailbox nobody has ever - * declared (or when the look could not be completed) — {@code pending}/{@code consumers} are - * meaningless in that case and always read {@code 0}. + * 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 exists whether the mailbox's queue is currently declared on the broker - * @param pending messages ready for delivery but not yet in a consumer's hands (0 when absent) - * @param consumers how many consumers are attached (0 when absent, or when nobody is reading it) + * @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, boolean exists, int pending, int consumers) { - /** The mailbox does not exist, or the look could not be completed. */ + 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, false, 0, 0); + 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 335c3e2..f9da75e 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; @@ -265,6 +266,20 @@ public final class LeadMailbox implements LeadChannel, AutoCloseable { * 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: + *

    + *
  • a genuine 404 — an {@link IOException} wrapping a {@link ShutdownSignalException} whose + * {@link AMQP.Channel.Close#getReplyCode()} is {@code 404} — reports + * {@link MailboxState#absent}; every other declare failure reports + * {@link MailboxState#unknown} instead of quietly becoming the same "absent" value; + *
  • {@code catch (RuntimeException e)} on both attempts matters as much as the checked + * catches: a connection that is already closed makes {@link Connection#createChannel()} + * throw {@link com.rabbitmq.client.AlreadyClosedException} (a {@link RuntimeException}, + * not an {@link IOException}) — an {@code inspect} that only caught {@code IOException} + * would let that escape, breaking the "never throws" contract this method promises. + *
*/ @Override public MailboxState inspect(String coordId) { @@ -272,17 +287,22 @@ public final class LeadMailbox implements LeadChannel, AutoCloseable { Channel probe; try { probe = connection.createChannel(); - } catch (IOException e) { + } catch (IOException | RuntimeException e) { log.debug("lead mailbox inspect: cannot open a probe channel for {}: {}", coordId, e.toString()); - return MailboxState.absent(coordId); + return MailboxState.unknown(coordId); } try { AMQP.Queue.DeclareOk declared = probe.queueDeclarePassive(queue); - return new MailboxState(coordId, true, declared.getMessageCount(), declared.getConsumerCount()); + return MailboxState.exists(coordId, declared.getMessageCount(), declared.getConsumerCount()); } catch (IOException e) { - // Missing queue (404) or any other declare failure: the broker (or the client library) - // has already closed `probe` for us — report "does not exist" rather than throw. - return MailboxState.absent(coordId); + // 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()) { @@ -294,6 +314,22 @@ public final class LeadMailbox implements LeadChannel, AutoCloseable { } } + /** + * {@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, no {@code ShutdownSignalException} cause, or none at all) is a declare failure of + * some other kind and must not be read as "confirmed absent". + */ + private 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/test/java/dev/ltms/fleet/mcp/FleetMcpLeadCoordTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpLeadCoordTest.java index d0986ab..2384c1c 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpLeadCoordTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpLeadCoordTest.java @@ -103,7 +103,7 @@ class FleetMcpLeadCoordTest { @Test void warnsWhenThePeerMailboxHasNoConsumersButStillReportsSuccess() { var channel = new FakeLeadChannel(SELF) - .withMailbox(PEER, new LeadChannel.MailboxState(PEER, true, 0, 0)); + .withMailbox(PEER, LeadChannel.MailboxState.exists(PEER, 0, 0)); McpSchema.CallToolResult res = FleetMcp.sendToLead(channel, PEER, "hi", null, null); @@ -116,7 +116,7 @@ class FleetMcpLeadCoordTest { @Test void staysQuietAboutConsumersWhenThePeerMailboxHasOne() { var channel = new FakeLeadChannel(SELF) - .withMailbox(PEER, new LeadChannel.MailboxState(PEER, true, 0, 1)); + .withMailbox(PEER, LeadChannel.MailboxState.exists(PEER, 0, 1)); McpSchema.CallToolResult res = FleetMcp.sendToLead(channel, PEER, "hi", null, null); @@ -125,4 +125,25 @@ class FleetMcpLeadCoordTest { 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 1c90883..554d767 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java @@ -37,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.*; @@ -580,7 +582,7 @@ class FleetMcpTest { 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", new LeadChannel.MailboxState("mac-opus", true, 0, 1)); + .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, @@ -593,11 +595,36 @@ class FleetMcpTest { // 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\":{\"pending\":0,\"consumers\":1}"), 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 void listOmitsTheCoordinatorRowWhenLeadCoordinationIsOff() { FakeHerdr h = new FakeHerdr(); @@ -636,21 +663,82 @@ class FleetMcpTest { 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", new LeadChannel.MailboxState("fleet01-lead", true, 2, 1)); + .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 peer would report. + // 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"))); + new FleetMcp.CoordinationSource(channel, List.of("fleet01-lead", "fleet02-lead", "fleet03-lead"))); String out = textOf(res); - assertTrue(out.contains("\"coordId\":\"fleet01-lead\",\"reachable\":true,\"pending\":2,\"consumers\":1"), out); - assertTrue(out.contains("\"coordId\":\"fleet02-lead\",\"reachable\":false"), out); - assertFalse(out.contains("\"coordId\":\"fleet02-lead\",\"reachable\":false,\"pending\""), - "pending/consumers must be omitted, not faked as zero, for an unreachable peer: " + out); + 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 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 6bf4915..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,12 +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; @@ -181,9 +187,32 @@ class LeadMailboxTest { 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 @@ -220,6 +249,7 @@ class LeadMailboxTest { // 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(). @@ -236,6 +266,45 @@ class LeadMailboxTest { } } + /** + * 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 { From f84824ee299030f4c012cd27599a37b1ca0f05c4 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 5 Sep 2026 13:10:27 +0700 Subject: [PATCH 4/6] fleetd #362 review fix: compose skill-seeding excludes with the operator's own excludesFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit core.excludesFile is single-valued, so pointing it at fleetd's own seeded-skill exclude file with --replace-all at worktree scope was SHADOWING whatever excludesFile the worktree already resolved (an operator's global config, most commonly) instead of adding to it. This repo's own .gitignore does not ignore target/ — only an operator's global excludesFile does — so every worker's `mvn clean install` would make target/ show up as untracked, and CB-576's deliberately untracked-inclusive hasUncommitted would then read every such worktree as dirty forever, so it is never cleaned up. excludeSeededSkillsFromGitStatus now reads whatever core.excludesFile resolves to BEFORE writing anything (falling back to git's own $XDG_CONFIG_HOME/git/ignore default when the key is unset entirely, per gitignore(5)), and writes that content into fleetd's own exclude file ahead of the seeded skill patterns, so every operator-configured pattern keeps applying inside the seeded worktree. Proven with a new test, seedSkillsComposesWithAnAlreadyEffectiveGlobalExcludesFile, which isolates a synthetic "operator's global config" via a new gitEnv test seam on GitWorktrees (GIT_CONFIG_GLOBAL pointed at a throwaway temp file, never the real machine's config) and drives the real add() path end to end. Also documents (FleetConfig javadoc + fleetd.example.yaml) that memberSkills copies every non-hidden subdirectory of its source wholesale, with no per-file allowlist. --- fleetd/fleetd.example.yaml | 4 +- .../dev/ltms/fleet/config/FleetConfig.java | 5 +- .../dev/ltms/fleet/session/GitWorktrees.java | 123 +++++++++++++++++- .../ltms/fleet/session/GitWorktreesTest.java | 50 +++++++ 4 files changed, 175 insertions(+), 7 deletions(-) diff --git a/fleetd/fleetd.example.yaml b/fleetd/fleetd.example.yaml index 32b0ced..6522753 100644 --- a/fleetd/fleetd.example.yaml +++ b/fleetd/fleetd.example.yaml @@ -782,7 +782,9 @@ guard: # .claude/skills/ is never overwritten — the repo's own copy always wins. Claude Code # members only; an opencode member reads a different path (.opencode/agent) this key does not # touch. Best-effort like worktreeGroup above: a missing/unreadable directory here is logged and -# skipped, never a failed spawn. +# skipped, never a failed spawn. Every non-hidden subdirectory of this directory is copied +# wholesale, with no per-file allowlist — don't park scratch files or drafts alongside the real +# skill folders, they will be copied into every provisioned worktree too. # memberSkills: /path/to/fleetd/checkout/.claude/skills # Session lifecycle limits (CB-303). All knobs are opt-in; omit or set to null to keep diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java index 2ac430c..74cc6dd 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java @@ -115,7 +115,10 @@ import java.util.regex.PatternSyntaxException; * today's behaviour. A skill folder the target repo already carries is never * overwritten — see {@link dev.ltms.fleet.session.GitWorktrees}. Claude Code * members only; an opencode member's equivalent lives under a different path - * ({@code .opencode/agent}) and is not covered by this key. + * ({@code .opencode/agent}) and is not covered by this key. Every non-hidden + * subdirectory of this directory is copied wholesale, with no per-file + * allowlist — do not park scratch files or drafts alongside the real skill + * folders, they will be copied into every provisioned worktree too. */ @JsonIgnoreProperties(ignoreUnknown = true) public record FleetConfig( diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java index 0bbf464..0f232b5 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -95,6 +95,12 @@ public final class GitWorktrees implements Worktrees { /** Source directory of skill folders for {@link #seedSkills} (fleetd #362, {@code memberSkills:} * in config); {@code null} ⇒ feature off, no worktree is touched beyond today's behaviour. */ private final String memberSkillsSource; + /** Extra environment merged into every {@code git} subprocess this instance runs. Always {@code + * Map.of()} from every production constructor. Test seam only (fleetd #362 review fix): lets + * {@code GitWorktreesTest} point {@code GIT_CONFIG_GLOBAL} at an isolated temp file so it can + * drive the real {@link #add} path against a controlled "operator's global git config" and + * prove the excludesFile composition below without ever touching the real machine's config. */ + private final Map gitEnv; private final Consumer afterWorktreeAdded; /** How the initial {@code git worktree add} command runs. Package-private test seam for an * interrupted command after Git has made worktree state. */ @@ -179,10 +185,25 @@ public final class GitWorktrees implements Worktrees { GitWorktrees(String configuredRoot, String group, Consumer afterWorktreeAdded, Function shareGroupRunner, Function worktreeAddRunner, String memberSkillsSource) { + this(configuredRoot, group, afterWorktreeAdded, shareGroupRunner, worktreeAddRunner, + memberSkillsSource, Map.of()); + } + + /** + * Full test seam, plus {@code gitEnv} (fleetd #362 review fix, verification only): extra + * environment merged into every {@code git} subprocess this instance runs, so a test can isolate + * something like {@code GIT_CONFIG_GLOBAL} from the real machine while still driving the real + * {@link #add} path end to end. Every production constructor above delegates here with {@code + * Map.of()}. + */ + GitWorktrees(String configuredRoot, String group, Consumer afterWorktreeAdded, + Function shareGroupRunner, Function worktreeAddRunner, + String memberSkillsSource, Map gitEnv) { this.configuredRoot = configuredRoot; this.group = (group == null || group.isBlank()) ? null : group; this.memberSkillsSource = (memberSkillsSource == null || memberSkillsSource.isBlank()) ? null : memberSkillsSource; + this.gitEnv = gitEnv == null ? Map.of() : gitEnv; this.afterWorktreeAdded = afterWorktreeAdded == null ? _ -> {} : afterWorktreeAdded; this.shareGroupRunner = shareGroupRunner != null ? shareGroupRunner : this::exec; this.worktreeAddRunner = worktreeAddRunner != null ? worktreeAddRunner : this::exec; @@ -634,6 +655,32 @@ public final class GitWorktrees implements Worktrees { * worktree or the primary checkout. Proven with a real {@code git status --porcelain} in * {@code GitWorktreesTest}, not by reasoning. * + *

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

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

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

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

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

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

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

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

A synthetic "operator's global git config" is isolated via {@code GIT_CONFIG_GLOBAL} + * pointed at a throwaway temp file, passed to {@link GitWorktrees} through its {@code gitEnv} + * test seam — never the real machine's own git config. That global config ignores {@code + * target}. A skill is then seeded through the real {@link GitWorktrees#add} path, and a file + * named {@code target} is written into the worktree afterward: {@code git status --porcelain} + * must still be empty, proving the operator's own global pattern kept applying after seeding. + */ + @Test + void seedSkillsComposesWithAnAlreadyEffectiveGlobalExcludesFile(@TempDir Path tmp) throws Exception { + Path globalExcludes = tmp.resolve("operator-global-ignore"); + Files.writeString(globalExcludes, "target\n"); + Path globalConfig = tmp.resolve("operator-global.gitconfig"); + Files.writeString(globalConfig, "[core]\n\texcludesFile = " + globalExcludes + "\n"); + Map gitEnv = Map.of( + "GIT_CONFIG_GLOBAL", globalConfig.toString(), + "GIT_CONFIG_SYSTEM", "/dev/null", + "GIT_TERMINAL_PROMPT", "0"); + + Path repo = initRepo(tmp.resolve("repo")); + Path skillsSource = tmp.resolve("skills-src"); + writeSkill(skillsSource, "implementer", "IMPLEMENTER SKILL\n"); + GitWorktrees gitWorktrees = new GitWorktrees(tmp.resolve("wts").toString(), null, _ -> {}, + null, null, skillsSource.toString(), gitEnv); + + String wt = gitWorktrees.add(repo.toString(), "cb-362-global-compose", "HEAD"); + assertEquals("IMPLEMENTER SKILL\n", + Files.readString(Path.of(wt, ".claude", "skills", "implementer", "SKILL.md")), + "fixture check — the skill really was seeded"); + + Files.writeString(Path.of(wt, "target"), "build output the operator's global config ignores\n"); + + String porcelain = fullStatus(Path.of(wt)); + assertEquals("", porcelain, + "the operator's own global excludesFile pattern ('target') must still apply after " + + "skill seeding ran — got:\n" + porcelain); + } } From 01492059d44621ecddaaab6c3dff881faa015133 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 5 Sep 2026 13:12:24 +0700 Subject: [PATCH 5/6] fleetd #361 review round 2: pin isMissingQueue's false branch The reviewer's mutation (isMissingQueue always returns true) restored the exact overstatement fleetd #361 exists to fix -- every declare failure reading as a confirmed absence -- and still left mvn clean install green (1389/1389), because no test drove a non-404 shape through inspect(). The false branch was the whole discriminator between MailboxState.absent() and MailboxState.unknown(), unpinned. Widened LeadMailbox.isMissingQueue from private to package-private and added LeadMailboxIsMissingQueueTest: five hermetic tests (no broker) covering the true case and all three false shapes isMissingQueue's own javadoc lists -- a different reply code, a ShutdownSignalException whose reason isn't a Channel.Close, and an IOException with no such cause at all (plus an IOException wrapping an unrelated exception type). Re-ran the reviewer's exact mutation locally: 4 of 5 new tests went red with the expected assertion messages; reverted, and mvn clean install is green again at 1394/1394 (1389 + 5 new). Also added a one-line javadoc note on LeadMailbox.inspect being honest about which of its two RuntimeException catches is proven by a test (the createChannel() one, end-to-end against a real broker) and which stays purely defensive (the declare-site one, for a connection-drops- mid-call race no test drives on purpose). --- .../java/dev/ltms/fleet/msg/LeadMailbox.java | 19 ++++- .../msg/LeadMailboxIsMissingQueueTest.java | 77 +++++++++++++++++++ 2 files changed, 93 insertions(+), 3 deletions(-) create mode 100644 fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxIsMissingQueueTest.java 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 f9da75e..a2f3b64 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java +++ b/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java @@ -280,6 +280,16 @@ public final class LeadMailbox implements LeadChannel, AutoCloseable { * not an {@link IOException}) — an {@code inspect} that only caught {@code IOException} * would let that escape, breaking the "never throws" contract this method promises. * + * + *

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) { @@ -320,10 +330,13 @@ public final class LeadMailbox implements LeadChannel, AutoCloseable { * 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, no {@code ShutdownSignalException} cause, or none at all) is a declare failure of - * some other kind and must not be read as "confirmed absent". + * 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. */ - private static boolean isMissingQueue(IOException e) { + static boolean isMissingQueue(IOException e) { if (!(e.getCause() instanceof ShutdownSignalException sse)) { return false; } 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"); + } +} From 9e813ec17963e7f0f17a2e4cbaf936861408b4c1 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 5 Sep 2026 13:23:39 +0700 Subject: [PATCH 6/6] fleetd #362 review fix 2: route the XDG excludesFile fallback through gitEnv too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1 (lead): the XDG fallback branch of previouslyEffectiveExcludesFileContent was unpinned — deleting it left the suite green (Tests run: 1386, Failures: 0). Added seedSkillsComposesWithTheXdgDefaultExcludesFileWhenNoneIsConfigured to pin it: isolates XDG_CONFIG_HOME via the gitEnv seam at a temp dir carrying a synthetic git/ignore, points GIT_CONFIG_GLOBAL at an empty file so core.excludesFile is genuinely unset (forcing the fallback branch), seeds a skill, and asserts a file matching the XDG-default pattern still reads as clean. Reverting the fix (mutating the fallback to resolve to "") turns this test red with a real pasted failure (see PR body): "expected: <> but was: ". Finding 2 (lead, the one that actually needed a code fix): the fallback read XDG_CONFIG_HOME and HOME straight from the JVM's own environment, not through the gitEnv seam every git subprocess in this class already honours — so no test could isolate it, and on a machine carrying a real ~/.config/git/ignore (this dev machine does), every seeding test silently composed with that real file. Added resolveEnv/resolveHome, which check gitEnv first and fall back to the JVM's real environment only when the seam doesn't supply a value (production behaviour, where gitEnv is always Map.of(), is unchanged). Added a hermeticGitEnv() test helper and routed every seeding test in GitWorktreesTest through it, so no test in the class can reach the real machine's home directory for this fallback. Also documents two non-defects the lead asked for one javadoc line each on: the composed excludesFile is a snapshot taken at seed time, not a live reference to the operator's file; and excludeSeededSkillsFromGitStatus assumes a fresh worktree (not idempotent, but the double-seed path does not exist today, so no guard was added for it). --- .../dev/ltms/fleet/session/GitWorktrees.java | 52 +++++++++- .../ltms/fleet/session/GitWorktreesTest.java | 96 ++++++++++++++++++- 2 files changed, 141 insertions(+), 7 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java index 0f232b5..af80e13 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -790,6 +790,15 @@ public final class GitWorktrees implements Worktrees { * fleetd's own exclude file ahead of the seeded skill patterns, and the worktree-scoped override * then points at that combined file — so every pattern the operator's own configuration already * applied keeps applying, plus the seeded skill paths. + * + *

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

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

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

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