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

The reload classifier says "compares every component the launcher reads at spawn" and misses four keys, so those reloads report success and do nothing #323

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

Found by a delegated hunter over config/ and rest/. I confirmed both instances by reading the code myself, and I corrected one overstated consequence.

What is wrong

ConfigRef classifies a reload into hot, deferred and cold keys. A deferred key is reported to the operator so they know a restart is needed. ConfigRef.sameLaunchSettings decides that for a profile, and its javadoc claims:

Compares every component the launcher reads at spawn

It does not. A key that is baked in at startup but missing from that list produces the outcome the surrounding code already names as the worst one:

// ConfigRef.java:249-252 (existing comment)
// ... the worst outcome a reload can produce, because the operator has no reason to doubt it.

Outcome.applied is true, deferred is empty, the summary is a bare config reloaded — and the running daemon keeps the old value.

Instance 1 — three profile keys, ConfigRef.java:267-302

sameLaunchSettings compares baseUrl, model, configDir, tokenEnv, argv, placement, workspace, tabLabel, mcpUrl, ideMcpUrl, cwd, parityOverlay, gitTokenEnv, gitHostEnv, kind, env, subscription, exhaustedPattern, errorPattern.

It never compares ideProjectDir, ideOpenCommand or autoCompactWindow.

I checked that all three really are read at spawn off a Profile that is frozen at daemon startup:

ClaudeCodeLauncher.java:267   PeerLauncher.ideProjectPath(spec.cwd(), cfg.ideProjectDir())
ClaudeCodeLauncher.java:269   PeerLauncher.openInIde(projectPath, cfg.ideOpenCommand(), log)
ClaudeCodeLauncher.java:926   if (cfg.autoCompactWindow() == null)
OpenCodeLauncher.java:474/480/486/650  the same three

And that cfg cannot be live. HerdrPeerLauncher holds both a frozen map and a live supplier, so this needed checking rather than assuming:

HerdrPeerLauncher.java:282   this.profiles = Map.copyOf(profiles);   // frozen at construction
HerdrPeerLauncher.java:453   FleetConfig.Profile cfg = profiles.get(name);   // requireProfile reads the frozen map

The live Supplier<FleetConfig> config is used at lines 1454, 1488, 1505, 1530 and 1716 — environment allow-lists and broker env names. It never supplies the Profile. So the spawn path is frozen and the finding holds.

Instance 2 — worktreeGroup, ConfigRef.java:210-265

GitWorktrees is built once, from the startup config:

Fleetd.java:251   new GitWorktrees(cfg.worktreeRoot(), cfg.worktreeGroup())
GitWorktrees.java:94    private final String group;
GitWorktrees.java:158   this.group = (group == null || group.isBlank()) ? null : group;

changedDeferredKeys checks the sibling key and not this one:

ConfigRef.java:221-222   if (!Objects.equals(old.worktreeRoot(), fresh.worktreeRoot())) { changed.add("worktreeRoot"); }

worktreeGroup appears nowhere in ConfigRef.java — not in the check, and not in the class doc's hot/deferred/cold lists either, though worktreeRoot is listed as deferred.

Correcting the hunter on the consequence

The hunter wrote that an operator turning worktreeGroup off is "revoking a different-OS-user member's write access" and silently failing to. That overstates it, and I want the overstatement out of the record before someone acts on it.

shareWithGroup runs chgrp/chmod on each worktree when that worktree is provisioned. Those permissions are already on disk. Turning the key off does not take them away from existing worktrees even after a full restart — it only stops new worktrees being shared. So config alone was never a revocation mechanism, and this defect does not break one.

The real harm is narrower and still worth fixing: newly provisioned worktrees keep the old sharing behaviour, and the operator is told the change is live. Both directions are silent.

The shape

This is the deferred-key list as a hand-maintained second copy of "what the launcher reads at spawn". It drifts, exactly the way the tool catalogue in docs/MCP-Contract.md drifted for a month (CB-609 / #114). Adding four fields fixes today's four and leaves the mechanism that produced them.

What I want

Goal: it must not be possible to add a field that is baked in at startup and have the reload classifier silently ignore it.

Invariants:

  1. A key that is genuinely read live must stay classified as hot. weight, maxLoad and credentialId are excluded on purpose and the javadoc says why. Do not turn those into deferred keys.
  2. Do not change what a reload actually applies. This ticket is about reporting the truth, not about making more keys hot. Making autoCompactWindow take effect live is a different, larger change and is not wanted here.
  3. The existing deferred reports must keep working. ConfigRefTest already pins model, exhaustedPattern and errorPattern as deferred. Those stay green.

Candidate mechanism, as a candidate only: add a test that enumerates every record component of FleetConfig.Profile by reflection and asserts each one is either compared by sameLaunchSettings or named on an explicit, commented exclusion list. A new field then fails the build until someone classifies it. Decide it yourself and justify it. If reflection over a record is awkward here, say so and propose what you would do instead — a checker that cannot enumerate its own denominator is the thing we are trying to stop building.

Whatever you choose, the check must print or assert its denominator: how many components exist, how many are compared, how many are excluded. A checker that silently checks nothing is the failure mode this repo keeps hitting.

Then fix the four instances: the three profile keys, and worktreeGroup in changedDeferredKeys plus the class doc's deferred list.

Two things to weigh, and I do not know the answers:

  • Is worktreeGroup deferred or cold? worktreeRoot is listed as deferred. GitWorktrees is never rebuilt, so a change needs a restart either way. Say which list it belongs on and why.
  • Does the same drift exist for the non-profile keys? changedDeferredKeys compares a hand-written set of top-level FleetConfig fields too. Check whether that list has the same gap, and report what you find. Fix it only if it is the same one-line shape; if it is bigger, report it and leave it.

Rules

  • Prove it with tests that fail without the fix: a reload changing only ideProjectDir (and one for worktreeGroup) must report the key as deferred.
  • Mutation proof required: revert the fix, quote the real failure output, restore it.
  • The javadoc on sameLaunchSettings currently states an invariant the code does not keep. Correct it, and make it say what the new mechanism guarantees rather than repeating a claim nobody can check.
  • fleetd/fleetd.yaml is gitignored and absent from your worktree. Do not report on its contents.
  • Never git stash — the stash is shared across every worktree here.
  • Never run git worktree remove or git worktree prune — other workers are live in those directories.
  • Stage files explicitly; never git add -A. Never merge.
  • Run cd fleetd && mvn clean install unpiped; quote the real Tests run: and BUILD lines. Never read $? after a pipe.
  • Put your full report in the PR body as well as in your fleet_reply.

Also noted, not part of this ticket

The hunter flagged that FleetConfig's coordinator: javadoc (around line 81) says "nothing here wires it into a live LeadMailbox; that is a separate ticket", while Fleetd.java:502 calls openLeadMailbox(cfg.coordinator(), ...) at startup. That looks like another stale comment. I have not verified it. Do not fix it here.

Found by a delegated hunter over `config/` and `rest/`. I confirmed both instances by reading the code myself, and I corrected one overstated consequence. ## What is wrong `ConfigRef` classifies a reload into hot, deferred and cold keys. A **deferred** key is reported to the operator so they know a restart is needed. `ConfigRef.sameLaunchSettings` decides that for a profile, and its javadoc claims: > Compares **every** component the launcher reads at spawn It does not. A key that is baked in at startup but missing from that list produces the outcome the surrounding code already names as the worst one: ```java // ConfigRef.java:249-252 (existing comment) // ... the worst outcome a reload can produce, because the operator has no reason to doubt it. ``` `Outcome.applied` is `true`, `deferred` is empty, the summary is a bare `config reloaded` — and the running daemon keeps the old value. ## Instance 1 — three profile keys, `ConfigRef.java:267-302` `sameLaunchSettings` compares `baseUrl`, `model`, `configDir`, `tokenEnv`, `argv`, `placement`, `workspace`, `tabLabel`, `mcpUrl`, `ideMcpUrl`, `cwd`, `parityOverlay`, `gitTokenEnv`, `gitHostEnv`, `kind`, `env`, `subscription`, `exhaustedPattern`, `errorPattern`. It never compares **`ideProjectDir`**, **`ideOpenCommand`** or **`autoCompactWindow`**. I checked that all three really are read at spawn off a `Profile` that is frozen at daemon startup: ``` ClaudeCodeLauncher.java:267 PeerLauncher.ideProjectPath(spec.cwd(), cfg.ideProjectDir()) ClaudeCodeLauncher.java:269 PeerLauncher.openInIde(projectPath, cfg.ideOpenCommand(), log) ClaudeCodeLauncher.java:926 if (cfg.autoCompactWindow() == null) OpenCodeLauncher.java:474/480/486/650 the same three ``` And that `cfg` cannot be live. `HerdrPeerLauncher` holds **both** a frozen map and a live supplier, so this needed checking rather than assuming: ```java HerdrPeerLauncher.java:282 this.profiles = Map.copyOf(profiles); // frozen at construction HerdrPeerLauncher.java:453 FleetConfig.Profile cfg = profiles.get(name); // requireProfile reads the frozen map ``` The live `Supplier<FleetConfig> config` is used at lines 1454, 1488, 1505, 1530 and 1716 — environment allow-lists and broker env names. It never supplies the `Profile`. So the spawn path is frozen and the finding holds. ## Instance 2 — `worktreeGroup`, `ConfigRef.java:210-265` `GitWorktrees` is built once, from the startup config: ```java Fleetd.java:251 new GitWorktrees(cfg.worktreeRoot(), cfg.worktreeGroup()) GitWorktrees.java:94 private final String group; GitWorktrees.java:158 this.group = (group == null || group.isBlank()) ? null : group; ``` `changedDeferredKeys` checks the sibling key and not this one: ```java ConfigRef.java:221-222 if (!Objects.equals(old.worktreeRoot(), fresh.worktreeRoot())) { changed.add("worktreeRoot"); } ``` `worktreeGroup` appears nowhere in `ConfigRef.java` — not in the check, and not in the class doc's hot/deferred/cold lists either, though `worktreeRoot` is listed as deferred. ## Correcting the hunter on the consequence The hunter wrote that an operator turning `worktreeGroup` off is "revoking a different-OS-user member's write access" and silently failing to. **That overstates it, and I want the overstatement out of the record before someone acts on it.** `shareWithGroup` runs `chgrp`/`chmod` on each worktree when that worktree is provisioned. Those permissions are already on disk. Turning the key off does not take them away from existing worktrees **even after a full restart** — it only stops *new* worktrees being shared. So config alone was never a revocation mechanism, and this defect does not break one. The real harm is narrower and still worth fixing: newly provisioned worktrees keep the old sharing behaviour, and the operator is told the change is live. Both directions are silent. ## The shape This is the deferred-key list as a hand-maintained second copy of "what the launcher reads at spawn". It drifts, exactly the way the tool catalogue in `docs/MCP-Contract.md` drifted for a month (CB-609 / #114). Adding four fields fixes today's four and leaves the mechanism that produced them. ## What I want **Goal:** it must not be possible to add a field that is baked in at startup and have the reload classifier silently ignore it. **Invariants:** 1. **A key that is genuinely read live must stay classified as hot.** `weight`, `maxLoad` and `credentialId` are excluded on purpose and the javadoc says why. Do not turn those into deferred keys. 2. **Do not change what a reload actually applies.** This ticket is about *reporting* the truth, not about making more keys hot. Making `autoCompactWindow` take effect live is a different, larger change and is not wanted here. 3. **The existing deferred reports must keep working.** `ConfigRefTest` already pins `model`, `exhaustedPattern` and `errorPattern` as deferred. Those stay green. **Candidate mechanism, as a candidate only:** add a test that enumerates every record component of `FleetConfig.Profile` by reflection and asserts each one is either compared by `sameLaunchSettings` or named on an explicit, commented exclusion list. A new field then fails the build until someone classifies it. **Decide it yourself and justify it.** If reflection over a record is awkward here, say so and propose what you would do instead — a checker that cannot enumerate its own denominator is the thing we are trying to stop building. Whatever you choose, **the check must print or assert its denominator**: how many components exist, how many are compared, how many are excluded. A checker that silently checks nothing is the failure mode this repo keeps hitting. Then fix the four instances: the three profile keys, and `worktreeGroup` in `changedDeferredKeys` plus the class doc's deferred list. Two things to weigh, and I do not know the answers: - **Is `worktreeGroup` deferred or cold?** `worktreeRoot` is listed as deferred. `GitWorktrees` is never rebuilt, so a change needs a restart either way. Say which list it belongs on and why. - **Does the same drift exist for the non-profile keys?** `changedDeferredKeys` compares a hand-written set of top-level `FleetConfig` fields too. Check whether that list has the same gap, and report what you find. Fix it only if it is the same one-line shape; if it is bigger, report it and leave it. ## Rules - Prove it with tests that fail without the fix: a reload changing only `ideProjectDir` (and one for `worktreeGroup`) must report the key as deferred. - Mutation proof required: revert the fix, quote the real failure output, restore it. - The javadoc on `sameLaunchSettings` currently states an invariant the code does not keep. Correct it, and make it say what the new mechanism guarantees rather than repeating a claim nobody can check. - `fleetd/fleetd.yaml` is gitignored and absent from your worktree. Do not report on its contents. - Never `git stash` — the stash is shared across every worktree here. - Never run `git worktree remove` or `git worktree prune` — other workers are live in those directories. - Stage files explicitly; never `git add -A`. Never merge. - Run `cd fleetd && mvn clean install` **unpiped**; quote the real `Tests run:` and `BUILD` lines. Never read `$?` after a pipe. - Put your full report in the PR body as well as in your `fleet_reply`. ## Also noted, not part of this ticket The hunter flagged that `FleetConfig`'s `coordinator:` javadoc (around line 81) says "nothing here wires it into a live `LeadMailbox`; that is a separate ticket", while `Fleetd.java:502` calls `openLeadMailbox(cfg.coordinator(), ...)` at startup. That looks like another stale comment. I have not verified it. Do not fix it here.
ltms commented 2026-09-04 09:32:40 +02:00
Author
Owner
Copy Link
Copy Source

Merged as the merge commit above, with one follow-up commit e545c08. Pushed to main.

Build on main: Tests run: 1339, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS, unpiped. The coverage test prints its denominator on every run:

ConfigRef.sameLaunchSettings coverage — 26 Profile components total, 23 compared, 3 excluded ([weight, maxLoad, credentialId])

The mechanism found a fifth gap by itself

The profile component — a profile's own identity string — was neither compared nor excluded. Nobody had noticed. That is the mechanism doing exactly what it was built for on its first run, and it is the best argument for building it rather than adding four fields.

The answers are right

worktreeGroup is deferred, not cold. The reasoning holds: cold is for a key whose new value would leave the daemon inconsistent with an already-open resource — a bound socket, an open broker, decided auth. worktreeGroup has none of that. It lands safely in the snapshot and simply does not re-chgrp worktrees that already exist. Its sibling worktreeRoot, baked into the same constructor call, is already deferred.

The top-level drift is real and correctly scoped out. health and coordinator are the interesting ones and they are worse than a missing check: the same key is read from the startup snapshot in one place and off config.get() in another. health at Fleetd.java:556-563 versus Fleetd.java:648; coordinator at Fleetd.java:502 versus HerdrPeerLauncher.java:1530. A key that is half hot and half deferred cannot be classified correctly by any list, so it needs a decision rather than a patch. The worker was right to report and not touch it. I will file that separately.

My own mutation, and what it found

The worker reverted the four field comparisons. I went after the mechanism instead, because a checker is only as good as the thing that stops it being switched off.

N — excuse ideProjectDir into the exclusion set and drop its comparison. This is the lazy move a failing coverage test invites. Caught, but not by the coverage test:

ConfigRefTest.changingAProfilesIdeProjectDirIsReportedAsDeferred:479  [] ==> expected: <1> but was: <0>

The coverage test itself passed happily — 22 compared, 4 excluded, denominator balanced.

O — excuse autoCompactWindow and ideOpenCommand, the two of the four fields that got no behavioural test.

ConfigRef.sameLaunchSettings coverage — 26 Profile components total, 21 compared, 5 excluded
Tests run: 18, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Green. Two of the four fields this ticket fixed could be silently re-broken, and the checker printed the change while asserting nothing about it.

So the mechanism caught a new field and not an excused existing one. That is the same one-way gate shape this ticket was raised about, reproduced inside the gate built to stop it. It is not a criticism of the design — the escape hatch has to exist, because weight, maxLoad and credentialId genuinely belong in it. It just was not pinned.

Fixed in e545c08: the coverage test now asserts LAUNCH_SETTINGS_EXCLUDED equals exactly {weight, maxLoad, credentialId}. Growing it now means editing that assertion too — a visible, deliberate diff instead of a quiet one. Re-running mutation O:

ConfigRefProfileCoverageTest.sameLaunchSettingsComparesEveryProfileComponentOrExcludesIt:164
  ConfigRef.LAUNCH_SETTINGS_EXCLUDED changed. A component belongs in it ONLY if it is read live off
  the config supplier, not baked into a launcher at startup. If you are adding one to silence this
  test, that is fleetd #323 happening again ...
  ==> expected: <[weight, credentialId, maxLoad]> but was: <[ideOpenCommand, weight, maxLoad, autoCompactWindow, credentialId]>

The general rule, for the next one of these. When a ticket asks for a checker instead of a fix, the checker needs its own mutation pass, and the question is always the same: what is the cheapest way to make this checker pass without doing the work? Here it was one line in a Set.of.

Also good

The worker caught itself mid-mutation: it noted that git checkout -- would have reverted to the pre-fix HEAD rather than restoring an uncommitted fix, and used a saved patch instead. Saying that out loud is worth more than a clean report that hides it.

Closing.

Merged as the merge commit above, with one follow-up commit `e545c08`. Pushed to `main`. Build on `main`: `Tests run: 1339, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`, unpiped. The coverage test prints its denominator on every run: ``` ConfigRef.sameLaunchSettings coverage — 26 Profile components total, 23 compared, 3 excluded ([weight, maxLoad, credentialId]) ``` ## The mechanism found a fifth gap by itself The `profile` component — a profile's own identity string — was neither compared nor excluded. Nobody had noticed. That is the mechanism doing exactly what it was built for on its first run, and it is the best argument for building it rather than adding four fields. ## The answers are right **`worktreeGroup` is deferred, not cold.** The reasoning holds: cold is for a key whose new value would leave the daemon inconsistent with an already-open resource — a bound socket, an open broker, decided auth. `worktreeGroup` has none of that. It lands safely in the snapshot and simply does not re-`chgrp` worktrees that already exist. Its sibling `worktreeRoot`, baked into the same constructor call, is already deferred. **The top-level drift is real and correctly scoped out.** `health` and `coordinator` are the interesting ones and they are worse than a missing check: the same key is read from the startup snapshot in one place and off `config.get()` in another. `health` at `Fleetd.java:556-563` versus `Fleetd.java:648`; `coordinator` at `Fleetd.java:502` versus `HerdrPeerLauncher.java:1530`. A key that is half hot and half deferred cannot be classified correctly by any list, so it needs a decision rather than a patch. The worker was right to report and not touch it. I will file that separately. ## My own mutation, and what it found The worker reverted the four field comparisons. I went after the mechanism instead, because a checker is only as good as the thing that stops it being switched off. **N — excuse `ideProjectDir` into the exclusion set and drop its comparison.** This is the lazy move a failing coverage test invites. Caught, but not by the coverage test: ``` ConfigRefTest.changingAProfilesIdeProjectDirIsReportedAsDeferred:479 [] ==> expected: <1> but was: <0> ``` The coverage test itself passed happily — `22 compared, 4 excluded`, denominator balanced. **O — excuse `autoCompactWindow` and `ideOpenCommand`, the two of the four fields that got no behavioural test.** ``` ConfigRef.sameLaunchSettings coverage — 26 Profile components total, 21 compared, 5 excluded Tests run: 18, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` **Green.** Two of the four fields this ticket fixed could be silently re-broken, and the checker printed the change while asserting nothing about it. So the mechanism caught a **new** field and not an **excused existing** one. That is the same one-way gate shape this ticket was raised about, reproduced inside the gate built to stop it. It is not a criticism of the design — the escape hatch has to exist, because `weight`, `maxLoad` and `credentialId` genuinely belong in it. It just was not pinned. **Fixed in `e545c08`:** the coverage test now asserts `LAUNCH_SETTINGS_EXCLUDED` equals exactly `{weight, maxLoad, credentialId}`. Growing it now means editing that assertion too — a visible, deliberate diff instead of a quiet one. Re-running mutation O: ``` ConfigRefProfileCoverageTest.sameLaunchSettingsComparesEveryProfileComponentOrExcludesIt:164 ConfigRef.LAUNCH_SETTINGS_EXCLUDED changed. A component belongs in it ONLY if it is read live off the config supplier, not baked into a launcher at startup. If you are adding one to silence this test, that is fleetd #323 happening again ... ==> expected: <[weight, credentialId, maxLoad]> but was: <[ideOpenCommand, weight, maxLoad, autoCompactWindow, credentialId]> ``` **The general rule, for the next one of these.** When a ticket asks for a checker instead of a fix, the checker needs its own mutation pass, and the question is always the same: what is the cheapest way to make this checker pass without doing the work? Here it was one line in a `Set.of`. ## Also good The worker caught itself mid-mutation: it noted that `git checkout --` would have reverted to the pre-fix `HEAD` rather than restoring an uncommitted fix, and used a saved patch instead. Saying that out loud is worth more than a clean report that hides it. Closing.
ltms closed this issue 2026-09-04 09:32:47 +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#323
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?