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

The lead-tab collision guard's case-insensitivity is load-bearing and unpinned — equalsIgnoreCase→equals survives #693

New Issue
Closed
opened 2026-10-03 22:47:29 +02:00 by ltms · 1 comment
ltms commented 2026-10-03 22:47:29 +02:00
Owner
Copy Link
Copy Source

Found by me while verifying PR #691 (#677). Not a defect in shipped behaviour — the code is correct. It is a test-coverage gap on a behaviour the runtime depends on.

The mutation

On the merged tree (main at bfee23a), FleetConfig.java:2746:

if (a.tab().equalsIgnoreCase(b.tab())) {

I changed it to a.tab().equals(b.tab()) in a throwaway worktree and ran mvn -o -Dtest=FleetConfigTest test:

Tests run: 155, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

The mutation survived. Restored afterwards; git diff on the file came back empty.

The PR's own mutation control was sound for the line it targeted — the worker set the condition to false && … and its collision test went RED. That proves the condition is reached. It does not touch the comparison's case behaviour, which is a separate claim.

Why this is not just cosmetic

The method's own javadoc advertises the behaviour:

or when two fleet.leaders entries carry the same exact tab (case-insensitively)

And the runtime genuinely matches that way. LeadTabScanner.java:145 builds the tab→name map keyed on the lowercased label:

out.put(tab.strip().toLowerCase(Locale.ROOT), name);

and :254 looks up through the same lowercasing:

return tabToName.get(PendingCloseMarker.strip(label).toLowerCase(Locale.ROOT));

Because that is a Map.put, two lead tabs differing only in case collide and one silently overwrites the other. That is exactly the harm the guard's own refusal message describes: "only one of two leads sharing a tab can ever be found — the other is silently unreachable."

So equalsIgnoreCase is the correct comparison, and it is correct because of LeadTabScanner's lowercasing. Swap it for equals and the guard stops catching the real collision while every test stays green. The project rule applies directly: a comment claiming an invariant is a free test case.

What to do

Add a case to FleetConfigTest: two fleet.leaders entries whose tab values differ only in case must refuse startup.

Two controls are needed, or the test can pass for the wrong reason:

  1. The existing same-case collision test must stay green — this one must add a case, not widen an existing assertion into something that no longer distinguishes the two.
  2. Mutating equalsIgnoreCase → equals must make the new test RED and leave the same-case one green. That is what proves the new test pins the case behaviour specifically rather than just re-covering the collision.

A second, cheaper option worth considering instead

The duplication is the real smell. FleetConfig lowercases with equalsIgnoreCase; LeadTabScanner lowercases with toLowerCase(Locale.ROOT). Two places decide what "the same tab" means, and they have to agree for the guard to be right. One shared normaliser — something like Tabs.key(String) — would make the agreement structural instead of a coincidence two tests have to defend separately.

I have not checked whether equalsIgnoreCase and toLowerCase(Locale.ROOT) actually agree on every input. They differ for some locale-sensitive characters, and equalsIgnoreCase is locale-independent while toLowerCase(Locale.ROOT) is pinned to root. For ASCII tab labels they agree. I did not look for a label where they do not, and I am not claiming one exists.

Not measured

I did not boot a daemon with two case-differing lead tabs to watch one become unreachable. The overwrite is a reading of LeadTabScanner.java:145 being a Map.put on a lowercased key, not an observed loss of a lead.

Found by me while verifying PR #691 (#677). Not a defect in shipped behaviour — the code is correct. It is a test-coverage gap on a behaviour the runtime depends on. ## The mutation On the merged tree (`main` at `bfee23a`), `FleetConfig.java:2746`: ```java if (a.tab().equalsIgnoreCase(b.tab())) { ``` I changed it to `a.tab().equals(b.tab())` in a throwaway worktree and ran `mvn -o -Dtest=FleetConfigTest test`: ``` Tests run: 155, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` **The mutation survived.** Restored afterwards; `git diff` on the file came back empty. The PR's own mutation control was sound for the line it targeted — the worker set the condition to `false && …` and its collision test went RED. That proves the condition is reached. It does not touch the comparison's case behaviour, which is a separate claim. ## Why this is not just cosmetic The method's own javadoc advertises the behaviour: > or when two `fleet.leaders` entries carry the same exact `tab` (case-insensitively) And the runtime genuinely matches that way. `LeadTabScanner.java:145` builds the tab→name map keyed on the lowercased label: ```java out.put(tab.strip().toLowerCase(Locale.ROOT), name); ``` and `:254` looks up through the same lowercasing: ```java return tabToName.get(PendingCloseMarker.strip(label).toLowerCase(Locale.ROOT)); ``` Because that is a `Map.put`, two lead tabs differing only in case **collide and one silently overwrites the other**. That is exactly the harm the guard's own refusal message describes: "only one of two leads sharing a tab can ever be found — the other is silently unreachable." So `equalsIgnoreCase` is the correct comparison, and it is correct *because of* `LeadTabScanner`'s lowercasing. Swap it for `equals` and the guard stops catching the real collision while every test stays green. The project rule applies directly: a comment claiming an invariant is a free test case. ## What to do Add a case to `FleetConfigTest`: two `fleet.leaders` entries whose `tab` values differ **only** in case must refuse startup. Two controls are needed, or the test can pass for the wrong reason: 1. The existing same-case collision test must stay green — this one must add a case, not widen an existing assertion into something that no longer distinguishes the two. 2. Mutating `equalsIgnoreCase` → `equals` must make the **new** test RED and leave the same-case one green. That is what proves the new test pins the case behaviour specifically rather than just re-covering the collision. ## A second, cheaper option worth considering instead The duplication is the real smell. `FleetConfig` lowercases with `equalsIgnoreCase`; `LeadTabScanner` lowercases with `toLowerCase(Locale.ROOT)`. Two places decide what "the same tab" means, and they have to agree for the guard to be right. One shared normaliser — something like `Tabs.key(String)` — would make the agreement structural instead of a coincidence two tests have to defend separately. I have **not** checked whether `equalsIgnoreCase` and `toLowerCase(Locale.ROOT)` actually agree on every input. They differ for some locale-sensitive characters, and `equalsIgnoreCase` is locale-independent while `toLowerCase(Locale.ROOT)` is pinned to root. For ASCII tab labels they agree. I did not look for a label where they do not, and I am not claiming one exists. ## Not measured I did not boot a daemon with two case-differing lead tabs to watch one become unreachable. The overwrite is a reading of `LeadTabScanner.java:145` being a `Map.put` on a lowercased key, not an observed loss of a lead.
ltms commented 2026-10-03 22:54:28 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged. Closing.

PR #695 merged to main as d0688c8, pushed (bfee23a..d0688c8). Test-only — git diff --stat origin/main...refs/pull/695/head -- fleetd/src/main/java is empty, so no production code moved.

New test: FleetConfigTest.twoLeadsSharingTheSameTabInDifferentCaseRefusesToStart, using tabs "Shared Tab" and "shared tab".

I re-ran the mutation myself, and got the split result this ticket asked for

mvn -o -Dtest=FleetConfigTest test with FleetConfig.java:2746 mutated from equalsIgnoreCase to equals:

[ERROR] Tests run: 156, Failures: 1, Errors: 0, Skipped: 0 <<< FAILURE! -- in dev.ltms.fleet.config.FleetConfigTest
[ERROR] dev.ltms.fleet.config.FleetConfigTest.twoLeadsSharingTheSameTabInDifferentCaseRefusesToStart(Path) <<< FAILURE!
BUILD FAILURE

Exactly one failure, and it is the new test. twoLeadsSharingTheSameExactTabRefusesToStart is absent from the failure list, so the same-case test stayed GREEN. That is the split this ticket required: the new test pins the case behaviour, not merely the collision. Unmutated, the same command gives 156 tests, 0 failures.

Restored afterwards; git diff on FleetConfig.java came back empty.

Build

Trial-merged onto origin/main in a throwaway worktree, rm -rf target/surefire-reports, mvn -o clean install: 1946 tests, 0 failures, BUILD SUCCESS, exit=0, 173 *.xml report files. 1945 + 1 = 1946. After merging, main's tree hash 00a281044ae8bac3485e13e3c574be94933c7c0d equals the tree I built.

One number that looked like a disagreement and was not

The worker reported 346 report files; I had been reporting 173. Both are right — surefire writes a .txt beside every .xml:

 173 txt
 173 xml

I counted *.xml, it counted every file. Worth recording because it is the "compare like with like" trap in miniature: two correct commands answering different questions read as a contradiction.

Not done, and deliberately

I did not take the shared-normaliser option this ticket floated as the cheaper fix. FleetConfig still compares with equalsIgnoreCase while LeadTabScanner normalises with toLowerCase(Locale.ROOT), so two places still decide what "the same tab" means and they agree by convention rather than by construction. The test now defends that agreement, which was the gap; unifying them is a design change I did not fold into a coverage fix.

I also still have not checked whether those two ever disagree on a real input. They agree for ASCII. I am not claiming a counterexample exists, only that I did not look.

## Fixed and merged. Closing. PR #695 merged to `main` as `d0688c8`, pushed (`bfee23a..d0688c8`). Test-only — `git diff --stat origin/main...refs/pull/695/head -- fleetd/src/main/java` is empty, so no production code moved. New test: `FleetConfigTest.twoLeadsSharingTheSameTabInDifferentCaseRefusesToStart`, using tabs `"Shared Tab"` and `"shared tab"`. ## I re-ran the mutation myself, and got the split result this ticket asked for `mvn -o -Dtest=FleetConfigTest test` with `FleetConfig.java:2746` mutated from `equalsIgnoreCase` to `equals`: ``` [ERROR] Tests run: 156, Failures: 1, Errors: 0, Skipped: 0 <<< FAILURE! -- in dev.ltms.fleet.config.FleetConfigTest [ERROR] dev.ltms.fleet.config.FleetConfigTest.twoLeadsSharingTheSameTabInDifferentCaseRefusesToStart(Path) <<< FAILURE! BUILD FAILURE ``` **Exactly one failure, and it is the new test.** `twoLeadsSharingTheSameExactTabRefusesToStart` is absent from the failure list, so the same-case test stayed GREEN. That is the split this ticket required: the new test pins the *case* behaviour, not merely the collision. Unmutated, the same command gives 156 tests, 0 failures. Restored afterwards; `git diff` on `FleetConfig.java` came back empty. ## Build Trial-merged onto `origin/main` in a throwaway worktree, `rm -rf target/surefire-reports`, `mvn -o clean install`: **1946 tests, 0 failures, BUILD SUCCESS**, `exit=0`, 173 `*.xml` report files. 1945 + 1 = 1946. After merging, `main`'s tree hash `00a281044ae8bac3485e13e3c574be94933c7c0d` equals the tree I built. ## One number that looked like a disagreement and was not The worker reported 346 report files; I had been reporting 173. Both are right — surefire writes a `.txt` beside every `.xml`: ``` 173 txt 173 xml ``` I counted `*.xml`, it counted every file. Worth recording because it is the "compare like with like" trap in miniature: two correct commands answering different questions read as a contradiction. ## Not done, and deliberately I did **not** take the shared-normaliser option this ticket floated as the cheaper fix. `FleetConfig` still compares with `equalsIgnoreCase` while `LeadTabScanner` normalises with `toLowerCase(Locale.ROOT)`, so two places still decide what "the same tab" means and they agree by convention rather than by construction. The test now defends that agreement, which was the gap; unifying them is a design change I did not fold into a coverage fix. I also still have **not** checked whether those two ever disagree on a real input. They agree for ASCII. I am not claiming a counterexample exists, only that I did not look.
ltms closed this issue 2026-10-03 22:54:31 +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#693
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?