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

SpawnRequest has the arity trap's two halves, but not yet a colliding arity — CompositePeerLauncher:372 will drop the next component added #382

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

Found by the repo-wide sweep #358 asked for. Nothing is broken today. This is the same defect shape as #357 and #358, one component short of firing.

The shape, restated

A record has back-compat constructors at older arities, and somewhere rebuilds itself by listing its own current field values in a literal new Record(...) call. Add a component plus a back-compat constructor at the old arity — the established pattern here — and that literal call silently rebinds to the shorter constructor. It compiles, the suite passes, and the new field is defaulted away on that path.

Where it sits now

dev.ltms.fleet.peer.SpawnRequest: canonical arity 6, back-compat constructors at 5 and 3.

fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java:372 builds a new one from an existing request's own accessors, to swap in a routed profile:

SpawnRequest routedReq = new SpawnRequest(chosen.profile(), req.requestedCwd(), req.callerCwd(),
        req.sessionName(), req.resumeSessionId(), req.role());

Six arguments, matching the canonical constructor. Correct right now.

Why it is worth a ticket rather than a shrug

The two guarded cases were also correct right up until they weren't. FleetConfig.withDefaults() was fine at 22 arguments; #357 exists because it stopped being fine at 23. This call site has both halves of the mechanism already assembled.

What makes this one worse than the two just guarded is where it fires. It is not a config default or a session field — it is on the spawn path, and the field it would drop is whatever was just added to SpawnRequest. Every profile-routed spawn would lose it, and only profile-routed spawns would, so the feature would appear to work whenever routing did not kick in.

The sweep numbers, measured

Correction: the first version of this ticket said "89 records, 12 with a back-compat ladder". Those were the worker's numbers and I published them without running the count myself. Mine differ. Measured on cbc7324:

for m in re.finditer(r"record\s+(\w+)\s*\((.*?)\)\s*(?:implements[^{]*)?\{", src, re.S):
    # then, in the same file, every  public <Name>(...)  at a SHORTER arity

81 records under src/main/java, 10 with an explicit constructor at a shorter arity:

Fleet                canonical  6   shorter [5]                              FleetConfig.java
FleetConfig          canonical 24   shorter [14,15,16,17,18,20,21,22,23]     FleetConfig.java
Leader               canonical  9   shorter [7]                              FleetConfig.java
MemberCredentials    canonical  4   shorter [3]                              FleetConfig.java
Profile              canonical 26   shorter [12,14,15,18,20,22,24,25]        FleetConfig.java
MemberSession        canonical 15   shorter [12,14]                          MemberSession.java
Reply                canonical  3   shorter [2]                              MessageService.java
Principal            canonical  4   shorter [3]                              Principal.java
Resolution           canonical  3   shorter [2]                              Rendezvous.java
SpawnRequest         canonical  6   shorter [3,5]                            SpawnRequest.java

The totals disagree with the worker's because the counting rule differs — mine requires an explicit public <Name>( in the same file, and my record regex will miss any header shape it does not match. Treat 81/10 as my method's answer, not a settled fact; the useful part is the per-record rows, which are checkable one at a time.

Where the two methods overlap they agree exactly, which is what matters here: Profile has 8 shorter constructors at 25, 24, 22, 20, 18, 15, 14, 12, and MemberSession has 2 at 14 and 12 — both matching the #358 work already merged.

Of the 10, only SpawnRequest has a live external rebuild call outside what #357 and #358 already guard. Fleet and MemberCredentials are rebuilt only inside withDefaults(), which #357 covers. Leader, Reply, Principal and Resolution have a ladder but nothing that rebuilds them from their own fields.

Suggested work

Extend the same guard, do not invent a second mechanism. FleetConfigWithDefaultsPreservesEveryComponentTest and the two files #358 added are the pattern: enumerate getRecordComponents(), resolve the true canonical constructor by exact component types (never by argument count), give every component a distinctive non-null value, run the rebuild, assert every component survives, print the denominator, and pin the exclusion list size.

The wrinkle: this rebuild is not an in-type withX() method, it is an external call site that also legitimately changes the profile. So the check has to drive CompositePeerLauncher's routing path rather than call a method on the record. If that turns out to need more scaffolding than the guard is worth, an honest alternative is to give SpawnRequest a withProfile(String) method, move the rebuild into it, and guard that — which is what the other two records already look like.

Decide which of those two; do not do both.

How to prove it

Do not write null to simulate the bug. Reproduce the real mechanism: drop the last argument from the call at CompositePeerLauncher.java:372 and confirm it still compiles, binding to the 5-arg constructor. A mutation that compiles by accident is the whole point; one that must be written as null proves less.

Not in scope

Deleting the back-compat constructors. They exist for real callers.

Related: #357, #358.

Found by the repo-wide sweep #358 asked for. **Nothing is broken today.** This is the same defect shape as #357 and #358, one component short of firing. ## The shape, restated A record has back-compat constructors at older arities, *and* somewhere rebuilds itself by listing its own current field values in a literal `new Record(...)` call. Add a component plus a back-compat constructor at the old arity — the established pattern here — and that literal call silently rebinds to the shorter constructor. It compiles, the suite passes, and the new field is defaulted away on that path. ## Where it sits now `dev.ltms.fleet.peer.SpawnRequest`: canonical arity **6**, back-compat constructors at **5** and **3**. `fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java:372` builds a new one from an existing request's own accessors, to swap in a routed profile: ```java SpawnRequest routedReq = new SpawnRequest(chosen.profile(), req.requestedCwd(), req.callerCwd(), req.sessionName(), req.resumeSessionId(), req.role()); ``` Six arguments, matching the canonical constructor. Correct right now. ## Why it is worth a ticket rather than a shrug The two guarded cases were also correct right up until they weren't. `FleetConfig.withDefaults()` was fine at 22 arguments; #357 exists because it stopped being fine at 23. This call site has both halves of the mechanism already assembled. What makes this one worse than the two just guarded is **where it fires**. It is not a config default or a session field — it is on the spawn path, and the field it would drop is whatever was just added to `SpawnRequest`. Every profile-routed spawn would lose it, and only profile-routed spawns would, so the feature would appear to work whenever routing did not kick in. ## The sweep numbers, measured **Correction:** the first version of this ticket said "89 records, 12 with a back-compat ladder". Those were the worker's numbers and I published them without running the count myself. Mine differ. Measured on `cbc7324`: ```python for m in re.finditer(r"record\s+(\w+)\s*\((.*?)\)\s*(?:implements[^{]*)?\{", src, re.S): # then, in the same file, every public <Name>(...) at a SHORTER arity ``` **81 records** under `src/main/java`, **10** with an explicit constructor at a shorter arity: ``` Fleet canonical 6 shorter [5] FleetConfig.java FleetConfig canonical 24 shorter [14,15,16,17,18,20,21,22,23] FleetConfig.java Leader canonical 9 shorter [7] FleetConfig.java MemberCredentials canonical 4 shorter [3] FleetConfig.java Profile canonical 26 shorter [12,14,15,18,20,22,24,25] FleetConfig.java MemberSession canonical 15 shorter [12,14] MemberSession.java Reply canonical 3 shorter [2] MessageService.java Principal canonical 4 shorter [3] Principal.java Resolution canonical 3 shorter [2] Rendezvous.java SpawnRequest canonical 6 shorter [3,5] SpawnRequest.java ``` The totals disagree with the worker's because the counting rule differs — mine requires an explicit `public <Name>(` in the same file, and my record regex will miss any header shape it does not match. Treat 81/10 as *my method's* answer, not a settled fact; the useful part is the per-record rows, which are checkable one at a time. Where the two methods overlap they agree exactly, which is what matters here: `Profile` has 8 shorter constructors at 25, 24, 22, 20, 18, 15, 14, 12, and `MemberSession` has 2 at 14 and 12 — both matching the #358 work already merged. Of the 10, only `SpawnRequest` has a live external rebuild call outside what #357 and #358 already guard. `Fleet` and `MemberCredentials` are rebuilt only inside `withDefaults()`, which #357 covers. `Leader`, `Reply`, `Principal` and `Resolution` have a ladder but nothing that rebuilds them from their own fields. ## Suggested work Extend the same guard, do not invent a second mechanism. `FleetConfigWithDefaultsPreservesEveryComponentTest` and the two files #358 added are the pattern: enumerate `getRecordComponents()`, resolve the true canonical constructor by exact component types (never by argument count), give every component a distinctive non-null value, run the rebuild, assert every component survives, print the denominator, and pin the exclusion list size. The wrinkle: this rebuild is **not** an in-type `withX()` method, it is an external call site that also legitimately changes the profile. So the check has to drive `CompositePeerLauncher`'s routing path rather than call a method on the record. If that turns out to need more scaffolding than the guard is worth, an honest alternative is to give `SpawnRequest` a `withProfile(String)` method, move the rebuild into it, and guard that — which is what the other two records already look like. Decide which of those two; do not do both. ## How to prove it Do not write `null` to simulate the bug. Reproduce the real mechanism: drop the last argument from the call at `CompositePeerLauncher.java:372` and confirm it **still compiles**, binding to the 5-arg constructor. A mutation that compiles by accident is the whole point; one that must be written as `null` proves less. ## Not in scope Deleting the back-compat constructors. They exist for real callers. Related: #357, #358.
ltms commented 2026-09-10 01:55:18 +02:00
Author
Owner
Copy Link
Copy Source

Merged as fb36c52 (PR #387). Verified here, not taken on the worker's word.

What landed. SpawnRequest.withProfile(String), and CompositePeerLauncher:372 now calls it instead of rebuilding the record from six accessors. Net effect: the routed spawn keeps every component it had, and adding a seventh component to the record can no longer silently drop it at this one call site.

The worker took the ticket's second option and added no routing scaffolding. It left the arity-3 and arity-5 back-compat constructors alone, correctly — they are out of scope and they are also the trap this ticket is about.

Build at the merged HEAD: Tests run: 1464, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, exit 0. Unpiped, to a file.

Mutation, on the half the worker did not pin. The worker proved the record: it dropped the last argument inside withProfile(), watched it bind silently to the 5-arg constructor, and watched its own test catch the lost role. Good proof, but it is proof about SpawnRequest in isolation. Nothing in it says the call site routes the chosen profile rather than the caller's — which is the entire point of the change.

So I mutated that instead:

- SpawnRequest routedReq = req.withProfile(chosen.profile());
+ SpawnRequest routedReq = req.withProfile(req.profileName());

Caught, and not narrowly — 9 failures and 2 errors across CompositePeerLauncherTest, the whole placement suite at once:

weightedPolicyGatesProfileAtMaxLoad:540   profile a is at maxLoad, so every spawn must land on b ==> expected: <b> but was: <a>
placementSkipsAQuarantinedProfileAndRoutesToAnUnquarantinedOne:973  expected: <b> but was: <sol>
placementSkipsACoolingOffProfileAndRoutesToAnotherOne:1101          expected: <b> but was: <sol>
theRolePoolOutranksTheGlobalDefaultProfile:850                      expected: <sonnet> but was: <opus>
weightedPolicyDistributesAccordingToWeightRatio:560                 expected: <30> but was: <40>
… and 6 more

That is the answer I wanted: placement is pinned by tests that were already there, so this refactor cannot quietly turn routing off. Restored and reconfirmed green.

Denominator, since the worker printed one: its guard test reports 6 components, 6 checked, 0 excluded, 6 survived, and pins the exclusion list at size 0 with its own assertion. That is the right shape — a checker that prints its own denominator and pins its escape hatch.

Minor, not blocking: the guard test sits in dev.ltms.fleet.peer beside SpawnRequest rather than mirroring the brief's reference test. The worker flagged this itself and followed the local convention already set by CharterReceiptTest and MemberRoleTest in that package. Correct call.

Closing.

Merged as `fb36c52` (PR #387). Verified here, not taken on the worker's word. **What landed.** `SpawnRequest.withProfile(String)`, and `CompositePeerLauncher:372` now calls it instead of rebuilding the record from six accessors. Net effect: the routed spawn keeps every component it had, and adding a seventh component to the record can no longer silently drop it at this one call site. The worker took the ticket's second option and added no routing scaffolding. It left the arity-3 and arity-5 back-compat constructors alone, correctly — they are out of scope and they are also the trap this ticket is about. **Build at the merged HEAD:** `Tests run: 1464, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, exit 0. Unpiped, to a file. **Mutation, on the half the worker did not pin.** The worker proved the record: it dropped the last argument inside `withProfile()`, watched it bind silently to the 5-arg constructor, and watched its own test catch the lost `role`. Good proof, but it is proof about `SpawnRequest` in isolation. Nothing in it says the **call site** routes the chosen profile rather than the caller's — which is the entire point of the change. So I mutated that instead: ```java - SpawnRequest routedReq = req.withProfile(chosen.profile()); + SpawnRequest routedReq = req.withProfile(req.profileName()); ``` Caught, and not narrowly — 9 failures and 2 errors across `CompositePeerLauncherTest`, the whole placement suite at once: ``` weightedPolicyGatesProfileAtMaxLoad:540 profile a is at maxLoad, so every spawn must land on b ==> expected: <b> but was: <a> placementSkipsAQuarantinedProfileAndRoutesToAnUnquarantinedOne:973 expected: <b> but was: <sol> placementSkipsACoolingOffProfileAndRoutesToAnotherOne:1101 expected: <b> but was: <sol> theRolePoolOutranksTheGlobalDefaultProfile:850 expected: <sonnet> but was: <opus> weightedPolicyDistributesAccordingToWeightRatio:560 expected: <30> but was: <40> … and 6 more ``` That is the answer I wanted: placement is pinned by tests that were already there, so this refactor cannot quietly turn routing off. Restored and reconfirmed green. **Denominator, since the worker printed one:** its guard test reports `6 components, 6 checked, 0 excluded, 6 survived`, and pins the exclusion list at size 0 with its own assertion. That is the right shape — a checker that prints its own denominator and pins its escape hatch. **Minor, not blocking:** the guard test sits in `dev.ltms.fleet.peer` beside `SpawnRequest` rather than mirroring the brief's reference test. The worker flagged this itself and followed the local convention already set by `CharterReceiptTest` and `MemberRoleTest` in that package. Correct call. Closing.
ltms closed this issue 2026-09-10 01:55:23 +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#382
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?