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 419253a..f3e8b5c 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java @@ -39,12 +39,18 @@ import java.util.function.Supplier; * keeps the old value until a restart: {@code lifecycle:}, {@code leadHeartbeat:}, * {@code spawnReadyTimeoutMs} / {@code spawnReadyPollMs}, {@code quarantineCooldownSeconds} * (CB-578 stage B — baked once into the {@code BackendQuarantine} built at startup), - * {@code guard:}, {@code worktreeRoot:}, adding or removing a profile (a new backend needs its own launcher, + * {@code guard:}, {@code worktreeRoot:} and {@code worktreeGroup:} (both baked once into the + * {@code GitWorktrees} built at {@code Fleetd.java:251} and never rebuilt — fleetd #323 + * instance 2 found {@code worktreeGroup} missing from this list and from + * {@link #changedDeferredKeys}), adding or removing a profile (a new backend needs its own launcher, * which is constructed once), and an existing profile's launch settings — * {@code model}, {@code baseUrl}, {@code argv}, {@code env}, {@code mcpUrl}, * {@code exhaustedPattern} (CB-578 stage A — compiled once into {@code Fleetd.main}'s * pattern map at startup), {@code errorPattern} (fleetd #201 Unit 5 — compiled once into - * {@code Fleetd.main}'s backend-error pattern map at startup, the same way), and the rest. + * {@code Fleetd.main}'s backend-error pattern map at startup, the same way), + * {@code ideProjectDir} / {@code ideOpenCommand} / {@code autoCompactWindow} (fleetd #323 + * instance 1 — all three are read at spawn off the same frozen profile map and were missing + * from {@link #sameLaunchSettings}), and the rest of {@link #sameLaunchSettings}. * {@code credentialId} (CB-578 stage B) is NOT on * this list — it is read live off the config supplier at every quarantine check and * exhaustion event, exactly like {@code weight} / {@code maxLoad}, so it is hot instead. @@ -221,6 +227,13 @@ public final class ConfigRef implements Supplier { if (!Objects.equals(old.worktreeRoot(), fresh.worktreeRoot())) { changed.add("worktreeRoot"); } + // Baked into the same GitWorktrees as worktreeRoot (Fleetd.java:251) and never rebuilt + // either — see the class doc. Missing this check was fleetd #323 instance 2: a reload + // that changed only worktreeGroup reported "config reloaded" with nothing deferred, and + // newly provisioned worktrees kept the old sharing behaviour. + if (!Objects.equals(old.worktreeGroup(), fresh.worktreeGroup())) { + changed.add("worktreeGroup"); + } if (!Objects.equals(old.spawnReadyTimeoutMs(), fresh.spawnReadyTimeoutMs()) || !Objects.equals(old.spawnReadyPollMs(), fresh.spawnReadyPollMs())) { changed.add("spawnReady*"); @@ -265,14 +278,34 @@ public final class ConfigRef implements Supplier { } /** - * Whether two versions of a profile would launch a peer identically. Compares every component - * the launcher reads at spawn; {@code weight}, {@code maxLoad} and {@code credentialId} are - * excluded because those are read live (by the placement policy and, for credentialId, by - * {@code CompositePeerLauncher}/the CB-578 stage B exhaustion sink) and really do take effect on - * the next spawn. + * {@link FleetConfig.Profile} record components deliberately left out of + * {@link #sameLaunchSettings} because they are read live, not baked in at spawn — see + * the class doc's Hot bullet. {@code weight} and {@code maxLoad} are read live by the + * placement policy on every spawn; {@code credentialId} is read live by + * {@code CompositePeerLauncher} and the CB-578 stage B exhaustion sink. Nothing else is + * excluded — see {@code sameLaunchSettingsComparesEveryProfileComponentOrExcludesIt} in + * {@code ConfigRefProfileCoverageTest}, which enumerates every {@code Profile} record component + * by reflection and fails the build if one is neither compared below nor named here. */ - private static boolean sameLaunchSettings(FleetConfig.Profile a, FleetConfig.Profile b) { - return Objects.equals(a.baseUrl(), b.baseUrl()) + static final Set LAUNCH_SETTINGS_EXCLUDED = Set.of("weight", "maxLoad", "credentialId"); + + /** + * Whether two versions of a profile would launch a peer identically. + * + *

This must compare every {@link FleetConfig.Profile} record component except the three in + * {@link #LAUNCH_SETTINGS_EXCLUDED}. That is not a claim this javadoc can make good on by + * itself — a javadoc saying "compares every component" is exactly what fleetd #323 found to be + * false for three fields (and a sibling method's field list, for a fourth). The actual + * guarantee comes from {@code ConfigRefProfileCoverageTest}: it enumerates every record + * component of {@code FleetConfig.Profile} by reflection, mutates each one not in + * {@code LAUNCH_SETTINGS_EXCLUDED} on a base profile, and asserts this method reports a + * difference — so a new component that is neither compared here nor added to + * {@code LAUNCH_SETTINGS_EXCLUDED} (with a reason) fails that test by name, rather than + * silently reporting "config reloaded" for a value the daemon never picked up. + */ + static boolean sameLaunchSettings(FleetConfig.Profile a, FleetConfig.Profile b) { + return Objects.equals(a.profile(), b.profile()) + && Objects.equals(a.baseUrl(), b.baseUrl()) && Objects.equals(a.model(), b.model()) && Objects.equals(a.configDir(), b.configDir()) && Objects.equals(a.tokenEnv(), b.tokenEnv()) @@ -298,6 +331,16 @@ public final class ConfigRef implements Supplier { // fleetd #201 Unit 5: errorPattern is compiled once into Fleetd.main's backend-error // pattern map at startup (see BackendErrorPatternLookup wiring), the same way // exhaustedPattern is — a reload never re-reads it either. - && Objects.equals(a.errorPattern(), b.errorPattern()); + && Objects.equals(a.errorPattern(), b.errorPattern()) + // fleetd #323 instance 1: ideProjectDir and ideOpenCommand are read at spawn off the + // same frozen profile map as ideMcpUrl above (ClaudeCodeLauncher.java:267/269, + // OpenCodeLauncher.java:474/480/486) and were missing from this comparison. + && Objects.equals(a.ideProjectDir(), b.ideProjectDir()) + && Objects.equals(a.ideOpenCommand(), b.ideOpenCommand()) + // fleetd #323 instance 1: autoCompactWindow is read at spawn the same way + // (ClaudeCodeLauncher.java:926, OpenCodeLauncher.java:650). Comparing it here only + // makes the reload REPORT that a restart is needed — it deliberately does not make + // autoCompactWindow take effect live, which is a separate, larger change. + && Objects.equals(a.autoCompactWindow(), b.autoCompactWindow()); } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefProfileCoverageTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefProfileCoverageTest.java new file mode 100644 index 0000000..cda5530 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefProfileCoverageTest.java @@ -0,0 +1,187 @@ +package dev.ltms.fleet.config; + +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Constructor; +import java.lang.reflect.RecordComponent; +import java.util.Arrays; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeSet; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * fleetd #323: {@code ConfigRef.sameLaunchSettings} javadoc used to claim it "compares every + * component the launcher reads at spawn". It did not — {@code ideProjectDir}, {@code + * ideOpenCommand} and {@code autoCompactWindow} were all baked in at daemon startup (see + * {@code ClaudeCodeLauncher}/{@code OpenCodeLauncher}) and missing from the comparison, so a reload + * that changed only one of them reported "config reloaded" and the running daemon kept the old + * value. + * + *

This class is the mechanism the issue asked for: it enumerates every record component of + * {@link FleetConfig.Profile} by reflection and proves — by actually mutating a base profile one + * field at a time and calling the real method — that each component is either compared by + * {@link ConfigRef#sameLaunchSettings} or named in {@link ConfigRef#LAUNCH_SETTINGS_EXCLUDED} with + * a reason. A new profile field that is neither fails this test by name, not a hand-maintained list + * going stale. + */ +class ConfigRefProfileCoverageTest { + + private static final RecordComponent[] COMPONENTS = FleetConfig.Profile.class.getRecordComponents(); + + /** + * One valid, non-blank value per record component — "the a value". None of these trip any + * defaulting/normalization in {@code Profile}'s compact constructor (see {@code + * FleetConfig.java}), so what goes in is what {@code sameLaunchSettings} sees back out. + */ + private static final Map BASE = baseValues(); + + /** The same shape, each value distinct from {@link #BASE} — "the b value". */ + private static final Map ALT = altValues(); + + private static Map baseValues() { + Map v = new LinkedHashMap<>(); + v.put("profile", "sonnet"); + v.put("baseUrl", "http://gx00.gw:8000"); + v.put("model", "sonnet"); + v.put("configDir", "/config/a"); + v.put("tokenEnv", "TOKEN_A"); + v.put("argv", List.of("claude", "--flag-a")); + v.put("placement", "tab"); + v.put("workspace", "workspace-a"); + v.put("tabLabel", "label-a"); + v.put("mcpUrl", "http://mcp-a"); + v.put("cwd", "/cwd/a"); + v.put("parityOverlay", List.of(".env", ".env.a")); + v.put("gitTokenEnv", "GIT_TOKEN_A"); + v.put("gitHostEnv", "GITEA_HOST_A"); + v.put("kind", "claude-code"); + v.put("env", Map.of("K", "A")); + v.put("weight", 1.0f); + v.put("maxLoad", 5); + v.put("subscription", Boolean.TRUE); + v.put("exhaustedPattern", "usage limit a"); + v.put("credentialId", "cred-a"); + v.put("ideMcpUrl", "http://ide-mcp-a"); + v.put("ideProjectDir", "modules/a"); + v.put("ideOpenCommand", "open-cmd-a {dir}"); + v.put("autoCompactWindow", 150000); + v.put("errorPattern", "error a"); + assertNamesMatchComponents(v); + return v; + } + + private static Map altValues() { + Map v = new LinkedHashMap<>(); + v.put("profile", "sonnet-b"); + v.put("baseUrl", "http://gx01.gw:8000"); + v.put("model", "haiku"); + v.put("configDir", "/config/b"); + v.put("tokenEnv", "TOKEN_B"); + v.put("argv", List.of("claude", "--flag-b")); + v.put("placement", "weighted"); + v.put("workspace", "workspace-b"); + v.put("tabLabel", "label-b"); + v.put("mcpUrl", "http://mcp-b"); + v.put("cwd", "/cwd/b"); + v.put("parityOverlay", List.of(".env", ".env.b")); + v.put("gitTokenEnv", "GIT_TOKEN_B"); + v.put("gitHostEnv", "GITEA_HOST_B"); + v.put("kind", "opencode"); + v.put("env", Map.of("K", "B")); + v.put("weight", 2.0f); + v.put("maxLoad", 9); + v.put("subscription", Boolean.FALSE); + v.put("exhaustedPattern", "usage limit b"); + v.put("credentialId", "cred-b"); + v.put("ideMcpUrl", "http://ide-mcp-b"); + v.put("ideProjectDir", "modules/b"); + v.put("ideOpenCommand", "open-cmd-b {dir}"); + v.put("autoCompactWindow", 250000); + v.put("errorPattern", "error b"); + assertNamesMatchComponents(v); + return v; + } + + private static void assertNamesMatchComponents(Map values) { + Set componentNames = new TreeSet<>(); + for (RecordComponent rc : COMPONENTS) { + componentNames.add(rc.getName()); + } + assertEquals(componentNames, new TreeSet<>(values.keySet()), + "this test's value map has drifted from FleetConfig.Profile's actual components — " + + "update BASE/ALT alongside the record"); + } + + private static FleetConfig.Profile profileOf(Map values) throws ReflectiveOperationException { + Class[] types = Arrays.stream(COMPONENTS).map(RecordComponent::getType).toArray(Class[]::new); + Object[] args = Arrays.stream(COMPONENTS) + .map(rc -> values.get(rc.getName())) + .toArray(); + Constructor ctor = FleetConfig.Profile.class.getDeclaredConstructor(types); + return ctor.newInstance(args); + } + + /** {@code BASE} with exactly one named component swapped for its {@code ALT} value. */ + private static FleetConfig.Profile mutate(String componentName) throws ReflectiveOperationException { + Map values = new LinkedHashMap<>(BASE); + values.put(componentName, ALT.get(componentName)); + return profileOf(values); + } + + /** + * The mechanism fleetd #323 asked for: enumerate {@link FleetConfig.Profile}'s record + * components, mutate each non-excluded one, and prove {@code sameLaunchSettings} actually + * notices — not just that some hand-maintained list claims it does. Prints the denominator + * (total / compared / excluded) the issue required: a checker that cannot state its own + * denominator is the failure this repo keeps hitting. + */ + @Test + void sameLaunchSettingsComparesEveryProfileComponentOrExcludesIt() throws ReflectiveOperationException { + int total = COMPONENTS.length; + Set excluded = ConfigRef.LAUNCH_SETTINGS_EXCLUDED; + + Set allNames = new TreeSet<>(); + for (RecordComponent rc : COMPONENTS) { + allNames.add(rc.getName()); + } + assertTrue(allNames.containsAll(excluded), + "ConfigRef.LAUNCH_SETTINGS_EXCLUDED names a component that does not exist on " + + "FleetConfig.Profile — check for a typo: " + excluded); + + FleetConfig.Profile base = profileOf(BASE); + List uncovered = new java.util.ArrayList<>(); + int compared = 0; + for (RecordComponent rc : COMPONENTS) { + String name = rc.getName(); + if (excluded.contains(name)) { + continue; + } + FleetConfig.Profile mutated = mutate(name); + if (ConfigRef.sameLaunchSettings(base, mutated)) { + uncovered.add(name); + } else { + compared++; + } + } + + System.out.printf( + "ConfigRef.sameLaunchSettings coverage — %d Profile components total, %d compared, " + + "%d excluded (%s)%n", + total, compared, excluded.size(), excluded); + + assertEquals(List.of(), uncovered, + "these FleetConfig.Profile components changed but ConfigRef.sameLaunchSettings " + + "reported no difference — add each one to the comparison (it is read at " + + "spawn and baked in until a restart) or to ConfigRef.LAUNCH_SETTINGS_EXCLUDED " + + "with a reason it is genuinely read live: " + uncovered); + assertEquals(total, compared + excluded.size(), + "every FleetConfig.Profile record component must be either compared or excluded — " + + total + " components, " + compared + " compared, " + excluded.size() + + " excluded"); + } +} 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 dceccd6..2de9750 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java @@ -434,6 +434,78 @@ class ConfigRefTest { assertEquals("provider 5xx", ref.get().profiles().get("sonnet").errorPattern()); } + /** + * fleetd #323 instance 1: {@code ideProjectDir} is read at spawn off the frozen profile map + * (see {@code ClaudeCodeLauncher}/{@code OpenCodeLauncher}) exactly like {@code model}, but was + * missing from {@code sameLaunchSettings} — a reload changing only this field used to report a + * bare "config reloaded" and the running daemon kept launching with the old value. + */ + @Test + void changingAProfilesIdeProjectDirIsReportedAsDeferred(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet + ideProjectDir: fleetd + guard: + offSubscriptionHosts: + - gx00.gw + """); + ConfigRef ref = refFor(f); + + Files.writeString(f, """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet + ideProjectDir: fleetd-renamed + guard: + offSubscriptionHosts: + - gx00.gw + """); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertEquals(1, out.deferred().size(), out.deferred().toString()); + assertTrue(out.deferred().getFirst().contains("sonnet"), out.deferred().toString()); + assertTrue(out.deferred().getFirst().contains("launch settings"), out.deferred().toString()); + // The snapshot still carries the new value — a restart is what makes it take effect. + assertEquals("fleetd-renamed", ref.get().profiles().get("sonnet").ideProjectDir()); + } + + /** + * fleetd #323 instance 2: {@code worktreeGroup} is baked into the same {@code GitWorktrees} + * as {@code worktreeRoot} (Fleetd.java:251) and never rebuilt, but only {@code worktreeRoot} + * was on {@code changedDeferredKeys} — a reload changing only the group reported a bare + * "config reloaded" and newly provisioned worktrees kept the old sharing behaviour. + */ + @Test + void changingWorktreeGroupIsReportedAsDeferred(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml("worktreeGroup: devgroup\n")); + ConfigRef ref = refFor(f); + + Files.writeString(f, yaml("worktreeGroup: devgroup2\n")); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertEquals(java.util.List.of("worktreeGroup"), out.deferred()); + assertTrue(out.summary().contains("needs a restart") || out.summary().contains("need a restart"), + out.summary()); + // The snapshot still carries the new value — a restart is what makes it take effect. + assertEquals("devgroup2", ref.get().worktreeGroup()); + } + @Test void aFixedRefHasNoFileAndRefusesToReload() { FleetConfig cfg = new FleetConfig(null, null, null, null, null, null,