From 6c2d6e93cb7f552428141737272b11fc6ff93800 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 19:55:02 +0700 Subject: [PATCH] fleetd #469: one canonical FleetTool set backs registration, authz and charter checks FleetConfig.validateCharters() only checked that a charter key is a role wire name and its text is non-blank; #464's CharterToolSurfaceTest compared charter text against the registered tool surface, but wrote its own charter into a @TempDir fixture, so nothing anyone wrote into the live fleetd.yaml could ever fail it. Add FleetTool, an enum in dev.ltms.fleet.mcp holding the one canonical set of registered tool wire names. FleetMcp's tool schemas now derive their names from it, its constructor asserts at startup that what it actually registers with the SDK equals FleetTool.wireNames() exactly, and its authz dispatch (toolAction/authzAction) resolves the wire string against FleetTool before switching on the enum itself with no default -- adding a tool without pinning its Authz.Action is now a compile error, not just a test gap. Add CharterToolSurface (mcp package, not config -- config loads before the MCP server exists) and call it from Fleetd.main right after cfg.validateAll(), so a charter naming a tool the server does not register refuses the daemon's startup, naming both the charter key and the unknown tool. FleetdStartupValidationTest proves this through Fleetd.main itself against a live-shaped config fixture (bridge_send, CB-634's own removed name). CharterToolSurfaceTest, FleetMcpAuthzTest and McpContractDocTest each kept an independent regex scrape of FleetMcp.java's source for the registered side of their own comparison -- three more copies of the same list nothing tied together. All three now read FleetTool.wireNames() instead. Proved canonical by removal: deleting FleetTool.ACK while ackTool() still referenced it broke mvn compile in two places (FleetMcp.java:940,:1898); registering a schema under a literal not backed by FleetTool ("fleet_ack_v2") failed FleetMcp's new startup assertion in every test that constructs it (12 errors, IllegalStateException at FleetMcp.). Both reverted before this commit. --- .../src/main/java/dev/ltms/fleet/Fleetd.java | 12 +++ .../ltms/fleet/mcp/CharterToolSurface.java | 67 +++++++++++++++ .../java/dev/ltms/fleet/mcp/FleetMcp.java | 75 +++++++++++------ .../java/dev/ltms/fleet/mcp/FleetTool.java | 82 +++++++++++++++++++ .../fleet/FleetdStartupValidationTest.java | 22 +++++ .../fleet/mcp/CharterToolSurfaceTest.java | 80 ++++++++++++++---- .../dev/ltms/fleet/mcp/FleetMcpAuthzTest.java | 29 +++---- .../ltms/fleet/mcp/McpContractDocTest.java | 30 +++---- 8 files changed, 327 insertions(+), 70 deletions(-) create mode 100644 fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java create mode 100644 fleetd/src/main/java/dev/ltms/fleet/mcp/FleetTool.java diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 14420b9..3501003 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -26,6 +26,7 @@ import dev.ltms.fleet.inject.MemberPresence; import dev.ltms.fleet.auth.MemberRegistry; import dev.ltms.fleet.auth.CallerResolver; import dev.ltms.fleet.mcp.FleetMcp; +import dev.ltms.fleet.mcp.CharterToolSurface; import dev.ltms.fleet.mcp.ConnectionIdentity; import dev.ltms.fleet.metrics.FleetMetrics; import dev.ltms.fleet.metrics.Metrics; @@ -163,6 +164,17 @@ public final class Fleetd { // and the Fleetd-startup tests actually pin — see FleetConfig#validateAll's javadoc for // why a name-by-name list here would have the same defect it replaces. cfg.validateAll(); + // fleetd #469, follow-up to #464: validateAll() (and validateCharters() inside it) only + // checks that a charter's KEY is a role wire name and its text is non-blank — it never + // looks at what the text actually names. This is the separate check that does: it asks + // dev.ltms.fleet.mcp.FleetTool (the canonical registered-tool set) whether every fleet_*/ + // bridge_* token a charter names is a tool this server actually registers. It cannot live + // inside FleetConfig#validateCharters() — config loads before the MCP server exists, and + // must not gain a dependency on the mcp package — so it runs here instead, at the one seam + // that already holds both a loaded FleetConfig and the mcp package, before anything below + // opens a socket or spawns a member. + CharterToolSurface.assertChartersNameOnlyRegisteredTools( + cfg.fleet() == null ? Map.of() : cfg.fleet().charters()); Path socket = cfg.herdrSocket() != null && !cfg.herdrSocket().isBlank() ? Path.of(cfg.herdrSocket()) diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java new file mode 100644 index 0000000..9f4e242 --- /dev/null +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java @@ -0,0 +1,67 @@ +package dev.ltms.fleet.mcp; + +import java.util.ArrayList; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * fleetd #469: a launch charter that names an MCP tool the server does not register must stop the + * daemon at startup, not wait for a member to discover the gap by calling something that is not + * there. + * + *

Deliberately its own class outside {@code dev.ltms.fleet.config}, not a case in {@link + * dev.ltms.fleet.config.FleetConfig#validateCharters()}. The canonical tool surface ({@link + * FleetTool}) lives in the {@code mcp} package; config is loaded before the MCP server exists and + * must not gain a dependency on it. So this check belongs at the seam that already holds both a + * loaded {@code FleetConfig} and the {@code mcp} package: {@code Fleetd.main}, called right after + * {@code cfg.validateAll()} and before anything opens a socket or spawns a member. + * + *

{@code #464}'s {@code CharterToolSurfaceTest} proved the same comparison against a charter + * fixture it wrote itself into a {@code @TempDir}, which meant nothing anyone wrote into the live + * {@code fleetd.yaml} could ever fail it. This class is what a real charter is actually checked + * against at boot; {@code FleetdStartupValidationTest} exercises it through {@code Fleetd.main} + * itself, the same way it proves every other {@code validateXxx()} still runs there. + */ +public final class CharterToolSurface { + + /** A {@code fleet_…} (current) or {@code bridge_…} (pre-CB-634) tool-shaped token in prose. */ + private static final Pattern TOOL_REFERENCE = Pattern.compile("(fleet_[a-z_]+|bridge_[a-z_]+)"); + + private CharterToolSurface() { + } + + /** + * @param charters the configured {@code fleet.charters:} map (role wire name → charter text); + * {@code null} or empty is a no-op, same as an absent {@code fleet:} block + * @throws IllegalStateException naming the charter key and every tool it names that {@link + * FleetTool} does not list, when any charter does so + */ + public static void assertChartersNameOnlyRegisteredTools(Map charters) { + if (charters == null || charters.isEmpty()) { + return; + } + Set registered = FleetTool.wireNames(); + List bad = new ArrayList<>(); + charters.forEach((key, text) -> { + if (text == null) { + return; + } + Set named = new LinkedHashSet<>(); + Matcher m = TOOL_REFERENCE.matcher(text); + while (m.find()) { + named.add(m.group(1)); + } + named.stream() + .filter(t -> !registered.contains(t)) + .forEach(unknown -> bad.add("fleet.charters." + key + " names '" + unknown + + "', which the server does not register (registered: " + registered + ").")); + }); + if (!bad.isEmpty()) { + throw new IllegalStateException("refusing to start: " + String.join(" ", bad)); + } + } +} 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 bb5a845..a6a1102 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -490,6 +490,21 @@ public final class FleetMcp { McpSchema.Tool fleetProfiles = profilesTool(); McpSchema.Tool fleetWhoami = whoamiTool(); + // fleetd #469: the tool schemas above are already named from FleetTool.wireName(), but + // this is the check that a schema was not accidentally dropped, duplicated, or added + // under a name FleetTool does not list. It runs once, at construction (startup), rather + // than being left to CharterToolSurface or a test to discover later — a canonical entry + // this server never registers, or a registration with no canonical entry backing it, is a + // startup failure, not a silent gap. + Set registeredToolNames = Set.of(fleetSend.name(), fleetReply.name(), fleetAsk.name(), + fleetStatus.name(), fleetPoll.name(), fleetAck.name(), fleetSpawn.name(), + fleetList.name(), fleetStop.name(), fleetProfiles.name(), fleetWhoami.name()); + if (!registeredToolNames.equals(FleetTool.wireNames())) { + throw new IllegalStateException("fleetd #469: registered MCP tools " + registeredToolNames + + " do not match the canonical tool set " + FleetTool.wireNames() + + " -- FleetTool is the single source of truth for what this server registers"); + } + this.server = McpServer.sync(transport) .serverInfo("fleet", "0.1.0") .capabilities(McpSchema.ServerCapabilities.builder().tools(true).build()) @@ -897,20 +912,34 @@ public final class FleetMcp { /** * The action a registered tool handler actually hands to the authorization gate. - * Keeping this choice beside the registered-tool inventory makes a new tool fail the coverage - * test until its action is pinned. + * + *

{@code toolName} is a raw string off the wire (an MCP call names its tool by string, and a + * malformed or stale client can send anything), so resolving it against {@link FleetTool} first + * — and throwing on a miss — is still a run-time check by necessity. What moved to compile time + * is the second step: {@link #authzAction(FleetTool, Map)} switches on the resolved {@link + * FleetTool} itself with no {@code default}, so a new {@link FleetTool} constant with no pinned + * action fails {@code mvn compile}, not just {@code FleetMcpAuthzTest} at run time. */ static Authz.Action toolAction(String toolName, Map arguments) { - return switch (toolName) { - case "fleet_send" -> Authz.Action.SEND; - case "fleet_reply" -> Authz.Action.REPLY; - case "fleet_ask" -> Authz.Action.ASK; - case "fleet_status", "fleet_list", "fleet_profiles", "fleet_whoami" -> Authz.Action.READ; - case "fleet_poll" -> pollAction(str(arguments, "target"), str(arguments, "coordId")); - case "fleet_ack" -> Authz.Action.DRAIN; - case "fleet_spawn" -> Authz.Action.SPAWN; - case "fleet_stop" -> Authz.Action.STOP; - default -> throw new IllegalArgumentException("unregistered tool: " + toolName); + FleetTool tool = FleetTool.byWireName(toolName) + .orElseThrow(() -> new IllegalArgumentException("unregistered tool: " + toolName)); + return authzAction(tool, arguments); + } + + /** + * Exhaustive over {@link FleetTool} on purpose — no {@code default}. Adding a tool to {@link + * FleetTool} without adding its case here is a compile error (fleetd #469). + */ + private static Authz.Action authzAction(FleetTool tool, Map arguments) { + return switch (tool) { + case SEND -> Authz.Action.SEND; + case REPLY -> Authz.Action.REPLY; + case ASK -> Authz.Action.ASK; + case STATUS, LIST, PROFILES, WHOAMI -> Authz.Action.READ; + case POLL -> pollAction(str(arguments, "target"), str(arguments, "coordId")); + case ACK -> Authz.Action.DRAIN; + case SPAWN -> Authz.Action.SPAWN; + case STOP -> Authz.Action.STOP; }; } @@ -1805,7 +1834,7 @@ public final class FleetMcp { // --- tool schemas -------------------------------------------------------------------------- private static McpSchema.Tool sendTool() { - return tool("fleet_send", + return tool(FleetTool.SEND.wireName(), "Delegate a task to a worker session. By default blocks until the worker replies and " + "returns its reply (or a 'still working / queued' note on timeout). Pass wait:false " + "for a long task to return a ticket immediately, then poll it with fleet_poll. To " @@ -1833,7 +1862,7 @@ public final class FleetMcp { private static McpSchema.Tool askTool() { // No target/session arg — the worker's identity is resolved from the connection. - return tool("fleet_ask", + return tool(FleetTool.ASK.wireName(), "Pause your current delegated turn to ask the primary a question, blocking until it " + "answers — then resume the same turn with the answer. Use this when only the " + "primary has a decision or detail you need to continue. You do not address the " @@ -1846,7 +1875,7 @@ public final class FleetMcp { } private static McpSchema.Tool pollTool() { - return tool("fleet_poll", + return tool(FleetTool.POLL.wireName(), "Check an async delegation (a fleet_send with wait:false) by its ticket: " + "pending, done (with the worker's reply), or failed. When target (a worker " + "session id) is present instead of ticket, drain that worker's inbox of " @@ -1866,7 +1895,7 @@ public final class FleetMcp { } private static McpSchema.Tool ackTool() { - return tool("fleet_ack", + return tool(FleetTool.ACK.wireName(), "Acknowledge (remove) a specific reply from a worker's inbox. Use when the primary " + "has processed a reply and wants to confirm it, leaving other pending replies " + "in the inbox for later drain.", @@ -1880,7 +1909,7 @@ public final class FleetMcp { } private static McpSchema.Tool spawnTool() { - return tool("fleet_spawn", + return tool(FleetTool.SPAWN.wireName(), "Spawn a new off-subscription member session. A member has two independent attributes: " + "role (what it is for) and profile (which backend it runs on). Pass role to pick " + "the contract — 'dev' implements a unit and opens its own PR, 'reviewer' reviews a " @@ -1912,7 +1941,7 @@ public final class FleetMcp { } private static McpSchema.Tool profilesTool() { - return tool("fleet_profiles", + return tool(FleetTool.PROFILES.wireName(), "List the configured worker profiles (backends) and which one fleet_spawn uses by " + "default. A 'quarantined' map is present when a backend-exhausted refusal put " + "a profile's credential on cooldown — fleet_spawn onto it is refused until " @@ -1930,7 +1959,7 @@ public final class FleetMcp { } private static McpSchema.Tool listTool() { - return tool("fleet_list", + return tool(FleetTool.LIST.wireName(), "List the whole fleet the bridge tracks, in two parts. 'leads' are your PEERS — other " + "orchestrators, each with its sessionId (the address to fleet_send to), " + "name, live status, and 'self': true on your own row; this is how you " @@ -1966,7 +1995,7 @@ public final class FleetMcp { } private static McpSchema.Tool stopTool() { - return tool("fleet_stop", + return tool(FleetTool.STOP.wireName(), "Tear down a worker session by its paneId (from fleet_spawn or fleet_list).", objectSchema(Map.of( "paneId", stringProp("The worker's paneId to stop")), @@ -1975,7 +2004,7 @@ public final class FleetMcp { private static McpSchema.Tool replyTool() { // No session/target arg — the caller's identity is resolved from the connection. - return tool("fleet_reply", + return tool(FleetTool.REPLY.wireName(), "Return your structured answer for a message you were sent, resolving the sender's " + "blocked fleet_send. A worker MUST end every delegated turn with exactly " + "one of these. A lead uses it only to answer another lead that messaged " @@ -1986,7 +2015,7 @@ public final class FleetMcp { } private static McpSchema.Tool statusTool() { - return tool("fleet_status", + return tool(FleetTool.STATUS.wireName(), "Get the live lifecycle status (idle/working/blocked/unknown) of a worker session.", objectSchema(Map.of( "sessionId", stringProp("The worker session id to query")), @@ -1994,7 +2023,7 @@ public final class FleetMcp { } private static McpSchema.Tool whoamiTool() { - return tool("fleet_whoami", + return tool(FleetTool.WHOAMI.wireName(), "Report who YOU are on the bridge — your role is resolved from your connection " + "(unforgeable), never from anything you claim. Returns role 'primary' (you " + "orchestrate: spawn/send/stop; reply ONLY to answer a peer lead that " diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetTool.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetTool.java new file mode 100644 index 0000000..de54be1 --- /dev/null +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetTool.java @@ -0,0 +1,82 @@ +package dev.ltms.fleet.mcp; + +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +/** + * The one canonical set of MCP tool names this daemon registers (fleetd #469, follow-up to #464). + * + *

Before this enum, the tool surface was written twice with nothing tying the copies together: + * once as the literal {@code "fleet_…"} string passed to each tool-schema builder in + * {@link FleetMcp}, and again as the case labels of {@link FleetMcp}'s authorization switch. A + * reader that needed "what does this server register" — a charter check, in particular — had no + * source to ask except scraping {@code FleetMcp.java}'s source text for {@code tool("…")} calls: a + * third copy of the same list, and the weakest of the three forms. + * + *

Every reader that needs the registered tool surface now asks this enum instead: + * + *

+ */ +public enum FleetTool { + + SEND("fleet_send"), + REPLY("fleet_reply"), + ASK("fleet_ask"), + STATUS("fleet_status"), + POLL("fleet_poll"), + ACK("fleet_ack"), + SPAWN("fleet_spawn"), + LIST("fleet_list"), + STOP("fleet_stop"), + PROFILES("fleet_profiles"), + WHOAMI("fleet_whoami"); + + private final String wireName; + + FleetTool(String wireName) { + this.wireName = wireName; + } + + /** The name this tool is registered under, and called by, on the wire ({@code "fleet_send"}, …). */ + public String wireName() { + return wireName; + } + + private static final Map BY_WIRE_NAME; + private static final Set WIRE_NAMES; + + static { + Map byName = new LinkedHashMap<>(); + Set names = new LinkedHashSet<>(); + for (FleetTool tool : values()) { + byName.put(tool.wireName, tool); + names.add(tool.wireName); + } + BY_WIRE_NAME = Map.copyOf(byName); + WIRE_NAMES = Set.copyOf(names); + } + + /** The tool named {@code wireName}, or empty when this daemon registers no such tool. */ + public static Optional byWireName(String wireName) { + return Optional.ofNullable(BY_WIRE_NAME.get(wireName)); + } + + /** Every wire name this daemon registers — the canonical tool surface. */ + public static Set wireNames() { + return WIRE_NAMES; + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdStartupValidationTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdStartupValidationTest.java index 6eac36c..640122a 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/FleetdStartupValidationTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdStartupValidationTest.java @@ -95,6 +95,28 @@ class FleetdStartupValidationTest { """, "architetc"); } + /** + * fleetd #469: closes the gap left by #464's {@code CharterToolSurfaceTest}, which wrote its + * own charter into a {@code @TempDir} fixture and so could never fail on anything anyone wrote + * into the live {@code fleetd.yaml}. {@code bridge_send} is CB-634's own motivating example — a + * tool name the rename removed — and the charter key ({@code dev}) is a real role wire name, so + * this fixture passes {@code cfg.validateAll()}'s charter check (key valid, text non-blank) and + * is refused only by the new {@code CharterToolSurface} call right after it. The failure message + * must name both the charter key and the unknown tool. + */ + @Test + void mainRefusesACharterNamingAnUnregisteredTool(@TempDir Path dir) throws Exception { + assertMainRefuses(dir, "charter-tool-surface.yaml", """ + bind: + host: 127.0.0.1 + port: 8765 + fleet: + charters: + dev: | + Send the final handoff through bridge_send. + """, "bridge_send"); + } + @Test void mainRefusesAnArchitectSlotNamingAnUnconfiguredProfile(@TempDir Path dir) throws Exception { assertMainRefuses(dir, "members.yaml", """ diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/CharterToolSurfaceTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/CharterToolSurfaceTest.java index 41ed8b8..5d99ae8 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/CharterToolSurfaceTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/CharterToolSurfaceTest.java @@ -3,7 +3,9 @@ package dev.ltms.fleet.mcp; import dev.ltms.fleet.config.FleetConfig; import java.nio.file.Files; import java.nio.file.Path; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; +import java.util.Map; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -11,13 +13,30 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -/** fleetd #464: launch charters must not name MCP tools the server does not register. */ +/** + * fleetd #464 shipped {@code configuredChartersNameOnlyRegisteredTools} below, comparing charter + * text against the registered tool surface — but it scraped both sides from source text: + * its own fixture charter, and a regex over {@code FleetMcp.java}'s {@code tool("…")} calls. #469's + * gap: nothing anyone wrote into the live {@code fleetd.yaml} could ever reach that test, because + * it never called production validation code. + * + *

This version keeps the charter-text extraction helper ({@code toolsNamedIn}) — charters are + * free-text config, so finding a {@code fleet_*}/{@code bridge_*} token inside one has no source + * but a scrape — but reads the registered side from {@link FleetTool}, the canonical enum + * {@code FleetMcp} itself now derives its tool schemas and authorization switch from, rather than a + * second scrape of {@code FleetMcp.java}'s source. It also exercises {@link + * CharterToolSurface#assertChartersNameOnlyRegisteredTools} directly — the method {@code + * Fleetd.main} actually calls at startup — both accepting and rejecting. {@code + * dev.ltms.fleet.FleetdStartupValidationTest#mainRefusesACharterNamingAnUnregisteredTool} is what + * closes #469's actual gap: it proves that call is wired into {@code Fleetd.main} itself, against a + * live-shaped config fixture, not just a unit call to the method in isolation. + */ class CharterToolSurfaceTest { - private static final Path MCP_SOURCE = Path.of("src/main/java/dev/ltms/fleet/mcp/FleetMcp.java"); - private static Set matches(String text, String regex) { Matcher m = Pattern.compile(regex).matcher(text); Set found = new LinkedHashSet<>(); @@ -33,13 +52,8 @@ class CharterToolSurfaceTest { "(fleet_[a-z_]+|bridge_[a-z_]+)"); } - /** Every tool {@link FleetMcp} registers, read from its {@code tool("…")} calls. */ - private static Set toolsTheServerRegisters() throws Exception { - return matches(Files.readString(MCP_SOURCE), "tool\\(\\\"(fleet_[a-z_]+)\\\""); - } - @Test - @DisplayName("[SOURCE TEXT] every tool named in a configured charter is registered by the server") + @DisplayName("every tool named in a configured charter is in the canonical FleetTool set") void configuredChartersNameOnlyRegisteredTools(@TempDir Path dir) throws Exception { Path configFile = dir.resolve("charters.yaml"); Files.writeString(configFile, """ @@ -53,20 +67,58 @@ class CharterToolSurfaceTest { FleetConfig config = FleetConfig.load(configFile); Set named = toolsNamedIn(config); - Set registered = toolsTheServerRegisters(); + Set registered = FleetTool.wireNames(); assertTrue(!named.isEmpty(), "the charter fixture named no fleet_* or bridge_* tool. This test would check nothing; " + "add charter text that names a tool before changing the extraction."); assertTrue(!registered.isEmpty(), - "the FleetMcp registration scrape found no tools. This test would check nothing; " - + "repair the tool(\"…\") extraction before changing the assertion."); + "FleetTool.wireNames() is empty. This test would check nothing; repair FleetTool " + + "before changing the assertion."); Set unknown = new LinkedHashSet<>(named); unknown.removeAll(registered); assertTrue(unknown.isEmpty(), - "configured charter text names " + unknown + ", but FleetMcp does not register it. " + "configured charter text names " + unknown + ", but FleetTool does not list it. " + "Checked " + named + " against " + registered + ". Fix the charter text or " - + "register the tool; do NOT weaken this test."); + + "add the tool to FleetTool; do NOT weaken this test."); + } + + /** + * The positive case for the actual production entry point: a charter naming only tools + * {@link FleetTool} lists must not throw. + */ + @Test + @DisplayName("CharterToolSurface accepts a charter that names only registered tools") + void charterToolSurfaceAcceptsKnownTools() { + assertDoesNotThrow(() -> CharterToolSurface.assertChartersNameOnlyRegisteredTools(Map.of( + "dev", "Send the final handoff through fleet_reply, using fleet_send to delegate.", + "reviewer", "Use fleet_ask only for the lead's decision."))); + } + + /** + * The negative case for the actual production entry point (fleetd #469's motivating example: + * {@code bridge_send} is the pre-CB-634 name, removed from the tool surface). The message must + * name both the offending charter key and the unknown tool, so an operator reading the startup + * log knows exactly which charter to fix. + */ + @Test + @DisplayName("CharterToolSurface rejects a charter naming a tool the server does not register") + void charterToolSurfaceRejectsAnUnregisteredTool() { + Map charters = new LinkedHashMap<>(); + charters.put("dev", "Send the final handoff through bridge_send."); + IllegalStateException e = assertThrows(IllegalStateException.class, + () -> CharterToolSurface.assertChartersNameOnlyRegisteredTools(charters)); + assertTrue(e.getMessage().contains("dev"), + "expected the charter key 'dev' in the failure message, got: " + e.getMessage()); + assertTrue(e.getMessage().contains("bridge_send"), + "expected the unknown tool 'bridge_send' in the failure message, got: " + e.getMessage()); + } + + @Test + @DisplayName("CharterToolSurface is a no-op on an absent or empty charter map") + void charterToolSurfaceIsANoOpWithNoCharters() { + assertDoesNotThrow(() -> CharterToolSurface.assertChartersNameOnlyRegisteredTools(null)); + assertDoesNotThrow(() -> CharterToolSurface.assertChartersNameOnlyRegisteredTools(Map.of())); } } 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 50f1b4c..305920b 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java @@ -26,7 +26,6 @@ import org.junit.jupiter.api.Test; import java.nio.file.Files; import java.nio.file.Path; -import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; import java.util.regex.Matcher; @@ -50,7 +49,6 @@ import static org.junit.jupiter.api.Assertions.*; class FleetMcpAuthzTest { private static final Path MCP_SOURCE = Path.of("src/main/java/dev/ltms/fleet/mcp/FleetMcp.java"); - private static final Pattern TOOL_REGISTRATION = Pattern.compile("tool\\(\\\"(fleet_[a-z_]+)\\\""); private final FakeHerdr herdr = new FakeHerdr(); private final AgentControl agents = new AgentControl(herdr); @@ -296,10 +294,18 @@ class FleetMcpAuthzTest { @Test void everyRegisteredToolHasItsHandlerActionPinned() { - Set registered = toolsTheServerRegisters(); + // fleetd #469: this used to scrape FleetMcp.java's tool("…") calls for the registered set — + // a third copy of the same list this file, CharterToolSurfaceTest and McpContractDocTest + // each kept independently. All three now read FleetTool.wireNames(), the canonical set + // FleetMcp itself derives its tool schemas AND its authorization switch from; adding a tool + // there without pinning its action in FleetMcp#authzAction is a compile error, so this test's + // per-tool assertions below are a run-time regression pin on top of that compile-time check, + // not the only thing standing between a new tool and an unpinned action. + Set registered = FleetTool.wireNames(); assertTrue(registered.size() >= 10, - "scraped only " + registered.size() + " tool registrations from FleetMcp (" + registered - + "); the server registers eleven, so the tool(\"…\") scrape has stopped matching"); + "FleetTool.wireNames() returned only " + registered.size() + " tool(s) (" + registered + + "); the server registers eleven, so FleetTool has stopped listing the real " + + "tool surface"); registered.forEach(tool -> assertDoesNotThrow(() -> FleetMcp.toolAction(tool, Map.of()), () -> tool + " is registered but has no pinned authorization action")); @@ -319,19 +325,6 @@ class FleetMcpAuthzTest { FleetMcp.toolAction("fleet_poll", Map.of("coordId", "mac-opus"))); } - private static Set toolsTheServerRegisters() { - try { - Matcher matcher = TOOL_REGISTRATION.matcher(Files.readString(MCP_SOURCE)); - Set tools = new LinkedHashSet<>(); - while (matcher.find()) { - tools.add(matcher.group(1)); - } - return tools; - } catch (Exception e) { - throw new AssertionError("could not scrape FleetMcp tool registrations", e); - } - } - @Test void aWorkerMayNotDrainAnotherSessionsInboxByPolling() { FleetMcp m = mcp(true); diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/McpContractDocTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/McpContractDocTest.java index fbcf43e..3b4763d 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/McpContractDocTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/McpContractDocTest.java @@ -27,16 +27,21 @@ import static org.junit.jupiter.api.Assertions.assertTrue; * somewhere to be readable, and that is exactly the sentence that rots. This test is what makes it * safe to write. * - *

It checks source text, not behaviour. It reads the Markdown and reads {@link FleetMcp}'s - * source, and it only catches a name in the doc that the server does not register. It cannot catch a - * flow that describes the wrong order, or a parameter name in prose — those are not name-shaped. The - * doc's own header carries that caveat for its readers. + *

It checks source text, not behaviour. It reads the Markdown, and it only catches a name + * in the doc that the server does not register. It cannot catch a flow that describes the wrong + * order, or a parameter name in prose — those are not name-shaped. The doc's own header carries + * that caveat for its readers. + * + *

fleetd #469: the registered side used to be its own scrape of {@code FleetMcp.java}'s {@code + * tool("…")} calls — a third copy of the same list {@code CharterToolSurfaceTest} and {@code + * FleetMcpAuthzTest} each kept their own copy of too. All three now read {@link + * FleetTool#wireNames()}, the one canonical set {@code FleetMcp} itself derives its tool schemas and + * authorization switch from. */ class McpContractDocTest { /** Tests run with the module directory as cwd, so the repo-root doc is one level up. */ private static final Path DOC = Path.of("../docs/MCP-Contract.md"); - private static final Path MCP_SOURCE = Path.of("src/main/java/dev/ltms/fleet/mcp/FleetMcp.java"); private static Set matches(Path file, String regex) throws Exception { Matcher m = Pattern.compile(regex).matcher(Files.readString(file)); @@ -52,15 +57,10 @@ class McpContractDocTest { return matches(DOC, "(fleet_[a-z_]+)"); } - /** Every tool {@link FleetMcp} actually registers, read from its {@code tool("…")} calls. */ - private static Set toolsTheServerRegisters() throws Exception { - return matches(MCP_SOURCE, "tool\\(\"(fleet_[a-z_]+)\""); - } - @Test @DisplayName("[SOURCE TEXT] every fleet_* tool named in MCP-Contract.md is one the server registers") void theDocNamesNoToolThatDoesNotExist() throws Exception { - Set registered = toolsTheServerRegisters(); + Set registered = FleetTool.wireNames(); Set named = toolsNamedInTheDoc(); Set unknown = new LinkedHashSet<>(named); @@ -84,13 +84,13 @@ class McpContractDocTest { @Test @DisplayName("[SOURCE TEXT] the doc/server name check is not vacuous — both sides found names") void theCheckActuallyHasSomethingToCheck() throws Exception { - Set registered = toolsTheServerRegisters(); + Set registered = FleetTool.wireNames(); Set named = toolsNamedInTheDoc(); assertTrue(registered.size() >= 10, - "scraped only " + registered.size() + " tool registrations from FleetMcp (" + registered - + "); the server registers eleven, so the tool(\"…\") scrape has stopped matching " - + "and the check above is now vacuous"); + "FleetTool.wireNames() returned only " + registered.size() + " tool(s) (" + registered + + "); the server registers eleven, so FleetTool has stopped listing the real " + + "tool surface and the check above is now vacuous"); assertTrue(named.size() >= 4, "docs/MCP-Contract.md names only " + named.size() + " fleet_* tool(s) (" + named + "). " + "The flows describe delegation, clarification, detached delivery and the "