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 e351f8a..f2d5144 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -426,7 +426,8 @@ public final class FleetMcp { return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, outage, leadSeats, callers == null ? Map.of() : callers.leads(), callerTerminal(exchange), - new CoordinationSource(leadChannel, peers)); + new CoordinationSource(leadChannel, peers), + coordinatorVisibleTo(principal(exchange))); }; BiFunction stopHandler = (exchange, req) -> { @@ -579,6 +580,20 @@ public final class FleetMcp { } } + /** + * fleetd #439: only the primary may read {@code fleet_list}'s {@code coordinator} row — + * lead-to-lead coordination state (coord-ids, mailbox facts, held-message previews), never the + * roster. Split out of the {@code fleet_list} handler, same reason as {@link #denyFor} and + * {@link #recordPrimarySingleton}: the decision must be unit-testable without fabricating an + * SDK {@code McpSyncServerExchange}, and the handler must call this named predicate rather than + * inlining the check, so a future edit cannot silently pass a literal instead of asking who + * called ({@code FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCoordinatorVisibleTo} + * reads the source and asserts the handler calls this method by name, not a literal). + */ + static boolean coordinatorVisibleTo(Principal caller) { + return caller.isPrimary(); + } + /** The worker identity resolved from this call's connection, or {@code null} if the primary. */ private static String callerTerminal(McpSyncServerExchange exchange) { Object v = exchange.transportContext().get(CALLER_TERMINAL); @@ -1344,12 +1359,44 @@ public final class FleetMcp { LeadSeatSource.none(), leads, selfTerm, coordination); } - /** As above, plus fleetd #176 lead-seat facts (see {@link LeadSeatSource}). */ + /** + * As above, plus fleetd #176 lead-seat facts (see {@link LeadSeatSource}). + * + *

Assumes the caller is the primary — every wrapper overload above delegates here without + * carrying a caller identity, which is exactly right for them: they exist for call sites (and + * unit tests) that have no {@link Principal} to hand over, and this preserves their pre-#439 + * behavior unchanged. The one call site that has a real caller ({@code fleet_list}'s MCP + * handler) uses {@link #listFleet(PeerLauncher, SessionManager, MessageService, CapacitySource, + * HealthCoverageSource, QuarantineSource, OutageSource, LeadSeatSource, Map, String, + * CoordinationSource, boolean)} instead, so it can pass the true answer. + */ static McpSchema.CallToolResult listFleet(PeerLauncher workers, SessionManager sessions, MessageService messages, CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine, OutageSource outage, LeadSeatSource leadSeats, Map leads, String selfTerm, CoordinationSource coordination) { + return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, outage, + leadSeats, leads, selfTerm, coordination, true); + } + + /** + * As above, gated by the caller's role (fleetd #439). The {@code coordinator} row is + * lead-to-lead coordination state — coordination between orchestrators, not roster + * observation — so it is assembled and included only when {@code callerIsPrimary} is + * {@code true}. A worker or an architect gets a result with the {@code coordinator} key + * absent, never an empty or redacted one, and never pays the cost of + * {@link #coordinatorView} probing peer mailboxes for a row it will not receive. + * + * @param callerIsPrimary whether the {@code fleet_list} caller is the primary; only the MCP + * handler computes this from the real connection (see + * {@code Principal#isPrimary()}) — every other overload passes + * {@code true} + */ + static McpSchema.CallToolResult listFleet(PeerLauncher workers, SessionManager sessions, MessageService messages, + CapacitySource capacity, HealthCoverageSource healthCoverage, + QuarantineSource quarantine, OutageSource outage, + LeadSeatSource leadSeats, Map leads, String selfTerm, + CoordinationSource coordination, boolean callerIsPrimary) { try { Map live = workers.list().stream() .map(Agent.class::cast) @@ -1371,9 +1418,14 @@ public final class FleetMcp { Map result = new LinkedHashMap<>(); result.put("leads", leadRows); result.put("members", out); result.put("healthCoverage", healthCoverage.value().get()); - Map coordinatorRow = coordinatorView(coordination); - if (coordinatorRow != null) { - result.put("coordinator", coordinatorRow); + // fleetd #439: coordinator/coordinatorView is lead-to-lead coordination state and must + // never reach a worker or an architect -- gate BEFORE assembling it, not after, so the + // key is absent rather than present-and-empty. + if (callerIsPrimary) { + 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, diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java index 016037c..50f1b4c 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java @@ -187,6 +187,71 @@ class FleetMcpAuthzTest { "no CallerResolver supplied ⇒ authorization not enforced (legacy behaviour)"); } + // --- fleetd #439: who may see fleet_list's coordinator row ---------------------------------- + + /** + * fleetd #439: {@link FleetMcp#coordinatorVisibleTo} is the whole policy decision for + * {@code fleet_list}'s {@code coordinator} row — lead-to-lead coordination state, not roster + * observation. Only the primary may see it; a worker, an architect, and (the case the previous + * pass of this ticket did not cover) an anonymous caller must all be refused. + */ + @Test + void onlyThePrimaryMaySeeTheCoordinatorRow() { + assertTrue(FleetMcp.coordinatorVisibleTo(PRIMARY), "the primary must see its own coordination state"); + assertFalse(FleetMcp.coordinatorVisibleTo(WORKER_A), "a worker must not see lead-to-lead coordination state"); + assertFalse(FleetMcp.coordinatorVisibleTo(ARCH_DESIGN), + "an architect holds READ today, but that must not extend to coordinator"); + assertFalse(FleetMcp.coordinatorVisibleTo(ANON), "authenticated as nothing must not see it either"); + } + + /** + * fleetd #439 / PR #462 review finding M2: the predicate above can be perfectly correct while + * the one production call site (the {@code fleet_list} MCP handler) never actually asks it — + * a literal {@code true} compiles, and the whole suite stayed green under that mutation because + * every existing test drives {@link FleetMcp#listFleet} directly and supplies the boolean + * itself. This test reads {@code FleetMcp.java}'s own source (same idiom as {@link + * #toolsTheServerRegisters()} / {@link #everyRegisteredToolHasItsHandlerActionPinned()}) and + * asserts the handler's call passes {@code coordinatorVisibleTo(principal(exchange))} — not a + * literal {@code true} or {@code false} — as {@code listFleet}'s trailing argument. + * + *

Anchored on argument position, not a bare substring search: {@code true} appears many + * times elsewhere in this file for unrelated reasons, so a plain {@code contains("true")} + * check would prove nothing. The pattern requires the literal text immediately before the + * closing {@code );} of the {@code listFleet(} call inside the handler block to be exactly + * {@code coordinatorVisibleTo(principal(exchange))}. + */ + @Test + void theFleetListHandlerActuallyConsultsCoordinatorVisibleTo() throws Exception { + String source = Files.readString(MCP_SOURCE); + + // Isolate the fleet_list handler block: from its declaration up to the next handler's + // declaration. A change to variable naming would break this scrape loudly (see the control + // assertion just below), rather than silently reporting "no violation found". + int start = source.indexOf("listHandler ="); + assertTrue(start >= 0, "could not find the fleet_list handler (listHandler) in " + MCP_SOURCE + + " -- the scrape has stopped matching, fix the anchor before trusting this test"); + int end = source.indexOf("stopHandler =", start); + assertTrue(end > start, "could not find the handler declared after listHandler to bound the scrape"); + String handlerBlock = source.substring(start, end); + + // CONTROL: the block we scraped really does contain a call to listFleet(...) -- if this + // fails, the anchors above moved and the assertion below would otherwise pass on nothing. + assertTrue(handlerBlock.contains("listFleet("), + "control failed: the scraped listHandler block contains no listFleet( call at all -- " + + "the anchors have drifted, this test is not testing what it claims to"); + + Pattern trailingArg = Pattern.compile( + "listFleet\\([^;]*?,\\s*(coordinatorVisibleTo\\(principal\\(exchange\\)\\)|true|false)\\s*\\)\\s*;", + Pattern.DOTALL); + Matcher m = trailingArg.matcher(handlerBlock); + assertTrue(m.find(), "could not locate listFleet(...)'s trailing boolean argument in the " + + "listHandler block -- the call shape changed, update this test's anchor: " + handlerBlock); + String trailing = m.group(1); + assertEquals("coordinatorVisibleTo(principal(exchange))", trailing, + "the fleet_list handler must ask coordinatorVisibleTo(principal(exchange)) who is " + + "calling, not pass a literal boolean -- found: " + trailing); + } + // --- which action each tool hands the gate (fleetd #272) ------------------------------------ /** 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 d1aac0d..3904167 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java @@ -676,6 +676,95 @@ class FleetMcpTest { "an ordinary fleet's output must be unchanged by this feature"); } + /** + * fleetd #439: a worker calling {@code fleet_list} must get a result with the {@code + * coordinator} key absent -- not an empty object, not a redacted one -- even + * though lead coordination is fully configured and would otherwise report a row. This drives + * the same {@code callerIsPrimary} value the MCP handler computes ({@code + * Principal.worker(...).isPrimary()}), so it pins the real production boolean, not a literal. + */ + @Test + void listOmitsTheCoordinatorKeyEntirelyForAWorkerEvenWhenLeadCoordinationIsOn() { + FakeHerdr h = new FakeHerdr(); + SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + FakeLeadChannel channel = new FakeLeadChannel("mac-opus") + .withMailbox("mac-opus", LeadChannel.MailboxState.exists("mac-opus", 0, 1)); + boolean callerIsPrimary = Principal.worker("term_a", 1).isPrimary(); + + 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(), FleetMcp.OutageSource.none(), FleetMcp.LeadSeatSource.none(), + Map.of(), "", new FleetMcp.CoordinationSource(channel, List.of()), callerIsPrimary); + + String out = textOf(res); + assertFalse(out.contains("\"coordinator\""), "a worker must never see the coordinator key at all: " + out); + assertFalse(out.contains("mac-opus"), "no fragment of the coordinator row may leak either: " + out); + assertTrue(out.contains("\"leads\""), "the rest of the result must still be present: " + out); + assertTrue(out.contains("\"members\""), out); + assertTrue(out.contains("\"healthCoverage\""), out); + } + + /** + * fleetd #439 acceptance criterion 2: an architect gets exactly the same treatment as a worker. + * This is a real, executed test (not just reasoning by analogy) -- it drives the actual + * {@code Principal.architect(...).isPrimary()} value the production handler would compute for + * an architect caller, through the same gate a worker's call goes through. + */ + @Test + void listOmitsTheCoordinatorKeyEntirelyForAnArchitectToo() { + FakeHerdr h = new FakeHerdr(); + SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + FakeLeadChannel channel = new FakeLeadChannel("mac-opus") + .withMailbox("mac-opus", LeadChannel.MailboxState.exists("mac-opus", 0, 1)); + boolean callerIsPrimary = Principal.architect("lead-designer", "term_design", 400).isPrimary(); + + 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(), FleetMcp.OutageSource.none(), FleetMcp.LeadSeatSource.none(), + Map.of(), "", new FleetMcp.CoordinationSource(channel, List.of()), callerIsPrimary); + + String out = textOf(res); + assertFalse(out.contains("\"coordinator\""), "an architect must never see the coordinator key either: " + out); + } + + /** + * fleetd #439 acceptance criterion 3: the primary's {@code fleet_list} is byte-for-byte + * unchanged by this fix. Proven by comparing the new gated overload (with {@code + * callerIsPrimary=true}, exactly what the MCP handler passes for the primary) against the + * pre-#439 overload that always assembled the row -- if the gate changed anything for a + * primary caller, these two strings would differ. + */ + @Test + void listIsByteForByteUnchangedForThePrimaryCaller() { + FakeHerdr h = new FakeHerdr(); + SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + FakeLeadChannel channel = new FakeLeadChannel("mac-opus") + .withMailbox("mac-opus", LeadChannel.MailboxState.exists("mac-opus", 0, 1)); + + String preExisting = textOf(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 gatedAsPrimary = textOf(FleetMcp.listFleet( + workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), sessions, null, + FleetMcp.CapacitySource.none(), new FleetMcp.HealthCoverageSource(() -> "off"), + FleetMcp.QuarantineSource.none(), FleetMcp.OutageSource.none(), FleetMcp.LeadSeatSource.none(), + Map.of(), "", new FleetMcp.CoordinationSource(channel, List.of()), true)); + + assertEquals(preExisting, gatedAsPrimary, + "a primary caller must see byte-for-byte the same result as before this fix"); + assertTrue(gatedAsPrimary.contains("\"coordinator\""), gatedAsPrimary); + assertTrue(gatedAsPrimary.contains("\"selfId\":\"mac-opus\""), gatedAsPrimary); + assertTrue(gatedAsPrimary.contains("\"mailbox\""), gatedAsPrimary); + assertTrue(gatedAsPrimary.contains("\"heldCount\""), gatedAsPrimary); + assertTrue(gatedAsPrimary.contains("\"heldDurable\""), gatedAsPrimary); + assertTrue(gatedAsPrimary.contains("\"held\""), gatedAsPrimary); + assertTrue(gatedAsPrimary.contains("\"peers\""), gatedAsPrimary); + } + @Test void listReportsHeldMessagesWithATruncatedPreviewNeverTheFullBody() { FakeHerdr h = new FakeHerdr();