fleetd #474: ConfigRef.reload() runs the charter tool-surface gate too
CI / build (pull_request) Successful in 1m25s
CI / contract (pull_request) Successful in 1m39s

A charter naming an MCP tool the server does not register refused Fleetd.main
at startup but slipped through ConfigRef.reload(), because reload() only ran
FleetConfig.validateAll(), which never looks at what a charter's text names.

CharterToolSurface stays in the mcp package (config must not depend on it), so
ConfigRef now accepts the check as a Consumer<FleetConfig> extraValidation,
run inside reload()'s same try/catch as validateAll(). Fleetd.main wires a new
package-private adapter, Fleetd.assertChartersNameOnlyRegisteredTools, into
both the startup call site and ConfigRef's constructor, so the two call sites
can never check different things.

Tests: ConfigRefTest (reload refuses/accepts, via a locally-built equivalent
consumer since Fleetd's method is package-private to dev.ltms.fleet) and the
new FleetdConfigRefCharterToolSurfaceWiringTest (same proof through the exact
Fleetd::assertChartersNameOnlyRegisteredTools reference production uses).
Verified deleting the new extraValidation.accept(fresh) call site fails both
new "refuses" tests by name.
This commit is contained in:
Dai Ha
2026-09-10 20:25:23 +07:00
parent 435e022560
commit 97e4c1d658
5 changed files with 292 additions and 4 deletions
@@ -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<FleetConfig>}
* 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}.
*
* <p>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<FleetConfig>} 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).
*
@@ -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;
* <p>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.
*
* <p><strong>fleetd #474</strong> — {@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<FleetConfig>} —
* {@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<FleetConfig> {
@@ -255,10 +274,27 @@ public final class ConfigRef implements Supplier<FleetConfig> {
private final Path path;
private final AtomicReference<FleetConfig> current;
private final Consumer<FleetConfig> 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<FleetConfig> 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<FleetConfig> {
// 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);
@@ -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.
*
* <p><strong>fleetd #474</strong> — 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<FleetConfig>} {@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 {
@@ -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}).
*
* <p>{@code dev.ltms.fleet.config.ConfigRefTest} pins the same behaviour through a locally-built
* {@code Consumer<FleetConfig>} 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());
}
}
@@ -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<FleetConfig>} {@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<FleetConfig> 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.