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

Charter text is still never checked against the live tool surface — one canonical tool-name set (follow-up to #464) #469

New Issue
Closed
opened 2026-09-10 14:13:11 +02:00 by ltms · 1 comment
ltms commented 2026-09-10 14:13:11 +02:00
Owner
Copy Link
Copy Source

What is still missing

#464 shipped CharterToolSurfaceTest (merged in 49df792). That test is sound — I ran four mutations on the merge and all four killed it, including one that proves it reads the real registration source. But it does not close the gap the ticket was filed for, and the reason is my own acceptance criteria, not the worker's work.

The test writes its own charter text into a @TempDir fixture. So nothing anyone writes into the live fleetd/fleetd.yaml charters can make it fail. Measured on 49df792:

  • the test mentions fleetd.yaml 0 times;
  • the commit changes 0 production files;
  • FleetConfig.validateCharters() (FleetConfig.java:2552) is 19 lines and 0 of them name a tool. Control: 5 of those lines name "charter", so the search ran.

Reading the body confirms it. validateCharters() does two things and no more: the key must be a MemberRole wire name, and the text must not be blank. The text itself is never inspected.

So a charter that tells the architect to call a tool the server does not register still starts the daemon cleanly, and the member discovers it at run time by calling something that does not exist.

Why this is a design ticket, not a defect fix

Two facts make the obvious placement wrong:

  1. There is no package edge to reuse. FleetMcp does not import dev.ltms.fleet.config, and FleetConfig does not import dev.ltms.fleet.mcp. Neither direction exists today. Putting the check inside validateCharters() would make the config package depend on the MCP server, which is backwards — config is loaded before the server exists.
  2. The tool names are already written twice, and a third scrape makes it worse. Inside FleetMcp there are 11 tool("fleet_…") registrations and an authz action switch at :879 with 11 case labels. That switch is well built — it ends in default -> throw new IllegalArgumentException("unregistered tool: " + toolName), so an unregistered name fails loudly. But a charter check that scrapes source text for tool("…") a third time would be the third copy of one list, and a scrape is the weakest of the three forms.

Scope

Make one canonical set of registered tool names the single source, then have all three readers use it. Directions, not requirements — the shape is the reviewer's call:

  • The canonical set lives where the tools do (the mcp package), exposed as a set or enum the other readers can ask for.
  • Registration derives from it, or is asserted against it, so a registered tool that is missing from the set fails at startup.
  • The authz switch keeps its default -> throw. Prefer a form where a new tool name is a compile error rather than a run-time throw, if that is reachable without a large change.
  • Charter validation asks that set, rather than scraping source. Because charters are config and the set is in mcp, the check probably belongs at the seam that already has both — where the server is stood up and the charters are read — not inside FleetConfig.
  • Then CharterToolSurfaceTest's fixture becomes a test of real production behaviour instead of a test of its own fixture, and the two extraction helpers it already has (toolsNamedIn, toolsTheServerRegisters) are the starting point.

Acceptance criteria

  • Break-and-restore, on the live config shape. A charter naming a tool the server does not register must make the daemon refuse to start, with a message naming the charter key and the unknown tool. Prove it by adding such a charter to a config fixture that has the live shape (see the "gitignored config breaks on merge" hazard — fleetd/fleetd.yaml is not visible to a worker, so reproduce its shape in a throwaway fixture rather than reasoning about it).
  • The set is canonical, proven by removal. Delete one tool from the canonical set while leaving it registered, or the reverse. Something must fail. State which.
  • A default-less form was considered. Either the new tool name is a compile error somewhere, or the ticket says why that was not reachable here. Do not leave this unstated.
  • Count the tool-name sites before and after. Today: 2 in FleetMcp plus 1 fixture in the test. Report the number after the change, and if it is not lower, say why that is still the better shape.
  • Report the exact mvn -B clean test summary line and its exit code on its own line, unpiped.

Note on how this ticket came to exist

#464's criteria asked for a test and got a correct one. The gap needed a production check. Recording that because it is the fifth time a defect in my own brief, not in the work, produced the residue — the pattern is that I named a mechanism (a test) where I should have named the goal (the live config cannot start with a charter that lies about the tool surface).

## What is still missing #464 shipped `CharterToolSurfaceTest` (merged in `49df792`). That test is sound — I ran four mutations on the merge and all four killed it, including one that proves it reads the real registration source. But it does **not** close the gap the ticket was filed for, and the reason is my own acceptance criteria, not the worker's work. The test writes its own charter text into a `@TempDir` fixture. So **nothing anyone writes into the live `fleetd/fleetd.yaml` charters can make it fail.** Measured on `49df792`: - the test mentions `fleetd.yaml` **0** times; - the commit changes **0** production files; - `FleetConfig.validateCharters()` (`FleetConfig.java:2552`) is 19 lines and **0** of them name a tool. Control: 5 of those lines name "charter", so the search ran. Reading the body confirms it. `validateCharters()` does two things and no more: the key must be a `MemberRole` wire name, and the text must not be blank. The text itself is never inspected. So a charter that tells the architect to call a tool the server does not register still starts the daemon cleanly, and the member discovers it at run time by calling something that does not exist. ## Why this is a design ticket, not a defect fix Two facts make the obvious placement wrong: 1. **There is no package edge to reuse.** `FleetMcp` does not import `dev.ltms.fleet.config`, and `FleetConfig` does not import `dev.ltms.fleet.mcp`. Neither direction exists today. Putting the check inside `validateCharters()` would make the config package depend on the MCP server, which is backwards — config is loaded before the server exists. 2. **The tool names are already written twice, and a third scrape makes it worse.** Inside `FleetMcp` there are 11 `tool("fleet_…")` registrations and an authz action switch at `:879` with 11 case labels. That switch is well built — it ends in `default -> throw new IllegalArgumentException("unregistered tool: " + toolName)`, so an unregistered name fails loudly. But a charter check that scrapes source text for `tool("…")` a third time would be the third copy of one list, and a scrape is the weakest of the three forms. ## Scope Make **one canonical set of registered tool names** the single source, then have all three readers use it. Directions, not requirements — the shape is the reviewer's call: - The canonical set lives where the tools do (the `mcp` package), exposed as a set or enum the other readers can ask for. - Registration derives from it, or is asserted against it, so a registered tool that is missing from the set fails at startup. - The authz switch keeps its `default -> throw`. Prefer a form where a **new** tool name is a compile error rather than a run-time throw, if that is reachable without a large change. - Charter validation asks that set, rather than scraping source. Because charters are config and the set is in `mcp`, the check probably belongs at the seam that already has both — where the server is stood up and the charters are read — not inside `FleetConfig`. - Then `CharterToolSurfaceTest`'s fixture becomes a test of real production behaviour instead of a test of its own fixture, and the two extraction helpers it already has (`toolsNamedIn`, `toolsTheServerRegisters`) are the starting point. ## Acceptance criteria - **Break-and-restore, on the live config shape.** A charter naming a tool the server does not register must make the daemon refuse to start, with a message naming the charter key and the unknown tool. Prove it by adding such a charter to a config fixture that has the **live** shape (see the "gitignored config breaks on merge" hazard — `fleetd/fleetd.yaml` is not visible to a worker, so reproduce its shape in a throwaway fixture rather than reasoning about it). - **The set is canonical, proven by removal.** Delete one tool from the canonical set while leaving it registered, or the reverse. Something must fail. State which. - **A `default`-less form was considered.** Either the new tool name is a compile error somewhere, or the ticket says why that was not reachable here. Do not leave this unstated. - **Count the tool-name sites before and after.** Today: 2 in `FleetMcp` plus 1 fixture in the test. Report the number after the change, and if it is not lower, say why that is still the better shape. - Report the exact `mvn -B clean test` summary line and its exit code on its own line, unpiped. ## Note on how this ticket came to exist #464's criteria asked for a test and got a correct one. The gap needed a production check. Recording that because it is the fifth time a defect in my own brief, not in the work, produced the residue — the pattern is that I named a mechanism (a test) where I should have named the goal (the live config cannot start with a charter that lies about the tool surface).
ltms commented 2026-09-10 15:10:51 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 1477e43 on main (tree e98654f). PR #472. Verified landed with git merge-base --is-ancestor, against an unmerged control branch that correctly reported no.

My own mutation battery, run on the merge commit

Not the worker's numbers — I do not promote a break-and-restore to a fact. Every cell below ran a full mvn -B clean test on tree e98654f, which is byte-identical to the tree I pushed.

CONTROL 1, unmutated: 1613 tests, 0 failures, 0 errors, BUILD SUCCESS.

cell mutation result
M1 add a 12th FleetTool constant, pin no action COMPILE ERROR — FleetMcp.java:[934,16] the switch expression does not cover all possible input values
M2 register a name FleetTool does not list KILLED — 12 errors, IllegalStateException from FleetMcp.<init>
M3 delete the CharterToolSurface call from Fleetd.main KILLED — FleetdStartupValidationTest.mainRefusesACharterNamingAnUnregisteredTool
M4 keep the call, make the method a no-op KILLED — 3 tests, including the startup one
M5 invert the filter (reject registered, accept unknown) KILLED — 3 tests, including the positive case
M6 make wireNames() return an empty set KILLED — 17 tests across all 3 former scrape sites

M1 is the cell that matters for the design claim. The compile error names the authzAction switch line, not a wireName() call site, so the compile-time property is real and not an artefact of the constant being referenced elsewhere.

M3 is the cell that closes this ticket's actual gap. That shape — the mechanism lands, the call site in Fleetd.main stays unpinned — survived three times recently (#446 M5 and M7, #466 M1). Here it is killed by name.

M6 answers the question my brief was written around. Before this change the registered tool set was scraped from FleetMcp.java's source text in three separate test classes. An empty scrape made two of them pass vacuously: they compared an empty set against the doc and found no violations. With one canonical source, an empty surface now fails in 17 places at once, including McpContractDocTest.theCheckActuallyHasSomethingToCheck.

My brief was wrong about the size of the problem

I wrote that there were two tool-name inventories plus one test fixture. There were five: the 11 schema registrations, the authz switch, and three independent source-text scrapes of FleetMcp.java in CharterToolSurfaceTest, FleetMcpAuthzTest and McpContractDocTest. The ticket mentioned none of the three.

The worker corrected me and widened the diff to fix them. I checked that claim myself on origin/main before accepting it: three test files did read FleetMcp.java as source text, with a control file at zero to prove the search discriminated. Once the literals moved behind FleetTool.X.wireName(), all three regexes matched zero names — one would have failed loudly on its vacuity guard, the other two would have gone quietly vacuous. So the wider diff was required, not scope creep. Inventories after this change: one.

Two of my own control labels in the battery were wrong, and I am recording them so the numbers above are read correctly: I wrote "must be 11" beside a constant count my regex reported as 10 (it missed the ;-terminated last constant — there are 11), and "must be 7" beside a switch my own count made 8 (8 case lines covering 11 constants, since several are grouped). Both were my annotations. Re-measured on main: 11 constants, 11 distinct constants covered.

One gap this does NOT close — filed separately

The new check runs at startup only. CharterToolSurface.assertChartersNameOnlyRegisteredTools has exactly one call site, Fleetd.java:176. ConfigRef.reload() calls fresh.validateAll(), which does not include it.

Charters are hot and read live at spawn — measured, not assumed: HerdrPeerLauncher.java:499 reads liveFleet.charterFor(role), and ConfigRefTest.aHotChangeIsAppliedAndReadThroughGet pins fleet: as hot. So the reachable path is: an operator edits fleet.charters.dev at run time to name bridge_send, validateAll() accepts it (the key is a valid role, the text is non-blank), the reload is applied, and the next spawned member gets a charter naming a tool that does not exist. Exactly this ticket's symptom, through the door this fix does not cover.

ConfigRef.java states the principle it breaks, in its own comment above that call: "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." Filed as its own ticket rather than reopening this one, because the startup direction is genuinely closed and this is the other half.

Merged as `1477e43` on `main` (tree `e98654f`). PR #472. Verified landed with `git merge-base --is-ancestor`, against an unmerged control branch that correctly reported `no`. ## My own mutation battery, run on the merge commit Not the worker's numbers — I do not promote a break-and-restore to a fact. Every cell below ran a full `mvn -B clean test` on tree `e98654f`, which is byte-identical to the tree I pushed. CONTROL 1, unmutated: **1613 tests, 0 failures, 0 errors, BUILD SUCCESS**. | cell | mutation | result | |---|---|---| | M1 | add a 12th `FleetTool` constant, pin no action | **COMPILE ERROR** — `FleetMcp.java:[934,16] the switch expression does not cover all possible input values` | | M2 | register a name `FleetTool` does not list | **KILLED** — 12 errors, `IllegalStateException` from `FleetMcp.<init>` | | M3 | delete the `CharterToolSurface` call from `Fleetd.main` | **KILLED** — `FleetdStartupValidationTest.mainRefusesACharterNamingAnUnregisteredTool` | | M4 | keep the call, make the method a no-op | **KILLED** — 3 tests, including the startup one | | M5 | invert the filter (reject registered, accept unknown) | **KILLED** — 3 tests, including the positive case | | M6 | make `wireNames()` return an empty set | **KILLED** — 17 tests across all 3 former scrape sites | M1 is the cell that matters for the design claim. The compile error names the `authzAction` switch line, not a `wireName()` call site, so the compile-time property is real and not an artefact of the constant being referenced elsewhere. M3 is the cell that closes this ticket's actual gap. That shape — the mechanism lands, the call site in `Fleetd.main` stays unpinned — survived three times recently (#446 M5 and M7, #466 M1). Here it is killed by name. M6 answers the question my brief was written around. Before this change the registered tool set was scraped from `FleetMcp.java`'s source text in three separate test classes. An empty scrape made two of them pass vacuously: they compared an empty set against the doc and found no violations. With one canonical source, an empty surface now fails in 17 places at once, including `McpContractDocTest.theCheckActuallyHasSomethingToCheck`. ## My brief was wrong about the size of the problem I wrote that there were two tool-name inventories plus one test fixture. There were **five**: the 11 schema registrations, the authz switch, and three independent source-text scrapes of `FleetMcp.java` in `CharterToolSurfaceTest`, `FleetMcpAuthzTest` and `McpContractDocTest`. The ticket mentioned none of the three. The worker corrected me and widened the diff to fix them. I checked that claim myself on `origin/main` before accepting it: three test files did read `FleetMcp.java` as source text, with a control file at zero to prove the search discriminated. Once the literals moved behind `FleetTool.X.wireName()`, all three regexes matched zero names — one would have failed loudly on its vacuity guard, the other two would have gone quietly vacuous. So the wider diff was required, not scope creep. Inventories after this change: **one**. Two of my own control labels in the battery were wrong, and I am recording them so the numbers above are read correctly: I wrote "must be 11" beside a constant count my regex reported as 10 (it missed the `;`-terminated last constant — there are 11), and "must be 7" beside a switch my own count made 8 (8 `case` lines covering 11 constants, since several are grouped). Both were my annotations. Re-measured on `main`: 11 constants, 11 distinct constants covered. ## One gap this does NOT close — filed separately The new check runs at **startup only**. `CharterToolSurface.assertChartersNameOnlyRegisteredTools` has exactly one call site, `Fleetd.java:176`. `ConfigRef.reload()` calls `fresh.validateAll()`, which does not include it. Charters are hot and read live at spawn — measured, not assumed: `HerdrPeerLauncher.java:499` reads `liveFleet.charterFor(role)`, and `ConfigRefTest.aHotChangeIsAppliedAndReadThroughGet` pins `fleet:` as hot. So the reachable path is: an operator edits `fleet.charters.dev` at run time to name `bridge_send`, `validateAll()` accepts it (the key is a valid role, the text is non-blank), the reload is applied, and the next spawned member gets a charter naming a tool that does not exist. Exactly this ticket's symptom, through the door this fix does not cover. `ConfigRef.java` states the principle it breaks, in its own comment above that call: *"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."* Filed as its own ticket rather than reopening this one, because the startup direction is genuinely closed and this is the other half.
ltms closed this issue 2026-09-10 15:11: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#469
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?