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

FleetdHerdrControlConstructionTest guards Fleetd.java only, but the boot composition moved to FleetdAssembly.java — a real router bypass there is green #642

New Issue
Open
opened 2026-10-02 04:00:58 +02:00 by ltms · 1 comment
ltms commented 2026-10-02 04:00:58 +02:00
Owner
Copy Link
Copy Source

What is wrong

FleetdHerdrControlConstructionTest is the guard that keeps AgentControl and WorkspaceControl behind HerdrRouter. The reason is in its own comment: AgentControl caches paneByTerminal, so the router must be its only production factory. A second instance means a second cache.

The whole test is five lines:

String source = Files.readString(Path.of("src/main/java/dev/ltms/fleet/Fleetd.java"));
assertFalse(source.contains("new AgentControl("));
assertFalse(source.contains("new WorkspaceControl("));

It reads one file. The boot composition it was written to protect now lives in FleetdAssembly.java as well, after the #612 assembly extraction. So the guard watches the file the code used to be in.

FleetdAssembly.java is ready for this to happen: it already imports AgentControl at line 10, it holds a HerdrClient herdr in scope, and AgentControl(HerdrClient) is public. A bypass there is one line and it compiles.

Measured, with a pair — on main = 141ae3b

I ran this in a throwaway detached worktree, so the running daemon's jar was never touched.

The mutant — a real, compiling router bypass inserted into FleetdAssembly.java after :145, where HerdrRouter itself is built:

AgentControl bypassCache = new AgentControl(herdr); // MUTANT: bypasses the router factory

mvn -o test -Dtest=FleetdHerdrControlConstructionTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS      ← the guard does not see it

The control — the same text placed in Fleetd.java instead:

Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
FleetdHerdrControlConstructionTest.fleetdDelegatesStatefulControlsToTheRouter:14
  expected: <false> but was: <true>
BUILD FAILURE      ← the guard does have teeth, for that one file

The control is what makes the green meaningful. Without it, the passing run is only an absence of evidence. With it, the conclusion is exact: the assertion works and its scope is wrong. Anchor counts were 1 before and after each insertion, and git diff --stat was empty after each revert.

Why it is this test and not its neighbour

There are exactly two tests left in the suite that read .java source text, and they are not equally built. The other one, FleetdConfigRefWiringTest, already solved this problem:

assertTrue(source.contains("public final class Fleetd"),
        "fleetdSource() did not read anything usable — ...");

It adds a positive anchor before its negative assertions, labels its display name [SOURCE TEXT], and its javadoc states plainly what a green result does and does not prove. Its own comment gives the reason: "a 'clean' negative check that actually checked nothing."

FleetdHerdrControlConstructionTest has no anchor, no label, and no statement of scope. One of the two learned the lesson; the other did not. That is the gap, and it is why this is worth a ticket rather than a shrug.

Note the two failure modes are different and both matter. A wrong path would throw IOException and fail loudly, so that one is covered. A right path to the wrong file passes silently, and that is the one that is live.

Severity

Latent, not live. I checked where these are actually constructed on 141ae3b:

src/main/java/dev/ltms/fleet/herdr/HerdrRouter.java:20   leadAgents   = new AgentControl(this.lead);
src/main/java/dev/ltms/fleet/herdr/HerdrRouter.java:21   memberAgents = ... new AgentControl(this.member);
src/main/java/dev/ltms/fleet/herdr/HerdrRouter.java:22   leadSpaces   = new WorkspaceControl(this.lead);
src/main/java/dev/ltms/fleet/herdr/HerdrRouter.java:23   memberSpaces = ... new WorkspaceControl(this.member);

Those four are the intended factory, which is the test's premise. Nothing bypasses the router today. The defect is that nothing would stop it, and the place it would most naturally be added is the file the guard cannot see.

Suggested fix

Smallest honest fix: read both Fleetd.java and FleetdAssembly.java, and add a positive anchor per file in the style FleetdConfigRefWiringTest already uses, so a bad read fails loudly rather than proving nothing.

Better, and in the spirit of #612: assert the invariant at runtime instead — drive FleetdAssembly.assembleAndStart and assert that the assembled AgentControl instances are the router's, by identity. A source-text guard cannot survive the next file split, and this ticket exists because that split already happened once.

If the source-text form is kept, give it the [SOURCE TEXT] display-name label and a javadoc sentence saying what it does not prove, matching its neighbour.

Related

  • #612 — the assembly extraction that moved the composition. This is a consequence of that move that the sweep did not cover: #612 lists three "covered by source-text match only" sites as unverified in both directions, and names three tests (FleetdCompletionResolverWiringTest, FleetdBackendQuarantineWiringTest, FleetdLeadRolloverWiringTest) that no longer exist under those names — they were renamed to *AssemblyTest and none of them asserts on source text any more. So #612's standing counter-example needs updating; the real remaining source-text tests are the two named here.
  • FleetdConfigRefWiringTest's javadoc still {@link}s those three removed class names. That is harmless to the build (javadoc is a comment to javac) but the references dangle.
## What is wrong `FleetdHerdrControlConstructionTest` is the guard that keeps `AgentControl` and `WorkspaceControl` behind `HerdrRouter`. The reason is in its own comment: `AgentControl` caches `paneByTerminal`, so the router must be its only production factory. A second instance means a second cache. The whole test is five lines: ```java String source = Files.readString(Path.of("src/main/java/dev/ltms/fleet/Fleetd.java")); assertFalse(source.contains("new AgentControl(")); assertFalse(source.contains("new WorkspaceControl(")); ``` It reads **one** file. The boot composition it was written to protect now lives in **`FleetdAssembly.java`** as well, after the #612 assembly extraction. So the guard watches the file the code used to be in. `FleetdAssembly.java` is ready for this to happen: it already imports `AgentControl` at line 10, it holds a `HerdrClient herdr` in scope, and `AgentControl(HerdrClient)` is public. A bypass there is one line and it compiles. ## Measured, with a pair — on `main` = `141ae3b` I ran this in a throwaway detached worktree, so the running daemon's jar was never touched. **The mutant** — a real, compiling router bypass inserted into `FleetdAssembly.java` after `:145`, where `HerdrRouter` itself is built: ```java AgentControl bypassCache = new AgentControl(herdr); // MUTANT: bypasses the router factory ``` ``` mvn -o test -Dtest=FleetdHerdrControlConstructionTest Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ← the guard does not see it ``` **The control** — the same text placed in `Fleetd.java` instead: ``` Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 FleetdHerdrControlConstructionTest.fleetdDelegatesStatefulControlsToTheRouter:14 expected: <false> but was: <true> BUILD FAILURE ← the guard does have teeth, for that one file ``` The control is what makes the green meaningful. Without it, the passing run is only an absence of evidence. With it, the conclusion is exact: **the assertion works and its scope is wrong.** Anchor counts were 1 before and after each insertion, and `git diff --stat` was empty after each revert. ## Why it is this test and not its neighbour There are exactly two tests left in the suite that read `.java` source text, and they are not equally built. The other one, `FleetdConfigRefWiringTest`, already solved this problem: ```java assertTrue(source.contains("public final class Fleetd"), "fleetdSource() did not read anything usable — ..."); ``` It adds a positive anchor **before** its negative assertions, labels its display name `[SOURCE TEXT]`, and its javadoc states plainly what a green result does and does not prove. Its own comment gives the reason: "a 'clean' negative check that actually checked nothing." `FleetdHerdrControlConstructionTest` has no anchor, no label, and no statement of scope. One of the two learned the lesson; the other did not. That is the gap, and it is why this is worth a ticket rather than a shrug. Note the two failure modes are different and both matter. A *wrong path* would throw `IOException` and fail loudly, so that one is covered. A *right path to the wrong file* passes silently, and that is the one that is live. ## Severity **Latent, not live.** I checked where these are actually constructed on `141ae3b`: ``` src/main/java/dev/ltms/fleet/herdr/HerdrRouter.java:20 leadAgents = new AgentControl(this.lead); src/main/java/dev/ltms/fleet/herdr/HerdrRouter.java:21 memberAgents = ... new AgentControl(this.member); src/main/java/dev/ltms/fleet/herdr/HerdrRouter.java:22 leadSpaces = new WorkspaceControl(this.lead); src/main/java/dev/ltms/fleet/herdr/HerdrRouter.java:23 memberSpaces = ... new WorkspaceControl(this.member); ``` Those four are the intended factory, which is the test's premise. Nothing bypasses the router today. The defect is that nothing would stop it, and the place it would most naturally be added is the file the guard cannot see. ## Suggested fix Smallest honest fix: read **both** `Fleetd.java` and `FleetdAssembly.java`, and add a positive anchor per file in the style `FleetdConfigRefWiringTest` already uses, so a bad read fails loudly rather than proving nothing. Better, and in the spirit of #612: assert the invariant at runtime instead — drive `FleetdAssembly.assembleAndStart` and assert that the assembled `AgentControl` instances are the router's, by identity. A source-text guard cannot survive the next file split, and this ticket exists because that split already happened once. If the source-text form is kept, give it the `[SOURCE TEXT]` display-name label and a javadoc sentence saying what it does not prove, matching its neighbour. ## Related - #612 — the assembly extraction that moved the composition. This is a consequence of that move that the sweep did not cover: #612 lists three "covered by source-text match only" sites as unverified in both directions, and names three tests (`FleetdCompletionResolverWiringTest`, `FleetdBackendQuarantineWiringTest`, `FleetdLeadRolloverWiringTest`) that **no longer exist under those names** — they were renamed to `*AssemblyTest` and none of them asserts on source text any more. So #612's standing counter-example needs updating; the real remaining source-text tests are the two named here. - `FleetdConfigRefWiringTest`'s javadoc still `{@link}`s those three removed class names. That is harmless to the build (javadoc is a comment to `javac`) but the references dangle.
ltms commented 2026-10-02 04:01:56 +02:00
Author
Owner
Copy Link
Copy Source

Correction to my own "suggested fix", and credit where it is due

Two things to fix in the issue above.

1. This was already measured on 2026-09-22, and I did not say so. A previous lead planted the guarded text into FleetdAssembly.java and ran this test: tests="1" failures="0" — green — then reverted. So the blindness was known a week and a half ago. What my run adds is narrower than "a finding": a real compiling router bypass rather than planted text, and a paired control in Fleetd.java proving the assertion has teeth for the file it does read. Useful confirmation, not a discovery. The gap this ticket genuinely closes is that the measurement had no ticket of its own until now.

2. My "smallest honest fix" contradicts a standing lead ruling, so withdraw it. I wrote that the smallest fix is to read both Fleetd.java and FleetdAssembly.java with a positive anchor per file. There is a lead ruling on #612 from 2026-09-22 that says the opposite, and it is right:

Do not re-point a source assertion at the file its subject moved to. It still freezes spelling, proves nothing about reaching the live consumer, and gets re-pointed again by the next refactor. Replace it behaviourally instead.

All three objections apply here. Re-pointing would also have to be re-pointed again the next time the composition is split — and this ticket exists precisely because that split already happened once. So the two-file version is not a smaller fix, it is a shorter-lived one.

The fix is the behavioural one only: drive FleetdAssembly.assembleAndStart and assert by identity that the assembled AgentControl and WorkspaceControl instances are the router's. That is a real pin — it survives a file move, and it tests the property the comment actually claims, which is that the router is the only production factory. The cache is the reason the rule exists, so identity is the right assertion.

One consequence worth stating plainly, because it is the part that usually gets skipped: by the same #612 ruling, the source-text test may only be deleted in the commit that adds a behavioural replacement which is red today — shown by reintroducing the bypass and watching the named new test fail. A deleted assertion and a superseded assertion look identical in a diff, and only that demonstration tells them apart.

Also withdrawn: the fallback I offered of keeping the source-text form with a [SOURCE TEXT] label. That was me hedging toward the cheap option. FleetdConfigRefWiringTest earns its source-text form because it pins a choice between two constructor shapes that has no runtime observable short of booting a daemon. This test has an obvious runtime observable — object identity — so it has no such excuse.

## Correction to my own "suggested fix", and credit where it is due Two things to fix in the issue above. **1. This was already measured on 2026-09-22, and I did not say so.** A previous lead planted the guarded text into `FleetdAssembly.java` and ran this test: `tests="1" failures="0"` — green — then reverted. So the blindness was known a week and a half ago. What my run adds is narrower than "a finding": a **real compiling router bypass** rather than planted text, and a **paired control** in `Fleetd.java` proving the assertion has teeth for the file it does read. Useful confirmation, not a discovery. The gap this ticket genuinely closes is that the measurement had no ticket of its own until now. **2. My "smallest honest fix" contradicts a standing lead ruling, so withdraw it.** I wrote that the smallest fix is to read **both** `Fleetd.java` and `FleetdAssembly.java` with a positive anchor per file. There is a lead ruling on #612 from 2026-09-22 that says the opposite, and it is right: > Do **not** re-point a source assertion at the file its subject moved to. It still freezes spelling, proves nothing about reaching the live consumer, and gets re-pointed again by the next refactor. Replace it behaviourally instead. All three objections apply here. Re-pointing would also have to be re-pointed *again* the next time the composition is split — and this ticket exists precisely because that split already happened once. So **the two-file version is not a smaller fix, it is a shorter-lived one.** **The fix is the behavioural one only:** drive `FleetdAssembly.assembleAndStart` and assert by identity that the assembled `AgentControl` and `WorkspaceControl` instances are the router's. That is a real pin — it survives a file move, and it tests the property the comment actually claims, which is that the router is the *only* production factory. The cache is the reason the rule exists, so identity is the right assertion. One consequence worth stating plainly, because it is the part that usually gets skipped: by the same #612 ruling, **the source-text test may only be deleted in the commit that adds a behavioural replacement which is red today** — shown by reintroducing the bypass and watching the named new test fail. A deleted assertion and a superseded assertion look identical in a diff, and only that demonstration tells them apart. Also withdrawn: the fallback I offered of keeping the source-text form with a `[SOURCE TEXT]` label. That was me hedging toward the cheap option. `FleetdConfigRefWiringTest` earns its source-text form because it pins a *choice between two constructor shapes* that has no runtime observable short of booting a daemon. This test has an obvious runtime observable — object identity — so it has no such excuse.
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#642
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?