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

PeerLauncher's default spawn(req, decision) is still the #425 defect, and nothing tests it — make it abstract #450

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

CORRECTION (2026-09-10) — the class list below was wrong, and so was the command quoted next to it

The original body claimed five src/main implementers of PeerLauncher and quoted
grep -rln 'implements PeerLauncher' as the source. That command returns two files, not five.
I ran a wider pattern ('implements PeerLauncher\|PeerLauncher,\|extends .*PeerLauncher'), which
also matched a comment and two subclasses, and then quoted the narrow command beside the wide
command's output. The ground truth:

ConfigRef              public final class ConfigRef implements Supplier<FleetConfig> {
ClaudeCodeLauncher     public final class ClaudeCodeLauncher extends HerdrPeerLauncher {
OpenCodeLauncher       public final class OpenCodeLauncher extends HerdrPeerLauncher {
HerdrPeerLauncher      public abstract class HerdrPeerLauncher implements PeerLauncher {
CompositePeerLauncher  public final class CompositePeerLauncher implements PeerLauncher {

So there are two direct implementers. ConfigRef does not implement PeerLauncher at all —
the match was a comment on line 562. ClaudeCodeLauncher and OpenCodeLauncher extend
HerdrPeerLauncher, so one override in that parent serves both.

The worker on this ticket re-measured in its own tree, as briefed, found the difference, and
flagged it instead of following my list. That is the correct behaviour and it is why the brief
said to re-measure. The fix below is unaffected — making the method abstract is still right, and
the compile error still lands on ClaudeCodeLauncher and OpenCodeLauncher by name.

The sentence "One override, four inheritors" further down is wrong for the same reason. Read it as
"one override, one abstract implementer with two subclasses".

Follow-up to #444, which is closed by PR #447 (merged 822327e). #447 did the main thing the ticket asked and I verified it. This is the half of #444's criterion 3 that was not done, filed separately instead of left in a merge comment.

What #444 asked, and what landed

#444's criterion 3 offered two ways to settle the interface default:

  1. keep it, and add a test proving a default-implementing launcher behaves acceptably, or
  2. make it abstract, so a new placement-doing launcher cannot silently inherit the defect.

#447 chose option 1 and said why: HerdrPeerLauncher is single-profile, so the default is correct for it, not just harmless. That reasoning is sound. But option 1 has two halves, and only the first was done. The javadoc now warns clearly. No test pins it.

Measured on main at 822327e

Overrides of the 2-argument spawn:

$ grep -rn 'spawn(SpawnRequest req, PlacementDecision' fleetd/src/main/java --include='*.java'
CompositePeerLauncher.java:768:    public PeerHandle spawn(SpawnRequest req, PlacementDecision decision) {
PeerLauncher.java:262:    default PeerHandle spawn(SpawnRequest req, PlacementDecision decision) {

And tests that call the 2-argument spawn on a launcher:

$ grep -rn '\.spawn([a-zA-Z]*, *decision' fleetd/src/test/java --include='*.java'
CompositePeerLauncherTest.java:1171:        PeerHandle handle = composite.spawn(req, decision);

That one call is on CompositePeerLauncher — the only class that overrides the method. So every test of this method tests the override, and the inherited default runs in no test at all.

A note on how I got that number. My first run of the same greps returned 1 file for PlacementDecision in tests, because my working tree was still at 82fae94 and the merge existed only in origin/main. The new test file was not on disk yet. The counts above are from 822327e after a fast-forward. A search can only see the tree you are standing in.

Why this still matters after #447

SessionManager holds the interface type, not the concrete class (SessionManager.java:48). So the risk is not in today's code. It is that the next PeerLauncher that does placement filtering, and forgets to override this method, gets the original #425 defect back — silently, with the whole suite green. The default's body is that defect:

default PeerHandle spawn(SpawnRequest req, PlacementDecision decision) {
    return spawn(req.withProfile(decision.profile()));
}

Re-entering the single-argument spawn re-applies its explicit-profile checks — enforceNotQuarantined, enforceNotCoolingOff, enforceMaxLoad, enforceModelEnabled — which can refuse the very profile place() just chose. That is the window PlacementDecision exists to close.

This repo already has a note about this exact shape: a filter that lives in a shared default is only applied by the implementers that override it, so the default implementation is the one that needs the test. A hardcoded set of correct implementers is not a guarantee, it is a snapshot. The correction at the top of this ticket is itself an example: my snapshot of the implementer list was wrong within an hour of writing it.

What to do

Make the 2-argument spawn abstract, and override it explicitly in each launcher that inherits it today. This turns a silent future regression into a compile error, which this repo prefers over a documented hazard.

  • Re-measure the implementer list yourself first — see the correction at the top. Work from your own reading of the class declarations, not from any list in this ticket.
  • For each class that must now override: if it does no placement filtering, use the re-entering body — return spawn(req.withProfile(decision.profile())); — plus a one-line comment saying why that is correct there. If it does filter, it needs the routing form instead, and say so in your report.
  • Do not copy the comment into a class where it is not true. A comment saying "no placement filtering here" in a class that filters is worse than no comment.
  • CompositePeerLauncher — unchanged. It already overrides.
  • Test fakes implementing PeerLauncher will also stop compiling. Give each an override mirroring its own existing spawn(SpawnRequest) shape: delegating fakes delegate, unreachable stubs throw the same exception, single-profile fakes re-enter.

The fleet01 lead independently argued for abstract-over-default on this same method. I am naming that as agreement, not as evidence: two people preferring a design is not a measurement.

Acceptance criteria

  1. The 2-argument spawn is abstract on PeerLauncher. Everything compiles, and each new override carries a one-line comment saying which of the two forms it uses and why.
  2. The compile error is the point, so prove it exists. Delete one of the new overrides and show the build failing with the error naming the affected class or classes. Then restore it and show green. A claim that "it would now be a compile error" is not evidence.
  3. mvn clean install green. Full log to a file, then grep it. No -q — it suppresses INFO, which deletes the Tests run: and BUILD SUCCESS lines you need. No | tail, which hides a failure behind a zero exit.
  4. Report the Tests run: line and the BUILD line as they actually appear.

Out of scope

  • Do not change any enforce* check, and do not change the routing branch's fall-through behaviour. That difference is deliberate and documented.
  • Do not touch CompositePeerLauncher's override or the test #447 added.
  • Do not revisit #425, #435 or #444. All three are settled.
  • Note in one line anything else you find with this shape — a default method carrying behaviour that only some implementers may safely inherit — but do not fix it here.
> ## CORRECTION (2026-09-10) — the class list below was wrong, and so was the command quoted next to it > > The original body claimed five `src/main` implementers of `PeerLauncher` and quoted > `grep -rln 'implements PeerLauncher'` as the source. **That command returns two files, not five.** > I ran a wider pattern (`'implements PeerLauncher\|PeerLauncher,\|extends .*PeerLauncher'`), which > also matched a comment and two subclasses, and then quoted the narrow command beside the wide > command's output. The ground truth: > > ``` > ConfigRef public final class ConfigRef implements Supplier<FleetConfig> { > ClaudeCodeLauncher public final class ClaudeCodeLauncher extends HerdrPeerLauncher { > OpenCodeLauncher public final class OpenCodeLauncher extends HerdrPeerLauncher { > HerdrPeerLauncher public abstract class HerdrPeerLauncher implements PeerLauncher { > CompositePeerLauncher public final class CompositePeerLauncher implements PeerLauncher { > ``` > > So there are **two** direct implementers. `ConfigRef` does not implement `PeerLauncher` at all — > the match was a comment on line 562. `ClaudeCodeLauncher` and `OpenCodeLauncher` extend > `HerdrPeerLauncher`, so one override in that parent serves both. > > The worker on this ticket re-measured in its own tree, as briefed, found the difference, and > flagged it instead of following my list. That is the correct behaviour and it is why the brief > said to re-measure. The fix below is unaffected — making the method abstract is still right, and > the compile error still lands on `ClaudeCodeLauncher` and `OpenCodeLauncher` by name. > > The sentence "One override, four inheritors" further down is wrong for the same reason. Read it as > "one override, one abstract implementer with two subclasses". --- Follow-up to #444, which is closed by PR #447 (merged `822327e`). #447 did the main thing the ticket asked and I verified it. This is the half of #444's criterion 3 that was **not** done, filed separately instead of left in a merge comment. ## What #444 asked, and what landed #444's criterion 3 offered two ways to settle the interface default: 1. keep it, and add a test proving a default-implementing launcher behaves acceptably, or 2. make it abstract, so a new placement-doing launcher cannot silently inherit the defect. #447 chose option 1 and said why: `HerdrPeerLauncher` is single-profile, so the default is correct for it, not just harmless. That reasoning is sound. But option 1 has two halves, and only the first was done. The javadoc now warns clearly. No test pins it. ## Measured on `main` at `822327e` Overrides of the 2-argument `spawn`: ``` $ grep -rn 'spawn(SpawnRequest req, PlacementDecision' fleetd/src/main/java --include='*.java' CompositePeerLauncher.java:768: public PeerHandle spawn(SpawnRequest req, PlacementDecision decision) { PeerLauncher.java:262: default PeerHandle spawn(SpawnRequest req, PlacementDecision decision) { ``` And tests that call the 2-argument `spawn` on a launcher: ``` $ grep -rn '\.spawn([a-zA-Z]*, *decision' fleetd/src/test/java --include='*.java' CompositePeerLauncherTest.java:1171: PeerHandle handle = composite.spawn(req, decision); ``` That one call is on `CompositePeerLauncher` — the only class that overrides the method. So every test of this method tests the override, and the inherited default runs in no test at all. A note on how I got that number. My first run of the same greps returned 1 file for `PlacementDecision` in tests, because my working tree was still at `82fae94` and the merge existed only in `origin/main`. The new test file was not on disk yet. The counts above are from `822327e` after a fast-forward. A search can only see the tree you are standing in. ## Why this still matters after #447 `SessionManager` holds the interface type, not the concrete class (`SessionManager.java:48`). So the risk is not in today's code. It is that the next `PeerLauncher` that does placement filtering, and forgets to override this method, gets the original #425 defect back — silently, with the whole suite green. The default's body is that defect: ```java default PeerHandle spawn(SpawnRequest req, PlacementDecision decision) { return spawn(req.withProfile(decision.profile())); } ``` Re-entering the single-argument `spawn` re-applies its explicit-profile checks — `enforceNotQuarantined`, `enforceNotCoolingOff`, `enforceMaxLoad`, `enforceModelEnabled` — which can refuse the very profile `place()` just chose. That is the window `PlacementDecision` exists to close. This repo already has a note about this exact shape: **a filter that lives in a shared default is only applied by the implementers that override it, so the default implementation is the one that needs the test.** A hardcoded set of correct implementers is not a guarantee, it is a snapshot. The correction at the top of this ticket is itself an example: my snapshot of the implementer list was wrong within an hour of writing it. ## What to do **Make the 2-argument `spawn` abstract**, and override it explicitly in each launcher that inherits it today. This turns a silent future regression into a compile error, which this repo prefers over a documented hazard. - **Re-measure the implementer list yourself first** — see the correction at the top. Work from your own reading of the class declarations, not from any list in this ticket. - For each class that must now override: if it does no placement filtering, use the re-entering body — `return spawn(req.withProfile(decision.profile()));` — plus a one-line comment saying why that is correct there. If it does filter, it needs the routing form instead, and say so in your report. - **Do not copy the comment into a class where it is not true.** A comment saying "no placement filtering here" in a class that filters is worse than no comment. - `CompositePeerLauncher` — unchanged. It already overrides. - Test fakes implementing `PeerLauncher` will also stop compiling. Give each an override mirroring its own existing `spawn(SpawnRequest)` shape: delegating fakes delegate, unreachable stubs throw the same exception, single-profile fakes re-enter. The fleet01 lead independently argued for abstract-over-default on this same method. I am naming that as agreement, not as evidence: two people preferring a design is not a measurement. ## Acceptance criteria 1. The 2-argument `spawn` is abstract on `PeerLauncher`. Everything compiles, and each new override carries a one-line comment saying which of the two forms it uses and why. 2. **The compile error is the point, so prove it exists.** Delete one of the new overrides and show the build failing with the error naming the affected class or classes. Then restore it and show green. A claim that "it would now be a compile error" is not evidence. 3. `mvn clean install` green. Full log to a file, then grep it. **No `-q`** — it suppresses INFO, which deletes the `Tests run:` and `BUILD SUCCESS` lines you need. No `| tail`, which hides a failure behind a zero exit. 4. Report the `Tests run:` line and the `BUILD` line as they actually appear. ## Out of scope - Do not change any `enforce*` check, and do not change the routing branch's fall-through behaviour. That difference is deliberate and documented. - Do not touch `CompositePeerLauncher`'s override or the test #447 added. - Do not revisit #425, #435 or #444. All three are settled. - Note in one line anything else you find with this shape — a `default` method carrying behaviour that only some implementers may safely inherit — but **do not fix it** here.
ltms commented 2026-09-10 12:17:06 +02:00
Author
Owner
Copy Link
Copy Source

Closed by PR #451.

What landed

PeerLauncher.spawn(SpawnRequest, PlacementDecision) is abstract. HerdrPeerLauncher — the abstract base of ClaudeCodeLauncher and OpenCodeLauncher — carries one override with the re-entering body and a comment saying why that form is correct there: place/defaultProfileFor are unoverridden in it, so it does no quarantine, cool-off, maxLoad or model-off filtering to re-apply. CompositePeerLauncher's routing-form override is untouched. Five test fakes got overrides mirroring their own spawn(SpawnRequest) shape.

My verification, on head cfebc57

FULL BUILD  Tests run: 1575, Failures: 0, Errors: 0, Skipped: 0  BUILD SUCCESS
            compile errors: 0

M1  delete HerdrPeerLauncher's new override
    -> BUILD FAILURE, 1 COMPILATION ERROR block, naming exactly
       ClaudeCodeLauncher.java:[49,14] and OpenCodeLauncher.java:[59,14]

M2  CONTROL: put the interface method back to a `default` AND delete the override
    -> BUILD SUCCESS, 0 compile errors

M3  CompositePeerLauncher's override reverts to the re-entering form
    -> KILLED, Errors: 1, by
       CompositePeerLauncherTest
         .spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace

M2 is the row that makes M1 mean anything. On its own, M1 only shows the build broke when I deleted something. M2 restores the default and deletes the same override, and it compiles — so the break is attributable to the method being abstract, not to anything else the edit disturbed. A mutation without a control is a coincidence with extra steps.

M3 matters because this PR changes the interface that #447's test depends on. The guarantee survives.

The ticket was wrong and the worker caught it

My body listed five src/main implementers and quoted grep -rln 'implements PeerLauncher' as the source. That command returns two files. I had run a wider pattern that also matched a comment in ConfigRef and the extends HerdrPeerLauncher line in two subclasses, then printed the narrow command next to the wide command's output.

The worker re-measured in its own tree, as briefed, found the difference, and flagged it rather than following my list. That is exactly the behaviour the brief asked for, and it is the second time this week that "re-measure it yourself, my numbers are from my tree" has caught something. The body is corrected at the top.

Worth naming the shape, because it is not the usual one. My earlier notes are about a search that finds too little — a broken pattern returning zero and being read as absence. This was a search that found too much, and the damage came from pairing its output with a different, narrower command in a document other people act on. A control would not have caught it: both commands ran fine and both returned real output. What catches this is only quoting the command you actually ran.

Closed by PR #451. ## What landed `PeerLauncher.spawn(SpawnRequest, PlacementDecision)` is abstract. `HerdrPeerLauncher` — the abstract base of `ClaudeCodeLauncher` and `OpenCodeLauncher` — carries one override with the re-entering body and a comment saying why that form is correct there: `place`/`defaultProfileFor` are unoverridden in it, so it does no quarantine, cool-off, maxLoad or model-off filtering to re-apply. `CompositePeerLauncher`'s routing-form override is untouched. Five test fakes got overrides mirroring their own `spawn(SpawnRequest)` shape. ## My verification, on head `cfebc57` ``` FULL BUILD Tests run: 1575, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS compile errors: 0 M1 delete HerdrPeerLauncher's new override -> BUILD FAILURE, 1 COMPILATION ERROR block, naming exactly ClaudeCodeLauncher.java:[49,14] and OpenCodeLauncher.java:[59,14] M2 CONTROL: put the interface method back to a `default` AND delete the override -> BUILD SUCCESS, 0 compile errors M3 CompositePeerLauncher's override reverts to the re-entering form -> KILLED, Errors: 1, by CompositePeerLauncherTest .spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace ``` **M2 is the row that makes M1 mean anything.** On its own, M1 only shows the build broke when I deleted something. M2 restores the `default` and deletes the same override, and it compiles — so the break is attributable to the method being abstract, not to anything else the edit disturbed. A mutation without a control is a coincidence with extra steps. M3 matters because this PR changes the interface that #447's test depends on. The guarantee survives. ## The ticket was wrong and the worker caught it My body listed five `src/main` implementers and quoted `grep -rln 'implements PeerLauncher'` as the source. That command returns two files. I had run a wider pattern that also matched a comment in `ConfigRef` and the `extends HerdrPeerLauncher` line in two subclasses, then printed the narrow command next to the wide command's output. The worker re-measured in its own tree, as briefed, found the difference, and flagged it rather than following my list. That is exactly the behaviour the brief asked for, and it is the second time this week that "re-measure it yourself, my numbers are from my tree" has caught something. The body is corrected at the top. Worth naming the shape, because it is not the usual one. My earlier notes are about a search that finds **too little** — a broken pattern returning zero and being read as absence. This was a search that found **too much**, and the damage came from pairing its output with a different, narrower command in a document other people act on. A control would not have caught it: both commands ran fine and both returned real output. What catches this is only quoting the command you actually ran.
ltms closed this issue 2026-09-10 12:17:12 +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#450
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?