Merge worker/t358-6e989b-1: t358: guard MemberSession's 5 rebuild sites and Profile.withProfile() against the back-compat-arity trap

2026-09-09 07:35:00 +07:00
parent 2302b3bc11 94ec77a1bc
commit 7057291739
2 changed files with 362 additions and 0 deletions
@@ -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.
+ *
+ * <p>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.
+ *
+ * <p>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()}.
+ *
+ * <p>{@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<String> EXCLUDED_FROM_SURVIVAL_CHECK = Set.of();
+
+    /** One real, distinctive, non-null value per component, chosen to survive the compact ctor. */
+    private static Map<String, Object> baseValues() {
+        Map<String, Object> 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<String, Object> values) {
+        Set<String> 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<String, Object> 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<FleetConfig.Profile> 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<String, Object> base = baseValues();
+        FleetConfig.Profile profile = profileOf(base);
+        FleetConfig.Profile renamed = profile.withProfile("renamed-profile-guard");
+
+        List<String> 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);
+    }
+}
@@ -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.
+ *
+ * <p>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.
+ *
+ * <p>{@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<String> EXCLUDED_FROM_SURVIVAL_CHECK = Set.of();
+
+    /** One real, distinctive, non-null value per component — none of the 15 is excluded. */
+    private static Map<String, Object> baseValues() {
+        Map<String, Object> 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<String, Object> values) {
+        Set<String> 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<String, Object> 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<MemberSession> 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<MemberSession, MemberSession> rebuild,
+                                  Map<String, Object> expectedOverrides) throws ReflectiveOperationException {
+        Map<String, Object> base = baseValues();
+        MemberSession session = sessionOf(base);
+        MemberSession result = rebuild.apply(session);
+
+        List<String> 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"));
+    }
+}