From 94ec77a1bcba7b37d094aa1cb3b126172d888e91 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Wed, 9 Sep 2026 07:19:12 +0700 Subject: [PATCH] t358: guard MemberSession's 5 rebuild sites and Profile.withProfile() against the back-compat-arity trap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #357 (FleetConfig.withDefaults()). Reflectively enumerate each record's own components, resolve the canonical constructor by exact component types, build a real non-null value per component, run each rebuild site, and assert every component survives (except the one it is documented to change). Exclusion lists pinned at 0 for both. Re-counted the Profile back-compat ladder directly against the source: 8 constructors (arities 25, 24, 22, 20, 18, 15, 14, 12) against a canonical arity of 26 — the ticket's own number was explicitly untrusted. --- ...ithProfilePreservesEveryComponentTest.java | 172 ++++++++++++++++ ...ionRebuildPreservesEveryComponentTest.java | 190 ++++++++++++++++++ 2 files changed, 362 insertions(+) create mode 100644 fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigProfileWithProfilePreservesEveryComponentTest.java create mode 100644 fleetd/src/test/java/dev/ltms/fleet/session/MemberSessionRebuildPreservesEveryComponentTest.java diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigProfileWithProfilePreservesEveryComponentTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigProfileWithProfilePreservesEveryComponentTest.java new file mode 100644 index 0000000..4eb6120 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigProfileWithProfilePreservesEveryComponentTest.java @@ -0,0 +1,172 @@ +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.Locale; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.TreeSet; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Fleetd #358, the same "defect factory" #357 guarded on {@code FleetConfig.withDefaults()} + * (see {@code FleetConfigWithDefaultsPreservesEveryComponentTest}), reproduced here on + * {@link FleetConfig.Profile}. {@code Profile} carries a long back-compat constructor ladder — 8 + * constructors, re-counted directly against the source rather than trusted from the ticket, at + * arities 25, 24, 22, 20, 18, 15, 14 and 12, against a canonical arity of 26 — and exactly ONE + * rebuild site, {@link FleetConfig.Profile#withProfile(String)}, whose own + * {@code return new Profile(...)} call is written at a literal 26-arg count. Add a 27th component + * and its established back-compat constructor at the old (26-arg) arity, and {@code withProfile}'s + * own call becomes a legal match for that new overload — silently dropping the new component every + * time a profile's name is defaulted from its {@code workers:} key. + * + *

Builds one {@link FleetConfig.Profile} through the TRUE canonical constructor — resolved by + * the record's own component types via {@code getDeclaredConstructor}, never by argument count — + * with a real, distinctive, non-null value in every component, calls {@link + * FleetConfig.Profile#withProfile(String)}, and asserts every component except {@code profile} + * itself survives unchanged, while {@code profile} comes back as the new name it was given. + * + *

Every value here is chosen so {@code Profile}'s own compact constructor (which normalizes + * several components — defaults {@code argv}/{@code kind}/{@code placement}/{@code workspace}/ + * {@code gitHostEnv}, nulls a handful of blank-checked strings, clamps {@code weight}, coerces + * {@code subscription}) leaves it unchanged: every String is non-blank and already in the shape the + * compact constructor would otherwise coerce it to (e.g. {@code placement} is already lowercase), + * and every collection is non-empty. That is what makes "must survive unchanged" a valid assertion + * for every component below, the same reasoning {@code FleetConfigWithDefaultsPreservesEveryComponentTest} + * documents for {@code withDefaults()}. + * + *

{@link #EXCLUDED_FROM_SURVIVAL_CHECK} is kept deliberately empty and size-pinned by + * {@link #exclusionListSizeIsPinned()} — a checker whose escape hatch can grow to silence a failure + * is not a checker. Every one of {@code Profile}'s 26 current components has a real, non-null, + * non-blank value here and none is excluded. + */ +class FleetConfigProfileWithProfilePreservesEveryComponentTest { + + private static final RecordComponent[] COMPONENTS = FleetConfig.Profile.class.getRecordComponents(); + + /** Deliberately empty today; grow it only with a matching justification, and re-pin the size. */ + private static final Set EXCLUDED_FROM_SURVIVAL_CHECK = Set.of(); + + /** One real, distinctive, non-null value per component, chosen to survive the compact ctor. */ + private static Map baseValues() { + Map v = new LinkedHashMap<>(); + v.put("profile", "profile-guard"); + v.put("baseUrl", "https://guard.example/base"); + v.put("model", "model-guard"); + v.put("configDir", "/config/guard"); + v.put("tokenEnv", "GUARD_TOKEN"); + v.put("argv", List.of("guard-cmd")); + v.put("placement", "guard-placement"); + v.put("workspace", "workspace-guard"); + v.put("tabLabel", "tab-guard"); + v.put("mcpUrl", "https://mcp.guard/"); + v.put("cwd", "/cwd/guard"); + v.put("parityOverlay", List.of(".guardrc")); + v.put("gitTokenEnv", "GUARD_GIT_TOKEN"); + v.put("gitHostEnv", "GUARD_GIT_HOST"); + v.put("kind", "claude-code"); + v.put("env", Map.of("GUARD_ENV", "1")); + v.put("weight", 2.5f); + v.put("maxLoad", 4); + v.put("subscription", Boolean.TRUE); + v.put("exhaustedPattern", "pattern-guard"); + v.put("credentialId", "cred-guard"); + v.put("ideMcpUrl", "https://ide.guard/"); + v.put("ideProjectDir", "ide-project-guard"); + v.put("ideOpenCommand", "open-guard {dir}"); + v.put("autoCompactWindow", 150_000); + v.put("errorPattern", "error-pattern-guard"); + assertNamesMatchComponents(v); + return v; + } + + /** + * Guards {@link #baseValues()} itself against drifting from the record's real shape — forgetting + * to add a new component here fails this assertion by name, rather than silently checking one + * component fewer than the record has. + */ + 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.Profile's actual components — " + + "update baseValues() alongside the record"); + } + + /** + * Builds a {@link FleetConfig.Profile} through the TRUE canonical constructor — resolved by the + * record's own component types, not by argument count — so this never accidentally exercises a + * back-compat overload the way a literal {@code new Profile(...)} call risks doing. + */ + 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); + } + + @Test + void exclusionListSizeIsPinned() { + assertEquals(0, EXCLUDED_FROM_SURVIVAL_CHECK.size(), + "EXCLUDED_FROM_SURVIVAL_CHECK grew from 0 — every entry needs a justification in " + + "this test class's javadoc AND this assertion re-pinned to the new size; a " + + "growing exclusion list that silences failures on its own is not a guard"); + } + + /** + * The mutation this is built to catch: make {@code withProfile(String)}'s final constructor call + * literal at some arg count, add one more component to the record with a new back-compat + * constructor at the old arity, and the stale call silently rebinds. Every component here is real + * and non-null/non-blank, so none of it should be replaced by {@code withProfile}, except + * {@code profile} itself, which the method is documented to replace. + */ + @Test + void withProfilePreservesEveryOtherComponent() throws ReflectiveOperationException { + Map base = baseValues(); + FleetConfig.Profile profile = profileOf(base); + FleetConfig.Profile renamed = profile.withProfile("renamed-profile-guard"); + + List dropped = new ArrayList<>(); + int checked = 0; + for (RecordComponent rc : COMPONENTS) { + String name = rc.getName(); + if (EXCLUDED_FROM_SURVIVAL_CHECK.contains(name)) { + continue; + } + checked++; + Object expected = "profile".equals(name) ? "renamed-profile-guard" : base.get(name); + Object actual; + try { + actual = rc.getAccessor().invoke(renamed); + } catch (ReflectiveOperationException e) { + throw new RuntimeException("failed to read FleetConfig.Profile." + name + "()", e); + } + if (!Objects.equals(expected, actual)) { + dropped.add(String.format(Locale.ROOT, + "%s: withProfile() was expected to carry (%s) for '%s' but returned %s — a " + + "component silently dropped by withProfile(), the shape of the " + + "defect this test exists to catch (its final \"return new " + + "Profile(...)\" call binding to a back-compat constructor instead " + + "of the true canonical one)", + name, expected, name, actual)); + } + } + + System.out.printf(Locale.ROOT, + "FleetConfig.Profile.withProfile() component-survival coverage — %d components, %d " + + "checked, %d excluded, %d survived%n", + COMPONENTS.length, checked, EXCLUDED_FROM_SURVIVAL_CHECK.size(), checked - dropped.size()); + assertEquals(List.of(), dropped, + "withProfile() silently dropped these components: " + dropped); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/MemberSessionRebuildPreservesEveryComponentTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/MemberSessionRebuildPreservesEveryComponentTest.java new file mode 100644 index 0000000..01afcea --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/session/MemberSessionRebuildPreservesEveryComponentTest.java @@ -0,0 +1,190 @@ +package dev.ltms.fleet.session; + +import dev.ltms.fleet.peer.CharterReceipt; +import dev.ltms.fleet.peer.MemberRole; +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.Locale; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.TreeSet; +import java.util.function.Function; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Fleetd #358, the same "defect factory" #357 guarded on {@code FleetConfig.withDefaults()} + * (see {@code FleetConfigWithDefaultsPreservesEveryComponentTest}), reproduced here on + * {@link MemberSession} — the worse of the two sibling cases named in #358, because this record + * has FIVE independent rebuild sites instead of one: {@link MemberSession#withState}, + * {@link MemberSession#withActivity}, {@link MemberSession#bumpTurn}, + * {@link MemberSession#withAgentSessionId} and {@link MemberSession#withFailureReason} each end in + * their own literal {@code new MemberSession(...)} call. Add a 16th component and add the + * established back-compat constructor at the old (15-arg) arity, and every one of those five + * literal calls becomes a legal match for that new overload — silently dropping the new component, + * independently, on whichever of the five paths a missed update leaves behind. That is harder to + * spot than #357's single call site: the field would survive through some transitions and vanish + * through others. + * + *

Each check below builds one {@link MemberSession} through the TRUE canonical constructor — + * resolved by the record's own component types via {@code getDeclaredConstructor}, never by + * argument count, so it can never itself land on a back-compat overload — with a real, distinctive, + * non-null value in every component, calls the real rebuild method under test, and asserts every + * component the method is not documented to change survives unchanged, while the component(s) it IS + * documented to change come back as the new value it was given. A component that comes back + * anything else was silently dropped or lost — the shape of the defect this test exists to catch. + * + *

{@link #EXCLUDED_FROM_SURVIVAL_CHECK} is kept deliberately empty and size-pinned by + * {@link #exclusionListSizeIsPinned()}, for the same reason {@code FleetConfig}'s guard pins its own + * exclusion list at zero: a checker whose escape hatch can grow to silence a failure is not a + * checker. Every one of {@link MemberSession}'s 15 current components has a real, non-null, + * non-blank value here and none is excluded. + */ +class MemberSessionRebuildPreservesEveryComponentTest { + + private static final RecordComponent[] COMPONENTS = MemberSession.class.getRecordComponents(); + + /** Deliberately empty today; grow it only with a matching justification, and re-pin the size. */ + private static final Set EXCLUDED_FROM_SURVIVAL_CHECK = Set.of(); + + /** One real, distinctive, non-null value per component — none of the 15 is excluded. */ + private static Map baseValues() { + Map v = new LinkedHashMap<>(); + v.put("paneId", "pane-guard"); + v.put("terminalId", "term-guard"); + v.put("profile", "profile-guard"); + v.put("role", MemberRole.REVIEWER); + v.put("cwd", "/wt/guard"); + v.put("ownerTerminal", "owner-guard"); + v.put("spawnedAtNanos", 111_111L); + v.put("lastActivityAtNanos", 222_222L); + v.put("turnCount", 7); + v.put("state", MemberSession.State.BUSY); + v.put("worktree", "/wt/guard-tree"); + v.put("branch", "worker/guard-branch"); + v.put("charterReceipt", new CharterReceipt( + MemberRole.DEV, "profile-guard", "fleet.charters.dev", "deadbeefguard", 42)); + v.put("agentSessionId", "agent-guard"); + v.put("failureReason", "reason-guard"); + assertNamesMatchComponents(v); + return v; + } + + /** + * Guards {@link #baseValues()} itself against drifting from the record's real shape — forgetting + * to add a new component here fails this assertion by name, rather than silently checking one + * component fewer than the record has. + */ + 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 MemberSession's actual components — " + + "update baseValues() alongside the record"); + } + + /** + * Builds a {@link MemberSession} through the TRUE canonical constructor — resolved by the + * record's own component types, not by argument count — so this never accidentally exercises a + * back-compat overload the way a literal {@code new MemberSession(...)} call risks doing. + */ + private static MemberSession sessionOf(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 = MemberSession.class.getDeclaredConstructor(types); + return ctor.newInstance(args); + } + + @Test + void exclusionListSizeIsPinned() { + assertEquals(0, EXCLUDED_FROM_SURVIVAL_CHECK.size(), + "EXCLUDED_FROM_SURVIVAL_CHECK grew from 0 — every entry needs a justification in " + + "this test class's javadoc AND this assertion re-pinned to the new size; a " + + "growing exclusion list that silences failures on its own is not a guard"); + } + + /** + * Shared check for one rebuild site: build a base session with a real value in every component, + * call {@code rebuild}, and assert every component comes back equal to {@code expectedOverrides} + * when named there, or equal to the base value otherwise. Prints the same denominator style as + * {@code FleetConfigWithDefaultsPreservesEveryComponentTest}. + */ + private void checkRebuildSite(String siteName, Function rebuild, + Map expectedOverrides) throws ReflectiveOperationException { + Map base = baseValues(); + MemberSession session = sessionOf(base); + MemberSession result = rebuild.apply(session); + + List dropped = new ArrayList<>(); + int checked = 0; + for (RecordComponent rc : COMPONENTS) { + String name = rc.getName(); + if (EXCLUDED_FROM_SURVIVAL_CHECK.contains(name)) { + continue; + } + checked++; + Object expected = expectedOverrides.containsKey(name) ? expectedOverrides.get(name) : base.get(name); + Object actual; + try { + actual = rc.getAccessor().invoke(result); + } catch (ReflectiveOperationException e) { + throw new RuntimeException("failed to read MemberSession." + name + "()", e); + } + if (!Objects.equals(expected, actual)) { + dropped.add(String.format(Locale.ROOT, + "%s: %s() was expected to carry (%s) for '%s' but returned %s — a component " + + "silently dropped by %s(), the shape of the defect this test exists " + + "to catch (its final \"return new MemberSession(...)\" call binding " + + "to a back-compat constructor instead of the true canonical one)", + name, siteName, expected, name, actual, siteName)); + } + } + + System.out.printf(Locale.ROOT, + "MemberSession.%s() component-survival coverage — %d components, %d checked, %d " + + "excluded, %d survived%n", + siteName, COMPONENTS.length, checked, EXCLUDED_FROM_SURVIVAL_CHECK.size(), + checked - dropped.size()); + assertEquals(List.of(), dropped, + siteName + "() silently dropped these components: " + dropped); + } + + @Test + void withStatePreservesEveryOtherComponent() throws ReflectiveOperationException { + checkRebuildSite("withState", s -> s.withState(MemberSession.State.FAILED), + Map.of("state", MemberSession.State.FAILED)); + } + + @Test + void withActivityPreservesEveryOtherComponent() throws ReflectiveOperationException { + checkRebuildSite("withActivity", s -> s.withActivity(999_999L), + Map.of("lastActivityAtNanos", 999_999L)); + } + + @Test + void bumpTurnPreservesEveryOtherComponent() throws ReflectiveOperationException { + checkRebuildSite("bumpTurn", s -> s.bumpTurn(999_999L), + Map.of("lastActivityAtNanos", 999_999L, "turnCount", 8)); + } + + @Test + void withAgentSessionIdPreservesEveryOtherComponent() throws ReflectiveOperationException { + checkRebuildSite("withAgentSessionId", s -> s.withAgentSessionId("agent-updated"), + Map.of("agentSessionId", "agent-updated")); + } + + @Test + void withFailureReasonPreservesEveryOtherComponent() throws ReflectiveOperationException { + checkRebuildSite("withFailureReason", s -> s.withFailureReason("reason-updated"), + Map.of("failureReason", "reason-updated")); + } +}