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

Two more records can silently drop a field the way FleetConfig could (MemberSession, FleetConfig.Profile) #358

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

Follow-up to #357 (merged in 650a4c1). That PR guarded FleetConfig.withDefaults(). The same
shape exists in two more places and is not guarded.

The shape

A record has a back-compat constructor ladder — extra constructors kept at older arities so old
callers still compile — and an internal method that rebuilds the record with a literal argument
list.

Add a component, add a back-compat constructor at the old arity (the established pattern here), and
the rebuild method's own call is now a legal match for that new back-compat constructor. It
compiles. The suite passes. The new field is silently defaulted away.

It is not a theoretical risk. It fired once already, on the branch that became #355.

Measured on 650a4c1

MemberSession   canonical arity 15   back-compat at 14, 12    5 rebuild sites
Profile         canonical arity 26   long back-compat ladder  1 rebuild site

Command used:

m = re.search(rf"record {rec}\((.*?)\)\s*(?:implements[^{{]*)?\{{", src, re.S)   # canonical
rebuilds = re.findall(rf"return new {rec}\(", src)                              # rebuild sites

Caveat on the ladder count: my regex for Profile also matched some non-constructor forms, so
treat "long ladder" as the reliable part and re-count before relying on an exact number.

dev.ltms.fleet.session.MemberSession

The worse of the two — five rebuild sites, not one:

withState   withActivity   bumpTurn   withAgentSessionId   withFailureReason

Each ends in a literal 15-arg new MemberSession(...). A 16th component would need all five
updated, and any one missed drops the field on that path only — so the field would work through
some transitions and vanish through others. That is harder to spot than #357's case, where a single
call site meant the field was simply always absent.

dev.ltms.fleet.config.FleetConfig.Profile

Nested in the same file #357 touched. One rebuild site, withProfile(String), a literal 26-arg
call. Correct today, exactly as withDefaults() was correct at 22 args before it wasn't.

Suggested work

Extend the guard from #357 rather than inventing a second mechanism. See
FleetConfigWithDefaultsPreservesEveryComponentTest: it enumerates
X.class.getRecordComponents(), resolves the true canonical constructor with
getDeclaredConstructor(exact component types) — never by argument count — gives every component a
distinctive non-null value, runs the rebuild, and asserts every value survives. It never hardcodes
the arity, and it pins the size of its own exclusion list so the escape hatch cannot grow quietly.

MemberSession needs one such check per rebuild method, since each is an independent call site.

Not in scope

Deleting the back-compat constructors. They exist for real callers. This is about protecting the
rebuild methods from them.

How to prove it

Do not simulate the bug by passing an explicit null. Reproduce the real mechanism: drop the last
argument from a rebuild call so it binds to the existing back-compat constructor. On #357 that
compiled cleanly and the guard failed by name — which is the point. A mutation that must be written
as null proves less than one that compiles by accident.

Follow-up to #357 (merged in `650a4c1`). That PR guarded `FleetConfig.withDefaults()`. The same shape exists in two more places and is **not** guarded. ## The shape A record has a **back-compat constructor ladder** — extra constructors kept at older arities so old callers still compile — **and** an internal method that rebuilds the record with a literal argument list. Add a component, add a back-compat constructor at the old arity (the established pattern here), and the rebuild method's own call is now a legal match for that new back-compat constructor. It compiles. The suite passes. The new field is silently defaulted away. It is not a theoretical risk. It fired once already, on the branch that became #355. ## Measured on `650a4c1` ``` MemberSession canonical arity 15 back-compat at 14, 12 5 rebuild sites Profile canonical arity 26 long back-compat ladder 1 rebuild site ``` Command used: ```python m = re.search(rf"record {rec}\((.*?)\)\s*(?:implements[^{{]*)?\{{", src, re.S) # canonical rebuilds = re.findall(rf"return new {rec}\(", src) # rebuild sites ``` Caveat on the ladder count: my regex for `Profile` also matched some non-constructor forms, so treat "long ladder" as the reliable part and re-count before relying on an exact number. ### `dev.ltms.fleet.session.MemberSession` The worse of the two — **five** rebuild sites, not one: ``` withState withActivity bumpTurn withAgentSessionId withFailureReason ``` Each ends in a literal 15-arg `new MemberSession(...)`. A 16th component would need all five updated, and any one missed drops the field on that path only — so the field would work through some transitions and vanish through others. That is harder to spot than #357's case, where a single call site meant the field was simply always absent. ### `dev.ltms.fleet.config.FleetConfig.Profile` Nested in the same file #357 touched. One rebuild site, `withProfile(String)`, a literal 26-arg call. Correct today, exactly as `withDefaults()` was correct at 22 args before it wasn't. ## Suggested work Extend the guard from #357 rather than inventing a second mechanism. See `FleetConfigWithDefaultsPreservesEveryComponentTest`: it enumerates `X.class.getRecordComponents()`, resolves the **true** canonical constructor with `getDeclaredConstructor(exact component types)` — never by argument count — gives every component a distinctive non-null value, runs the rebuild, and asserts every value survives. It never hardcodes the arity, and it pins the size of its own exclusion list so the escape hatch cannot grow quietly. `MemberSession` needs one such check **per rebuild method**, since each is an independent call site. ## Not in scope Deleting the back-compat constructors. They exist for real callers. This is about protecting the rebuild methods from them. ## How to prove it Do not simulate the bug by passing an explicit `null`. Reproduce the real mechanism: **drop the last argument** from a rebuild call so it binds to the existing back-compat constructor. On #357 that compiled cleanly and the guard failed by name — which is the point. A mutation that must be written as `null` proves less than one that compiles by accident.
ltms commented 2026-09-09 02:39:09 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main in 7057291 (pushed as part of 6f828b8). PR #378.

What I verified myself

I did not take the worker's build result. I merged all three of tonight's branches onto current main and ran mvn clean install from fleetd/, unpiped, output to a file: MVN_EXIT=0, Tests run: 1450, Failures: 0, Errors: 0, Skipped: 0, and separately counted compile errors (0). The arithmetic matches exactly: 1441 on main before tonight, plus 8 from this ticket, plus 1 from #373, plus 0 from #365.

The mutation I ran, which is not the one the worker ran

The worker mutated all 6 rebuild sites and showed each guard failing by name. That proves the guards catch a dropped argument. It does not prove the guard's denominator is honest — a component-survival test that silently checks one component fewer than the record has is the failure this whole family of tickets exists to prevent.

So I mutated the other half. I deleted one entry (branch) from baseValues() in MemberSessionRebuildPreservesEveryComponentTest. That is what a future person adding a record component and forgetting this file would effectively do.

Tests run: 6, Failures: 5
AssertionFailedError: this test's value map has drifted from MemberSession's actual
components — update baseValues() alongside the record
 ==> expected: <[agentSessionId, branch, charterReceipt, ...]>
     but was:  <[agentSessionId, charterReceipt, ...]>

5 of the 6 fail, by name. The 6th is exclusionListSizeIsPinned, which does not call baseValues() — correct, not a gap. Restored and confirmed byte-identical.

That is the property I actually care about here: this guard cannot go quiet by shrinking. Both exclusion lists are empty and pinned at 0 by their own test, and both checks print their denominator.

A correction to this ticket's own numbers

The worker re-counted what this ticket asserted. FleetConfig.Profile's back-compat ladder is 8 constructors, at arities 25, 24, 22, 20, 18, 15, 14, 12. This ticket said only "long ladder" and flagged its own number as untrusted, which was the right call.

Follow-up filed separately

The sweep found one place with the same shape that is not exposed yet: CompositePeerLauncher.java:372 rebuilds a SpawnRequest from an existing one's own fields at the full canonical arity, and SpawnRequest has back-compat constructors at 5 and 3. It is correct today. It becomes the same silent-drop defect the moment someone adds a 7th component. Filing that as its own ticket rather than widening this one.

Closing.

Merged to `main` in 7057291 (pushed as part of 6f828b8). PR #378. ## What I verified myself I did not take the worker's build result. I merged all three of tonight's branches onto current `main` and ran `mvn clean install` from `fleetd/`, unpiped, output to a file: **`MVN_EXIT=0`, `Tests run: 1450, Failures: 0, Errors: 0, Skipped: 0`**, and separately counted compile errors (`0`). The arithmetic matches exactly: 1441 on `main` before tonight, plus 8 from this ticket, plus 1 from #373, plus 0 from #365. ## The mutation I ran, which is not the one the worker ran The worker mutated all 6 rebuild sites and showed each guard failing by name. That proves the guards catch a dropped argument. It does not prove the guard's **denominator** is honest — a component-survival test that silently checks one component fewer than the record has is the failure this whole family of tickets exists to prevent. So I mutated the other half. I deleted one entry (`branch`) from `baseValues()` in `MemberSessionRebuildPreservesEveryComponentTest`. That is what a future person adding a record component and forgetting this file would effectively do. ``` Tests run: 6, Failures: 5 AssertionFailedError: this test's value map has drifted from MemberSession's actual components — update baseValues() alongside the record ==> expected: <[agentSessionId, branch, charterReceipt, ...]> but was: <[agentSessionId, charterReceipt, ...]> ``` 5 of the 6 fail, by name. The 6th is `exclusionListSizeIsPinned`, which does not call `baseValues()` — correct, not a gap. Restored and confirmed byte-identical. That is the property I actually care about here: this guard cannot go quiet by shrinking. Both exclusion lists are empty and pinned at 0 by their own test, and both checks print their denominator. ## A correction to this ticket's own numbers The worker re-counted what this ticket asserted. `FleetConfig.Profile`'s back-compat ladder is **8 constructors**, at arities 25, 24, 22, 20, 18, 15, 14, 12. This ticket said only "long ladder" and flagged its own number as untrusted, which was the right call. ## Follow-up filed separately The sweep found one place with the same shape that is not exposed yet: `CompositePeerLauncher.java:372` rebuilds a `SpawnRequest` from an existing one's own fields at the full canonical arity, and `SpawnRequest` has back-compat constructors at 5 and 3. It is correct today. It becomes the same silent-drop defect the moment someone adds a 7th component. Filing that as its own ticket rather than widening this one. Closing.
ltms closed this issue 2026-09-09 02:39:14 +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#358
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?