From 3aca53b967a9774356f68962941d772630928e9f Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 15:36:40 +0700 Subject: [PATCH] fleetd#333: fleet.leaders is split too, and split membership now proves reporting exists F1: fleet: was sitting in ConfigRefTopLevelCoverageTest's HOT_EXCLUDED_TOP_LEVEL_KEYS escape hatch, even though fleet.leaders is read only at startup (LeadTabScanner's identity map, LeadLauncher.ensureLeads) while the rest of fleet: (role pools, charters, tabLabel) is live. A reload changing only fleet.leaders reported a bare "config reloaded" -- the operator edits a lead's tab: label, sees the reload succeed, and the pane keeps resolving as a worker. Moved fleet into ConfigRef.SPLIT_KEYS; changedSplitKeys now compares fleet.leaders specifically (not the whole Fleet record, which would over-claim "restart" for a tabLabel-only change) and names both halves in the message. F2: membership in SPLIT_KEYS/COLD_KEYS never proved a matching branch existed in changedSplitKeys/changedColdKeys -- measured by dropping the coordinator branch while leaving "coordinator" in SPLIT_KEYS: both ConfigRefTopLevelCoverageTest and the in-method "kept in step" assert stayed green. Added ConfigRefTopLevelReportingCoverageTest, the ConfigRefProfileCoverageTest mechanism one level up: reflection-built FleetConfig pairs that differ in exactly one top-level component, calling the real (now package-private) changedColdKeys/ changedSplitKeys to prove each COLD_KEYS/SPLIT_KEYS member is actually reported. Scoped to split+cold, not deferred -- see the new test's javadoc for why and what that leaves open. Both findings carry a behavioural test in ConfigRefTest plus a mutation proof (revert -> real failure -> restore) recorded in the PR description. --- .../java/dev/ltms/fleet/config/ConfigRef.java | 153 +++++++++--- .../dev/ltms/fleet/config/ConfigRefTest.java | 73 ++++++ .../config/ConfigRefTopLevelCoverageTest.java | 22 +- ...onfigRefTopLevelReportingCoverageTest.java | 219 ++++++++++++++++++ 4 files changed, 423 insertions(+), 44 deletions(-) create mode 100644 fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java 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 f736af9..09d2e73 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java @@ -27,14 +27,14 @@ import java.util.function.Supplier; * * * - *

The denominator, measured on 2026-09-04 (fleetd #330). {@code FleetConfig} has - * 22 top-level record components. Two of them are named nowhere in this file, and the reason is the - * same for both: {@code memberCredentials} and {@code memberLoginShell} are hot and - * correctly absent — both are read live off {@code config.get()} at spawn time - * ({@code Fleetd.java:198, 205, 729} and {@code HerdrPeerLauncher#configuredMemberLoginShell}), so a - * reload takes effect on the next spawn with no entry needed here. + *

The denominator, measured on 2026-09-04 (fleetd #330; recounted for fleetd #333). + * {@code FleetConfig} has 22 top-level record components: 5 cold, 11 deferred, 3 split, 3 + * hot-excluded. Three of them are named nowhere in this file, and the reason is the same for all + * three: {@code placement}, {@code memberCredentials} and {@code memberLoginShell} are + * hot and correctly absent — all three are read live off {@code config.get()} + * (placement through the {@code CompositePeerLauncher} supplier the Hot bullet names; + * {@code memberCredentials}/{@code memberLoginShell} at spawn time, {@code Fleetd.java:198, 205, 729} + * and {@code HerdrPeerLauncher#configuredMemberLoginShell}), so a reload takes effect on the next + * spawn with no entry needed here. * {@code health} and {@code coordinator} used to be a third kind — undecided, not * hot — until fleetd #330 added the split class above and gave them a home. A * reload touching either used to report a bare "config reloaded", which under-claimed; now it names - * the key and says which half is which. + * the key and says which half is which. {@code fleet} was the same story in reverse: fleetd #330's + * own fact-find named {@code health}/{@code coordinator} as "the complete set of split-shaped keys" + * and filed {@code fleet.leaders}'s restart requirement as a documented caveat sitting in the + * hot-excluded escape hatch instead — correctly documented, but in the one bucket + * this file's own coverage test cannot check the truth of (see that test's javadoc). fleetd #333 + * moved it into split, where {@link #changedSplitKeys} actually reports it. *

The point of writing the count down: "not mentioned in this file" looks identical for a key - * that is correctly hot and for a key nobody triaged. Twice now — {@code worktreeGroup} (#323) and - * {@code primary}/{@code configReload} (#326) — the second kind hid among the first. A top-level - * coverage checker in the {@link ConfigRefProfileCoverageTest} shape (one level up, over - * {@code FleetConfig} itself rather than {@code FleetConfig.Profile}) proves this file's four - * classes exhaust the record's components — see {@code ConfigRefTopLevelCoverageTest}. + * that is correctly hot and for a key nobody triaged. Three times now — {@code worktreeGroup} (#323), + * {@code primary}/{@code configReload} (#326), and {@code fleet.leaders} sitting in the escape hatch + * (#333) — the second kind hid among the first. A top-level coverage checker in the + * {@link ConfigRefProfileCoverageTest} shape (one level up, over {@code FleetConfig} itself rather + * than {@code FleetConfig.Profile}) proves this file's four classes exhaust the record's components + * — see {@code ConfigRefTopLevelCoverageTest}. That test proves the record's shape is fully + * triaged; it does NOT prove a {@code SPLIT_KEYS}/{@code COLD_KEYS} member has any reporting code + * behind it at all — {@code ConfigRefTopLevelReportingCoverageTest} is what fleetd #333 added for + * that, after measuring that a {@code SPLIT_KEYS} entry with its reporting branch deleted passes + * both this file's own "kept in step" assert and {@code ConfigRefTopLevelCoverageTest} unchanged. * *

A cold change refuses the whole reload. Not the hot half applied and the cold * half warned about: that would leave the running daemon in a state matching no file on disk, which @@ -153,7 +189,7 @@ public final class ConfigRef implements Supplier { * {@code true}) and reported by name, with a message naming which half is live and which needs * a restart. */ - static final Set SPLIT_KEYS = Set.of("health", "coordinator"); + static final Set SPLIT_KEYS = Set.of("health", "coordinator", "fleet"); private final Path path; private final AtomicReference current; @@ -283,8 +319,14 @@ public final class ConfigRef implements Supplier { return out; } - /** Cold keys whose value differs between the running config and the candidate. */ - private static List changedColdKeys(FleetConfig old, FleetConfig fresh) { + /** + * Cold keys whose value differs between the running config and the candidate. + * + *

Package-private (not {@code private}) so {@code ConfigRefTopLevelReportingCoverageTest} + * can call it directly with a reflection-built {@code FleetConfig} pair, the same reason + * {@link #sameLaunchSettings} is package-private — see that test's class doc (fleetd #333). + */ + static List changedColdKeys(FleetConfig old, FleetConfig fresh) { List changed = new ArrayList<>(); if (!Objects.equals(old.bind(), fresh.bind())) { changed.add("bind"); @@ -389,12 +431,24 @@ public final class ConfigRef implements Supplier { /** * Split keys whose value differs between the running config and the candidate — see the class - * doc's Split bullet (fleetd #330). Unlike {@link #changedDeferredKeys}, this does not try to - * tell which sub-field moved: any change to {@code health:} or {@code coordinator:} gets the - * same fixed message, because the message already names both halves every time, so there is no - * "which half changed" question left for the caller to answer. + * doc's Split bullet (fleetd #330, extended for {@code fleet:} by fleetd #333). Unlike + * {@link #changedDeferredKeys}, this does not try to tell which sub-field moved for {@code + * health:} or {@code coordinator:}: any change to either gets the same fixed message, because + * the message already names both halves every time, so there is no "which half changed" + * question left for the caller to answer. {@code fleet:} is different on purpose — see below. + * + *

Package-private (not {@code private}) so {@code ConfigRefTopLevelReportingCoverageTest} + * can call it directly with a reflection-built {@code FleetConfig} pair, the same reason + * {@link #sameLaunchSettings} is package-private — see that test's class doc (fleetd #333). That + * test exists because membership in {@link #SPLIT_KEYS} proves nothing about this method on its + * own: fleetd #333 measured that dropping the {@code coordinator} branch out of this method + * while leaving {@code "coordinator"} in {@code SPLIT_KEYS} left the whole suite green except a + * hand-written {@code ConfigRefTest} case — neither {@code ConfigRefTopLevelCoverageTest} (it + * only reads the set) nor the "kept in step" assert below (it only checks the reported keys are + * a SUBSET of {@code SPLIT_KEYS}, never that every {@code SPLIT_KEYS} member has a branch here) + * would have caught it. */ - private static List changedSplitKeys(FleetConfig old, FleetConfig fresh) { + static List changedSplitKeys(FleetConfig old, FleetConfig fresh) { List changed = new ArrayList<>(); if (!Objects.equals(old.health(), fresh.health())) { changed.add("health: the monitor itself (enabled, interval, workingSuspectAfter) is " @@ -406,13 +460,42 @@ public final class ConfigRef implements Supplier { + "opened once and needs a restart; the broker URI env-var name kept out of a " + "member's environment is read live on every spawn and already applied"); } + // fleetd #333: unlike health/coordinator above, most of `fleet:` (architects, developers, + // reviewers, charters, tabLabel) is genuinely hot — ConfigRefTest.aHotChangeIsAppliedAndRead- + // ThroughGet and aCharterChangeIsHotAndReachesTheLiveConfig prove it reaches the live config + // with no restart note. Only fleet.leaders is frozen (Fleetd.java:281 reads + // cfg.fleet().leaders() off the startup snapshot to build both the LeadTabScanner's + // tab-label-to-name map, wired into CallerResolver.withLeadsAndMembers at Fleetd.java:620/624, + // and — when herdr answered — LeadLauncher(...).ensureLeads() at Fleetd.java:315, which + // auto-launches each lead up to its `instances` count; neither is rebuilt on reload). So this + // compares fleet.leaders alone, not the whole Fleet record: comparing the whole record would + // report "split" for a tabLabel-only or charters-only change that is actually fully hot, + // which is the over-claim mirror of the under-claim bug this class exists to prevent. + if (!Objects.equals(leadersOf(old), leadersOf(fresh))) { + changed.add("fleet: fleet.leaders (each lead's tab, workspace, cwd, profile and " + + "instances count) is read once at startup to build the LeadTabScanner's " + + "identity map and to auto-launch leads, and neither is rebuilt on reload, so a " + + "lead added, removed, or given a new tab: label needs a restart — until then it " + + "stays unrecognised, and a caller from its new tab resolves as a worker, not a " + + "lead; the rest of fleet: (architects, developers, reviewers, charters, " + + "tabLabel) is read live through the supplier on CompositePeerLauncher and " + + "already applied"); + } // Kept in step with SPLIT_KEYS the same way changedColdKeys is kept in step with COLD_KEYS — - // every message here must be traceable to one of the two split keys the class doc documents. + // every message here must be traceable to one of the split keys the class doc documents. + // NOTE what this does NOT prove, per the javadoc above: it does not catch a SPLIT_KEYS + // member with no branch above at all, only a branch whose message is mis-worded relative to + // the set. ConfigRefTopLevelReportingCoverageTest is what proves the former. assert changed.stream().allMatch(m -> SPLIT_KEYS.stream().anyMatch(k -> m.startsWith(k + ":"))) : "a split entry was reported that does not start with a SPLIT_KEYS name: " + changed; return changed; } + /** {@code cfg.fleet().leaders()}, defensively, in case a caller hands in a non-defaulted config. */ + private static Map leadersOf(FleetConfig cfg) { + return cfg.fleet() == null ? Map.of() : cfg.fleet().leaders(); + } + /** * {@link FleetConfig.Profile} record components deliberately left out of * {@link #sameLaunchSettings} because they are read live, not baked in at spawn — see 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 b5411a9..e2a653b 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java @@ -638,6 +638,79 @@ class ConfigRefTest { assertEquals("mac-b", ref.get().coordinator().selfId()); } + /** + * fleetd #333: {@code fleet:} is split too — {@code fleet.leaders} is read only at + * {@code Fleetd.java:281} to build the {@code LeadTabScanner}'s identity map (fed into + * {@code CallerResolver}) and to auto-launch leads via {@code LeadLauncher.ensureLeads()}, and + * neither is rebuilt on reload, while the rest of {@code fleet:} (role pools, charters, + * tabLabel) is read live through the {@code CompositePeerLauncher} supplier. Before this fix + * {@code fleet} sat in the coverage checker's hot-excluded escape hatch, so a reload that + * changed only {@code fleet.leaders} reported a bare "config reloaded" — exactly the + * under-claim the split class exists to prevent for {@code health}/{@code coordinator}. + */ + @Test + void changingFleetLeadersIsReportedAsSplit(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(""" + fleet: + leaders: + opus: + tab: "lead: opus-a" + profile: sonnet + """)); + ConfigRef ref = refFor(f); + + Files.writeString(f, yaml(""" + fleet: + leaders: + opus: + tab: "lead: opus-b" + profile: sonnet + """)); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertTrue(out.deferred().isEmpty(), out.deferred().toString()); + assertEquals(1, out.split().size(), out.split().toString()); + assertTrue(out.split().getFirst().startsWith("fleet:"), out.split().toString()); + assertTrue(out.split().getFirst().contains("restart"), out.split().toString()); + assertTrue(out.split().getFirst().contains("live"), out.split().toString()); + assertTrue(out.summary().contains("partially live"), out.summary()); + // The snapshot still carries the new value — the LeadTabScanner's identity map and + // LeadLauncher's auto-launch are what wait for a restart; a reload rebuilds neither. + assertEquals("lead: opus-b", ref.get().fleet().leaders().get("opus").tab()); + } + + /** + * fleetd #333: {@code fleet.leaders} is the ONLY frozen part of {@code fleet:}. A reload that + * changes {@code tabLabel} (or charters, or a role pool) without touching {@code fleet.leaders} + * must stay fully hot with nothing reported — proving {@link ConfigRef#changedSplitKeys} + * compares {@code fleet.leaders} specifically rather than the whole {@code Fleet} record, which + * would over-claim "needs a restart" for a change that is genuinely all live (the mirror + * mistake of the under-claim this class exists to prevent). + */ + @Test + void changingFleetTabLabelWithoutLeadersStaysFullyHot(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(""" + fleet: + tabLabel: "{role}: {profile} #{n}" + """)); + ConfigRef ref = refFor(f); + + Files.writeString(f, yaml(""" + fleet: + tabLabel: "[{profile}] {role}" + """)); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertTrue(out.deferred().isEmpty(), out.deferred().toString()); + assertTrue(out.split().isEmpty(), out.split().toString()); + assertEquals("config reloaded", out.summary()); + assertEquals("[{profile}] {role}", ref.get().fleet().tabLabel()); + } + /** * A split change must not refuse the reload (invariant 2 of fleetd #330) and * {@code Outcome.applied()} must stay {@code true} (invariant 3) — unlike a cold change, the diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java index 750742b..90525fb 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java @@ -76,20 +76,24 @@ class ConfigRefTopLevelCoverageTest { *

  • {@code placement} — read live by the placement policy on every spawn (class doc, Hot * bullet; {@code ConfigRefTest.aConsumerHoldingTheRefSeesTheNewValue} proves it * behaviourally).
    • - *
  • {@code fleet} — role pools, {@code charters} and {@code tabLabel} are read live through - * the supplier on {@code CompositePeerLauncher} (class doc, Hot bullet). The one - * documented exception, {@code fleet.leaders}, is read only at startup and genuinely needs - * a restart — a real gap in {@code changedDeferredKeys}, but one the class doc already - * carries and that fleetd #330 explicitly did not re-open (its "seven readers" fact-find - * named {@code health}/{@code coordinator} as the complete set of split-shaped keys, not - * {@code fleet}). Reported as a caveat, not fixed here.
    • *
  • {@code memberCredentials} — read live at {@code Fleetd.java:198, 205, 729}.
    • *
  • {@code memberLoginShell} — read live at * {@code HerdrPeerLauncher#configuredMemberLoginShell}.
    • * + * + *

    {@code fleet} used to sit here too, on the strength of most of it (role pools, charters, + * tabLabel) being read the same live way — but {@code fleet.leaders} inside the same key is + * read only at startup and genuinely needs a restart, which is a real gap this escape hatch + * cannot represent: it excuses a whole top-level key from reload bookkeeping, and {@code fleet} + * needed exactly half of it excused. fleetd #333 moved it into {@link ConfigRef#SPLIT_KEYS} + * instead, where {@code changedSplitKeys} reports the frozen half by name. That is the + * cautionary tale this test's own javadoc already told: this checker proves the record's shape + * is triaged, never that a bucket a key sits in is the right one — a person has to read the + * source, which is exactly how fleetd #333 found {@code fleet} sitting in the wrong bucket + * while this test stayed green throughout.

    */ private static final Set HOT_EXCLUDED_TOP_LEVEL_KEYS = - Set.of("placement", "fleet", "memberCredentials", "memberLoginShell"); + Set.of("placement", "memberCredentials", "memberLoginShell"); @Test void everyTopLevelComponentIsAccountedForInExactlyOneClass() { @@ -116,7 +120,7 @@ class ConfigRefTopLevelCoverageTest { // The escape hatch is pinned. Growing it requires editing this line — a visible, deliberate // diff, not a quiet one. See the field javadoc above for what "belongs here" actually means. - assertEquals(Set.of("placement", "fleet", "memberCredentials", "memberLoginShell"), hot, + assertEquals(Set.of("placement", "memberCredentials", "memberLoginShell"), hot, "HOT_EXCLUDED_TOP_LEVEL_KEYS changed. A component belongs here ONLY if it is read " + "live off the config supplier, never because adding it makes this test " + "pass. If you are adding one to silence this test, that is fleetd #323 " diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java new file mode 100644 index 0000000..00b77a5 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java @@ -0,0 +1,219 @@ +package dev.ltms.fleet.config; + +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Constructor; +import java.lang.reflect.RecordComponent; +import java.util.ArrayList; +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; + +/** + * fleetd #333, finding F2: {@link ConfigRefTopLevelCoverageTest} proves every {@link FleetConfig} + * top-level component sits in exactly one of {@link ConfigRef#COLD_KEYS}, {@code + * DEFERRED_TOP_LEVEL_KEYS}, {@link ConfigRef#SPLIT_KEYS} or the hot-excluded set. It does + * not prove that a key's membership in {@code COLD_KEYS} or {@code SPLIT_KEYS} + * corresponds to any actual comparison in {@link ConfigRef}: a key can sit in either set with no + * branch in {@code changedColdKeys}/{@code changedSplitKeys} checking it, and both + * {@link ConfigRefTopLevelCoverageTest} and the "kept in step" {@code assert} inside each of those + * methods stay green, because neither one reads the method body — the coverage test only reads + * set membership, and the assert only checks that reported entries are a SUBSET of the set, never + * that every set member produced a reported entry. + * + *

    Measured directly, live, while fixing fleetd #333: dropping the {@code coordinator} branch out + * of {@code ConfigRef.changedSplitKeys} while leaving {@code "coordinator"} in + * {@link ConfigRef#SPLIT_KEYS} left {@link ConfigRefTopLevelCoverageTest} and the in-method assert + * both green — only a hand-written behavioural case in {@link ConfigRefTest} caught it, because it + * happened to name that exact key. This is the {@link ConfigRefProfileCoverageTest} mechanism one + * level up, generalised over every {@code COLD_KEYS}/{@code SPLIT_KEYS} member rather than one + * hand-picked field: enumerate {@link FleetConfig}'s own record components by reflection, build "a + * config where only {@code } differs" for each cold/split key, and call the real + * {@link ConfigRef#changedColdKeys}/{@link ConfigRef#changedSplitKeys} methods (made + * package-private for exactly this, the same reason {@link ConfigRef#sameLaunchSettings} already + * is) to prove each one is actually reported — not assumed from a set literal. + * + *

    What this deliberately does NOT cover

    + * {@code DEFERRED_TOP_LEVEL_KEYS} is not exercised here. That bucket carries the identical + * one-way risk in principle — a key added to it with no matching branch in + * {@code changedDeferredKeys} would pass {@link ConfigRefTopLevelCoverageTest} exactly the way + * {@code coordinator} passed it above — but this test stays narrow to {@code COLD_KEYS} and + * {@code SPLIT_KEYS} for two reasons. First, that is where fleetd #333 actually found and measured + * the gap (F1 was a live instance of it). Second, most of {@code DEFERRED_TOP_LEVEL_KEYS} already + * carries an individual behavioural test in {@link ConfigRefTest} naming it by key — {@code + * worktreeGroup}, {@code primary}, {@code configReload}, {@code profiles}' launch settings / + * weight-maxLoad / exhaustedPattern / errorPattern / ideProjectDir — which is the same protection + * this class gives {@code COLD_KEYS}/{@code SPLIT_KEYS}, just written by hand per key instead of + * generated by reflection over the whole set. {@code guard}, {@code lifecycle}, + * {@code leadHeartbeat}, {@code spawnReadyTimeoutMs}/{@code spawnReadyPollMs} and + * {@code quarantineCooldownSeconds} do NOT have a dedicated behavioural test naming them, so the + * one-way gap fleetd's own memory notes ("pre-existing on COLD_KEYS and on the test's own + * DEFERRED_TOP_LEVEL_KEYS") is real and not fully closed by this class — extending this mechanism's + * {@code BASE}/{@code ALT} map to cover every top-level component and adding a third + * {@code everyDeferredKeyIsActuallyReportedByChangedDeferredKeys} test is the natural next step, left + * for whoever next finds a deferred key with the same shape as this ticket's {@code fleet.leaders}. + */ +class ConfigRefTopLevelReportingCoverageTest { + + private static final RecordComponent[] COMPONENTS = FleetConfig.class.getRecordComponents(); + + /** + * One valid value per top-level {@link FleetConfig} component — "the a value". Components not + * exercised by either test below ({@code profiles}, {@code guard}, {@code worktreeRoot}, …) are + * left {@code null}/empty; {@link FleetConfig}'s compact constructor only normalizes + * {@code profiles}, so every other field accepts {@code null} unmutated. + */ + private static final Map BASE = baseValues(); + + /** The same shape, each value distinct from {@link #BASE} — "the b value" — for COLD_KEYS/SPLIT_KEYS only. */ + private static final Map ALT = altValues(); + + private static Map baseValues() { + Map v = new LinkedHashMap<>(); + v.put("bind", new FleetConfig.Bind("127.0.0.1", 8765)); + v.put("herdrSocket", "~/.config/herdr/a.sock"); + v.put("memberHerdrSocket", "~/.config/herdr/member-a.sock"); + v.put("profiles", Map.of()); + v.put("guard", null); + v.put("worktreeRoot", null); + v.put("lifecycle", null); + v.put("spawnReadyTimeoutMs", null); + v.put("spawnReadyPollMs", null); + v.put("broker", new FleetConfig.Broker("amqp://a", null, 1)); + v.put("primary", null); + v.put("fleet", new FleetConfig.Fleet( + Map.of("opus", new FleetConfig.Leader("sonnet", "lead: opus-a", 1, null, 10, + "claude", null, null, null)), + Map.of(), Map.of(), Map.of(), Map.of(), "{role}: {profile} #{n}")); + v.put("leadHeartbeat", null); + v.put("health", new FleetConfig.Health(true, 30, 600, null, null)); + v.put("placement", null); + v.put("auth", new FleetConfig.Auth("loopback-trust", null)); + v.put("configReload", null); + v.put("quarantineCooldownSeconds", null); + v.put("memberCredentials", null); + v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-a", null, "self-a", 1)); + v.put("worktreeGroup", null); + v.put("memberLoginShell", null); + assertNamesMatchComponents(v); + return v; + } + + private static Map altValues() { + Map v = new LinkedHashMap<>(); + v.put("bind", new FleetConfig.Bind("127.0.0.2", 8766)); + v.put("herdrSocket", "~/.config/herdr/b.sock"); + v.put("memberHerdrSocket", "~/.config/herdr/member-b.sock"); + v.put("profiles", Map.of()); + v.put("guard", null); + v.put("worktreeRoot", null); + v.put("lifecycle", null); + v.put("spawnReadyTimeoutMs", null); + v.put("spawnReadyPollMs", null); + v.put("broker", new FleetConfig.Broker("amqp://b", null, 2)); + v.put("primary", null); + // Differs from BASE.fleet only in fleet.leaders (a different tab for "opus") — the frozen + // sub-field ConfigRef.changedSplitKeys actually compares. A Fleet that instead differed only + // in tabLabel would correctly NOT be reported (see + // ConfigRefTest.changingFleetTabLabelWithoutLeadersStaysFullyHot) and would wrongly fail this + // test — that is by design, not a gap: this map exists to prove fleet.leaders is covered. + v.put("fleet", new FleetConfig.Fleet( + Map.of("opus", new FleetConfig.Leader("sonnet", "lead: opus-b", 1, null, 10, + "claude", null, null, null)), + Map.of(), Map.of(), Map.of(), Map.of(), "{role}: {profile} #{n}")); + v.put("leadHeartbeat", null); + v.put("health", new FleetConfig.Health(false, 90, 900, null, null)); + v.put("placement", null); + v.put("auth", new FleetConfig.Auth("token", "TOKEN_ENV")); + v.put("configReload", null); + v.put("quarantineCooldownSeconds", null); + v.put("memberCredentials", null); + v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-b", null, "self-b", 2)); + v.put("worktreeGroup", null); + v.put("memberLoginShell", null); + assertNamesMatchComponents(v); + return v; + } + + private static void assertNamesMatchComponents(Map values) { + Set names = new TreeSet<>(); + for (RecordComponent rc : COMPONENTS) { + names.add(rc.getName()); + } + assertEquals(names, new TreeSet<>(values.keySet()), + "this test's value map has drifted from FleetConfig's actual top-level components — " + + "update BASE/ALT alongside the record"); + } + + private static FleetConfig configOf(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.class.getDeclaredConstructor(types); + return ctor.newInstance(args); + } + + /** {@code BASE} with exactly one named top-level component swapped for its {@code ALT} value. */ + private static FleetConfig mutate(String componentName) throws ReflectiveOperationException { + Map values = new LinkedHashMap<>(BASE); + values.put(componentName, ALT.get(componentName)); + return configOf(values); + } + + /** + * The mechanism fleetd #333 F2 asked for, applied to {@link ConfigRef#COLD_KEYS}: mutate each + * cold key in isolation and prove {@code changedColdKeys} actually names it, not just that + * {@code COLD_KEYS} claims it does. + */ + @Test + void everyColdKeyIsActuallyReportedByChangedColdKeys() throws ReflectiveOperationException { + FleetConfig base = configOf(BASE); + List uncovered = new ArrayList<>(); + for (String key : new TreeSet<>(ConfigRef.COLD_KEYS)) { + FleetConfig mutated = mutate(key); + if (!ConfigRef.changedColdKeys(base, mutated).contains(key)) { + uncovered.add(key); + } + } + System.out.printf( + "ConfigRef.changedColdKeys reporting coverage — %d COLD_KEYS, %d verified%n", + ConfigRef.COLD_KEYS.size(), ConfigRef.COLD_KEYS.size() - uncovered.size()); + assertEquals(List.of(), uncovered, + "these keys are in ConfigRef.COLD_KEYS but mutating them alone produces no matching " + + "entry from changedColdKeys — a set entry with no comparison behind it: " + + uncovered); + } + + /** + * The same mechanism applied to {@link ConfigRef#SPLIT_KEYS}: mutate each split key in + * isolation and prove {@code changedSplitKeys} actually names it (message starting with + * {@code ":"}), not just that {@code SPLIT_KEYS} claims it does. This is the exact check + * that would have failed fleetd #333's own reproduction — dropping the {@code coordinator} + * branch from {@code changedSplitKeys} while {@code "coordinator"} stayed in {@code SPLIT_KEYS} + * — see the mutation proof in the fleetd #333 PR description; the earlier checkers here (the + * shape test and the in-method assert) do not. + */ + @Test + void everySplitKeyIsActuallyReportedByChangedSplitKeys() throws ReflectiveOperationException { + FleetConfig base = configOf(BASE); + List uncovered = new ArrayList<>(); + for (String key : new TreeSet<>(ConfigRef.SPLIT_KEYS)) { + FleetConfig mutated = mutate(key); + List split = ConfigRef.changedSplitKeys(base, mutated); + if (split.stream().noneMatch(s -> s.startsWith(key + ":"))) { + uncovered.add(key); + } + } + System.out.printf( + "ConfigRef.changedSplitKeys reporting coverage — %d SPLIT_KEYS, %d verified%n", + ConfigRef.SPLIT_KEYS.size(), ConfigRef.SPLIT_KEYS.size() - uncovered.size()); + assertEquals(List.of(), uncovered, + "these keys are in ConfigRef.SPLIT_KEYS but mutating them alone produces no matching " + + "entry from changedSplitKeys — a set entry with no comparison behind it, " + + "exactly the fleetd #333 F2 shape: " + uncovered); + } +}