fleetd #439: pin the caller at the fleet_list call site, not just the gate
CI / contract (pull_request) Successful in 1m29s
CI / build (pull_request) Successful in 1m33s

Review of PR #462 found M2: the coordinatorVisibleTo gate (then an inline
principal(exchange).isPrimary() check) could survive a mutation that
replaced the argument with a literal true at the one production call
site, because every existing test drove listFleet directly and supplied
the boolean itself -- nothing exercised the handler's own call.

- Name the decision: FleetMcp.coordinatorVisibleTo(Principal), a small
  package-private predicate next to denyFor/recordPrimarySingleton. The
  fleet_list handler now calls coordinatorVisibleTo(principal(exchange))
  instead of inlining .isPrimary().
- Pin the predicate's role table in FleetMcpAuthzTest
  (onlyThePrimaryMaySeeTheCoordinatorRow), covering primary/worker/
  architect and, newly, anonymous.
- Add a source-reading detector at the boundary
  (theFleetListHandlerActuallyConsultsCoordinatorVisibleTo), same idiom as
  toolsTheServerRegisters/everyRegisteredToolHasItsHandlerActionPinned: it
  reads FleetMcp.java, isolates the listHandler block, asserts (as a
  control) that the block actually contains a listFleet( call, then
  asserts the call's trailing boolean argument is exactly
  coordinatorVisibleTo(principal(exchange)) -- not a literal true/false.

Both mutations from the review were reproduced and killed by these tests,
then reverted; see the PR body for the full break-and-restore transcript.
This commit is contained in:
Dai Ha
2026-09-10 18:28:16 +07:00
parent e54e3d87ea
commit c1ca6273fc
2 changed files with 80 additions and 1 deletions
@@ -427,7 +427,7 @@ public final class FleetMcp {
leadSeats, callers == null ? Map.of() : callers.leads(),
callerTerminal(exchange),
new CoordinationSource(leadChannel, peers),
principal(exchange).isPrimary());
coordinatorVisibleTo(principal(exchange)));
};
BiFunction<McpSyncServerExchange, McpSchema.CallToolRequest, McpSchema.CallToolResult> stopHandler =
(exchange, req) -> {
@@ -580,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);
@@ -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.
*
* <p>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) ------------------------------------
/**