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

A charter naming an unregistered tool is refused at startup but accepted on reload #474

New Issue
Closed
opened 2026-09-10 15:11:17 +02:00 by ltms · 2 comments
ltms commented 2026-09-10 15:11:17 +02:00
Owner
Copy Link
Copy Source

#469 made the daemon refuse to start when a launch charter names an MCP tool the server does not register. That gate covers the startup direction only. The reload direction is open, so a config that cannot boot can still be installed into a running daemon.

The reachable path

  1. The daemon is running with a valid config.
  2. The operator edits fleetd.yaml and puts bridge_send — the pre-CB-634 name, removed from the tool surface — into fleet.charters.dev.
  3. ConfigWatcher fires, ConfigRef.reload() runs fresh.validateAll(). That passes: validateCharters() only checks that the key is a role wire name and the text is not blank. It never looks at what the text names.
  4. The reload is applied.
  5. The next spawned dev member gets a charter telling it to call a tool that does not exist.

That is exactly the symptom #469 was filed for, arriving through the one door #469 does not cover.

Measured, not assumed

$ grep -rn 'assertChartersNameOnlyRegisteredTools' fleetd/src/main/java
Fleetd.java:176:        CharterToolSurface.assertChartersNameOnlyRegisteredTools(
mcp/CharterToolSurface.java:43:    public static void assertChartersNameOnlyRegisteredTools(...)

One call site, in Fleetd.main. ConfigRef.java:362 calls fresh.validateAll(), which does not include it.

Charters really are hot and really are read per spawn:

  • HerdrPeerLauncher.java:499 — String roleCharter = liveFleet == null ? null : liveFleet.charterFor(role);
  • FleetConfig.java:1221 — "the launcher reads fleet.charters() from the live config"
  • ConfigRef.java:549 names charters as hot, pinned by ConfigRefTest.aHotChangeIsAppliedAndReadThroughGet

So the bad charter reaches a member without a restart. Measured on main at 1477e43.

Why this is worth its own ticket

ConfigRef.java already states the rule this breaks, in the comment directly above the validateAll() call it makes:

The same gate startup runs. A config that would have refused to boot must not be able to slip in through a reload — that is how a daemon ends up in a state it could never have started in, which is the hardest kind to debug.

And the Features page says the same thing about this specific validation: "Validation runs on both the startup path and the reload path. Wiring only one of the two is the whole bug." Right now that sentence is true of validateCharters() and false of the tool-name check beside it.

This is also the shape of #149/#258: a gate added after an incident closes only the direction the incident came from. The incident was a charter found at startup. Nobody asked which other states open the same door.

Scope

Make the reload path run the same check as startup, so both directions refuse the same config.

The design constraint that pushed this check out of FleetConfig still applies and is not up for revisiting: CharterToolSurface lives in the mcp package because the canonical tool set does, and config is loaded before the MCP server exists, so FleetConfig must not depend on mcp. Whatever the fix is, it must not put that dependency back. The seam to look at is where ConfigRef.reload() runs its gate, not validateAll() itself.

Acceptance criteria

  1. A reload whose charter names an unregistered tool is refused, the running config is kept, and the warning names both the charter key and the unknown tool — the same information the startup failure gives.
  2. A test that drives the reload path (not a unit call to the check) and proves the refusal. ConfigRefTest is where the hot/cold reload behaviour is already pinned.
  3. A test proving a reload with a valid charter is still accepted — the positive case. An inverted filter passes the negative test alone; #469's own M5 cell showed that.
  4. Deleting the new call site from the reload path must fail a test by name. This is the cell that survived three times recently (#446, #466) and was only closed in #469 by a dedicated test. Do not leave the new call site unpinned.
  5. FleetConfig still has no dependency on the mcp package. State how you checked.
  6. Report the full-suite count before and after, unpiped, with the real exit code.

Not in scope

Changing what counts as a tool-shaped token in charter text, or adding config to turn the check off.

#469 made the daemon refuse to start when a launch charter names an MCP tool the server does not register. That gate covers the startup direction only. The reload direction is open, so a config that cannot boot can still be installed into a running daemon. ## The reachable path 1. The daemon is running with a valid config. 2. The operator edits `fleetd.yaml` and puts `bridge_send` — the pre-CB-634 name, removed from the tool surface — into `fleet.charters.dev`. 3. `ConfigWatcher` fires, `ConfigRef.reload()` runs `fresh.validateAll()`. That passes: `validateCharters()` only checks that the key is a role wire name and the text is not blank. It never looks at what the text names. 4. The reload is applied. 5. The next spawned `dev` member gets a charter telling it to call a tool that does not exist. That is exactly the symptom #469 was filed for, arriving through the one door #469 does not cover. ## Measured, not assumed ``` $ grep -rn 'assertChartersNameOnlyRegisteredTools' fleetd/src/main/java Fleetd.java:176: CharterToolSurface.assertChartersNameOnlyRegisteredTools( mcp/CharterToolSurface.java:43: public static void assertChartersNameOnlyRegisteredTools(...) ``` One call site, in `Fleetd.main`. `ConfigRef.java:362` calls `fresh.validateAll()`, which does not include it. Charters really are hot and really are read per spawn: - `HerdrPeerLauncher.java:499` — `String roleCharter = liveFleet == null ? null : liveFleet.charterFor(role);` - `FleetConfig.java:1221` — "the launcher reads `fleet.charters()` from the live config" - `ConfigRef.java:549` names `charters` as hot, pinned by `ConfigRefTest.aHotChangeIsAppliedAndReadThroughGet` So the bad charter reaches a member without a restart. Measured on `main` at `1477e43`. ## Why this is worth its own ticket `ConfigRef.java` already states the rule this breaks, in the comment directly above the `validateAll()` call it makes: > The same gate startup runs. A config that would have refused to boot must not be able to slip in through a reload — that is how a daemon ends up in a state it could never have started in, which is the hardest kind to debug. And the Features page says the same thing about this specific validation: *"Validation runs on both the startup path and the reload path. Wiring only one of the two is the whole bug."* Right now that sentence is true of `validateCharters()` and false of the tool-name check beside it. This is also the shape of #149/#258: a gate added after an incident closes only the direction the incident came from. The incident was a charter found at startup. Nobody asked which other states open the same door. ## Scope Make the reload path run the same check as startup, so both directions refuse the same config. The design constraint that pushed this check out of `FleetConfig` still applies and is not up for revisiting: `CharterToolSurface` lives in the `mcp` package because the canonical tool set does, and config is loaded before the MCP server exists, so `FleetConfig` must not depend on `mcp`. Whatever the fix is, it must not put that dependency back. The seam to look at is where `ConfigRef.reload()` runs its gate, not `validateAll()` itself. ## Acceptance criteria 1. A reload whose charter names an unregistered tool is **refused**, the running config is kept, and the warning names both the charter key and the unknown tool — the same information the startup failure gives. 2. A test that drives the **reload path** (not a unit call to the check) and proves the refusal. `ConfigRefTest` is where the hot/cold reload behaviour is already pinned. 3. A test proving a reload with a valid charter is still **accepted** — the positive case. An inverted filter passes the negative test alone; #469's own M5 cell showed that. 4. Deleting the new call site from the reload path must fail a test **by name**. This is the cell that survived three times recently (#446, #466) and was only closed in #469 by a dedicated test. Do not leave the new call site unpinned. 5. `FleetConfig` still has no dependency on the `mcp` package. State how you checked. 6. Report the full-suite count before and after, unpiped, with the real exit code. ## Not in scope Changing what counts as a tool-shaped token in charter text, or adding config to turn the check off.
ltms commented 2026-09-10 15:41:51 +02:00
Author
Owner
Copy Link
Copy Source

The fix is on main as 4466ee0. ConfigRef.reload() now runs the same charter tool-surface gate Fleetd.main runs at startup, so a charter naming an unregistered tool can no longer be installed into a running daemon. 1633 tests green on that tree.

FleetConfig still has no dependency on mcp — 0 files under dev/ltms/fleet/config/ import it. The design constraint I marked as not-to-revisit was respected: ConfigRef takes the check as a Consumer<FleetConfig>, and Fleetd is the seam that already holds both a loaded config and the mcp package.

Keeping this ticket open for one thing. My mutation battery found that nothing pins the call site. Reverting Fleetd.java:154 from

new ConfigRef(configPath, cfg, Fleetd::assertChartersNameOnlyRegisteredTools)

to the two-argument new ConfigRef(configPath, cfg) turns the live gate off and leaves all 1633 tests green. The reason is subtle and worth writing down: both new tests build their own ConfigRef with the method reference, so they prove the combination works and say nothing about what main chose.

Four other cells all died — deleting the accept(fresh) call, making the constructor store a no-op, swapping the config in before validating, and moving the call outside reload()'s try/catch. So the behaviour is well pinned. Only the daemon's choice to wire it is not.

This is the fourth Fleetd.main call site to survive a battery (#446 M5 and M7, #466 M1, now this). The pattern is now clear enough to state as a rule: extracting a check into a well-tested helper moves the untested surface up, into the one line that chooses to call it. Every extraction needs a cell on the call site as well as one on the value.

The repo already holds the answer in three source-text wiring tests — FleetdBackendQuarantineWiringTest, FleetdLeadSeatWiringTest, FleetdCompletionResolverWiringTest — each reading Fleetd.java and asserting main still contains the exact call. There is a second, weaker sibling pattern that builds the wiring itself; that is the one this ticket's work followed. Delegated as a follow-up with the surviving mutation as its acceptance criterion. I will close this ticket when that test kills M2.

The fix is on `main` as `4466ee0`. `ConfigRef.reload()` now runs the same charter tool-surface gate `Fleetd.main` runs at startup, so a charter naming an unregistered tool can no longer be installed into a running daemon. 1633 tests green on that tree. `FleetConfig` still has no dependency on `mcp` — 0 files under `dev/ltms/fleet/config/` import it. The design constraint I marked as not-to-revisit was respected: `ConfigRef` takes the check as a `Consumer<FleetConfig>`, and `Fleetd` is the seam that already holds both a loaded config and the `mcp` package. **Keeping this ticket open for one thing.** My mutation battery found that nothing pins the call site. Reverting `Fleetd.java:154` from ```java new ConfigRef(configPath, cfg, Fleetd::assertChartersNameOnlyRegisteredTools) ``` to the two-argument `new ConfigRef(configPath, cfg)` turns the live gate off and leaves all **1633** tests green. The reason is subtle and worth writing down: both new tests build their own `ConfigRef` **with** the method reference, so they prove the combination works and say nothing about what `main` chose. Four other cells all died — deleting the `accept(fresh)` call, making the constructor store a no-op, swapping the config in before validating, and moving the call outside `reload()`'s try/catch. So the behaviour is well pinned. Only the daemon's choice to wire it is not. This is the **fourth** `Fleetd.main` call site to survive a battery (#446 M5 and M7, #466 M1, now this). The pattern is now clear enough to state as a rule: **extracting a check into a well-tested helper moves the untested surface up, into the one line that chooses to call it.** Every extraction needs a cell on the call site as well as one on the value. The repo already holds the answer in three source-text wiring tests — `FleetdBackendQuarantineWiringTest`, `FleetdLeadSeatWiringTest`, `FleetdCompletionResolverWiringTest` — each reading `Fleetd.java` and asserting `main` still contains the exact call. There is a second, weaker sibling pattern that builds the wiring itself; that is the one this ticket's work followed. Delegated as a follow-up with the surviving mutation as its acceptance criterion. I will close this ticket when that test kills M2.
ltms commented 2026-09-10 23:15:31 +02:00
Author
Owner
Copy Link
Copy Source

Closing. I said on this ticket that I would close it when a test kills M2. That test now exists and kills M2 by name.

What shipped

Two commits, both on origin/main:

  • 4466ee0 — the production fix. ConfigRef takes a Consumer<FleetConfig> extraValidation, called inside reload()'s existing try, so a refused reload still returns Outcome.failed and keeps the running config rather than throwing. Fleetd.main passes Fleetd::assertChartersNameOnlyRegisteredTools, a package-private adapter that reads cfg.fleet().charters(). The two-argument constructor stays and delegates with a no-op consumer.
  • 49a404d — the follow-up merge (PR #476), one new test file, no production change.

git rev-list --count origin/main..HEAD = 0. origin/main = 49a404d.

Why the follow-up was needed

My battery on 4466ee0 (tree fbb9b58) ran five mutations, each a full build against a 1633-test baseline:

cell mutation result
M1 delete extraValidation.accept(fresh) from reload() KILLED — 2 tests
M2 revert Fleetd.java:154 to the two-argument constructor SURVIVED — 1633 green, BUILD SUCCESS
M3 3-arg constructor ignores its argument, stores a no-op KILLED — 2 tests
M4 current.set(fresh) before validating KILLED — 6 tests
M5 move accept(fresh) outside the try/catch KILLED — 2 errors

M2 is the one that mattered. ConfigRef was correct, the adapter was correct, and both new tests kept passing, because each builds its own ConfigRef with the method reference. Only what the daemon wires changed — and with that one line reverted the live reload gate was off and this ticket was undone, with nothing red.

That is the fifth Fleetd.main call site to survive a battery in this repo (#446 M5 and M7, #466 M1, now this). The shape is settled: extracting a check into a well-tested helper moves the untested surface up, into the one line that chooses to call it. Two Fleetd*WiringTest patterns look interchangeable and are not — building the same wiring yourself proves the combination works but cannot prove main chose it; only reading Fleetd.java as text and asserting the exact call can.

The gap is closed

FleetdConfigRefWiringTest (80 lines, one file) reads Fleetd.java and asserts both halves — the 3-arg text is present, the 2-arg form is absent — behind a vacuity guard on an unrelated anchor. Re-measured on the merge (tree 701bf04):

  • CONTROL 1, unmutated: Tests run: 1634, Failures: 0, Errors: 0 — BUILD SUCCESS, rc=0.
  • M2 reproduced: rc=1, 1 failure, FleetdConfigRefWiringTest.mainStillWiresTheThreeArgumentConfigRefConstructor, on the negative assertion.
  • Anchor reflowed (same tokens, same behaviour, text moved): rc=1, fails by the same name. The anchor is real, not a scrape that found nothing.
  • Read broken (bad path): rc=1, fails by the same name — as an Error from Files.readString, not on the guard's message. So a missing file cannot pass silently, but the guard itself is still unproven against a read that succeeds and returns the wrong content. Recorded on PR #476 rather than left implied.

What I am not claiming

The new test proves the text is in main. It does not prove the call executes at startup, and it does not prove the gate works — ConfigRefTest and FleetdConfigRefCharterToolSurfaceWiringTest cover the behaviour.

Nothing here has been exercised on the live daemon. The running fleetd started 2026-09-10 16:34:27 and 48 commits on main are dated after that, this fix among them. The reload gate is live code only after a redeploy, which is owed and separate.

Wiki entry updated: 11-Features.md now says a reload is checked too, not only startup, with the reason (config loads before mcp exists) and the remaining gotcha.

Filed separately, from the same review pass: #478 (a directory in parityOverlay is reported as copied and arrives empty). fleet01's note on this ticket — that a one-call-site check in main while reload runs only validateAll is the same shape as the CI job's hardcoded -Dtest list — is the sentence worth keeping.

Closing. I said on this ticket that I would close it when a test kills M2. That test now exists and kills M2 by name. ## What shipped Two commits, both on `origin/main`: - `4466ee0` — the production fix. `ConfigRef` takes a `Consumer<FleetConfig> extraValidation`, called inside `reload()`'s existing try, so a refused reload still returns `Outcome.failed` and keeps the running config rather than throwing. `Fleetd.main` passes `Fleetd::assertChartersNameOnlyRegisteredTools`, a package-private adapter that reads `cfg.fleet().charters()`. The two-argument constructor stays and delegates with a no-op consumer. - `49a404d` — the follow-up merge (PR #476), one new test file, no production change. `git rev-list --count origin/main..HEAD` = 0. `origin/main` = `49a404d`. ## Why the follow-up was needed My battery on `4466ee0` (tree `fbb9b58`) ran five mutations, each a full build against a 1633-test baseline: | cell | mutation | result | |---|---|---| | M1 | delete `extraValidation.accept(fresh)` from `reload()` | KILLED — 2 tests | | M2 | revert `Fleetd.java:154` to the two-argument constructor | **SURVIVED — 1633 green, BUILD SUCCESS** | | M3 | 3-arg constructor ignores its argument, stores a no-op | KILLED — 2 tests | | M4 | `current.set(fresh)` before validating | KILLED — 6 tests | | M5 | move `accept(fresh)` outside the try/catch | KILLED — 2 errors | M2 is the one that mattered. `ConfigRef` was correct, the adapter was correct, and both new tests kept passing, because each builds its own `ConfigRef` with the method reference. Only what the **daemon** wires changed — and with that one line reverted the live reload gate was off and this ticket was undone, with nothing red. That is the fifth `Fleetd.main` call site to survive a battery in this repo (#446 M5 and M7, #466 M1, now this). The shape is settled: **extracting a check into a well-tested helper moves the untested surface up, into the one line that chooses to call it.** Two `Fleetd*WiringTest` patterns look interchangeable and are not — building the same wiring yourself proves the combination works but cannot prove `main` chose it; only reading `Fleetd.java` as text and asserting the exact call can. ## The gap is closed `FleetdConfigRefWiringTest` (80 lines, one file) reads `Fleetd.java` and asserts both halves — the 3-arg text is present, the 2-arg form is absent — behind a vacuity guard on an unrelated anchor. Re-measured on the merge (tree `701bf04`): - CONTROL 1, unmutated: `Tests run: 1634, Failures: 0, Errors: 0` — `BUILD SUCCESS`, rc=0. - M2 reproduced: rc=1, 1 failure, `FleetdConfigRefWiringTest.mainStillWiresTheThreeArgumentConfigRefConstructor`, on the negative assertion. - Anchor reflowed (same tokens, same behaviour, text moved): rc=1, fails by the same name. The anchor is real, not a scrape that found nothing. - Read broken (bad path): rc=1, fails by the same name — as an Error from `Files.readString`, not on the guard's message. So a missing file cannot pass silently, but the guard itself is still unproven against a read that *succeeds* and returns the wrong content. Recorded on PR #476 rather than left implied. ## What I am not claiming The new test proves the text is in `main`. It does not prove the call executes at startup, and it does not prove the gate works — `ConfigRefTest` and `FleetdConfigRefCharterToolSurfaceWiringTest` cover the behaviour. **Nothing here has been exercised on the live daemon.** The running `fleetd` started 2026-09-10 16:34:27 and 48 commits on `main` are dated after that, this fix among them. The reload gate is live code only after a redeploy, which is owed and separate. Wiki entry updated: `11-Features.md` now says a reload is checked too, not only startup, with the reason (config loads before `mcp` exists) and the remaining gotcha. Filed separately, from the same review pass: #478 (a directory in `parityOverlay` is reported as copied and arrives empty). fleet01's note on this ticket — that a one-call-site check in `main` while `reload` runs only `validateAll` is the same shape as the CI job's hardcoded `-Dtest` list — is the sentence worth keeping.
ltms closed this issue 2026-09-10 23:15:36 +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#474
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?