diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java index f28f23f..f4eced5 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java @@ -369,8 +369,7 @@ public final class CompositePeerLauncher implements PeerLauncher { // CB-547a: route the chosen profile but keep the caller's session identity — dropping it // here would silently sever the resume handle on every policy-routed spawn. CB-557: the // role rides along for the same reason, or a routed spawn would be labelled as a dev. - SpawnRequest routedReq = new SpawnRequest(chosen.profile(), req.requestedCwd(), req.callerCwd(), - req.sessionName(), req.resumeSessionId(), req.role()); + SpawnRequest routedReq = req.withProfile(chosen.profile()); try { PeerHandle handle = d.spawn(routedReq); spawnedBy.put(handle.id(), d); diff --git a/fleetd/src/main/java/dev/ltms/fleet/peer/SpawnRequest.java b/fleetd/src/main/java/dev/ltms/fleet/peer/SpawnRequest.java index 00f0ffd..943dd37 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/peer/SpawnRequest.java +++ b/fleetd/src/main/java/dev/ltms/fleet/peer/SpawnRequest.java @@ -29,4 +29,9 @@ public record SpawnRequest(String profileName, String requestedCwd, String calle String sessionName, String resumeSessionId) { this(profileName, requestedCwd, callerCwd, sessionName, resumeSessionId, null); } + + /** Return a copy of this request with {@code profileName} replaced by {@code profile}. */ + public SpawnRequest withProfile(String profile) { + return new SpawnRequest(profile, requestedCwd, callerCwd, sessionName, resumeSessionId, role); + } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/peer/SpawnRequestWithProfilePreservesEveryComponentTest.java b/fleetd/src/test/java/dev/ltms/fleet/peer/SpawnRequestWithProfilePreservesEveryComponentTest.java new file mode 100644 index 0000000..a75abe9 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/peer/SpawnRequestWithProfilePreservesEveryComponentTest.java @@ -0,0 +1,144 @@ +package dev.ltms.fleet.peer; + +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 #382, the same "defect factory" #357 and #358 guarded on {@code FleetConfig.withDefaults()} + * and {@link dev.ltms.fleet.session.MemberSession}'s rebuild sites — reproduced here on + * {@link SpawnRequest#withProfile(String)}. + * + *

{@code SpawnRequest} has back-compat constructors at arity 3 and 5 alongside its canonical + * arity-6 constructor. Before this ticket, {@code CompositePeerLauncher} routed a profile by + * building a fresh {@code SpawnRequest} from a literal {@code new SpawnRequest(...)} call listing + * six of the original request's own accessors. That call is only correct because it happens to + * name exactly six arguments today — add a 7th component and the established back-compat pattern + * (a new constructor at the old, now-shorter arity) and a call one argument short of the new + * canonical arity would silently rebind to that back-compat constructor, dropping the new + * component on every profile-routed spawn without any compile error. {@link #withProfile} replaces + * that literal call, so this test guards the ONE rebuild site instead of a call site scattered + * through a launcher. + * + *

The check below builds a {@link SpawnRequest} 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 {@link SpawnRequest#withProfile(String)}, and asserts + * every component the method is not documented to change survives unchanged, while {@code + * profileName} comes back as the new value it was given. A component that comes back anything else + * was silently dropped — 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 the other two guards pin theirs at + * zero: a checker whose escape hatch can grow to silence a failure is not a checker. Every one of + * {@link SpawnRequest}'s 6 current components has a real, non-null value here and none is excluded. + */ +class SpawnRequestWithProfilePreservesEveryComponentTest { + + private static final RecordComponent[] COMPONENTS = SpawnRequest.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 6 is excluded. */ + private static Map baseValues() { + Map v = new LinkedHashMap<>(); + v.put("profileName", "profile-guard"); + v.put("requestedCwd", "/wt/requested-guard"); + v.put("callerCwd", "/wt/caller-guard"); + v.put("sessionName", "session-guard"); + v.put("resumeSessionId", "resume-guard"); + v.put("role", MemberRole.REVIEWER); + 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 SpawnRequest's actual components — " + + "update baseValues() alongside the record"); + } + + /** + * Builds a {@link SpawnRequest} 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 SpawnRequest(...)} call risks doing. + */ + private static SpawnRequest requestOf(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 = SpawnRequest.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"); + } + + @Test + void withProfilePreservesEveryOtherComponent() throws ReflectiveOperationException { + Map base = baseValues(); + SpawnRequest request = requestOf(base); + SpawnRequest result = request.withProfile("profile-updated"); + + Map expectedOverrides = Map.of("profileName", "profile-updated"); + + 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 SpawnRequest." + 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 SpawnRequest(...)\" " + + "call binding to a back-compat constructor instead of the true " + + "canonical one)", + name, expected, name, actual)); + } + } + + System.out.printf(Locale.ROOT, + "SpawnRequest.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); + } +}