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

Nothing tests that FleetMcp uses the CallerResolver — forcing the legacy identity path leaves the whole suite green at 1696/0 #518

New Issue
Closed
opened 2026-09-12 06:07:00 +02:00 by ltms · 2 comments
ltms commented 2026-09-12 06:07:00 +02:00
Owner
Copy Link
Copy Source

Found by mutating a line #515 did not touch, while adjudicating it (merged, fixes #509).

The mutation, and what survived it

FleetMcp.java:326 is the single line that decides who every MCP caller is:

Principal p = callers != null
        ? callers.resolve(req.getRemoteAddr(), req.getRemotePort(),
                req.getHeader("Authorization"))
        : legacyPrincipal(identity, req.getRemoteAddr(), req.getRemotePort());

I replaced it with an unconditional call to the legacy path:

Principal p = legacyPrincipal(identity, req.getRemoteAddr(), req.getRemotePort());

Proven applied: grep -n 'Principal p = legacyPrincipal' → line 326; grep -n 'Principal p = callers != null' → no match; plus a re-read of lines 323-330.

mvn -f fleetd/pom.xml clean install → exit 0, Tests run: 1696, Failures: 0, Errors: 0, Skipped: 0. No test failed.

Restored byte-identical (517279e6897a577dc123114c064ea98379519869d17e24bf78f3920c6eb95390), control run green at the same 1696/0.

Harness proof, on this same tree, minutes earlier: I re-applied MUTANTC to PaneLocator.java:117 and PaneLocatorTest went red — 16 run, 1 failure, anEarlierClientsErrorSurvivesALaterClientsCleanNegative ... expected: <false> but was: <true>. So the suite can fail. It does not fail for this.

What the mutant actually does

It routes every MCP caller through the legacy identity path, skipping CallerResolver entirely. That discards, in one line:

  • isLoopback(remoteAddr) — the check that a caller is local at all
  • c.scanComplete() — the guard #505 added, and the one #509 just pinned on its input side
  • token mode — req.getHeader("Authorization") is not even read on the legacy path

Not equivalent by a wide margin. Post-#515 the legacy path returns worker(...) or anonymous() and never primary, so on a live daemon this mutant would deny every orchestration call and the fleet would stop working within one message. The live system is the only thing that would catch it.

Why nothing catches it

CallerResolver is well tested. legacyPrincipal is now tested, because #515 made it package-private for exactly that. What is untested is the wiring — that FleetMcp calls the resolver rather than the legacy path.

The contextExtractor closure only runs on a real MCP request over the transport. No test drives one. So every test of who-a-caller-is stops at the seam, and the decision above the seam is unexamined.

This is the shape already recorded against #393 and #415: a test on the seam does not prove the caller, and extraction moves the untested surface up. Coverage rises while a new uncovered decision appears, and every signal says the code got safer. Here the uncovered decision is the highest-stakes one in the MCP surface.

It is worth saying plainly what makes this different from an ordinary coverage gap: the thing nobody tests is which authorization system is in use. Every test below it assumes the answer.

The fix

Two parts. The first is the one that matters.

1. Apply #415's antidote — make the choice unrepresentable, not merely tested

The reason a defaulted decision survives a green suite is that writing nothing is a legal way to get the wrong answer. callers != null ? ... : ... is exactly that: the fallback is reached by omission.

So do what #415 did. Options, in my order of preference:

  • (a) Make the legacy mode explicit at construction. Replace "callers is null" with a named value the caller must choose — a second constructor, a factory (FleetMcp.withoutAuthorization(...)), or an enum parameter with no default. The test that wants legacy mode names it; production cannot reach it by leaving a field unset. Then the ternary disappears and there is no decision left to get wrong.
  • (b) If (a) is too large, assert the wiring in a source-shape test the way test_recovery_patterns_match_source and test_swap_ordered_after_wait_and_before_start already do in scripts/test-redeploy-fleetd.sh. Cheaper, and strictly worse: see #517 — a source-text assertion pins what the text says and never whether it is reached.

Prefer (a). A compile error beats a test, because a test can be deleted by the same change that breaks the thing it guards.

2. Drive the contextExtractor at least once

Independently of (1), there should be one test that goes through the real transport path and asserts the resulting Principal came from the CallerResolver. One test, not a suite — the point is to make the closure reachable at all, so a future change to it is not invisible.

Acceptance

  • mvn -f fleetd/pom.xml clean install passes. Report the Tests run / Failures / Errors / Skipped line verbatim, and do not pipe the command — a pipe hides a failure behind a zero exit.
  • The mutation above must now be killed, or be impossible to write. If you take option (a), show that the mutant no longer compiles and quote the compiler error. If you take (b), show the suite going red and quote the failing assertion.
  • Reproduce, restore, confirm byte-identical with shasum -a 256, and run a green control. Prove the mutation applied with two greps using different search strings and a grep -n re-read of the line — a mis-quoted search string silently reports zero and looks exactly like a mutation that did not apply.
  • FleetMcpAuthzTest.theLegacyConstructorLeavesTheGateOpen must keep working, in whatever form legacy mode takes. It is deliberate, not dead code.

Related

  • #509 / #515 — the change this was found in.
  • #505 — the guard the mutant discards.
  • #415 — the required-parameter antidote this should use.
  • #393 — a test on the seam does not prove the caller.
  • #517 — why option (b) is the weaker choice.
Found by mutating a line #515 did not touch, while adjudicating it (merged, fixes #509). ## The mutation, and what survived it `FleetMcp.java:326` is the single line that decides who **every** MCP caller is: ```java Principal p = callers != null ? callers.resolve(req.getRemoteAddr(), req.getRemotePort(), req.getHeader("Authorization")) : legacyPrincipal(identity, req.getRemoteAddr(), req.getRemotePort()); ``` I replaced it with an unconditional call to the legacy path: ```java Principal p = legacyPrincipal(identity, req.getRemoteAddr(), req.getRemotePort()); ``` Proven applied: `grep -n 'Principal p = legacyPrincipal'` → line 326; `grep -n 'Principal p = callers != null'` → no match; plus a re-read of lines 323-330. `mvn -f fleetd/pom.xml clean install` → exit 0, **`Tests run: 1696, Failures: 0, Errors: 0, Skipped: 0`**. No test failed. Restored byte-identical (`517279e6897a577dc123114c064ea98379519869d17e24bf78f3920c6eb95390`), control run green at the same 1696/0. **Harness proof, on this same tree, minutes earlier:** I re-applied MUTANTC to `PaneLocator.java:117` and `PaneLocatorTest` went red — 16 run, 1 failure, `anEarlierClientsErrorSurvivesALaterClientsCleanNegative ... expected: <false> but was: <true>`. So the suite can fail. It does not fail for this. ## What the mutant actually does It routes every MCP caller through the legacy identity path, skipping `CallerResolver` entirely. That discards, in one line: - `isLoopback(remoteAddr)` — the check that a caller is local at all - `c.scanComplete()` — the guard #505 added, and the one #509 just pinned on its input side - token mode — `req.getHeader("Authorization")` is not even read on the legacy path Not equivalent by a wide margin. Post-#515 the legacy path returns `worker(...)` or `anonymous()` and never `primary`, so on a live daemon this mutant would deny every orchestration call and the fleet would stop working within one message. **The live system is the only thing that would catch it.** ## Why nothing catches it `CallerResolver` is well tested. `legacyPrincipal` is now tested, because #515 made it package-private for exactly that. What is untested is the **wiring** — that `FleetMcp` calls the resolver rather than the legacy path. The `contextExtractor` closure only runs on a real MCP request over the transport. No test drives one. So every test of who-a-caller-is stops at the seam, and the decision above the seam is unexamined. This is the shape already recorded against #393 and #415: *a test on the seam does not prove the caller*, and *extraction moves the untested surface up*. Coverage rises while a new uncovered decision appears, and every signal says the code got safer. Here the uncovered decision is the highest-stakes one in the MCP surface. It is worth saying plainly what makes this different from an ordinary coverage gap: **the thing nobody tests is which authorization system is in use.** Every test below it assumes the answer. ## The fix Two parts. The first is the one that matters. ### 1. Apply #415's antidote — make the choice unrepresentable, not merely tested The reason a defaulted decision survives a green suite is that writing nothing is a legal way to get the wrong answer. `callers != null ? ... : ...` is exactly that: the fallback is reached by omission. So do what #415 did. Options, in my order of preference: - **(a) Make the legacy mode explicit at construction.** Replace "`callers` is null" with a named value the caller must choose — a second constructor, a factory (`FleetMcp.withoutAuthorization(...)`), or an enum parameter with no default. The test that wants legacy mode names it; production cannot reach it by leaving a field unset. Then the ternary disappears and there is no decision left to get wrong. - **(b) If (a) is too large,** assert the wiring in a source-shape test the way `test_recovery_patterns_match_source` and `test_swap_ordered_after_wait_and_before_start` already do in `scripts/test-redeploy-fleetd.sh`. Cheaper, and strictly worse: see #517 — a source-text assertion pins what the text says and never whether it is reached. Prefer (a). A compile error beats a test, because a test can be deleted by the same change that breaks the thing it guards. ### 2. Drive the contextExtractor at least once Independently of (1), there should be one test that goes through the real transport path and asserts the resulting `Principal` came from the `CallerResolver`. One test, not a suite — the point is to make the closure reachable at all, so a future change to it is not invisible. ## Acceptance - `mvn -f fleetd/pom.xml clean install` passes. Report the `Tests run / Failures / Errors / Skipped` line verbatim, and do not pipe the command — a pipe hides a failure behind a zero exit. - **The mutation above must now be killed**, or be impossible to write. If you take option (a), show that the mutant no longer compiles and quote the compiler error. If you take (b), show the suite going red and quote the failing assertion. - Reproduce, restore, confirm byte-identical with `shasum -a 256`, and run a green control. Prove the mutation applied with two greps using *different* search strings **and** a `grep -n` re-read of the line — a mis-quoted search string silently reports zero and looks exactly like a mutation that did not apply. - `FleetMcpAuthzTest.theLegacyConstructorLeavesTheGateOpen` must keep working, in whatever form legacy mode takes. It is deliberate, not dead code. ## Related - #509 / #515 — the change this was found in. - #505 — the guard the mutant discards. - #415 — the required-parameter antidote this should use. - #393 — a test on the seam does not prove the caller. - #517 — why option (b) is the weaker choice.
ltms commented 2026-09-12 06:23:52 +02:00
Author
Owner
Copy Link
Copy Source

Severity: deleting a config key silently downgrades authorization

Raised by the fleet01 lead, and it sharpens the severity line above. I had written this up as "there
is an untested branch". That understates it.

The fallback is reached by omission. callers != null means writing nothing selects the legacy
path. So:

A deploy that drops a config key is a security regression with a green suite.

That is a different and worse property than an untested branch. An untested branch needs someone to
write wrong code. This needs someone to write no code — or to delete a line from fleetd.yaml
during an unrelated cleanup. The suite stays at 1696/0 either way, and so does the build, and so does
the startup log.

It is the "a detector whose failure mode grants authority" class again, arriving by a third route:
not a wrong fold (#509), not a conflated sentinel (#505), but an absent configuration.

This is the argument for option (a) over option (b) in the fix above, and I want it stated plainly: a
test cannot fix this, because the thing that goes wrong is not code that a test could execute. Only
making the input required removes the failure.

Why the axis framing matters here

The peer's generalisation, and it is the third instance of a rule we already have:

Coverage counts tests, not axes. Fifteen PaneLocator tests held the two-client axis fixed
(#509). Here the entire authorization suite holds the which-resolver axis fixed. All 1696 tests
run with the seam pinned at the value my mutation changed, so the mutated line executes constantly
and nothing varies the one thing that matters.

The suite is one data point on the only axis this ticket is about. That is why 1696/0 is not
reassuring and why the number itself is the thing doing the concealing.

Caution on the legacyPrincipal test

#515 made legacyPrincipal package-private so it could be tested. That was right, and it has a cost
worth naming in the code rather than discovering later:

Making an unsafe path more testable increases the chance a test pins its behaviour as correct, which
makes the path harder to delete. A future session that wants to remove legacy mode will find a
passing test asserting how it behaves, and will reasonably read that as a supported contract.

So: keep testing it, and name it in the test as the legacy path being retired, not as a supported
behaviour. FleetMcpAuthzTest.theLegacyConstructorLeavesTheGateOpen already reads that way; whatever
form legacy mode takes after option (a), keep that framing in the test name and in a comment saying
why it exists.

## Severity: deleting a config key silently downgrades authorization Raised by the fleet01 lead, and it sharpens the severity line above. I had written this up as "there is an untested branch". That understates it. The fallback is reached by **omission**. `callers != null` means writing nothing selects the legacy path. So: > **A deploy that drops a config key is a security regression with a green suite.** That is a different and worse property than an untested branch. An untested branch needs someone to write wrong code. This needs someone to write *no* code — or to delete a line from `fleetd.yaml` during an unrelated cleanup. The suite stays at 1696/0 either way, and so does the build, and so does the startup log. It is the "a detector whose failure mode grants authority" class again, arriving by a third route: not a wrong fold (#509), not a conflated sentinel (#505), but an absent configuration. This is the argument for option (a) over option (b) in the fix above, and I want it stated plainly: a test cannot fix this, because the thing that goes wrong is not code that a test could execute. Only making the input **required** removes the failure. ## Why the axis framing matters here The peer's generalisation, and it is the third instance of a rule we already have: Coverage counts **tests**, not **axes**. Fifteen `PaneLocator` tests held the two-client axis fixed (#509). Here the entire authorization suite holds the **which-resolver** axis fixed. All 1696 tests run with the seam pinned at the value my mutation changed, so the mutated line executes constantly and nothing varies the one thing that matters. **The suite is one data point on the only axis this ticket is about.** That is why 1696/0 is not reassuring and why the number itself is the thing doing the concealing. ## Caution on the `legacyPrincipal` test #515 made `legacyPrincipal` package-private so it could be tested. That was right, and it has a cost worth naming in the code rather than discovering later: **Making an unsafe path more testable increases the chance a test pins its behaviour as correct, which makes the path harder to delete.** A future session that wants to remove legacy mode will find a passing test asserting how it behaves, and will reasonably read that as a supported contract. So: keep testing it, and name it in the test as **the legacy path being retired**, not as a supported behaviour. `FleetMcpAuthzTest.theLegacyConstructorLeavesTheGateOpen` already reads that way; whatever form legacy mode takes after option (a), keep that framing in the test name and in a comment saying why it exists.
ltms closed this issue 2026-09-12 07:10:23 +02:00
ltms commented 2026-09-12 07:11:34 +02:00
Author
Owner
Copy Link
Copy Source

Closed by #524 (merged). Verified by my own build and my own mutations, not read off the PR body.

The fix section

asked done
(a) make legacy mode explicit at construction — "an enum parameter with no default … then the ternary disappears" — preferred over (b) (a), exactly. FleetMcp.AuthorizationMode { ENFORCED, UNENFORCED } as a required parameter; callers now required and non-null; the ternary is gone; legacyPrincipal deleted rather than left testable
2. drive the contextExtractor at least once through the real transport FleetMcpContextExtractorTest — one test, real HttpServletStreamableServerTransportProvider on a real Jetty server (port 0, loopback, FakeHerdr), driven by a real MCP client over HTTP

The implementer took the option this ticket preferred, and deleted the second heuristic instead of keeping it testable — which is better than what was asked. My own note from #515 was that making an unsafe path more testable raises the chance a test later pins it as correct; deleting it removes that risk entirely.

Acceptance, ticked

  • mvn clean install passes, the line quoted verbatim, command not piped — I redirected to a file and captured the exit status directly, so no pipe hid anything:

    [INFO] BUILD SUCCESS
Tests run: 1697, Failures: 0, Errors: 0, Skipped: 0

    1697 rather than 1696+1 because this branch predates #522's two new tests; no file overlap with anything main changed since its branch point, so not a stale-branch merge.

  • "show that the mutant no longer compiles and quote the compiler error" — done. I wrote this ticket's mutant back verbatim:

    Principal p = legacyPrincipal(identity, req.getRemoteAddr(), req.getRemotePort());

    [ERROR] .../mcp/FleetMcp.java:[360,35] cannot find symbol
  symbol:   method legacyPrincipal(dev.ltms.fleet.mcp.ConnectionIdentity,java.lang.String,int)
  location: class dev.ltms.fleet.mcp.FleetMcp

    mvn clean compile exit 1. The mutation this ticket is about is now unwritable, which is the whole point of option (a): a compile error cannot be deleted by the change that breaks the thing it guards.

  • A second mutation, because "unwritable" only covers the old spelling. I reinstated the deleted heuristic's behaviour — resolve from the connection only, never reading the Authorization header — which compiles fine. FleetMcpContextExtractorTest fails by name:

    a valid bearer token must resolve as PRIMARY and pass fleet_whoami's READ gate:
unauthenticated: anonymous may not READ ==> expected: <false> but was: <true>

    Then the measurement that actually settles part 2's value: with that mutation still applied I ran the whole suite. Tests run: 1697, Failures: 1 — and the single failure is the new test class. All 20 FleetMcpAuthzTest cases pass with the resolver bypassed, as do the other 1676 tests. So this ticket's claim was right and is now measured: nothing in the pre-existing suite could see this, because none of it goes through the transport.

  • two greps with different strings, plus a re-read — mutant marker present = 1, original callers.resolve(...) call absent = 0, with a control showing it present = 1 in a saved copy.

  • restored byte-identical, green control — hash back to 21a92ff97f706f65a66be337edef4bb54a891424df389ae65d9e556afc8fb1a4, tree clean, control build exit 0.

  • FleetMcpAuthzTest.theLegacyConstructorLeavesTheGateOpen still works — still present at FleetMcpAuthzTest.java:191, now driven by AuthorizationMode.UNENFORCED instead of a missing resolver, with its assertion message updated to say so.

  • legacyPrincipal fully gone — 0 declarations, 0 calls. The four remaining mentions are prose that correctly describes it as deleted.

One limit I am recording rather than claiming as covered

callers being required stops the fallback being reached by omission, which was the defect. An explicit literal null is still something a caller could write, and the Objects.requireNonNull(callers, "callers") that catches it has no test of its own. That is the intended bar for #415's antidote, not a gap worth its own ticket.

Process note

The #518 worker's pane and worktree were taken by the idle reaper before I finished verifying. Nothing was lost — the branch was pushed and the tree clean — but the worktree goes with the pane, so everything above was built and mutated in a worktree I created myself from the pushed head. Worth knowing: the 1800s idle TTL removes the worktree too, not just the ticket and the pane.

Closed by #524 (merged). Verified by my own build and my own mutations, not read off the PR body. ## The fix section | asked | done | |---|---| | **(a)** make legacy mode explicit at construction — "an enum parameter with no default … then the ternary disappears" — preferred over (b) | **(a)**, exactly. `FleetMcp.AuthorizationMode { ENFORCED, UNENFORCED }` as a required parameter; `callers` now required and non-null; the ternary is gone; `legacyPrincipal` **deleted** rather than left testable | | **2.** drive the contextExtractor at least once through the real transport | `FleetMcpContextExtractorTest` — one test, real `HttpServletStreamableServerTransportProvider` on a real Jetty server (port 0, loopback, `FakeHerdr`), driven by a real MCP client over HTTP | The implementer took the option this ticket preferred, and deleted the second heuristic instead of keeping it testable — which is better than what was asked. My own note from #515 was that making an unsafe path more testable raises the chance a test later pins it as correct; deleting it removes that risk entirely. ## Acceptance, ticked - **`mvn clean install` passes, the line quoted verbatim, command not piped** — I redirected to a file and captured the exit status directly, so no pipe hid anything: ``` [INFO] BUILD SUCCESS Tests run: 1697, Failures: 0, Errors: 0, Skipped: 0 ``` 1697 rather than 1696+1 because this branch predates #522's two new tests; no file overlap with anything main changed since its branch point, so not a stale-branch merge. - **"show that the mutant no longer compiles and quote the compiler error"** — done. I wrote this ticket's mutant back verbatim: ```java Principal p = legacyPrincipal(identity, req.getRemoteAddr(), req.getRemotePort()); ``` ``` [ERROR] .../mcp/FleetMcp.java:[360,35] cannot find symbol symbol: method legacyPrincipal(dev.ltms.fleet.mcp.ConnectionIdentity,java.lang.String,int) location: class dev.ltms.fleet.mcp.FleetMcp ``` `mvn clean compile` exit 1. The mutation this ticket is about is now unwritable, which is the whole point of option (a): a compile error cannot be deleted by the change that breaks the thing it guards. - **A second mutation, because "unwritable" only covers the old spelling.** I reinstated the deleted heuristic's *behaviour* — resolve from the connection only, never reading the `Authorization` header — which compiles fine. `FleetMcpContextExtractorTest` fails by name: ``` a valid bearer token must resolve as PRIMARY and pass fleet_whoami's READ gate: unauthenticated: anonymous may not READ ==> expected: <false> but was: <true> ``` Then the measurement that actually settles part 2's value: **with that mutation still applied I ran the whole suite.** `Tests run: 1697, Failures: 1` — and the single failure is the new test class. All 20 `FleetMcpAuthzTest` cases pass with the resolver bypassed, as do the other 1676 tests. So this ticket's claim was right and is now measured: nothing in the pre-existing suite could see this, because none of it goes through the transport. - **two greps with different strings, plus a re-read** — mutant marker present = 1, original `callers.resolve(...)` call absent = 0, with a control showing it present = 1 in a saved copy. - **restored byte-identical, green control** — hash back to `21a92ff97f706f65a66be337edef4bb54a891424df389ae65d9e556afc8fb1a4`, tree clean, control build exit 0. - **`FleetMcpAuthzTest.theLegacyConstructorLeavesTheGateOpen` still works** — still present at `FleetMcpAuthzTest.java:191`, now driven by `AuthorizationMode.UNENFORCED` instead of a missing resolver, with its assertion message updated to say so. - **`legacyPrincipal` fully gone** — 0 declarations, 0 calls. The four remaining mentions are prose that correctly describes it as deleted. ## One limit I am recording rather than claiming as covered `callers` being required stops the fallback being reached by **omission**, which was the defect. An explicit literal `null` is still something a caller could write, and the `Objects.requireNonNull(callers, "callers")` that catches it has no test of its own. That is the intended bar for #415's antidote, not a gap worth its own ticket. ## Process note The #518 worker's pane and worktree were taken by the idle reaper before I finished verifying. Nothing was lost — the branch was pushed and the tree clean — but the worktree goes with the pane, so everything above was built and mutated in a worktree I created myself from the pushed head. Worth knowing: the 1800s idle TTL removes the *worktree* too, not just the ticket and the pane.
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#518
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?