diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 3501003..5fa48e2 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -148,7 +148,10 @@ public final class Fleetd { // wiring below reads it, and must, because those decisions cannot be unmade. `config` is the // live reference the hot paths read per use. Which keys can actually move is ConfigRef's // contract; adding a reader here does not make a key reloadable by itself. - ConfigRef config = new ConfigRef(configPath, cfg); + // fleetd #474: pass the charter/tool-surface check in as ConfigRef's extraValidation, so + // ConfigRef#reload() runs the same gate main() runs below, without dev.ltms.fleet.config + // gaining a dependency on dev.ltms.fleet.mcp — Fleetd is the seam that already holds both. + ConfigRef config = new ConfigRef(configPath, cfg, Fleetd::assertChartersNameOnlyRegisteredTools); // The primary/host env that launched fleetd must not be tainted. SubscriptionGuard guard = new SubscriptionGuard(cfg.guard().hostSet()); @@ -172,9 +175,9 @@ public final class Fleetd { // 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()); + // opens a socket or spawns a member. fleetd #474: the same check is also wired into `config` + // above as ConfigRef's extraValidation, so a reload refuses what this line refuses at startup. + assertChartersNameOnlyRegisteredTools(cfg); Path socket = cfg.herdrSocket() != null && !cfg.herdrSocket().isBlank() ? Path.of(cfg.herdrSocket()) @@ -1594,6 +1597,28 @@ public final class Fleetd { } } + /** + * fleetd #474: the one place both the startup call (right after {@code cfg.validateAll()} in + * {@link #main}) and the reload call (wired into {@code config}'s {@code extraValidation} above, + * via a method reference to this method) go through, so the two can never drift into checking + * different things. Extracted only to give {@link ConfigRef}'s {@code Consumer} + * hook a {@code FleetConfig -> void} shape to bind to — {@link CharterToolSurface} itself still + * takes the raw charter map and knows nothing about {@code ConfigRef} or {@code Fleetd}. + * + *

Package-private so a test can call it directly the same way the other startup-report + * helpers above are tested, without needing to drive {@link #main} for a unit-level check; + * {@code FleetdStartupValidationTest} proves the startup call site, and {@code + * FleetdConfigRefCharterToolSurfaceWiringTest} — by constructing {@code ConfigRef} with this + * exact method reference, the same way {@code main} does above — proves the reload call site. + * {@code dev.ltms.fleet.config.ConfigRefTest} pins the same reload behaviour too, through an + * equivalent {@code Consumer} it builds locally (it cannot see this package-private + * method from {@code dev.ltms.fleet.config}). + */ + static void assertChartersNameOnlyRegisteredTools(FleetConfig cfg) { + CharterToolSurface.assertChartersNameOnlyRegisteredTools( + cfg.fleet() == null ? Map.of() : cfg.fleet().charters()); + } + /** * Poll herdr's {@code ping} until it answers or {@link #HERDR_WAIT_SECONDS} elapses (CB-504). * diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java index 3e7e6a7..4549c4b 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java @@ -11,6 +11,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; import java.util.function.Supplier; /** @@ -211,6 +212,24 @@ import java.util.function.Supplier; *

A reload that fails to parse or fails validation is also refused, and the previous config keeps * running. A config file being edited is normally read once mid-save; degrading a working daemon * because it caught a half-written file would be a bad trade. + * + *

fleetd #474 — {@link FleetConfig#validateAll()} is not the only gate startup + * runs before a config takes effect: {@code Fleetd.main} also calls {@code + * dev.ltms.fleet.mcp.CharterToolSurface#assertChartersNameOnlyRegisteredTools}, right after {@code + * cfg.validateAll()}, to refuse a charter that names an MCP tool the server does not register. That + * check cannot live inside {@link FleetConfig} — {@code CharterToolSurface} lives in the {@code mcp} + * package because the canonical tool set ({@code FleetTool}) does, and config is loaded before the + * MCP server exists, so {@code FleetConfig} must not gain a dependency on {@code mcp}. {@link + * #reload} cannot import {@code mcp} either, for the same reason applied one layer up: {@code + * dev.ltms.fleet.config} is loaded before {@code dev.ltms.fleet.mcp} exists, same as {@code + * FleetConfig}. So this class accepts the check as a {@code Consumer} — + * {@link #extraValidation} — supplied by whichever caller already sits at the seam that holds both + * a loaded {@code FleetConfig} and the {@code mcp} package: {@code Fleetd.main}. It is invoked + * inside the same try/catch as {@code fresh.validateAll()}, so a charter that would have refused to + * boot refuses a reload too, and keeps the running config exactly like any other {@code + * validateAll()} failure. A ref built through the two-argument constructor (every test fixture that + * does not care about this check, and {@link #fixed}) gets a no-op consumer, so nothing outside + * {@code Fleetd.main} needs to know this hook exists. */ public final class ConfigRef implements Supplier { @@ -255,10 +274,27 @@ public final class ConfigRef implements Supplier { private final Path path; private final AtomicReference current; + private final Consumer extraValidation; + /** Equivalent to the three-argument constructor with a no-op {@code extraValidation}. */ public ConfigRef(Path path, FleetConfig initial) { + this(path, initial, cfg -> { }); + } + + /** + * @param extraValidation run on every {@link #reload} candidate, inside the same try/catch as + * {@code fresh.validateAll()} — see the class doc's fleetd #474 note. + * {@code Fleetd.main} passes {@code + * Fleetd::assertChartersNameOnlyRegisteredTools} (a package-private + * {@code FleetConfig -> void} adapter over {@code + * CharterToolSurface#assertChartersNameOnlyRegisteredTools}), so a reload + * runs the same gate startup does without this class depending on the + * {@code mcp} package. + */ + public ConfigRef(Path path, FleetConfig initial, Consumer extraValidation) { this.path = path; this.current = new AtomicReference<>(Objects.requireNonNull(initial, "initial config")); + this.extraValidation = Objects.requireNonNull(extraValidation, "extraValidation"); } /** A fixed reference that never reloads — for tests and for wiring built from a config in code. */ @@ -360,6 +396,12 @@ public final class ConfigRef implements Supplier { // at all — see FleetConfig#validateAll's javadoc for why the fix is one reflective call, // not a longer hand-maintained list. fresh.validateAll(); + // fleetd #474: validateAll() does not cover everything startup refuses on — the charter + // tool-surface check (Fleetd.main, right after cfg.validateAll()) lives outside + // FleetConfig on purpose (see this class's doc) and is supplied here as extraValidation. + // Same try/catch as validateAll() above, on purpose: either failure must refuse the whole + // reload and keep the running config the same way. + extraValidation.accept(fresh); } catch (RuntimeException e) { String msg = e.getMessage() == null ? e.toString() : e.getMessage(); log.warn("config reload from {} refused, keeping the running config: {}", path, msg); diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java index 9f4e242..779bfa3 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java @@ -25,6 +25,20 @@ import java.util.regex.Pattern; * {@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. + * + *

fleetd #474 — startup was not the only door: {@code + * dev.ltms.fleet.config.ConfigRef#reload()} used to run {@code FleetConfig#validateAll()} alone, + * which does not look at what a charter's text names, so a charter naming an unregistered tool + * that could not have booted the daemon could still be installed into a running one through a + * reload. This class still knows nothing about {@code ConfigRef} — {@code Fleetd.main} wires a + * small {@code FleetConfig -> void} adapter over {@link #assertChartersNameOnlyRegisteredTools} + * ({@code Fleetd::assertChartersNameOnlyRegisteredTools}) into {@code ConfigRef}'s constructor as + * its {@code Consumer} {@code extraValidation}, run inside {@code reload()}'s same + * try/catch as {@code validateAll()}, so both call sites — {@code Fleetd.main} at startup and + * {@code ConfigRef#reload()} afterwards — go through this one method and can never check different + * things. {@code dev.ltms.fleet.config.ConfigRefTest} and {@code + * FleetdConfigRefCharterToolSurfaceWiringTest} are what prove the reload call site, the same way + * {@code FleetdStartupValidationTest} proves the startup one. */ public final class CharterToolSurface { diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdConfigRefCharterToolSurfaceWiringTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdConfigRefCharterToolSurfaceWiringTest.java new file mode 100644 index 0000000..e7cc966 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdConfigRefCharterToolSurfaceWiringTest.java @@ -0,0 +1,106 @@ +package dev.ltms.fleet; + +import dev.ltms.fleet.config.ConfigRef; +import dev.ltms.fleet.config.FleetConfig; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * fleetd #474: proves the exact wiring {@code Fleetd.main} uses to construct its live {@code + * ConfigRef} — {@code new ConfigRef(configPath, cfg, Fleetd::assertChartersNameOnlyRegisteredTools)} + * — actually makes {@link ConfigRef#reload()} refuse a charter that names an MCP tool the server + * does not register, the same way {@code Fleetd.main} itself refuses one at startup (see {@code + * FleetdStartupValidationTest#mainRefusesACharterNamingAnUnregisteredTool}). + * + *

{@code dev.ltms.fleet.config.ConfigRefTest} pins the same behaviour through a locally-built + * {@code Consumer} adapter that calls the same production {@code CharterToolSurface} + * method, because that test lives in {@code dev.ltms.fleet.config} and cannot see {@code + * Fleetd#assertChartersNameOnlyRegisteredTools} (package-private to {@code dev.ltms.fleet}). This + * class is the companion proof that lives where the real method reference is visible, so the literal + * expression {@code Fleetd::assertChartersNameOnlyRegisteredTools} — not just an equivalent — is + * what gets exercised. {@code Fleetd.main} itself cannot be driven this far in a unit test: every + * fixture in {@code FleetdStartupValidationTest} is deliberately invalid so {@code main} throws + * before opening a socket, binding Javalin, or doing anything else with a real side effect, so a + * test cannot get {@code main} far enough to hold a running daemon it could then reload — this test + * builds the {@code ConfigRef} the same way {@code main} does and drives {@link ConfigRef#reload()} + * directly instead, the same shape {@code FleetdExhaustionDetectionArmedWiringTest} and its + * siblings already use for the rest of {@code Fleetd.main}'s wiring. + */ +class FleetdConfigRefCharterToolSurfaceWiringTest { + + private static final String BASE = """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet + guard: + offSubscriptionHosts: + - gx00.gw + """; + + @Test + void reloadRefusesACharterNamingAnUnregisteredToolThroughFleetdsOwnWiring(@TempDir Path dir) + throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, BASE + """ + fleet: + charters: + dev: | + Send the final handoff through fleet_reply. + """); + ConfigRef config = new ConfigRef(f, FleetConfig.load(f), + Fleetd::assertChartersNameOnlyRegisteredTools); + FleetConfig before = config.get(); + + Files.writeString(f, BASE + """ + fleet: + charters: + dev: | + Send the final handoff through bridge_send. + """); + ConfigRef.Outcome out = config.reload(); + + assertFalse(out.applied()); + assertNotNull(out.error()); + assertTrue(out.error().contains("dev"), out.error()); + assertTrue(out.error().contains("bridge_send"), out.error()); + assertSame(before, config.get()); + } + + @Test + void reloadAcceptsACharterNamingOnlyRegisteredToolsThroughFleetdsOwnWiring(@TempDir Path dir) + throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, BASE + """ + fleet: + charters: + dev: old charter + """); + ConfigRef config = new ConfigRef(f, FleetConfig.load(f), + Fleetd::assertChartersNameOnlyRegisteredTools); + + Files.writeString(f, BASE + """ + fleet: + charters: + dev: | + Send the final handoff through fleet_reply. + """); + ConfigRef.Outcome out = config.reload(); + + assertTrue(out.applied()); + assertEquals("config reloaded", out.summary()); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java index 6382405..4cf81ba 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java @@ -1,10 +1,13 @@ package dev.ltms.fleet.config; +import dev.ltms.fleet.mcp.CharterToolSurface; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Map; +import java.util.function.Consumer; import static org.junit.jupiter.api.Assertions.*; @@ -38,6 +41,23 @@ class ConfigRefTest { return new ConfigRef(f, FleetConfig.load(f)); } + /** + * The exact {@code Consumer} {@code Fleetd.main} wires into {@code ConfigRef}'s + * constructor as {@code extraValidation} (fleetd #474) — an adapter from {@code FleetConfig} to + * the raw charter map {@link CharterToolSurface#assertChartersNameOnlyRegisteredTools} takes. + * Built here rather than referencing {@code dev.ltms.fleet.Fleetd} directly, because that method + * is package-private to {@code dev.ltms.fleet} and this test lives in {@code + * dev.ltms.fleet.config} — but it calls the SAME production {@link CharterToolSurface} method + * {@code Fleetd} calls, so this proves the real check runs on reload, not a stand-in for it. + */ + private static final Consumer CHARTER_TOOL_SURFACE = cfg -> + CharterToolSurface.assertChartersNameOnlyRegisteredTools( + cfg.fleet() == null ? Map.of() : cfg.fleet().charters()); + + private static ConfigRef refForWithCharterToolSurface(Path f) { + return new ConfigRef(f, FleetConfig.load(f), CHARTER_TOOL_SURFACE); + } + @Test void aHotChangeIsAppliedAndReadThroughGet(@TempDir Path dir) throws Exception { Path f = dir.resolve("fleetd.yaml"); @@ -124,6 +144,87 @@ class ConfigRefTest { assertSame(before, ref.get()); } + /** + * fleetd #474: {@code validateAll()} (via {@code validateCharters()}) 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 + * names, so a charter naming {@code bridge_send} (the pre-CB-634 name, removed from the tool + * surface — #469's own motivating example) passes {@code validateAll()} and used to be applied + * on reload with nothing refusing it, even though the identical charter refuses {@code + * Fleetd.main} at startup ({@code FleetdStartupValidationTest + * #mainRefusesACharterNamingAnUnregisteredTool}). This is the reload-path proof: it drives + * {@link ConfigRef#reload()} itself (not a direct call to {@link + * CharterToolSurface#assertChartersNameOnlyRegisteredTools}), through the exact {@code + * extraValidation} wiring {@code Fleetd.main} uses, and the failure message must name both the + * charter key and the unknown tool — the same information the startup failure gives (ticket + * acceptance criterion 1). + */ + @Test + void aReloadRefusesACharterNamingAnUnregisteredTool(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(""" + fleet: + charters: + dev: | + Send the final handoff through fleet_reply. + """)); + ConfigRef ref = refForWithCharterToolSurface(f); + FleetConfig before = ref.get(); + + Files.writeString(f, yaml(""" + fleet: + charters: + dev: | + Send the final handoff through bridge_send. + """)); + ConfigRef.Outcome out = ref.reload(); + + assertFalse(out.applied()); + assertNotNull(out.error()); + assertTrue(out.error().contains("dev"), + "expected the charter key 'dev' in the refusal, got: " + out.error()); + assertTrue(out.error().contains("bridge_send"), + "expected the unknown tool 'bridge_send' in the refusal, got: " + out.error()); + assertTrue(out.summary().startsWith("config reload refused"), out.summary()); + // The running config must not move at all — half-applying this would leave the daemon in a + // state that could never have booted, exactly the outcome ConfigRef.java's class doc warns + // a cold-key refusal must avoid, and this check must avoid the same way. + assertSame(before, ref.get()); + assertEquals("Send the final handoff through fleet_reply.\n", + ref.get().fleet().charterFor(dev.ltms.fleet.peer.MemberRole.DEV)); + } + + /** + * fleetd #474 acceptance criterion 3, the positive case: a reload whose charter names only + * registered tools must still be ACCEPTED. An inverted filter (one that refuses every charter, + * or refuses on any {@code fleet_*}/{@code bridge_*} token regardless of registration) would pass + * the refusal test above alone — #469's own M5 mutation cell showed exactly that shape surviving + * a negative-only suite. This is the test that catches it. + */ + @Test + void aReloadAcceptsACharterNamingOnlyRegisteredTools(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(""" + fleet: + charters: + dev: old charter, no tool names + """)); + ConfigRef ref = refForWithCharterToolSurface(f); + + Files.writeString(f, yaml(""" + fleet: + charters: + dev: | + Send the final handoff through fleet_reply, using fleet_send to delegate. + """)); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertTrue(out.deferred().isEmpty(), out.deferred().toString()); + assertEquals("config reloaded", out.summary()); + assertEquals("Send the final handoff through fleet_reply, using fleet_send to delegate.\n", + ref.get().fleet().charterFor(dev.ltms.fleet.peer.MemberRole.DEV)); + } + /** * The point of the whole class: a consumer holding the ref sees the new value without being * rebuilt. A component that captured {@code get()} into a field would still show the old one.