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

No validator compares a lead's exact tab to a member label — the collision guard checks the vestigial tabPrefix #677

New Issue
Closed
opened 2026-10-03 20:59:19 +02:00 by ltms · 2 comments
ltms commented 2026-10-03 20:59:19 +02:00
Owner
Copy Link
Copy Source

Found by an architect working #669 and confirmed by me in the main clone at 6f27522. Pre-existing, and independent of #669 — this is about the lead namespace as it ships today.

What the guard checks, and what identity uses

validateLeadTabPrefixes() exists to stop a member being labelled so that it reads back as a lead. Its own error message says so:

Every member labelled that way would be read back as a lead and granted spawn/stop/send on the whole fleet.

But it compares against leader.tabPrefix():

String prefix = leader.tabPrefix();
if (startsWithIgnoreCase(fleet.tabLabel(), prefix)) { … }
profiles().entrySet().stream()
        .filter(e -> startsWithIgnoreCase(e.getValue().tabLabel(), prefix))

Identity does not use tabPrefix. It uses the exact tab. tabPrefix is vestigial — FleetConfig.java:1132-1136 says so in its own words: "no longer used to find a lead's tab — tab is matched exactly. Its only remaining job is the startup collision guard." It defaults to "lead:".

I checked every place FleetConfig.java reads a leader's real tab:

$ grep -n '\.tab()' fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java
2760:   .anyMatch(leader -> leader != null && leader.tab() != null && !leader.tab().isBlank());
2927:   if (leader.tab() == null || leader.tab().isBlank()) {

Line 2760 is the early return inside validatePanePlacementAgainstLeadTabs() — it only asks whether any lead has a tab, never what it is. Line 2927 is validateMembers() checking a leader names a tab at all.

Positive control, so the empty result above is not a broken pattern: the same file has 1 use of tabPrefix(), and grep -n 'public void validate' … lists all eight validators. I read each. None compares a lead's exact tab to a member label.

The reachable configuration

fleet:
  tabLabel: "alpha"          # or any profile tabLabel override
  leaders:
    alpha:
      tab: "alpha"           # identity is matched on THIS, exactly, case-insensitively

tabPrefix defaults to "lead:". "alpha" does not start with "lead:", so validateLeadTabPrefixes() passes. Nothing else looks. The daemon starts.

A member labelled alpha then occupies a tab whose exact label is a configured lead tab, and CallerResolver.resolve() checks the lead map before the worker fallback with no SessionManager lookup:

211: String lead = leadTerminals.get().get(c.terminal());
217:     return Principal.leader(lead, c.terminal(), c.pid());
...
228: return Principal.worker(c.terminal(), c.pid());

Principal.leader(...) is Role.PRIMARY, which Authz.java:72 grants SPAWN, STOP, DRAIN and HANDOVER.

Why #661 does not cover this

#661 added validatePanePlacementAgainstLeadTabs(), which refuses placement: pane while any lead names a tab. That closes the route where a pane-placed member lands in a lead's tab. It does not look at labels at all, so a placement: tab member whose rendered label happens to equal a lead's exact tab goes straight through.

I confirmed #661 changed no resolver code: git diff --stat f288cee~1 b4b7cf5 lists FleetConfig.java, LeadTabScanner.java, LeadContextGauge.java, LeadLauncher.java and tests. CallerResolver.java is not among them.

This is the one-way-gate shape again: a guard added after an incident closes the direction that incident came from.

Not exploitable on this fleet right now, and why that is not reassuring

I checked the live fleetd/fleetd.yaml. All eight profiles use placement: tab, and the current tabLabel template does not render to any configured lead tab, so no member is mislabelled today. The hole needs a specific operator配置 mistake to open — but it is a mistake nothing warns about, and the consequence is a worker holding primary rights.

The fix

Add an exact-label collision check: refuse at startup when fleet.tabLabel, or any profile tabLabel override, can render to a configured lead's exact tab. Placeholders in the template ({role}, {profile}, {n}) must be treated as wildcards, because the rendered value is what the scanner matches, not the template.

Then decide what tabPrefix is for. If the exact check supersedes it, remove it rather than leaving two guards where one is the real one — a reader who sees validateLeadTabPrefixes() reasonably assumes the collision case is handled.

Acceptance criteria

Properties under a change, not names of constructs:

  1. Setting fleet.tabLabel (or any profile override) to a value that renders to a configured lead's exact tab makes validateAll() refuse startup, and the message names both sides of the collision. Changing either side makes it start.
  2. A template containing placeholders that can render to a lead tab is refused; one that cannot is accepted. Give both cases, so the test is not satisfied by refusing everything.
  3. The new validator is reached by validateAll() with no edit to validateAll() itself — invokeAllValidators is a reflective sweep. Add the case to the reachability enumeration in FleetConfigValidateAllTest, which #668 just made the single place for that.
  4. The existing tabPrefix behaviour either still holds or is deliberately removed. If removed, say in the commit message which line it used to pin and why that can no longer go wrong.

Related, and the stronger fix

#669's Unit D proposes that CallerResolver look up SessionManager before any tab map, and return a spawned member's own role without consulting lead or collaborator tabs. That would close this at the resolver, which is where it actually bites, and turn every config validator here into defence in depth. If that unit lands first, this ticket shrinks to the startup warning.

Not verified by me

Whether the same gap exists for architect slot labels. I looked at the lead namespace because #669 took me there. I did not trace whether a member label can collide with an architect slot's tab.

Found by an architect working #669 and confirmed by me in the main clone at `6f27522`. **Pre-existing, and independent of #669** — this is about the lead namespace as it ships today. ## What the guard checks, and what identity uses `validateLeadTabPrefixes()` exists to stop a member being labelled so that it reads back as a lead. Its own error message says so: > Every member labelled that way would be read back as a lead and granted spawn/stop/send on the whole fleet. But it compares against `leader.tabPrefix()`: ```java String prefix = leader.tabPrefix(); if (startsWithIgnoreCase(fleet.tabLabel(), prefix)) { … } profiles().entrySet().stream() .filter(e -> startsWithIgnoreCase(e.getValue().tabLabel(), prefix)) ``` **Identity does not use `tabPrefix`. It uses the exact `tab`.** `tabPrefix` is vestigial — `FleetConfig.java:1132-1136` says so in its own words: *"no longer used to find a lead's tab — `tab` is matched exactly. Its only remaining job is the startup collision guard."* It defaults to `"lead:"`. I checked every place `FleetConfig.java` reads a leader's real tab: ``` $ grep -n '\.tab()' fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java 2760: .anyMatch(leader -> leader != null && leader.tab() != null && !leader.tab().isBlank()); 2927: if (leader.tab() == null || leader.tab().isBlank()) { ``` Line 2760 is the early return inside `validatePanePlacementAgainstLeadTabs()` — it only asks *whether any* lead has a tab, never *what* it is. Line 2927 is `validateMembers()` checking a leader names a tab at all. Positive control, so the empty result above is not a broken pattern: the same file has **1** use of `tabPrefix()`, and `grep -n 'public void validate' …` lists all eight validators. I read each. **None compares a lead's exact `tab` to a member label.** ## The reachable configuration ```yaml fleet: tabLabel: "alpha" # or any profile tabLabel override leaders: alpha: tab: "alpha" # identity is matched on THIS, exactly, case-insensitively ``` `tabPrefix` defaults to `"lead:"`. `"alpha"` does not start with `"lead:"`, so `validateLeadTabPrefixes()` passes. Nothing else looks. The daemon starts. A member labelled `alpha` then occupies a tab whose exact label is a configured lead tab, and `CallerResolver.resolve()` checks the lead map before the worker fallback with **no** `SessionManager` lookup: ``` 211: String lead = leadTerminals.get().get(c.terminal()); 217: return Principal.leader(lead, c.terminal(), c.pid()); ... 228: return Principal.worker(c.terminal(), c.pid()); ``` `Principal.leader(...)` is `Role.PRIMARY`, which `Authz.java:72` grants `SPAWN`, `STOP`, `DRAIN` and `HANDOVER`. ## Why #661 does not cover this #661 added `validatePanePlacementAgainstLeadTabs()`, which refuses `placement: pane` while any lead names a tab. That closes the route where a *pane-placed* member lands in a lead's tab. It does not look at labels at all, so a `placement: tab` member whose rendered label happens to equal a lead's exact tab goes straight through. I confirmed #661 changed no resolver code: `git diff --stat f288cee~1 b4b7cf5` lists `FleetConfig.java`, `LeadTabScanner.java`, `LeadContextGauge.java`, `LeadLauncher.java` and tests. `CallerResolver.java` is not among them. This is the one-way-gate shape again: a guard added after an incident closes the direction that incident came from. ## Not exploitable on this fleet right now, and why that is not reassuring I checked the live `fleetd/fleetd.yaml`. All eight profiles use `placement: tab`, and the current `tabLabel` template does not render to any configured lead tab, so no member is mislabelled today. The hole needs a specific operator配置 mistake to open — but it is a mistake nothing warns about, and the consequence is a worker holding primary rights. ## The fix Add an exact-label collision check: refuse at startup when `fleet.tabLabel`, or any profile `tabLabel` override, can render to a configured lead's exact `tab`. Placeholders in the template (`{role}`, `{profile}`, `{n}`) must be treated as wildcards, because the rendered value is what the scanner matches, not the template. Then decide what `tabPrefix` is for. If the exact check supersedes it, remove it rather than leaving two guards where one is the real one — a reader who sees `validateLeadTabPrefixes()` reasonably assumes the collision case is handled. ## Acceptance criteria Properties under a change, not names of constructs: 1. Setting `fleet.tabLabel` (or any profile override) to a value that renders to a configured lead's exact `tab` makes `validateAll()` refuse startup, and the message names both sides of the collision. Changing either side makes it start. 2. A template containing placeholders that **can** render to a lead tab is refused; one that cannot is accepted. Give both cases, so the test is not satisfied by refusing everything. 3. The new validator is reached by `validateAll()` with no edit to `validateAll()` itself — `invokeAllValidators` is a reflective sweep. Add the case to the reachability enumeration in `FleetConfigValidateAllTest`, which #668 just made the single place for that. 4. The existing `tabPrefix` behaviour either still holds or is deliberately removed. If removed, say in the commit message which line it used to pin and why that can no longer go wrong. ## Related, and the stronger fix #669's Unit D proposes that `CallerResolver` look up `SessionManager` before any tab map, and return a spawned member's own role without consulting lead or collaborator tabs. **That would close this at the resolver**, which is where it actually bites, and turn every config validator here into defence in depth. If that unit lands first, this ticket shrinks to the startup warning. ## Not verified by me Whether the same gap exists for architect slot labels. I looked at the lead namespace because #669 took me there. I did not trace whether a member label can collide with an architect slot's tab.
ltms commented 2026-10-03 22:26:45 +02:00
Author
Owner
Copy Link
Copy Source

Lead review of PR #686 — confirmed defect, high. Do not merge as is

PR #686 extends validateLeadTabPrefixes() instead of adding a new validator. That choice is fine,
and it means validateAll() needs no change because its reflective sweep already reaches the
method. I checked the test deletion too and it is safe (see the note at the end).

But the validator misses the main case the ticket asked for.

What is wrong

I read FleetConfig.java:2693-2721 myself. The loop walks fleet.leaders(), and for each lead it
compares that lead's tab and tabPrefix against two things only:

  • fleet.tabLabel(), the member label template
  • each profile's tabLabel() override

It never compares one lead's tab against another lead's tab. So two leads configured with
the same exact tab label pass validation.

That is the case this ticket exists for. The tab label is the only test that decides whether a
session in a pane is read back as a lead, so two leads sharing one tab is the collision that
matters most here. It is also the state that blocked #359.

Measured on the PR branch

I made a throwaway worktree on refs/remotes/pr/686 (cb4a686) and added my own test with a
control:

bind: {port: 8080}
fleet:
  leaders:
    opus:   {tab: "shared tab"}
    sonnet: {tab: "shared tab"}

Result of mvn -o -Dtest=LeadAdjudicationProbeTest test:

Tests run: 2, Failures: 1, Errors: 0, Skipped: 0
twoLeadsSharingOneExactTabMustBeRefused ==> Expected java.lang.IllegalStateException to be thrown, but nothing was thrown.

The control in the same run, two leads with distinct tabs, passed. So the validator does run
and does accept valid configs. The failure is the validator accepting the collision, not a broken
fixture.

I then removed my probe test, and git status was clean.

A reviewer I put on the validator found the same thing on its own, from the code, and rated it
high. We agree, and we reached it by different routes: it read the comparison set, I ran a config
through it.

What the fix needs

  1. Add a pass that collects every leader.tab() and refuses when two leads share one. Compare
    case-insensitively, the same way startsWithIgnoreCase already does for prefixes. Keep this
    independent of the member-template checks, because it is a different relation.
  2. Decide and state what happens for a lead tab that collides with another lead's tabPrefix.
    My reading is that this should also be refused, but the ticket did not ask for it, so say which
    way you went and why.
  3. Two tests, each with a control: two leads with the same tab must be refused; two leads with
    different tabs must load. Show both.
  4. Mutation: disable the new pass and show the collision test goes RED. Then restore it.
  5. Keep the existing exact-tab and tabPrefix checks and their tests green.

On the deleted test lines — no problem found

I checked the 76 removed lines in FleetConfigValidateAllTest.java myself rather than taking the
report. 58 are comment lines and 3 are code. No @Test and no assertion was removed, and the test
method count is 7 before and 7 after. The 3 code lines are an assertion message reword and the
fixture changing from a template scenario (tabLabel: "lead: {role} {profile}" / tab: "lead: opus")
to a literal one (alpha / alpha).

The reachability property is still pinned. A reviewer mutated invokeAllValidators to skip
validateLeadTabPrefixes and the sweep test went RED with
lead-tab-prefixes.yaml: validateAll() must refuse this config.

One thing to keep in mind while fixing the above: the fixture swap moved this class's case from a
template collision to a literal one. That is acceptable for a reachability test, but please make
sure the template-vs-exact collision stays covered by a test in FleetConfigTest.

## Lead review of PR #686 — confirmed defect, high. Do not merge as is PR #686 extends `validateLeadTabPrefixes()` instead of adding a new validator. That choice is fine, and it means `validateAll()` needs no change because its reflective sweep already reaches the method. I checked the test deletion too and it is safe (see the note at the end). But the validator misses the main case the ticket asked for. ### What is wrong I read `FleetConfig.java:2693-2721` myself. The loop walks `fleet.leaders()`, and for each lead it compares that lead's `tab` and `tabPrefix` against two things only: - `fleet.tabLabel()`, the member label template - each profile's `tabLabel()` override It never compares one lead's `tab` against **another lead's** `tab`. So two leads configured with the same exact tab label pass validation. That is the case this ticket exists for. The tab label is the only test that decides whether a session in a pane is read back as a lead, so two leads sharing one tab is the collision that matters most here. It is also the state that blocked #359. ### Measured on the PR branch I made a throwaway worktree on `refs/remotes/pr/686` (`cb4a686`) and added my own test with a control: ```yaml bind: {port: 8080} fleet: leaders: opus: {tab: "shared tab"} sonnet: {tab: "shared tab"} ``` Result of `mvn -o -Dtest=LeadAdjudicationProbeTest test`: ``` Tests run: 2, Failures: 1, Errors: 0, Skipped: 0 twoLeadsSharingOneExactTabMustBeRefused ==> Expected java.lang.IllegalStateException to be thrown, but nothing was thrown. ``` The control in the same run, two leads with **distinct** tabs, passed. So the validator does run and does accept valid configs. The failure is the validator accepting the collision, not a broken fixture. I then removed my probe test, and `git status` was clean. A reviewer I put on the validator found the same thing on its own, from the code, and rated it high. We agree, and we reached it by different routes: it read the comparison set, I ran a config through it. ### What the fix needs 1. Add a pass that collects every `leader.tab()` and refuses when two leads share one. Compare case-insensitively, the same way `startsWithIgnoreCase` already does for prefixes. Keep this independent of the member-template checks, because it is a different relation. 2. Decide and state what happens for a lead `tab` that collides with another lead's `tabPrefix`. My reading is that this should also be refused, but the ticket did not ask for it, so say which way you went and why. 3. Two tests, each with a control: two leads with the same tab must be refused; two leads with different tabs must load. Show both. 4. Mutation: disable the new pass and show the collision test goes RED. Then restore it. 5. Keep the existing exact-tab and `tabPrefix` checks and their tests green. ### On the deleted test lines — no problem found I checked the 76 removed lines in `FleetConfigValidateAllTest.java` myself rather than taking the report. 58 are comment lines and 3 are code. No `@Test` and no assertion was removed, and the test method count is 7 before and 7 after. The 3 code lines are an assertion message reword and the fixture changing from a template scenario (`tabLabel: "lead: {role} {profile}"` / `tab: "lead: opus"`) to a literal one (`alpha` / `alpha`). The reachability property is still pinned. A reviewer mutated `invokeAllValidators` to skip `validateLeadTabPrefixes` and the sweep test went RED with `lead-tab-prefixes.yaml: validateAll() must refuse this config`. One thing to keep in mind while fixing the above: the fixture swap moved this class's case from a template collision to a literal one. That is acceptable for a reachability test, but please make sure the template-vs-exact collision stays covered by a test in `FleetConfigTest`.
ltms commented 2026-10-03 22:46:41 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged. Closing.

PR #691 merged to main as bfee23a, pushed (7dec74f..bfee23a). It supersedes PR #686, which I closed unmerged — #691 carries #686's commit plus the exact-tab collision guard on top.

Two things landed together:

  • templateCanRenderAs(...) — the replacement collision check this ticket asked for. A member tabLabel template is refused when, treating {role}, {profile}, {model} and {n} as wildcards, it can render as a configured lead's exact tab. The old tabPrefix check is kept as a second, weaker arm.
  • A pair-walk in validateLeadTabPrefixes() that refuses two fleet.leaders entries sharing one exact tab, case-insensitively, with its own IllegalStateException so each message stays true to its own relation.

The delegated decision, and how it went

I told the worker to decide for itself whether a lead's tab starting with a different lead's tabPrefix should also be refused, and to say which way it went. It chose not to refuse it, and I accept the reasoning:

tabPrefix is never read by CallerResolver — only the exact tab decides identity. The default tabPrefix is "lead:", and the codebase's own multi-lead fixtures use exactly that shared default with distinct tabs (FleetConfigTest.leadersBlockRegistersEveryPaneByName uses "lead: opus-5.0" and "lead: gpt-sol-5.6"). Refusing that case would reject the normal, safe two-lead convention to guard against something that cannot be exploited, because tabPrefix carries no identity.

My own reading before delegating was that it should also be refused. The worker's reason is better than mine and I changed my mind on the evidence, not on who said it.

Verified by me

Build, trial-merged in a throwaway worktree, rm -rf target/surefire-reports, mvn -o clean install: 1945 tests, 0 failures, BUILD SUCCESS, exit=0, 173 report files as the control. 1942 + 1 + 2 = 1945. After merging, main's tree hash 30c0fc51d8a3a6411c913acef97e681ff53c93bb equals the tree I built, so that green build covers exactly what landed.

The guard, read not inferred. Sorted names, unordered pairs, null and blank tabs skipped, separate exception.

A mutation the worker did not run. It mutated the new condition to false && … and saw RED — sound for that line. I mutated equalsIgnoreCase → equals and ran FleetConfigTest: 155 tests, 0 failures — the mutation survived. So the case-insensitivity is unpinned.

I checked whether equalsIgnoreCase is even correct, and it is. LeadTabScanner.java:145 keys the tab→name map on tab.strip().toLowerCase(Locale.ROOT) and :254 looks up through the same lowercasing, so two tabs differing only in case collide at runtime and the map put silently overwrites one — exactly the harm this guard's message names. Correct, load-bearing, and untested. Filed separately as a coverage gap; it did not block the merge.

What I did not do

I did not boot a daemon with two colliding lead tabs. "The daemon refuses to start" rests on a code reading plus a unit test on the validator, not on an observed boot failure.

#676 rode along in this PR but is not fully done — one acceptance item remains. It stays open, with my measurement on that ticket.

## Fixed and merged. Closing. PR #691 merged to `main` as `bfee23a`, pushed (`7dec74f..bfee23a`). It supersedes PR #686, which I closed unmerged — #691 carries #686's commit plus the exact-tab collision guard on top. Two things landed together: - `templateCanRenderAs(...)` — the replacement collision check this ticket asked for. A member `tabLabel` template is refused when, treating `{role}`, `{profile}`, `{model}` and `{n}` as wildcards, it can render as a configured lead's **exact** `tab`. The old `tabPrefix` check is kept as a second, weaker arm. - A pair-walk in `validateLeadTabPrefixes()` that refuses two `fleet.leaders` entries sharing one exact `tab`, case-insensitively, with its own `IllegalStateException` so each message stays true to its own relation. ## The delegated decision, and how it went I told the worker to decide for itself whether a lead's `tab` starting with a **different** lead's `tabPrefix` should also be refused, and to say which way it went. **It chose not to refuse it**, and I accept the reasoning: `tabPrefix` is never read by `CallerResolver` — only the exact `tab` decides identity. The default `tabPrefix` is `"lead:"`, and the codebase's own multi-lead fixtures use exactly that shared default with distinct tabs (`FleetConfigTest.leadersBlockRegistersEveryPaneByName` uses `"lead: opus-5.0"` and `"lead: gpt-sol-5.6"`). Refusing that case would reject the normal, safe two-lead convention to guard against something that cannot be exploited, because `tabPrefix` carries no identity. My own reading before delegating was that it *should* also be refused. The worker's reason is better than mine and I changed my mind on the evidence, not on who said it. ## Verified by me **Build**, trial-merged in a throwaway worktree, `rm -rf target/surefire-reports`, `mvn -o clean install`: **1945 tests, 0 failures, BUILD SUCCESS**, `exit=0`, 173 report files as the control. 1942 + 1 + 2 = 1945. After merging, `main`'s tree hash `30c0fc51d8a3a6411c913acef97e681ff53c93bb` equals the tree I built, so that green build covers exactly what landed. **The guard, read not inferred.** Sorted names, unordered pairs, null and blank tabs skipped, separate exception. **A mutation the worker did not run.** It mutated the new condition to `false && …` and saw RED — sound for that line. I mutated `equalsIgnoreCase` → `equals` and ran `FleetConfigTest`: **155 tests, 0 failures — the mutation survived.** So the case-insensitivity is unpinned. I checked whether `equalsIgnoreCase` is even correct, and it is. `LeadTabScanner.java:145` keys the tab→name map on `tab.strip().toLowerCase(Locale.ROOT)` and `:254` looks up through the same lowercasing, so two tabs differing only in case collide at runtime and the map `put` silently overwrites one — exactly the harm this guard's message names. Correct, load-bearing, and untested. Filed separately as a coverage gap; it did not block the merge. ## What I did not do I did not boot a daemon with two colliding lead tabs. "The daemon refuses to start" rests on a code reading plus a unit test on the validator, not on an observed boot failure. #676 rode along in this PR but is **not** fully done — one acceptance item remains. It stays open, with my measurement on that ticket.
ltms closed this issue 2026-10-03 22:46:45 +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#677
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?