fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

PeerLauncher.place(role) and defaultProfileFor(role) are default methods that silently ignore the role #453

New Issue
Closed
opened 2026-09-10 12:18:15 +02:00 by ltms · 1 comment
ltms commented 2026-09-10 12:18:15 +02:00
Owner
Copy Link
Copy Source

Found by the worker on #450 as a one-line out-of-scope note, exactly as its brief asked. I checked it myself on main at bdcf285.

Lower priority than #450 was, and I want to be clear about why before anyone picks it up. #450's default body was a known defect — the #425 bug, shipped as an inheritable default. These two are not defects. They are reasonable degenerate answers that stop being reasonable if a launcher ever gains more than one profile.

What is there

// PeerLauncher.java:172
default String defaultProfileFor(MemberRole role) {
    return defaultProfile();
}

// PeerLauncher.java:234
default PlacementDecision place(MemberRole role) {
    return new PlacementDecision(defaultProfile());
}

Both take a MemberRole and both ignore it. For a launcher that owns exactly one profile that is correct — there is nothing to choose between, so every role resolves to the same place.

Who overrides them in src/main:

$ grep -rn 'defaultProfileFor(MemberRole\|PlacementDecision place(MemberRole' fleetd/src/main/java --include='*.java'
FleetConfig.java:1690:    public String defaultProfileFor(MemberRole role) {
CompositePeerLauncher.java:623:    public String defaultProfileFor(MemberRole role) {
CompositePeerLauncher.java:701:    public PlacementDecision place(MemberRole role) {
PeerLauncher.java:172:    default String defaultProfileFor(MemberRole role) {
PeerLauncher.java:234:    default PlacementDecision place(MemberRole role) {

(FleetConfig's is a same-named method on a different type, not an override of this interface — check that before counting it.)

So CompositePeerLauncher overrides both, and HerdrPeerLauncher — with ClaudeCodeLauncher and OpenCodeLauncher under it — inherits both.

The risk, stated at its real size

A future launcher that routes several profiles, and forgets to override these, gets role-blind placement with a green suite. Every role lands on defaultProfile(). Nothing fails, nothing logs, and the symptom is that role-based routing quietly stops working for that adapter.

That is the same shape as #450 — a default carrying behaviour only some implementers may safely inherit — but the failure is milder. #450's inherited body could refuse a spawn that placement had already approved. This one just makes a less good choice.

What I am NOT asking for

Do not make these abstract by reflex because #450 did. The argument that won #450 was "the default body is a live defect". That argument does not apply here, and copying the conclusion without the argument is how a codebase fills up with ceremony.

What to decide

Read both methods and their javadoc, then pick one and say why:

  1. Leave them. The default is correct for a single-profile launcher, and a comment plus the existing javadoc is enough. If you choose this, say what a future multi-profile launcher's author would have to read to know they must override — and if the honest answer is "nothing points them at it", that is an argument against this option.
  2. Make them abstract, and override in HerdrPeerLauncher with the current bodies plus a comment saying why ignoring role is right for a single-profile launcher. Turns the future mistake into a compile error, at the cost of two overrides.
  3. Something narrower — for example, a test that pins what an inheriting launcher does with a role, so the behaviour is at least written down.

Whatever you pick, the acceptance criterion is the same as #450's: if you claim a compile error or a test now guards this, prove it by breaking it and paste the failure, then restore and show green.

Out of scope

  • Do not change CompositePeerLauncher's overrides.
  • Do not revisit #425, #435, #444 or #450.
  • FleetConfig.defaultProfileFor is a different type's method with the same name. Leave it alone unless you can show it is actually implementing this interface.
Found by the worker on #450 as a one-line out-of-scope note, exactly as its brief asked. I checked it myself on `main` at `bdcf285`. **Lower priority than #450 was, and I want to be clear about why before anyone picks it up.** #450's default body was a known defect — the #425 bug, shipped as an inheritable default. These two are not defects. They are reasonable degenerate answers that stop being reasonable if a launcher ever gains more than one profile. ## What is there ```java // PeerLauncher.java:172 default String defaultProfileFor(MemberRole role) { return defaultProfile(); } // PeerLauncher.java:234 default PlacementDecision place(MemberRole role) { return new PlacementDecision(defaultProfile()); } ``` Both take a `MemberRole` and both ignore it. For a launcher that owns exactly one profile that is correct — there is nothing to choose between, so every role resolves to the same place. Who overrides them in `src/main`: ``` $ grep -rn 'defaultProfileFor(MemberRole\|PlacementDecision place(MemberRole' fleetd/src/main/java --include='*.java' FleetConfig.java:1690: public String defaultProfileFor(MemberRole role) { CompositePeerLauncher.java:623: public String defaultProfileFor(MemberRole role) { CompositePeerLauncher.java:701: public PlacementDecision place(MemberRole role) { PeerLauncher.java:172: default String defaultProfileFor(MemberRole role) { PeerLauncher.java:234: default PlacementDecision place(MemberRole role) { ``` (`FleetConfig`'s is a same-named method on a different type, not an override of this interface — check that before counting it.) So `CompositePeerLauncher` overrides both, and `HerdrPeerLauncher` — with `ClaudeCodeLauncher` and `OpenCodeLauncher` under it — inherits both. ## The risk, stated at its real size A future launcher that routes several profiles, and forgets to override these, gets **role-blind placement** with a green suite. Every role lands on `defaultProfile()`. Nothing fails, nothing logs, and the symptom is that role-based routing quietly stops working for that adapter. That is the same shape as #450 — a `default` carrying behaviour only some implementers may safely inherit — but the failure is milder. #450's inherited body could refuse a spawn that placement had already approved. This one just makes a less good choice. ## What I am NOT asking for Do not make these abstract by reflex because #450 did. The argument that won #450 was "the default body is a live defect". That argument does not apply here, and copying the conclusion without the argument is how a codebase fills up with ceremony. ## What to decide Read both methods and their javadoc, then pick one and say why: 1. **Leave them.** The default is correct for a single-profile launcher, and a comment plus the existing javadoc is enough. If you choose this, say what a future multi-profile launcher's author would have to read to know they must override — and if the honest answer is "nothing points them at it", that is an argument against this option. 2. **Make them abstract**, and override in `HerdrPeerLauncher` with the current bodies plus a comment saying why ignoring `role` is right for a single-profile launcher. Turns the future mistake into a compile error, at the cost of two overrides. 3. **Something narrower** — for example, a test that pins what an inheriting launcher does with a role, so the behaviour is at least written down. Whatever you pick, the acceptance criterion is the same as #450's: if you claim a compile error or a test now guards this, **prove it by breaking it** and paste the failure, then restore and show green. ## Out of scope - Do not change `CompositePeerLauncher`'s overrides. - Do not revisit #425, #435, #444 or #450. - `FleetConfig.defaultProfileFor` is a different type's method with the same name. Leave it alone unless you can show it is actually implementing this interface.
ltms commented 2026-09-10 13:06:29 +02:00
Author
Owner
Copy Link
Copy Source

Closed by #456, merged as e29227d, plus one follow-up commit 3c5873d.

Option 1 accepted: javadoc only, both methods stay default. The worker's reason for
rejecting Option 2 is the one that decides it, and I checked it myself rather than taking the
number. The ticket estimated "two overrides". It is not two. Five test doubles implement
PeerLauncher directly and rely on both defaults:

FleetdBackendErrorSinkTest.java:111   NeverSpawnsLauncher
SessionManagerTest.java:997           RaceLauncher
SessionManagerTest.java:1590          NoResumeLauncher
SessionManagerTest.java:1652          ClearContextSpyLauncher
SessionManagerTest.java:1859          LazyIdLauncher

Making both methods abstract forces about ten one-line overrides into five unrelated test files
for no test value. My ticket text warned against exactly that ceremony, so my own estimate was
the wrong half of the argument.

What I verified, and how. Local merge of 0788d84 onto 2af13ab, merge commit 5b46538:

  • mvn -B clean test in fleetd/: Tests run: 1578, Failures: 0, Errors: 0, Skipped: 0,
    BUILD SUCCESS, exit code 0.
  • Javadoc reference lint, which for a doc-only diff is the closest thing to a test:
    mvn -B -DskipTests javadoc:javadoc -Ddoclint=reference. The merge reports 5
    reference not found. main at 2af13ab reports the same 5, in the same 5 files, none of
    them a file this PR touched. So this PR adds no broken link. Those 5 are now #459.
  • The quoted phrase is real: are unoverridden here and just wrap {@link #defaultProfile()} is
    at member/HerdrPeerLauncher.java:604. Control on that search: one match in the whole main
    tree before the PR, so the grep reached the tree.
  • place is not overloaded (PeerLauncher.java:234 and CompositePeerLauncher.java:701 are the
    only declarations), so the bare {@link #place} is unambiguous.
  • FleetConfig.defaultProfileFor(role) really is unrelated. FleetConfig is a bare
    public record with no implements clause, so it is correctly excluded from the count.

One thing I fixed rather than sending back (3c5873d):

  1. The first new paragraph wrote "HerdrPeerLauncher's own {@link #spawn(SpawnRequest, PlacementDecision)} javadoc". That link resolves to PeerLauncher's own abstract
    declaration, not to HerdrPeerLauncher's override, so a reader who follows it lands on the
    wrong text. The second paragraph already used the plain {@code HerdrPeerLauncher.spawn(...)}
    form. Both now match.
  2. The PR body claims "any future subclass of HerdrPeerLauncher MUST write that method's body
    (it's abstract), so they cannot avoid reading the paragraph". That is not true for a subclass.
    HerdrPeerLauncher already implements spawn(SpawnRequest, PlacementDecision) concretely at
    member/HerdrPeerLauncher.java:611, so a subclass inherits the body and is forced to read
    nothing. The forced read is real only for a direct implementer of the interface, which is
    where the new paragraph sits — so the protection works, but for a different reader than the
    PR describes. The javadoc now says which reader is which.

That second point is the useful lesson here, and it is not the worker's fault: it is the same
shape as the defect the ticket was about. A guarantee that rests on "they must write this method"
stops holding the moment an intermediate class writes it for them.

Out of scope and untouched, as the PR reports: PeerLauncher.disabledModels() /
modelGateState() have the same default-that-ignores-the-concept shape.

Closed by #456, merged as `e29227d`, plus one follow-up commit `3c5873d`. **Option 1 accepted: javadoc only, both methods stay `default`.** The worker's reason for rejecting Option 2 is the one that decides it, and I checked it myself rather than taking the number. The ticket estimated "two overrides". It is not two. Five test doubles implement `PeerLauncher` directly and rely on both defaults: ``` FleetdBackendErrorSinkTest.java:111 NeverSpawnsLauncher SessionManagerTest.java:997 RaceLauncher SessionManagerTest.java:1590 NoResumeLauncher SessionManagerTest.java:1652 ClearContextSpyLauncher SessionManagerTest.java:1859 LazyIdLauncher ``` Making both methods abstract forces about ten one-line overrides into five unrelated test files for no test value. My ticket text warned against exactly that ceremony, so my own estimate was the wrong half of the argument. **What I verified, and how.** Local merge of `0788d84` onto `2af13ab`, merge commit `5b46538`: - `mvn -B clean test` in `fleetd/`: `Tests run: 1578, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, exit code 0. - Javadoc reference lint, which for a doc-only diff is the closest thing to a test: `mvn -B -DskipTests javadoc:javadoc -Ddoclint=reference`. The merge reports 5 `reference not found`. `main` at `2af13ab` reports **the same 5, in the same 5 files**, none of them a file this PR touched. So this PR adds no broken link. Those 5 are now #459. - The quoted phrase is real: `are unoverridden here and just wrap {@link #defaultProfile()}` is at `member/HerdrPeerLauncher.java:604`. Control on that search: one match in the whole main tree before the PR, so the grep reached the tree. - `place` is not overloaded (`PeerLauncher.java:234` and `CompositePeerLauncher.java:701` are the only declarations), so the bare `{@link #place}` is unambiguous. - `FleetConfig.defaultProfileFor(role)` really is unrelated. `FleetConfig` is a bare `public record` with no `implements` clause, so it is correctly excluded from the count. **One thing I fixed rather than sending back** (`3c5873d`): 1. The first new paragraph wrote "`HerdrPeerLauncher`'s own `{@link #spawn(SpawnRequest, PlacementDecision)}` javadoc". That link resolves to `PeerLauncher`'s own abstract declaration, not to `HerdrPeerLauncher`'s override, so a reader who follows it lands on the wrong text. The second paragraph already used the plain `{@code HerdrPeerLauncher.spawn(...)}` form. Both now match. 2. The PR body claims "any future subclass of `HerdrPeerLauncher` MUST write that method's body (it's abstract), so they cannot avoid reading the paragraph". That is not true for a subclass. `HerdrPeerLauncher` already implements `spawn(SpawnRequest, PlacementDecision)` concretely at `member/HerdrPeerLauncher.java:611`, so a subclass inherits the body and is forced to read nothing. The forced read is real only for a **direct** implementer of the interface, which is where the new paragraph sits — so the protection works, but for a different reader than the PR describes. The javadoc now says which reader is which. That second point is the useful lesson here, and it is not the worker's fault: it is the same shape as the defect the ticket was about. A guarantee that rests on "they must write this method" stops holding the moment an intermediate class writes it for them. Out of scope and untouched, as the PR reports: `PeerLauncher.disabledModels()` / `modelGateState()` have the same default-that-ignores-the-concept shape.
ltms closed this issue 2026-09-10 13:07:40 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#453
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?