A role charter is free text in config that tells a member which tools to call, and nothing checked that those tools exist. A charter naming bridge_send - a name CB-634 removed - started the daemon cleanly, and the member found out at run time by calling something that was not there. #464 shipped a test for this, but it wrote its own charter into a @TempDir, so nothing anyone put in the real config could fail it. That was a defect in my acceptance criteria, not in that work. Now: FleetTool is one enum of the 11 registered wire names, and every reader goes through it. - FleetMcp's schema builders pass FleetTool.X.wireName() instead of a literal. - FleetMcp's constructor asserts at startup that what it registers with the SDK equals FleetTool.wireNames() exactly, in both directions. - toolAction(String, Map) resolves arbitrary wire input against FleetTool.byWireName() and keeps its run-time throw, which is necessary - network input has no closed compile-time form. It then hands off to authzAction(FleetTool, Map), a switch over the enum with NO default, so a new tool is a compile error at that layer. - CharterToolSurface lives in mcp, not config, and Fleetd.main calls it right after validateAll(). Config must not depend on the MCP server: config loads before the server exists. My brief undercounted the problem and the worker corrected it. I said there were two tool-name inventories plus a test fixture. There were FIVE: the registrations, the authz switch, and three separate source-text scrapes of FleetMcp.java in CharterToolSurfaceTest, FleetMcpAuthzTest and McpContractDocTest - none of which the ticket mentioned. Fixing those three was required, not scope creep: once the literals moved into FleetTool their regexes matched zero names, so one would have failed on its vacuity guard and the other two would have gone quietly vacuous. Inventories after: one. Verified independently on origin/main before accepting the wider diff: three test files did read FleetMcp.java as source text, with a control file at zero to prove the search discriminated. Build number and my own mutation results are on the ticket and the PR, measured on this merge commit rather than on the branch.
This commit is contained in:
Dai Ha
@@ -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())
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>{@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<String, String> charters) {
|
||||
if (charters == null || charters.isEmpty()) {
|
||||
return;
|
||||
}
|
||||
Set<String> registered = FleetTool.wireNames();
|
||||
List<String> bad = new ArrayList<>();
|
||||
charters.forEach((key, text) -> {
|
||||
if (text == null) {
|
||||
return;
|
||||
}
|
||||
Set<String> 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));
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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<String> 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.
|
||||
*
|
||||
* <p>{@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<String, Object> 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<String, Object> 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 "
|
||||
|
||||
@@ -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).
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>Every reader that needs the registered tool surface now asks this enum instead:
|
||||
*
|
||||
* <ul>
|
||||
* <li>the tool-schema builders in {@code FleetMcp} pass {@code wireName()} rather than a literal;
|
||||
* <li>{@code FleetMcp}'s constructor asserts, at startup, that the set of tool names it actually
|
||||
* registers with the MCP SDK equals {@link #wireNames()} exactly — a canonical entry that is
|
||||
* never registered, or a registration with no canonical entry backing it, fails the daemon's
|
||||
* own boot rather than only a test's;
|
||||
* <li>{@code FleetMcp.toolAction}'s dispatch onto {@code Authz.Action} switches on the enum
|
||||
* (not the raw string) with no {@code default}, so adding a tool here without pinning its
|
||||
* action is a compile error, not a run-time throw;
|
||||
* <li>{@link CharterToolSurface} asks {@link #wireNames()} to check a configured launch charter
|
||||
* against the live tool surface, instead of scraping source text a third time.
|
||||
* </ul>
|
||||
*/
|
||||
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<String, FleetTool> BY_WIRE_NAME;
|
||||
private static final Set<String> WIRE_NAMES;
|
||||
|
||||
static {
|
||||
Map<String, FleetTool> byName = new LinkedHashMap<>();
|
||||
Set<String> 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<FleetTool> byWireName(String wireName) {
|
||||
return Optional.ofNullable(BY_WIRE_NAME.get(wireName));
|
||||
}
|
||||
|
||||
/** Every wire name this daemon registers — the canonical tool surface. */
|
||||
public static Set<String> wireNames() {
|
||||
return WIRE_NAMES;
|
||||
}
|
||||
}
|
||||
@@ -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", """
|
||||
|
||||
@@ -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 <em>both</em> 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.
|
||||
*
|
||||
* <p>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 <em>registered</em> 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<String> matches(String text, String regex) {
|
||||
Matcher m = Pattern.compile(regex).matcher(text);
|
||||
Set<String> 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<String> 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<String> named = toolsNamedIn(config);
|
||||
Set<String> registered = toolsTheServerRegisters();
|
||||
Set<String> 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<String> 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<String, String> 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()));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<String> 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<String> 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<String> toolsTheServerRegisters() {
|
||||
try {
|
||||
Matcher matcher = TOOL_REGISTRATION.matcher(Files.readString(MCP_SOURCE));
|
||||
Set<String> 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);
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p><b>It checks source text, not behaviour.</b> 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.
|
||||
* <p><b>It checks source text, not behaviour.</b> 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.
|
||||
*
|
||||
* <p>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<String> 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<String> 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<String> registered = toolsTheServerRegisters();
|
||||
Set<String> registered = FleetTool.wireNames();
|
||||
Set<String> named = toolsNamedInTheDoc();
|
||||
|
||||
Set<String> 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<String> registered = toolsTheServerRegisters();
|
||||
Set<String> registered = FleetTool.wireNames();
|
||||
Set<String> 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 "
|
||||
|
||||
Reference in New Issue
Block a user