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 "