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

Fleetd's composition root is untested: a feature can be silently unwired and every test stays green #248

New Issue
Closed
opened 2026-09-03 06:55:33 +02:00 by ltms · 1 comment
ltms commented 2026-09-03 06:55:33 +02:00
Owner
Copy Link
Copy Source

Measured on main at 5cf3ca9 on 2026-09-03, while merging #241 on top of #201/#227 Unit 5.

Both tickets changed the same statement in Fleetd.java — the new CompletionResolver(...) call in main. I resolved the conflict onto the full 8-argument constructor, then tried to prove the resolution had actually kept both features. It cannot be proved, because nothing tests it.

The measurement

Two mutations of Fleetd.java, each run alone, each reverted afterwards, each with a separate compile-error count so a compile failure could not be misread as a pass:

Mutation Compile errors Result
Drop #241's worktree/branch lookup — pass _ -> null 0 GREEN — 1215 tests, 0 failures
Drop Unit 5's backendErrorPatterns + backendErrorSink — pass BackendErrorPatternLookup.legacy() and BackendErrorSink.none() 0 GREEN — 1215 tests, 0 failures

So either feature can be fully unwired at the composition root and the build stays clean. The shipped daemon would lose the feature; CI would say nothing.

Why this happened

This is the seam-versus-caller trap, and it is worth naming exactly.

Unit 5's own BackendOutageFlowTest is a good test. Its report says it "mirrors Fleetd.main's backendErrorSink lambda line-for-line". That is the problem: it is a copy. It proves the copy behaves correctly. It cannot notice that the original was deleted. The same is true of every other test here — they construct a CompletionResolver themselves with the arguments they want, so they test the class, never the wiring.

Fleetd.main is a long method that builds and connects everything, and a main method has no seam a test can reach.

This is not a new idea in this codebase

Fleetd already solves it twice, and both existing patterns work:

  • Fleetd.deliverableTo(presence, leads) — a package-private static factory that returns the predicate main uses. FleetDeliverabilityTest calls it directly. This is the good pattern.
  • FleetdFleetAppConstructionTest — reads Fleetd.java's source text and asserts on it. Crude, but honest about being a last resort when no seam exists.

So the fix is to extend a pattern the file already has, not to invent one.

What to do

  1. Extract each argument main builds inline into a package-private static factory on Fleetd, following deliverableTo exactly:
    • the worktree/branch lookup (closes over SessionManager only — easy),
    • backendErrorPatterns (closes over SessionManager + config),
    • backendErrorSink (closes over more — sessions, pushLoop, the lead's pane; if it cannot be extracted cleanly, say why in the ticket rather than forcing it, and fall back to a source-text assertion for that one).
  2. Test each factory directly for its behaviour.
  3. Add one test that main actually passes them — this is the part that is missing today. If the factories cannot be observed from outside, a FleetdFleetAppConstructionTest-style source assertion naming each factory call is acceptable, as long as the test says plainly that it checks source text and not behaviour.

Acceptance criteria

  1. Re-running both mutations above turns at least one test red, each with 0 compile errors. Report the two counts separately — a compile failure prints zero test failures, so "no failures" reads as green if you only look at the test line.
  2. mvn clean install from fleetd/ is green, unpiped, with the test count reported.
  3. No production behaviour changes. This is extraction plus tests only.
  4. If any argument genuinely cannot be covered, say which one and why. An honest gap is fine; a silent one is not.

Scope note

Do not try to cover all of Fleetd.main. The scope is the CompletionResolver construction and the arguments it takes. Anything else you notice, report in one line and leave alone.

Related: #113 (our checkers cover less than they look like they cover).

Measured on `main` at `5cf3ca9` on 2026-09-03, while merging #241 on top of #201/#227 Unit 5. Both tickets changed the same statement in `Fleetd.java` — the `new CompletionResolver(...)` call in `main`. I resolved the conflict onto the full 8-argument constructor, then tried to prove the resolution had actually kept both features. It cannot be proved, because nothing tests it. ## The measurement Two mutations of `Fleetd.java`, each run alone, each reverted afterwards, each with a **separate** compile-error count so a compile failure could not be misread as a pass: | Mutation | Compile errors | Result | |---|---|---| | Drop #241's worktree/branch lookup — pass `_ -> null` | 0 | **GREEN** — 1215 tests, 0 failures | | Drop Unit 5's `backendErrorPatterns` + `backendErrorSink` — pass `BackendErrorPatternLookup.legacy()` and `BackendErrorSink.none()` | 0 | **GREEN** — 1215 tests, 0 failures | So either feature can be fully unwired at the composition root and the build stays clean. The shipped daemon would lose the feature; CI would say nothing. ## Why this happened This is the seam-versus-caller trap, and it is worth naming exactly. Unit 5's own `BackendOutageFlowTest` is a good test. Its report says it "mirrors `Fleetd.main`'s `backendErrorSink` lambda line-for-line". That is the problem: **it is a copy.** It proves the copy behaves correctly. It cannot notice that the original was deleted. The same is true of every other test here — they construct a `CompletionResolver` themselves with the arguments they want, so they test the class, never the wiring. `Fleetd.main` is a long method that builds and connects everything, and a `main` method has no seam a test can reach. ## This is not a new idea in this codebase `Fleetd` already solves it twice, and both existing patterns work: - **`Fleetd.deliverableTo(presence, leads)`** — a package-private `static` factory that returns the predicate `main` uses. `FleetDeliverabilityTest` calls it directly. This is the good pattern. - **`FleetdFleetAppConstructionTest`** — reads `Fleetd.java`'s **source text** and asserts on it. Crude, but honest about being a last resort when no seam exists. So the fix is to extend a pattern the file already has, not to invent one. ## What to do 1. Extract each argument `main` builds inline into a package-private `static` factory on `Fleetd`, following `deliverableTo` exactly: - the worktree/branch lookup (closes over `SessionManager` only — easy), - `backendErrorPatterns` (closes over `SessionManager` + config), - `backendErrorSink` (closes over more — `sessions`, `pushLoop`, the lead's pane; if it cannot be extracted cleanly, say why in the ticket rather than forcing it, and fall back to a source-text assertion for that one). 2. Test each factory directly for its behaviour. 3. Add one test that `main` actually **passes** them — this is the part that is missing today. If the factories cannot be observed from outside, a `FleetdFleetAppConstructionTest`-style source assertion naming each factory call is acceptable, as long as the test says plainly that it checks source text and not behaviour. ## Acceptance criteria 1. Re-running both mutations above turns at least one test **red**, each with **0** compile errors. Report the two counts separately — a compile failure prints zero test failures, so "no failures" reads as green if you only look at the test line. 2. `mvn clean install` from `fleetd/` is green, unpiped, with the test count reported. 3. No production behaviour changes. This is extraction plus tests only. 4. If any argument genuinely cannot be covered, say which one and why. An honest gap is fine; a silent one is not. ## Scope note Do not try to cover all of `Fleetd.main`. The scope is the `CompletionResolver` construction and the arguments it takes. Anything else you notice, report in one line and leave alone. Related: #113 (our checkers cover less than they look like they cover).
ltms commented 2026-09-03 07:25:10 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged to main (PR #251). Build: 1228 tests, 0 failures, 0 compile errors.

I re-ran both mutations myself against the merged code, since the whole ticket is about not trusting a green suite:

Mutation Compile errors Result
Drop the worktree lookup — worktreeBranchLookup(sessions::roster) → _ -> null 0 RED — worktreeBranchLookupIsStillPassedAtTheCallSite:60
Drop Unit 5's pair — backendErrorPatterns, backendErrorSink → legacy(), none() 0 RED — backendErrorArgumentsAreStillNamedAtTheCallSite:47

Each mutation reverted and diff -q-confirmed clean. Criterion 1 met.

What was built

Three package-private/public static factories on Fleetd, following the deliverableTo pattern the file already had, each with its own behaviour test:

  • worktreeBranchLookup(Supplier<List<MemberSession>>)
  • backendErrorPatternLookup(Supplier<List<MemberSession>>, Map<String,Pattern>)
  • backendErrorSink(...) — made public, deliberately, so a cross-package test can drive the real production object rather than a hand-mirrored copy of it

Taking a roster Supplier instead of a whole SessionManager is what makes the first two testable with a hand-built list. That is the part that turns "untestable composition root" into an ordinary tested function.

The wiring test, and why a source-text test is the right answer here

FleetdCompletionResolverWiringTest is a plain source-text assertion on Fleetd.java. Every test name and message is prefixed [SOURCE TEXT], and the class javadoc states it never constructs a CompletionResolver and never runs main. That labelling is not decoration — the failure mode this whole ticket is about is a check that looks stronger than it is.

It is not vacuous, and the structure is what makes it sound. Four assertions form a chain:

  1. backendErrorPatterns is assigned from the factory;
  2. backendErrorSink is assigned from the factory;
  3. those variables reach the call site;
  4. the worktree lookup reaches the call site.

Checks 1 and 2 are what stop the obvious cheat — naming a variable backendErrorPatterns while assigning it legacy(). Without them, a source check on the call site alone would pass while the feature was dead.

Known cost, accepted and recorded in the merge commit: the assertions match exact source substrings, so reformatting that statement breaks them. That is the price of covering a main method, and a spurious failure here is loud, immediate and obvious — the right direction to fail.

Follow-up the worker identified and correctly did not do

BackendOutageFlowTest — the test this ticket named as the sharpest example — is still a hand-mirrored copy of the sink logic. It was left alone to stay in scope. Now that Fleetd.backendErrorSink(...) is public, that test can call the real factory and retire the copy for good. Worth doing; it is the actual removal of the problem, where this ticket only made the problem detectable.

Also untouched, and correctly so: exhaustedPatterns and exhaustionSink, the other two of the eight constructor arguments. They are pre-existing local variables rather than inline lambdas, and neither appeared in this ticket's measured mutations. Whether they deserve the same treatment is a separate question, not a gap in this work.

Fixed and merged to `main` (PR #251). Build: **1228 tests, 0 failures, 0 compile errors.** **I re-ran both mutations myself against the merged code**, since the whole ticket is about not trusting a green suite: | Mutation | Compile errors | Result | |---|---|---| | Drop the worktree lookup — `worktreeBranchLookup(sessions::roster)` → `_ -> null` | **0** | **RED** — `worktreeBranchLookupIsStillPassedAtTheCallSite:60` | | Drop Unit 5's pair — `backendErrorPatterns, backendErrorSink` → `legacy(), none()` | **0** | **RED** — `backendErrorArgumentsAreStillNamedAtTheCallSite:47` | Each mutation reverted and `diff -q`-confirmed clean. Criterion 1 met. ## What was built Three package-private/public static factories on `Fleetd`, following the `deliverableTo` pattern the file already had, each with its own behaviour test: - `worktreeBranchLookup(Supplier<List<MemberSession>>)` - `backendErrorPatternLookup(Supplier<List<MemberSession>>, Map<String,Pattern>)` - `backendErrorSink(...)` — made **public**, deliberately, so a cross-package test can drive the real production object rather than a hand-mirrored copy of it Taking a roster `Supplier` instead of a whole `SessionManager` is what makes the first two testable with a hand-built list. That is the part that turns "untestable composition root" into an ordinary tested function. ## The wiring test, and why a source-text test is the right answer here `FleetdCompletionResolverWiringTest` is a plain source-text assertion on `Fleetd.java`. Every test name and message is prefixed `[SOURCE TEXT]`, and the class javadoc states it never constructs a `CompletionResolver` and never runs `main`. That labelling is not decoration — the failure mode this whole ticket is about is a check that looks stronger than it is. **It is not vacuous, and the structure is what makes it sound.** Four assertions form a chain: 1. `backendErrorPatterns` is assigned **from the factory**; 2. `backendErrorSink` is assigned **from the factory**; 3. those variables **reach the call site**; 4. the worktree lookup **reaches the call site**. Checks 1 and 2 are what stop the obvious cheat — naming a variable `backendErrorPatterns` while assigning it `legacy()`. Without them, a source check on the call site alone would pass while the feature was dead. **Known cost, accepted and recorded in the merge commit:** the assertions match exact source substrings, so reformatting that statement breaks them. That is the price of covering a `main` method, and a spurious failure here is loud, immediate and obvious — the right direction to fail. ## Follow-up the worker identified and correctly did not do `BackendOutageFlowTest` — the test this ticket named as the sharpest example — is **still** a hand-mirrored copy of the sink logic. It was left alone to stay in scope. Now that `Fleetd.backendErrorSink(...)` is public, that test can call the real factory and retire the copy for good. Worth doing; it is the actual removal of the problem, where this ticket only made the problem detectable. Also untouched, and correctly so: `exhaustedPatterns` and `exhaustionSink`, the other two of the eight constructor arguments. They are pre-existing local variables rather than inline lambdas, and neither appeared in this ticket's measured mutations. Whether they deserve the same treatment is a separate question, not a gap in this work.
ltms closed this issue 2026-09-03 07:25:13 +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#248
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?