fleetd #453: document PeerLauncher.defaultProfileFor/place override obligation #456

2026-09-10T12:50:23+02:00

agent commented

2026-09-10 12:50:23 +02:00

fleetd #453: PeerLauncher.defaultProfileFor(role) and place(role) are default methods that ignore role.

Decision: OPTION 1 -- leave both as default methods, javadoc-only change.

Why: neither default is a live defect today. HerdrPeerLauncher is the only
implementer that inherits them (grep below), it owns exactly one profile per
launcher instance, and "ignore role, return defaultProfile()" is the correct
answer for that shape. The risk the ticket names is real but speculative: a
future launcher that routes several profiles per role and forgets to
override. That risk already has a concrete, forced-read pointer at it:
HerdrPeerLauncher.spawn(SpawnRequest, PlacementDecision)'s own javadoc
(added by #450, which made that method abstract) already names both
defaultProfileFor and place explicitly as "unoverridden here ... just wrap
defaultProfile()" and explains why that's correct only for a single-profile
adapter. Any future subclass of HerdrPeerLauncher MUST write that method's
body (it's abstract), so they cannot avoid reading the paragraph that flags
the sibling methods. This PR strengthens that with an explicit MUST-override
sentence in the interface's own two javadocs, cross-referencing that
HerdrPeerLauncher javadoc by name.

Rejected: OPTION 2 (make both abstract). Ruled out by a fact the ticket asked
me to check first: it is NOT "two overrides" as the ticket estimated. Besides
CompositePeerLauncher (already overrides both) and HerdrPeerLauncher (would
need one override each, 1 file), five test doubles implement PeerLauncher
DIRECTLY (not via HerdrPeerLauncher) and currently rely on the default for
both methods:

FleetdBackendErrorSinkTest.NeverSpawnsLauncher
SessionManagerTest.RaceLauncher / NoResumeLauncher / ClearContextSpyLauncher / LazyIdLauncher
None of them call defaultProfileFor or place. Making the methods abstract
would force ~10 boilerplate one-line overrides into 5 unrelated test files
for zero test value -- exactly the "ceremony" the ticket's own text warns
against. That measured cost is what ruled Option 2 out, not a reflexive
"don't copy #450."

Also checked per the ticket's request:

FleetConfig.defaultProfileFor(role) (config/FleetConfig.java:1690) is a
same-named method on public record FleetConfig(...) (no
implements/extends clause at all, confirmed by printing its full
record header) -- NOT an override of this interface. Correctly excluded
from the override count, as the ticket suspected.
Re-measured overrides in my own tree:
grep -rn 'implements PeerLauncher' fleetd/src/main/java --include='*.java'
-> CompositePeerLauncher.java:71, HerdrPeerLauncher.java:66 (2 matches;
control: find fleetd/src/main/java -name '*.java' | wc -l = 109, so the
search reached the tree). Adding \|extends .*PeerLauncher surfaces
ClaudeCodeLauncher and OpenCodeLauncher, both extends HerdrPeerLauncher
(inherit through it, don't declare their own overrides).
grep -rn 'defaultProfileFor(MemberRole\|PlacementDecision place(MemberRole' fleetd/src/main/java --include='*.java'
confirms only CompositePeerLauncher.java:623/701 declare both; PeerLauncher.java:172/234
are the interface defaults; FleetConfig.java:1690 is the unrelated record method.

Also found the same shape twice more in this interface (out of scope, not
touched): PeerLauncher.disabledModels()/modelGateState() (lines ~340, ~358)
default to "no models: block" and are overridden only by
CompositePeerLauncher -- an accepted, already-documented instance of the
same "safe default that stops being safe if a launcher gains the concept"
pattern.

Build: mvn -B clean test in fleetd/ -- Tests run: 1578, Failures: 0, Errors: 0, Skipped: 0. BUILD SUCCESS.

No compile-time or test guard is claimed by this PR (Option 1 doesn't need
one) -- this is a documentation-only diff, verified by the full green test
run above.

Out of scope, not touched: CompositePeerLauncher's overrides; #425/#435/#444/#450 themselves; FleetConfig.defaultProfileFor.

fleetd #453: PeerLauncher.defaultProfileFor(role) and place(role) are default methods that ignore role. Decision: OPTION 1 -- leave both as default methods, javadoc-only change. Why: neither default is a live defect today. HerdrPeerLauncher is the only implementer that inherits them (grep below), it owns exactly one profile per launcher instance, and "ignore role, return defaultProfile()" is the correct answer for that shape. The risk the ticket names is real but speculative: a future launcher that routes several profiles per role and forgets to override. That risk already has a concrete, forced-read pointer at it: HerdrPeerLauncher.spawn(SpawnRequest, PlacementDecision)'s own javadoc (added by #450, which made that method abstract) already names both defaultProfileFor and place explicitly as "unoverridden here ... just wrap defaultProfile()" and explains why that's correct only for a single-profile adapter. Any future subclass of HerdrPeerLauncher MUST write that method's body (it's abstract), so they cannot avoid reading the paragraph that flags the sibling methods. This PR strengthens that with an explicit MUST-override sentence in the interface's own two javadocs, cross-referencing that HerdrPeerLauncher javadoc by name. Rejected: OPTION 2 (make both abstract). Ruled out by a fact the ticket asked me to check first: it is NOT "two overrides" as the ticket estimated. Besides CompositePeerLauncher (already overrides both) and HerdrPeerLauncher (would need one override each, 1 file), five test doubles implement PeerLauncher DIRECTLY (not via HerdrPeerLauncher) and currently rely on the default for both methods: - FleetdBackendErrorSinkTest.NeverSpawnsLauncher - SessionManagerTest.RaceLauncher / NoResumeLauncher / ClearContextSpyLauncher / LazyIdLauncher None of them call defaultProfileFor or place. Making the methods abstract would force ~10 boilerplate one-line overrides into 5 unrelated test files for zero test value -- exactly the "ceremony" the ticket's own text warns against. That measured cost is what ruled Option 2 out, not a reflexive "don't copy #450." Also checked per the ticket's request: - FleetConfig.defaultProfileFor(role) (config/FleetConfig.java:1690) is a same-named method on `public record FleetConfig(...)` (no `implements`/`extends` clause at all, confirmed by printing its full record header) -- NOT an override of this interface. Correctly excluded from the override count, as the ticket suspected. - Re-measured overrides in my own tree: `grep -rn 'implements PeerLauncher' fleetd/src/main/java --include='*.java'` -> CompositePeerLauncher.java:71, HerdrPeerLauncher.java:66 (2 matches; control: `find fleetd/src/main/java -name '*.java' | wc -l` = 109, so the search reached the tree). Adding `\|extends .*PeerLauncher` surfaces ClaudeCodeLauncher and OpenCodeLauncher, both `extends HerdrPeerLauncher` (inherit through it, don't declare their own overrides). `grep -rn 'defaultProfileFor(MemberRole\|PlacementDecision place(MemberRole' fleetd/src/main/java --include='*.java'` confirms only CompositePeerLauncher.java:623/701 declare both; PeerLauncher.java:172/234 are the interface defaults; FleetConfig.java:1690 is the unrelated record method. Also found the same shape twice more in this interface (out of scope, not touched): PeerLauncher.disabledModels()/modelGateState() (lines ~340, ~358) default to "no models: block" and are overridden only by CompositePeerLauncher -- an accepted, already-documented instance of the same "safe default that stops being safe if a launcher gains the concept" pattern. Build: `mvn -B clean test` in fleetd/ -- Tests run: 1578, Failures: 0, Errors: 0, Skipped: 0. BUILD SUCCESS. No compile-time or test guard is claimed by this PR (Option 1 doesn't need one) -- this is a documentation-only diff, verified by the full green test run above. Out of scope, not touched: CompositePeerLauncher's overrides; #425/#435/#444/#450 themselves; FleetConfig.defaultProfileFor.

agent added 1 commit 2026-09-10 12:50:23 +02:00

fleetd #453 : document the override obligation on PeerLauncher.defaultProfileFor/place

CI / contract (pull_request) Successful in 50s

Details

CI / build (pull_request) Successful in 1m34s

Details

0788d84be8

Decision: leave both as default methods (option 1), not abstract. Neither
default is a live defect today — HerdrPeerLauncher is the sole single-profile
implementer and the degenerate answer (ignore role, always defaultProfile())
is correct for it. Making them abstract would force ~10 boilerplate one-line
overrides across 5 unrelated PeerLauncher test doubles (NeverSpawnsLauncher,
RaceLauncher, NoResumeLauncher, ClearContextSpyLauncher, LazyIdLauncher) that
never call either method, for a risk that is speculative (no multi-profile
HerdrPeerLauncher subclass exists or is planned).

Strengthens both javadocs with an explicit MUST-override warning and cross-
references HerdrPeerLauncher.spawn(SpawnRequest, PlacementDecision)'s existing
#450 javadoc, which already names both methods as "unoverridden here" and
ties that to being a single-profile adapter -- the concrete place a future
multi-profile launcher author would read, since #450 made that method
abstract and any subclass must write its body.

ltms merged commit e29227d5f4 into main

2026-09-10 13:04:10 +02:00

ltms deleted branch worker/453-peerlauncher-defaults-5ee028-7

2026-09-10 13:04:10 +02:00

ltms referenced this issue from a commit

2026-09-10 13:05:10 +02:00

fleetd #453: point the new javadoc at the right javadoc

ltms referenced this pull request

2026-09-10 13:05:41 +02:00

Five {@link} targets in main do not exist, and nothing in CI would ever say so #459

ltms referenced this pull request

2026-09-10 13:06:29 +02:00