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

adding a FleetConfig field must not silently drop it #357

Closed
agent wants to merge 0 commits from worker/withdefaults-guard-561704 into main
pull from: worker/withdefaults-guard-561704
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-05 00:53:04 +02:00
Member
Copy Link
Copy Source

Why

PR #355 (parked, not merged, per lead's instruction — the fleet is moving off macOS so the
idle-sleep workaround it built is no longer the direction) surfaced a live bug while I was adding
a component to FleetConfig: withDefaults()'s final return new FleetConfig(...) call was still
written at the pre-addition arg count, so it silently bound to the freshly-added back-compat
constructor at that arity instead of the new canonical constructor — the new field came back null
from every load(). My own new tests for that field caught it; nothing else in the suite did.

The lead re-measured this independently on main and named the mechanism a "defect factory":
the exact pattern this file uses to keep old callers compiling — add a component (record grows by
one arg) and add a back-compat constructor at the OLD arity — also lets that new back-compat
constructor swallow withDefaults()'s own literal-arity call, because that call is now a legal
overload match too. It compiles. Every other test passes, because nothing else exercises the new
field. The new component is defaulted away, silently, on every load. This has fired once already
(the sleep-guard branch) and will fire again on the next config key — which matters more right now
because a new Linux host is about to get a hand-written fleetd.yaml.

Goal: make it impossible for a component to be added to FleetConfig and silently not survive
withDefaults().

Scope: fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java and its tests only. This PR
does not add the idleSleepGuard field — that belongs to the parked PR #355 and does not ride
along here. This unit is built and tested against main as it stands today: 22 top-level
components, back-compat constructors at 21/20/18/17/16/15/14 (re-verified fresh on this branch —
see "What I verified" below).

Mechanism chosen, and why

A reflective test, not a compile-time fix. New file:
fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigWithDefaultsPreservesEveryComponentTest.java.

It mirrors the exact reflective-construction pattern this file's ecosystem already uses in
ConfigRefTopLevelReportingCoverageTest (FleetConfig.class.getRecordComponents(), then
getDeclaredConstructor(exact component types) to resolve the TRUE canonical constructor — the same
way Jackson resolves it, never by argument count):

  1. Build one FleetConfig through that true canonical constructor, giving every one of the 22
    top-level components a real, distinctive, non-null value (non-blank for placement, the one
    String whose blankness has meaning).
  2. Call the real withDefaults().
  3. Assert every component's value survives unchanged.

Why this is a valid check for every component: withDefaults()'s own comments document that it
only ever replaces a component when the incoming value is null (or blank, for placement) —
broker/primary/leadHeartbeat/configReload/coordinator/worktreeGroup/memberLoginShell
are passed through unconditionally, and bind/guard/lifecycle/auth/fleet/
quarantineCooldownSeconds/memberCredentials/placement are replaced only on null/blank input.
A value that's never null or blank going in must never change coming out — for every current
component, with no exceptions.

Why not the compile-time route the brief also offered as a candidate: stopping withDefaults()
from binding to a back-compat overload at all, without deleting any back-compat constructor, would
need either a code-generation/annotation-processor step (out of proportion to one file) or routing
the call through some indirection that still has to be updated by hand every time a component is
added — which is exactly the same "a human has to remember" failure this bug already demonstrated
once. The reflective test instead makes the omission fail loudly, which is what constraint 2
asks for, and it does so using a pattern this codebase has already reviewed and trusted once.

Never hardcodes the arity: the test enumerates COMPONENTS.length at runtime, so it keeps working
however many components the record grows to (constraint 3). No back-compat constructor is touched,
deleted, or restructured (constraint 1).

Exclusion list

EXCLUDED_FROM_SURVIVAL_CHECK is declared, and empty. Given every component a real, non-null
(non-blank where relevant) value, all 22 current components are documented to survive
withDefaults() unchanged, so none needs excluding today.

It's still declared, and its size is pinned by its own test —
exclusionListSizeIsPinned() asserts EXCLUDED_FROM_SURVIVAL_CHECK.size() == 0 — so a future
component that withDefaults() is documented to transform unconditionally (unlike any field
today) has one obvious, justified place to go, and growing that set to make a failure go away is a
visible diff to a pinned assertion, not a silent one. This is the "print the denominator" and "pin
the escape hatch" requirement from the brief, applied to a currently-empty case.

The coverage line the test itself prints, from a clean run:

FleetConfig.withDefaults() component-survival coverage — 22 components, 22 checked, 0 excluded, 22 survived

Mutation proof

Copied FleetConfig.java aside with cp (never git checkout --) before mutating, and restored
from that copy afterward (verified byte-identical with diff — see below).

Mutated withDefaults()'s final constructor call to drop worktreeGroup (a real, non-excluded
component — the exclusion list is empty, so there is no excluded component to test the negative
case against; see "What the empty exclusion list means for coverage" below):

-                quarantineCooldown, mc, coordinator, worktreeGroup, memberLoginShell);
+                quarantineCooldown, mc, coordinator, null, memberLoginShell);

Ran the FULL suite unpiped (mvn clean install). New test failed, by name and line, naming the
dropped component:

[ERROR]   FleetConfigWithDefaultsPreservesEveryComponentTest.everyComponentGivenARealValueSurvivesWithDefaults:189
withDefaults() silently dropped these real, given components: [worktreeGroup: withDefaults() was
given a real, non-null value (group-guard) for 'worktreeGroup' but returned null — a component
silently dropped by withDefaults(), the shape of the defect this test exists to catch (its final
"return new FleetConfig(...)" call binding to a back-compat constructor instead of the true
canonical one)] ==> expected: <[]> but was: <[worktreeGroup: ...]>

Same mutation also failed three pre-existing tests that already happened to name worktreeGroup
specifically (ConfigRefTest.changingWorktreeGroupIsReportedAsDeferred,
FleetConfigTest.absentWorktreeGroupSurvivesTheBackCompatConstructorChain,
FleetConfigTest.worktreeGroupKeyParses) plus three downstream tests whose behaviour depends on
worktreeGroup reaching them (HerdrPeerLauncherAllowListWiringTest x2,
ClaudeCodeLauncherTest/OpenCodeLauncherTest x3 as errors). That's expected — worktreeGroup
already has hand-written behavioural coverage elsewhere in the suite. It does not diminish this
test's purpose: the point of a reflective, component-enumerating test is to catch the next
component too, the one nobody happens to write a behavioural test for — which is exactly what
bit the sleep-guard branch, and exactly what six of the eleven DEFERRED_KEYS members in
ConfigRefTopLevelReportingCoverageTest's own history (fleetd #337) turned out to be.

Full mutated run: Tests run: 1374, Failures: 6, Errors: 3, Skipped: 0 / BUILD FAILURE.

Restored from the /tmp copy and confirmed byte-identical:

$ diff /tmp/FleetConfig.java.orig fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java && echo "RESTORED: byte-identical"
RESTORED: byte-identical

What the empty exclusion list means for coverage

The brief asks to also confirm the mutation does not fail for a component legitimately on the
exclusion list, and to say what it means if it doesn't. My exclusion list has no members, so there
is no such component to run that check against. What that means concretely: this test currently
checks all 22 top-level components with no exemption — the strongest form of that
guarantee, not a weaker one. If a future component genuinely needs excluding (because
withDefaults() is documented to transform it unconditionally, unlike anything today), adding it
to EXCLUDED_FROM_SURVIVAL_CHECK will require bumping exclusionListSizeIsPinned()'s pinned count
in the same change, which is exactly the visible-diff requirement the brief asks for.

Shape report (read-only — nothing below was changed)

Is FleetConfig the only record in this codebase with a back-compat constructor ladder plus a
withDefaults()-style rebuild method that calls its own constructor with a literal argument list?
No — two more instances of the same shape exist, both untouched by this PR:

  • dev.ltms.fleet.config.FleetConfig.Profile (nested record, same file). Canonical 26-arg
    constructor, 7 back-compat constructors at older arities (documented "Backward-compatible
    constructor" javadoc, same convention as the outer record), and its own rebuild method
    withProfile(String p) — a literal 26-arg new Profile(...) call. Same shape, same latent risk,
    as of today it happens to be correct (matches the 26-arg canonical), same as withDefaults() was
    correct at 22 args before the sleep-guard branch. Not fixed here — out of the stated scope
    (FleetConfig.java's outer record and withDefaults() only).
  • dev.ltms.fleet.session.MemberSession (fleetd/src/main/java/dev/ltms/fleet/session/MemberSession.java).
    Canonical 15-arg constructor, 2 back-compat constructors (12-arg "no charter receipt/agent
    session id", 14-arg "no failure reason") — and five internal rebuild ("wither") methods —
    withState, withActivity, bumpTurn, withAgentSessionId, withFailureReason — each ending
    in a literal 15-arg new MemberSession(...) call. This is the closest sibling to
    FleetConfig.withDefaults()'s shape found in the codebase: five separate call sites, not one,
    each of which would silently rebind to a new back-compat constructor and drop a 16th field the
    same way withDefaults() did. Not fixed here — out of scope.

No other record in src/main/java combines a back-compat constructor ladder (more than one
constructor beyond the canonical, added to preserve an older arity) with an internal method that
rebuilds an instance of that same record via a literal-argument constructor call. Checked every
public record declaration in src/main/java (SpawnRequest has a 2-rung back-compat ladder but
no internal rebuild call; every other record has at most one convenience constructor, not a ladder,
or no extra constructor at all).

What I verified fresh on main before implementing

canonical record arity: 22 (bind, herdrSocket, memberHerdrSocket, profiles, guard, worktreeRoot,
lifecycle, spawnReadyTimeoutMs, spawnReadyPollMs, broker, primary, fleet, leadHeartbeat, health,
placement, auth, configReload, quarantineCooldownSeconds, memberCredentials, coordinator,
worktreeGroup, memberLoginShell)
back-compat constructors: 21, 20, 18, 17, 16, 15, 14 args
withDefaults()'s return statement: 22-arg call, all 22 components passed — correct today

Matches what the lead independently measured and reported in the brief.

Build

cd fleetd && /Users/dai.ha/Softwares/apache-maven/bin/mvn clean install

Unpiped. Final result:

Tests run: 1374, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

(1372 on main + this PR's 2 new tests — exclusionListSizeIsPinned and
everyComponentGivenARealValueSurvivesWithDefaults — = 1374.)

Rules honored

  • No git add -A — staged only the one new test file explicitly.
  • No git stash used.
  • .mcp.json, opencode.json, .autoenv, wiki/ untouched.
  • No environment variable value printed (existence-checked only, ${VAR:+yes}).
  • Did not merge, did not restart the daemon.
  • Branched from origin/main (b6b88c5), not from the parked worker/sleepguard-82076d-1 branch.
  • Did not touch branch worker/sleepguard-82076d-1 or PR #355.
  • No back-compat constructor deleted or modified — git diff main -- fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java is empty; the only change in this PR is the new test file.
## Why PR #355 (parked, not merged, per lead's instruction — the fleet is moving off macOS so the idle-sleep workaround it built is no longer the direction) surfaced a live bug while I was adding a component to `FleetConfig`: `withDefaults()`'s final `return new FleetConfig(...)` call was still written at the pre-addition arg count, so it silently bound to the freshly-added back-compat constructor at that arity instead of the new canonical constructor — the new field came back `null` from every `load()`. My own new tests for that field caught it; nothing else in the suite did. The lead re-measured this independently on `main` and named the mechanism a **"defect factory"**: the exact pattern this file uses to keep old callers compiling — add a component (record grows by one arg) and add a back-compat constructor at the OLD arity — also lets that new back-compat constructor swallow `withDefaults()`'s own literal-arity call, because that call is now a legal overload match too. It compiles. Every other test passes, because nothing else exercises the new field. The new component is defaulted away, silently, on every load. This has fired once already (the sleep-guard branch) and will fire again on the next config key — which matters more right now because a new Linux host is about to get a hand-written `fleetd.yaml`. **Goal**: make it impossible for a component to be added to `FleetConfig` and silently not survive `withDefaults()`. **Scope**: `fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java` and its tests only. This PR does **not** add the `idleSleepGuard` field — that belongs to the parked PR #355 and does not ride along here. This unit is built and tested against `main` as it stands today: 22 top-level components, back-compat constructors at 21/20/18/17/16/15/14 (re-verified fresh on this branch — see "What I verified" below). ## Mechanism chosen, and why **A reflective test**, not a compile-time fix. New file: `fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigWithDefaultsPreservesEveryComponentTest.java`. It mirrors the exact reflective-construction pattern this file's ecosystem already uses in `ConfigRefTopLevelReportingCoverageTest` (`FleetConfig.class.getRecordComponents()`, then `getDeclaredConstructor(exact component types)` to resolve the TRUE canonical constructor — the same way Jackson resolves it, never by argument count): 1. Build one `FleetConfig` through that true canonical constructor, giving every one of the 22 top-level components a real, distinctive, non-null value (non-blank for `placement`, the one String whose blankness has meaning). 2. Call the real `withDefaults()`. 3. Assert every component's value survives unchanged. Why this is a valid check for every component: `withDefaults()`'s own comments document that it only ever *replaces* a component when the incoming value is `null` (or blank, for `placement`) — `broker`/`primary`/`leadHeartbeat`/`configReload`/`coordinator`/`worktreeGroup`/`memberLoginShell` are passed through unconditionally, and `bind`/`guard`/`lifecycle`/`auth`/`fleet`/ `quarantineCooldownSeconds`/`memberCredentials`/`placement` are replaced only on null/blank input. A value that's never null or blank going in must never change coming out — for every current component, with no exceptions. Why not the compile-time route the brief also offered as a candidate: stopping `withDefaults()` from binding to a back-compat overload at all, without deleting any back-compat constructor, would need either a code-generation/annotation-processor step (out of proportion to one file) or routing the call through some indirection that still has to be updated by hand every time a component is added — which is exactly the same "a human has to remember" failure this bug already demonstrated once. The reflective test instead makes the omission fail *loudly*, which is what constraint 2 asks for, and it does so using a pattern this codebase has already reviewed and trusted once. Never hardcodes the arity: the test enumerates `COMPONENTS.length` at runtime, so it keeps working however many components the record grows to (constraint 3). No back-compat constructor is touched, deleted, or restructured (constraint 1). ## Exclusion list `EXCLUDED_FROM_SURVIVAL_CHECK` is declared, and **empty**. Given every component a real, non-null (non-blank where relevant) value, all 22 current components are documented to survive `withDefaults()` unchanged, so none needs excluding today. It's still declared, and its size is pinned by its own test — `exclusionListSizeIsPinned()` asserts `EXCLUDED_FROM_SURVIVAL_CHECK.size() == 0` — so a future component that `withDefaults()` is *documented* to transform unconditionally (unlike any field today) has one obvious, justified place to go, and growing that set to make a failure go away is a visible diff to a pinned assertion, not a silent one. This is the "print the denominator" and "pin the escape hatch" requirement from the brief, applied to a currently-empty case. The coverage line the test itself prints, from a clean run: ``` FleetConfig.withDefaults() component-survival coverage — 22 components, 22 checked, 0 excluded, 22 survived ``` ## Mutation proof Copied `FleetConfig.java` aside with `cp` (never `git checkout --`) before mutating, and restored from that copy afterward (verified byte-identical with `diff` — see below). Mutated `withDefaults()`'s final constructor call to drop `worktreeGroup` (a real, non-excluded component — the exclusion list is empty, so there is no excluded component to test the negative case against; see "What the empty exclusion list means for coverage" below): ```diff - quarantineCooldown, mc, coordinator, worktreeGroup, memberLoginShell); + quarantineCooldown, mc, coordinator, null, memberLoginShell); ``` Ran the FULL suite unpiped (`mvn clean install`). New test failed, by name and line, naming the dropped component: ``` [ERROR] FleetConfigWithDefaultsPreservesEveryComponentTest.everyComponentGivenARealValueSurvivesWithDefaults:189 withDefaults() silently dropped these real, given components: [worktreeGroup: withDefaults() was given a real, non-null value (group-guard) for 'worktreeGroup' but returned null — a component silently dropped by withDefaults(), the shape of the defect this test exists to catch (its final "return new FleetConfig(...)" call binding to a back-compat constructor instead of the true canonical one)] ==> expected: <[]> but was: <[worktreeGroup: ...]> ``` Same mutation also failed three pre-existing tests that already happened to name `worktreeGroup` specifically (`ConfigRefTest.changingWorktreeGroupIsReportedAsDeferred`, `FleetConfigTest.absentWorktreeGroupSurvivesTheBackCompatConstructorChain`, `FleetConfigTest.worktreeGroupKeyParses`) plus three downstream tests whose behaviour depends on `worktreeGroup` reaching them (`HerdrPeerLauncherAllowListWiringTest` x2, `ClaudeCodeLauncherTest`/`OpenCodeLauncherTest` x3 as errors). That's expected — `worktreeGroup` already has hand-written behavioural coverage elsewhere in the suite. It does not diminish this test's purpose: the point of a reflective, component-enumerating test is to catch the *next* component too, the one nobody happens to write a behavioural test for — which is exactly what bit the sleep-guard branch, and exactly what six of the eleven `DEFERRED_KEYS` members in `ConfigRefTopLevelReportingCoverageTest`'s own history (fleetd #337) turned out to be. Full mutated run: `Tests run: 1374, Failures: 6, Errors: 3, Skipped: 0` / `BUILD FAILURE`. Restored from the `/tmp` copy and confirmed byte-identical: ``` $ diff /tmp/FleetConfig.java.orig fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java && echo "RESTORED: byte-identical" RESTORED: byte-identical ``` ### What the empty exclusion list means for coverage The brief asks to also confirm the mutation does **not** fail for a component legitimately on the exclusion list, and to say what it means if it doesn't. My exclusion list has no members, so there is no such component to run that check against. What that means concretely: this test currently checks **all 22** top-level components with **no** exemption — the strongest form of that guarantee, not a weaker one. If a future component genuinely needs excluding (because `withDefaults()` is documented to transform it unconditionally, unlike anything today), adding it to `EXCLUDED_FROM_SURVIVAL_CHECK` will require bumping `exclusionListSizeIsPinned()`'s pinned count in the same change, which is exactly the visible-diff requirement the brief asks for. ## Shape report (read-only — nothing below was changed) Is `FleetConfig` the only record in this codebase with a back-compat constructor ladder plus a `withDefaults()`-style rebuild method that calls its own constructor with a literal argument list? No — two more instances of the same shape exist, both untouched by this PR: - **`dev.ltms.fleet.config.FleetConfig.Profile`** (nested record, same file). Canonical 26-arg constructor, 7 back-compat constructors at older arities (documented "Backward-compatible constructor" javadoc, same convention as the outer record), and its own rebuild method `withProfile(String p)` — a literal 26-arg `new Profile(...)` call. Same shape, same latent risk, as of today it happens to be correct (matches the 26-arg canonical), same as `withDefaults()` was correct at 22 args before the sleep-guard branch. Not fixed here — out of the stated scope (`FleetConfig.java`'s outer record and `withDefaults()` only). - **`dev.ltms.fleet.session.MemberSession`** (`fleetd/src/main/java/dev/ltms/fleet/session/MemberSession.java`). Canonical 15-arg constructor, 2 back-compat constructors (12-arg "no charter receipt/agent session id", 14-arg "no failure reason") — and **five** internal rebuild ("wither") methods — `withState`, `withActivity`, `bumpTurn`, `withAgentSessionId`, `withFailureReason` — each ending in a literal 15-arg `new MemberSession(...)` call. This is the closest sibling to `FleetConfig.withDefaults()`'s shape found in the codebase: five separate call sites, not one, each of which would silently rebind to a new back-compat constructor and drop a 16th field the same way `withDefaults()` did. Not fixed here — out of scope. No other record in `src/main/java` combines a back-compat constructor ladder (more than one constructor beyond the canonical, added to preserve an older arity) with an internal method that rebuilds an instance of that same record via a literal-argument constructor call. Checked every `public record` declaration in `src/main/java` (`SpawnRequest` has a 2-rung back-compat ladder but no internal rebuild call; every other record has at most one convenience constructor, not a ladder, or no extra constructor at all). ## What I verified fresh on `main` before implementing ``` canonical record arity: 22 (bind, herdrSocket, memberHerdrSocket, profiles, guard, worktreeRoot, lifecycle, spawnReadyTimeoutMs, spawnReadyPollMs, broker, primary, fleet, leadHeartbeat, health, placement, auth, configReload, quarantineCooldownSeconds, memberCredentials, coordinator, worktreeGroup, memberLoginShell) back-compat constructors: 21, 20, 18, 17, 16, 15, 14 args withDefaults()'s return statement: 22-arg call, all 22 components passed — correct today ``` Matches what the lead independently measured and reported in the brief. ## Build ``` cd fleetd && /Users/dai.ha/Softwares/apache-maven/bin/mvn clean install ``` Unpiped. Final result: ``` Tests run: 1374, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` (1372 on `main` + this PR's 2 new tests — `exclusionListSizeIsPinned` and `everyComponentGivenARealValueSurvivesWithDefaults` — = 1374.) ## Rules honored - No `git add -A` — staged only the one new test file explicitly. - No `git stash` used. - `.mcp.json`, `opencode.json`, `.autoenv`, `wiki/` untouched. - No environment variable value printed (existence-checked only, `${VAR:+yes}`). - Did not merge, did not restart the daemon. - Branched from `origin/main` (`b6b88c5`), not from the parked `worker/sleepguard-82076d-1` branch. - Did not touch branch `worker/sleepguard-82076d-1` or PR #355. - No back-compat constructor deleted or modified — `git diff main -- fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java` is empty; the only change in this PR is the new test file.
agent added 1 commit 2026-09-05 00:53:04 +02:00
config: guard FleetConfig.withDefaults() against silently dropping a component
CI / contract (pull_request) Successful in 1m13s
Details
CI / build (pull_request) Successful in 1m53s
Details
dbf6fef0e9 
Adding a component to FleetConfig follows an established pattern: the
record grows by one arg, and a back-compat constructor is added at the
OLD arity so existing callers keep compiling. That back-compat
constructor also silently captures withDefaults()'s own literal-arity
'return new FleetConfig(...)' call the next time this happens, since
that call is now a legal overload match too. It compiles, every other
test passes, and the new component is defaulted away on every load().
This is not hypothetical - it happened live while building the (now
parked) idle-sleep-guard PR, caught only because that branch's own new
tests asserted on the new field.

Add a reflective test that builds a FleetConfig through the true
canonical constructor (resolved by record-component types, not arg
count - the same pattern ConfigRefTopLevelReportingCoverageTest already
uses in this file) with a real, non-null value in every component, runs
the real withDefaults(), and asserts every value survives unchanged.
Never hardcodes the arity - it enumerates
FleetConfig.class.getRecordComponents() - so it keeps working as the
record grows. No back-compat constructor is touched or removed.
ltms closed this pull request 2026-09-05 00:57:31 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m13s
Details
CI / build (pull_request) Successful in 1m53s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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#357
Delete Branch "worker/withdefaults-guard-561704"

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?