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

A pane-placed member lands in the focused tab, so it can be granted PRIMARY — and the exclusion the test cites as the defence is empty in production #661

New Issue
Closed
opened 2026-10-03 18:49:55 +02:00 by ltms · 5 comments
ltms commented 2026-10-03 18:49:55 +02:00
Owner
Copy Link
Copy Source

What

A member spawned with placement: pane is created by splitting the currently focused tab. If the focused tab is a lead's labelled tab, that member's pane sits inside the lead's tab. LeadTabScanner maps every pane in a labelled tab to that lead name, and CallerResolver grants a lead pane full PRIMARY rights. So the member can spawn, stop, drain and hand over.

The test that pins "every pane in a lead tab is that lead" says in its own comment that this is safe because "nothing fleetd placed can land here (see the worker-space test above)". That argument rests on excludedWorkspaceLabels. Production passes an empty set.

This is not reachable with today's fleetd.yaml. All 8 profiles use placement: tab. It takes one config edit to reach. I am filing it as a latent defect with a live config edit in front of it, not as something firing now.

Measured on 136bec8

Every line below came from a command I ran in the main clone.

1. Production passes an empty exclusion set, and the only non-empty one is a test.

$ grep -rn "new LeadTabScanner" --include='*.java' fleetd/src
fleetd/src/test/java/dev/ltms/fleet/herdr/LeadTabScannerTest.java:163:        return new LeadTabScanner(herdr, tabToName, Set.of("fleetd-workers"), TTL, clock::get);
fleetd/src/main/java/dev/ltms/fleet/FleetdAssembly.java:265:            leads = new LeadTabScanner(herdr, tabToName, Set.of(),

FleetdAssembly.assembleAndStart is the production boot path. The exclusion is consumed at LeadTabScanner.java:182:

if (ws.workspaceId() == null || excludedWorkspaceLabels.contains(ws.label())) {

With an empty set that test is never true, so no workspace is ever skipped.

2. A pane spawn targets the focused tab, with no tab or workspace argument.

WorkspaceControl.splitPane sends only direction, cwd and env:

public String splitPane(String cwd, Map<String, String> env) {
    Map<String, Object> params = new LinkedHashMap<>();
    params.put("direction", "right");
    ...
    return herdr.call("pane.split", params).path("pane").path("pane_id").asText(null);
}

Its own javadoc says "Split the currently-focused tab". HerdrPeerLauncher.java:518-520 picks the path, and spawnAsPane at :710 calls spaces.splitPane(cwd, workerEnv) at :714.

3. Every pane in a labelled tab resolves as that lead, and the test's safety argument is the exclusion.

LeadTabScannerTest.java:234:

void everyPaneInALeadTabResolvesAsThatLead() {
    // A human may split their own lead tab. Both panes are theirs, so both are that lead —
    // nothing fleetd placed can land here (see the worker-space test above).

The comment is the whole defence, and point 1 shows it does not hold in production.

4. A lead pane outranks everything in the resolver.

CallerResolver.resolve checks leadTerminals first and returns Principal.leader(...) before the architect registry and before the worker fallback. Authz then grants SPAWN/STOP/DRAIN/HANDOVER to a primary.

5. No guard exists.

$ grep -rn "leadTab\|isLeadTab" --include='*.java' fleetd/src/main/java
(no output)

6. Not reachable today.

$ grep -c "placement: tab" fleetd/fleetd.yaml
8

All 8 profiles are placement: tab, so no spawn currently takes the pane path.

Why this is worth fixing rather than documenting

Three source files state that member spaces are excluded from the lead scan. All three are wrong against FleetdAssembly.java:265:

  • herdr/LeadTabScanner.java:40-42 — "Worker spaces are excluded wholesale (excludedWorkspaceLabels), so a worker cannot become a lead by being placed — as a split, say — inside a matching tab."
  • config/FleetConfig.java:2690-2692 — "The member-space exclusion in LeadTabScanner already blocks the realistic path".
  • wiki/11-Features.md:679 — "the configured worker spaces … are excluded from the scan wholesale".

Meanwhile FleetConfig.java:1150-1152 says the opposite and matches the code: "The scanner no longer excludes member spaces". Two javadocs in one file disagree, which is how this survived.

The failure mode is a config edit that looks harmless. An operator setting placement: pane on one profile gets no warning, and the escalation depends on which tab happens to be focused at spawn time — so it would be intermittent and very hard to diagnose.

Suggested fix

Make the bad state unrepresentable at startup rather than checking at spawn time. Add a validator, called from validateAll next to validateLeadTabPrefixes, that refuses a config with any profile on placement: pane while fleet.leaders is non-empty. It is fatal at boot, costs no herdr round trip on the spawn path, and matches what validateAuthExposure and validateLeadTabPrefixes already do.

The alternative — read the pane's tab label after a pane spawn and tear down when it names a configured lead — keeps placement: pane usable for a project with no lead. That is a policy call about whether placement: pane is still a supported mode at all, so it needs a decision before anyone implements.

Acceptance criteria

  1. No member any spawn creates occupies a tab whose label matches a configured fleet.leaders.<name>.tab. Removing the guard must turn a test red.
  2. The refusal message names both the offending profile and the lead whose tab is at risk. A message that names neither leaves the operator to guess which of 8 profiles to change.
  3. No source file or wiki page states that any workspace is excluded from the lead scan while FleetdAssembly passes an empty set. If the third constructor parameter is kept, a mutation of the argument at FleetdAssembly.java:265 must turn a test red — today you can change it to Set.of("fleet") and the suite stays green, because the only test exercising it supplies its own set.
  4. Deleting LeadTabScannerTest:206 (aTabInAWorkerSpaceIsNeverALeadEvenWhenItsLabelMatches) is allowed only if the report names the line it pinned and why that line can no longer go wrong.

Found how

Found while planning a change to herdr workspace layout — one workspace per project, with the lead as the first tab and members as sibling tabs. That proposal makes a lead tab a far more likely focus target, which is what turned a dormant hole into something worth filing. The hole itself does not depend on that proposal and exists today.

I verified each step above myself in the main clone. I ran no build and no test for this report, so I am claiming no test result.

## What A member spawned with `placement: pane` is created by splitting the **currently focused** tab. If the focused tab is a lead's labelled tab, that member's pane sits inside the lead's tab. `LeadTabScanner` maps every pane in a labelled tab to that lead name, and `CallerResolver` grants a lead pane full `PRIMARY` rights. So the member can spawn, stop, drain and hand over. The test that pins "every pane in a lead tab is that lead" says in its own comment that this is safe because *"nothing fleetd placed can land here (see the worker-space test above)"*. That argument rests on `excludedWorkspaceLabels`. **Production passes an empty set.** **This is not reachable with today's `fleetd.yaml`.** All 8 profiles use `placement: tab`. It takes one config edit to reach. I am filing it as a latent defect with a live config edit in front of it, not as something firing now. ## Measured on `136bec8` Every line below came from a command I ran in the main clone. **1. Production passes an empty exclusion set, and the only non-empty one is a test.** ``` $ grep -rn "new LeadTabScanner" --include='*.java' fleetd/src fleetd/src/test/java/dev/ltms/fleet/herdr/LeadTabScannerTest.java:163: return new LeadTabScanner(herdr, tabToName, Set.of("fleetd-workers"), TTL, clock::get); fleetd/src/main/java/dev/ltms/fleet/FleetdAssembly.java:265: leads = new LeadTabScanner(herdr, tabToName, Set.of(), ``` `FleetdAssembly.assembleAndStart` is the production boot path. The exclusion is consumed at `LeadTabScanner.java:182`: ```java if (ws.workspaceId() == null || excludedWorkspaceLabels.contains(ws.label())) { ``` With an empty set that test is never true, so no workspace is ever skipped. **2. A pane spawn targets the focused tab, with no tab or workspace argument.** `WorkspaceControl.splitPane` sends only `direction`, `cwd` and `env`: ```java public String splitPane(String cwd, Map<String, String> env) { Map<String, Object> params = new LinkedHashMap<>(); params.put("direction", "right"); ... return herdr.call("pane.split", params).path("pane").path("pane_id").asText(null); } ``` Its own javadoc says "Split the currently-focused tab". `HerdrPeerLauncher.java:518-520` picks the path, and `spawnAsPane` at `:710` calls `spaces.splitPane(cwd, workerEnv)` at `:714`. **3. Every pane in a labelled tab resolves as that lead, and the test's safety argument is the exclusion.** `LeadTabScannerTest.java:234`: ```java void everyPaneInALeadTabResolvesAsThatLead() { // A human may split their own lead tab. Both panes are theirs, so both are that lead — // nothing fleetd placed can land here (see the worker-space test above). ``` The comment is the whole defence, and point 1 shows it does not hold in production. **4. A lead pane outranks everything in the resolver.** `CallerResolver.resolve` checks `leadTerminals` first and returns `Principal.leader(...)` before the architect registry and before the worker fallback. `Authz` then grants `SPAWN`/`STOP`/`DRAIN`/`HANDOVER` to a primary. **5. No guard exists.** ``` $ grep -rn "leadTab\|isLeadTab" --include='*.java' fleetd/src/main/java (no output) ``` **6. Not reachable today.** ``` $ grep -c "placement: tab" fleetd/fleetd.yaml 8 ``` All 8 profiles are `placement: tab`, so no spawn currently takes the pane path. ## Why this is worth fixing rather than documenting Three source files state that member spaces are excluded from the lead scan. All three are wrong against `FleetdAssembly.java:265`: - `herdr/LeadTabScanner.java:40-42` — "Worker spaces are excluded wholesale (`excludedWorkspaceLabels`), so a worker cannot become a lead by being placed — as a split, say — inside a matching tab." - `config/FleetConfig.java:2690-2692` — "The member-space exclusion in `LeadTabScanner` already blocks the realistic path". - `wiki/11-Features.md:679` — "the configured worker spaces … are excluded from the scan wholesale". Meanwhile `FleetConfig.java:1150-1152` says the opposite and matches the code: "The scanner no longer excludes member spaces". Two javadocs in one file disagree, which is how this survived. The failure mode is a config edit that looks harmless. An operator setting `placement: pane` on one profile gets no warning, and the escalation depends on which tab happens to be focused at spawn time — so it would be intermittent and very hard to diagnose. ## Suggested fix Make the bad state unrepresentable at startup rather than checking at spawn time. Add a validator, called from `validateAll` next to `validateLeadTabPrefixes`, that refuses a config with any profile on `placement: pane` while `fleet.leaders` is non-empty. It is fatal at boot, costs no herdr round trip on the spawn path, and matches what `validateAuthExposure` and `validateLeadTabPrefixes` already do. The alternative — read the pane's tab label after a pane spawn and tear down when it names a configured lead — keeps `placement: pane` usable for a project with no lead. That is a policy call about whether `placement: pane` is still a supported mode at all, so it needs a decision before anyone implements. ## Acceptance criteria 1. No member any spawn creates occupies a tab whose label matches a configured `fleet.leaders.<name>.tab`. Removing the guard must turn a test red. 2. The refusal message names both the offending profile and the lead whose tab is at risk. A message that names neither leaves the operator to guess which of 8 profiles to change. 3. No source file or wiki page states that any workspace is excluded from the lead scan while `FleetdAssembly` passes an empty set. If the third constructor parameter is kept, a mutation of the argument at `FleetdAssembly.java:265` must turn a test red — today you can change it to `Set.of("fleet")` and the suite stays green, because the only test exercising it supplies its own set. 4. Deleting `LeadTabScannerTest:206` (`aTabInAWorkerSpaceIsNeverALeadEvenWhenItsLabelMatches`) is allowed only if the report names the line it pinned and why that line can no longer go wrong. ## Found how Found while planning a change to herdr workspace layout — one workspace per project, with the lead as the first tab and members as sibling tabs. That proposal makes a lead tab a far more likely focus target, which is what turned a dormant hole into something worth filing. The hole itself does not depend on that proposal and exists today. I verified each step above myself in the main clone. I ran no build and no test for this report, so I am claiming no test result.
ltms commented 2026-10-03 19:08:28 +02:00
Author
Owner
Copy Link
Copy Source

Decision: Option A — refuse at startup

Two architects worked this question independently, on different profiles (opus and sol). Both
chose Option A: a new FleetConfig validator that refuses to start when a profile uses
placement: pane while a lead tab is configured. I adjudicated the parts where they differed.

Why A and not B

Option B checks the pane's tab label after the spawn and tears the member down. Both architects
rejected it for the same reason, and I agree: it is a check-after-the-fact on a privilege boundary.
HerdrPeerLauncher.spawnAsPane starts the agent before any tab read could run, so the member is
alive and resolves as PRIMARY for the length of a herdr round trip. The teardown can also fail.
Option B also makes a valid spawn depend on which tab had focus, so it would fail intermittently.

Option A costs nothing per spawn and matches the existing rejectUnknownPlacement pattern, which
already runs at config load.

The predicate — narrow, not blanket

Refuse when a profile has placement: pane and at least one fleet.leaders entry has a
non-blank tab.

The opus architect argued for this narrower form and I took it. A leader with no tab feeds
nothing into LeadTabScanner, so it creates no hazard, and keying on fleet.leaders being
non-empty alone would refuse a safe config. The sol architect proposed the blanket form; the
narrow one is better because the scanner is what creates the hazard, and the scanner reads the tab
label.

The validator must test the resolved placement value, not only the raw YAML. I checked this
myself: FleetConfig.Profile.tabPlacement() at line 846 is "tab".equals(placement), so any other
value falls through to pane. rejectUnknownPlacement(yaml) already catches typos at load, and the
javadoc at lines 2470-2472 states this fall-through explicitly.

The chain is confirmed

Both architects checked all five links in the code and found none wrong. Between them they
confirmed: WorkspaceControl.splitPane sends only direction, cwd and env;
LeadTabScanner.scan joins pane.list to nameByTab by tab_id alone, so every pane in a
labelled tab maps to that lead; CallerResolver.resolve checks leadTerminals first and returns
Principal.leader; and Authz grants SPAWN, STOP, DRAIN and HANDOVER on
caller.isPrimary().

Neither architect read herdr's own server code, so link 1 rests on splitPane's contract and its
callers, not on the daemon's behaviour.

Still not reachable today — I measured this myself

Neither architect could check it, because fleetd/fleetd.yaml is gitignored and so is absent from
a member's worktree. I ran it in the main clone:

$ grep -c "placement: tab" fleetd/fleetd.yaml
8

and the profiles: block declares 8 profiles (local, local-direct, gx, opus, sonnet,
sol, terra, xf). So every profile sets placement: tab and no spawn takes the pane path.
The file's other placement: line, placement: weighted at line 246, is the separate top-level
placement strategy key, not a profile's.

Option A does not block the operator's layout work

Both architects checked this and agree. LeadLauncher.launch calls ensureWorkspace, then
createTab, and starts into the tab's rootPaneId. It never reads Profile.placement(), and
grep -rn "placement" fleetd/src/main/java/dev/ltms/fleet/lead/ returns nothing. So a refusal keyed
on that member config key cannot reach the lead's own start path, including a future rewrite that
uses workspace.create's active_tab_id.

Stale comments to correct in the same change

Three places still claim the member-space exclusion blocks this, and all three are false because
production passes Set.of() at FleetdAssembly.java:265:

  • herdr/LeadTabScanner.java:40-42 — the "worker spaces are excluded wholesale" trust property
  • config/FleetConfig.java:2690-2692 — the same claim inside validateLeadTabPrefixes
  • LeadTabScannerTest.java:234-236 — the test's own "nothing fleetd placed can land here"
    justification, which should point at the new validator instead

config/FleetConfig.java:1150-1152 is the correct statement and should stay the one place this
fact lives. wiki/11-Features.md also needs the correction; only the lead can edit the wiki, so
that is mine.

One correction to the sol architect's report: it also named LeadLauncher.countLeads as
carrying the same false claim. I read that comment and it does not — it says a workspace can no
longer be excluded wholesale and that the exact tab label is the sole discriminator, which agrees
with the code. Leave it alone.

Not done yet

This comment settles the approach only. No code is changed and no validator is written yet.

## Decision: Option A — refuse at startup Two architects worked this question independently, on different profiles (`opus` and `sol`). Both chose **Option A**: a new `FleetConfig` validator that refuses to start when a profile uses `placement: pane` while a lead tab is configured. I adjudicated the parts where they differed. ### Why A and not B Option B checks the pane's tab label after the spawn and tears the member down. Both architects rejected it for the same reason, and I agree: it is a check-after-the-fact on a privilege boundary. `HerdrPeerLauncher.spawnAsPane` starts the agent before any tab read could run, so the member is alive and resolves as PRIMARY for the length of a herdr round trip. The teardown can also fail. Option B also makes a valid spawn depend on which tab had focus, so it would fail intermittently. Option A costs nothing per spawn and matches the existing `rejectUnknownPlacement` pattern, which already runs at config load. ### The predicate — narrow, not blanket Refuse when a profile has `placement: pane` **and** at least one `fleet.leaders` entry has a non-blank `tab`. The `opus` architect argued for this narrower form and I took it. A leader with no `tab` feeds nothing into `LeadTabScanner`, so it creates no hazard, and keying on `fleet.leaders` being non-empty alone would refuse a safe config. The `sol` architect proposed the blanket form; the narrow one is better because the scanner is what creates the hazard, and the scanner reads the tab label. The validator must test the **resolved** placement value, not only the raw YAML. I checked this myself: `FleetConfig.Profile.tabPlacement()` at line 846 is `"tab".equals(placement)`, so any other value falls through to pane. `rejectUnknownPlacement(yaml)` already catches typos at load, and the javadoc at lines 2470-2472 states this fall-through explicitly. ### The chain is confirmed Both architects checked all five links in the code and found none wrong. Between them they confirmed: `WorkspaceControl.splitPane` sends only `direction`, `cwd` and `env`; `LeadTabScanner.scan` joins `pane.list` to `nameByTab` by `tab_id` alone, so every pane in a labelled tab maps to that lead; `CallerResolver.resolve` checks `leadTerminals` first and returns `Principal.leader`; and `Authz` grants `SPAWN`, `STOP`, `DRAIN` and `HANDOVER` on `caller.isPrimary()`. Neither architect read herdr's own server code, so link 1 rests on `splitPane`'s contract and its callers, not on the daemon's behaviour. ### Still not reachable today — I measured this myself Neither architect could check it, because `fleetd/fleetd.yaml` is gitignored and so is absent from a member's worktree. I ran it in the main clone: ``` $ grep -c "placement: tab" fleetd/fleetd.yaml 8 ``` and the `profiles:` block declares 8 profiles (`local`, `local-direct`, `gx`, `opus`, `sonnet`, `sol`, `terra`, `xf`). So every profile sets `placement: tab` and no spawn takes the pane path. The file's other `placement:` line, `placement: weighted` at line 246, is the separate top-level placement strategy key, not a profile's. ### Option A does not block the operator's layout work Both architects checked this and agree. `LeadLauncher.launch` calls `ensureWorkspace`, then `createTab`, and starts into the tab's `rootPaneId`. It never reads `Profile.placement()`, and `grep -rn "placement" fleetd/src/main/java/dev/ltms/fleet/lead/` returns nothing. So a refusal keyed on that member config key cannot reach the lead's own start path, including a future rewrite that uses `workspace.create`'s `active_tab_id`. ### Stale comments to correct in the same change Three places still claim the member-space exclusion blocks this, and all three are false because production passes `Set.of()` at `FleetdAssembly.java:265`: - `herdr/LeadTabScanner.java:40-42` — the "worker spaces are excluded wholesale" trust property - `config/FleetConfig.java:2690-2692` — the same claim inside `validateLeadTabPrefixes` - `LeadTabScannerTest.java:234-236` — the test's own "nothing fleetd placed can land here" justification, which should point at the new validator instead `config/FleetConfig.java:1150-1152` is the correct statement and should stay the one place this fact lives. `wiki/11-Features.md` also needs the correction; only the lead can edit the wiki, so that is mine. **One correction to the `sol` architect's report:** it also named `LeadLauncher.countLeads` as carrying the same false claim. I read that comment and it does not — it says a workspace can *no longer* be excluded wholesale and that the exact tab label is the sole discriminator, which agrees with the code. Leave it alone. ### Not done yet This comment settles the approach only. No code is changed and no validator is written yet.
ltms commented 2026-10-03 19:39:34 +02:00
Author
Owner
Copy Link
Copy Source

Lead review of PR #667 — three changes before merge

The validator itself is right. I read the whole diff. Three things to fix, and one of them means my own earlier adjudication on this ticket was wrong.

Verified good

  • validatePanePlacementAgainstLeadTabs() is public void, no args, so invokeAllValidators picks it up with no wiring. It guards on fleet == null || leaders().isEmpty(), then on no leader having a non-blank tab, then collects offending profiles sorted and throws IllegalStateException naming them. That matches validateLeadTabPrefixes's shape.
  • The predicate is the decided one. It tests the resolved value via !Profile.tabPlacement(), not raw YAML, and the leader side is the exact FleetdAssembly.java:259-261 shape (leader != null && leader.tab() != null && !leader.tab().isBlank()). It is the narrow form: a leader with no tab refuses nothing.
  • The three stale comments named in my brief are corrected honestly. LeadTabScanner.java now says excludedWorkspaceLabels can filter a workspace out but that a caller may pass an empty set "and the daemon does" — which is the true statement.
  • The canary test was not weakened. It is still an exact-set equality, with the new name added and the test's own embedded instructions followed.
  • FleetConfig.java:1150-1152 left alone, as asked.

Finding 1 — I was wrong about LeadLauncher, and the sol architect was right

My earlier comment said: "it also named LeadLauncher.countLeads as carrying the same false claim. I read that comment and it does not… Leave it alone." My brief repeated that instruction.

That was a mistake, and the worker caught it. There are two comments about twenty lines apart, saying opposite things, and I checked the wrong one.

LeadLauncher.java:203-205 — the javadoc on the LeadCount record:

Member workspaces are excluded, exactly as the scanner excludes them: a member must not be counted as a lead because it happens to sit in a matching tab.

That is false, for exactly the reason the other three comments were false: FleetdAssembly.java:265 passes Set.of().

LeadLauncher.java:224-229 — the body comment inside countLeads:

a workspace can no longer be excluded wholesale — the lead lives in the member workspace by design. The sole discriminator is the exact tab label

That one is correct. I read this one, found it accurate, and concluded the architect was wrong. The architect was pointing at the javadoc above it.

Fix the LeadCount javadoc at LeadLauncher.java:201-205 in this change. It is the same false claim as the other three and it directly contradicts the correct comment in the method it documents. Do not touch the body comment at 224-229 — that one stays.

Recording the general lesson, because it has cost us twice today: verifying a different sentence than the one under dispute proves nothing about the dispute. When a report names a location, check that location.

Finding 2 — the renamed test leaves a dangling {@link}

FleetConfigValidateAllTest.java:65 still reads:

* #fleetConfigDeclaresExactlyTheseSixValidatorsToday()}: it fails the moment a seventh validator

but the method is now fleetConfigDeclaresExactlyTheseSevenValidatorsToday. That {@link} no longer resolves. The build does not catch it — mvn clean install is green at 1927 — so it needs fixing by hand. Update the link, and the "a seventh validator" wording with it.

Finding 3 — the visible denominator is off by one, and already was

This test exists to show a number. The number is wrong, before and after this PR. Measured:

real public void validate*() minus validateAll names in the expected Set.of the method name says the javadoc says
main 7 7 Six six
PR #667 head 8 8 Seven seven

The assertion's set has always been right. The word in the method name and the javadoc has been one behind since before this change, and this PR incremented the word instead of counting the set. A test whose stated purpose is "a visible denominator… so a reader adding a seventh sees this assertion name the new count" fails that purpose when a reader counts eight names under the word "seven".

Fix it so the count is right. My preference, but your call: drop the number from the method name entirely — fleetConfigDeclaresExactlyTheseValidatorsToday — and keep the count in the javadoc only. The Set.of literal is the real denominator, and one fact in one place cannot drift out of step with itself. If you keep a number in the name, make it eight.

The pre-existing half of this is not the worker's fault. It is in scope because this PR edits those exact lines.

Not blocking

The build is green at 1927 (1923 + 4 new tests) and the non-vacuousness check is sound: inverting !tabPlacement() turned all three relevant tests red, with the two "allowed" cases flipping to unexpected throws and the "refuses" case to nothing thrown. I will re-run my own build and mutation on the updated head before merging.

## Lead review of PR #667 — three changes before merge The validator itself is right. I read the whole diff. Three things to fix, and one of them means **my own earlier adjudication on this ticket was wrong**. ### Verified good - `validatePanePlacementAgainstLeadTabs()` is `public void`, no args, so `invokeAllValidators` picks it up with no wiring. It guards on `fleet == null || leaders().isEmpty()`, then on no leader having a non-blank `tab`, then collects offending profiles sorted and throws `IllegalStateException` naming them. That matches `validateLeadTabPrefixes`'s shape. - **The predicate is the decided one.** It tests the resolved value via `!Profile.tabPlacement()`, not raw YAML, and the leader side is the exact `FleetdAssembly.java:259-261` shape (`leader != null && leader.tab() != null && !leader.tab().isBlank()`). It is the narrow form: a leader with no `tab` refuses nothing. - The three stale comments named in my brief are corrected honestly. `LeadTabScanner.java` now says `excludedWorkspaceLabels` *can* filter a workspace out but that a caller may pass an empty set "and the daemon does" — which is the true statement. - The canary test was **not** weakened. It is still an exact-set equality, with the new name added and the test's own embedded instructions followed. - `FleetConfig.java:1150-1152` left alone, as asked. ### Finding 1 — I was wrong about `LeadLauncher`, and the `sol` architect was right My [earlier comment](https://git.ltms.dev/fleet/fleetd/issues/661#issuecomment-18018) said: *"it also named `LeadLauncher.countLeads` as carrying the same false claim. I read that comment and it does not… Leave it alone."* My brief repeated that instruction. That was a mistake, and the worker caught it. There are **two** comments about twenty lines apart, saying opposite things, and I checked the wrong one. `LeadLauncher.java:203-205` — the javadoc on the `LeadCount` record: > Member workspaces are excluded, exactly as the scanner excludes them: a member must not be counted as a lead because it happens to sit in a matching tab. That is false, for exactly the reason the other three comments were false: `FleetdAssembly.java:265` passes `Set.of()`. `LeadLauncher.java:224-229` — the body comment inside `countLeads`: > a workspace can no longer be excluded wholesale — the lead lives in the member workspace by design. The sole discriminator is the exact tab label That one is correct. I read this one, found it accurate, and concluded the architect was wrong. The architect was pointing at the javadoc above it. **Fix the `LeadCount` javadoc at `LeadLauncher.java:201-205` in this change.** It is the same false claim as the other three and it directly contradicts the correct comment in the method it documents. Do not touch the body comment at 224-229 — that one stays. Recording the general lesson, because it has cost us twice today: *verifying a different sentence than the one under dispute proves nothing about the dispute.* When a report names a location, check that location. ### Finding 2 — the renamed test leaves a dangling `{@link}` `FleetConfigValidateAllTest.java:65` still reads: ``` * #fleetConfigDeclaresExactlyTheseSixValidatorsToday()}: it fails the moment a seventh validator ``` but the method is now `fleetConfigDeclaresExactlyTheseSevenValidatorsToday`. That `{@link}` no longer resolves. The build does not catch it — `mvn clean install` is green at 1927 — so it needs fixing by hand. Update the link, and the "a seventh validator" wording with it. ### Finding 3 — the visible denominator is off by one, and already was This test exists to show a number. The number is wrong, before and after this PR. Measured: | | real `public void validate*()` minus `validateAll` | names in the expected `Set.of` | the method name says | the javadoc says | |---|---|---|---|---| | `main` | **7** | 7 | Six | six | | PR #667 head | **8** | 8 | Seven | seven | The assertion's set has always been right. The *word* in the method name and the javadoc has been one behind since before this change, and this PR incremented the word instead of counting the set. A test whose stated purpose is "a visible denominator… so a reader adding a seventh sees this assertion name the new count" fails that purpose when a reader counts eight names under the word "seven". Fix it so the count is right. My preference, but your call: drop the number from the method name entirely — `fleetConfigDeclaresExactlyTheseValidatorsToday` — and keep the count in the javadoc only. The `Set.of` literal is the real denominator, and one fact in one place cannot drift out of step with itself. If you keep a number in the name, make it **eight**. The pre-existing half of this is not the worker's fault. It is in scope because this PR edits those exact lines. ### Not blocking The build is green at **1927** (1923 + 4 new tests) and the non-vacuousness check is sound: inverting `!tabPlacement()` turned all three relevant tests red, with the two "allowed" cases flipping to unexpected throws and the "refuses" case to nothing thrown. I will re-run my own build and mutation on the updated head before merging.
ltms commented 2026-10-03 19:46:29 +02:00
Author
Owner
Copy Link
Copy Source

Second review round — findings 1-3 are fixed. One more, then I merge.

All three earlier findings are correctly done, and I verified each:

  • LeadLauncher.java LeadCount javadoc now says a member is not counted because its tab carries a different label, "not because any workspace is excluded from this count". True, and the correct body comment inside countLeads is untouched.
  • The dangling {@link} is gone.
  • The canary's number is dropped from the method name and the count now lives only in its javadoc ("today there are eight"). That is the right shape: one fact, one place.

The worker also flagged, rather than silently fixing, a count it was not asked about. That was the right call and it led me to the real problem.

Finding 4 — the reachability enumeration covers six of eight, and its javadoc claims it covers all

validateAllReachesEveryOneOfTodaysSixValidators is the test the file calls "the heart of claim 2": one minimal failing config per validator, each asserted to fail through validateAll(). Measured on the PR head, it covers six:

// validateAuthExposure   // validateCharters   // validateLeadTabPrefixes
// validateMembers        // validateModels     // validateSubscriptionProfiles

There are eight validators. The two missing are validateLeadRollover and validatePanePlacementAgainstLeadTabs.

The method name is therefore honest — it really does reach six. What is false is the surrounding claim that those six are "today's" validators. Today there are eight.

Your new validator is covered, by FleetConfigTest.validateAllAlsoRefusesPanePlacementAgainstALeadTab (FleetConfigTest.java:835). That satisfied my acceptance criterion, so this is not a coverage hole for #661. It is a placement problem: validateAll-reachability now lives in two files, and the file whose job is to enumerate it does not know about the new one. That is how the next validator gets missed.

validateLeadRollover's missing case is pre-existing and is now fleetd #668. Do not fix it here.

What to change in this PR

  1. Add the pane-placement case to the enumeration in validateAllReachesEveryOneOfTodaysSixValidators, in the same assertValidateAllRefuses(dir, "...yaml", """…""", "…") shape as the other six. You may keep or drop your FleetConfigTest version — your call — but the enumeration must contain it, so one file lists every validateAll-reachability case.
  2. Rename that method to drop the hardcoded number, same rule you just applied to the canary: validateAllReachesEveryOneOfTodaysRealValidators, or similar. Fix every reference to the old name — last time one was left behind.
  3. Make the current-state claims truthful. After step 1 the enumeration covers seven of eight. Say that, and say which one is missing and why, pointing at #668. The lines that assert what is covered now are:
    • the class-level <ol> item, around lines 47 and 49 ("reaches each of today's six real validators" / "every one of those six failures")
    • the Claim 2 section header, around line 212 ("reaches all six today")
    • that method's own javadoc and its closing sentence, around lines 267 and 271

Explicitly leave these alone

Lines around 25-26, 30, 32, 60 and 63, and 70 also say "six", and they are not in scope. They record what was measured or replaced at the time — "the fix replaces the six individual cfg.validateXxx() calls", "a hardcoded list of today's six method calls leaves the whole suite green (measured at review: 1491 tests)". Those were true when written, and rewriting the number inside a past measurement would make them false. Changing a recorded measurement to match today is worse than leaving it dated.

(The separate question of whether that much history belongs in a source comment at all is a house-style matter for the whole repo, not something to settle inside this PR.)

Then re-run mvn clean install, report the real Tests run: line, and push.

## Second review round — findings 1-3 are fixed. One more, then I merge. All three earlier findings are correctly done, and I verified each: - `LeadLauncher.java` `LeadCount` javadoc now says a member is not counted because its tab carries a different label, "not because any workspace is excluded from this count". True, and the correct body comment inside `countLeads` is untouched. - The dangling `{@link}` is gone. - The canary's number is dropped from the method name and the count now lives only in its javadoc ("today there are eight"). That is the right shape: one fact, one place. The worker also flagged, rather than silently fixing, a count it was not asked about. That was the right call and it led me to the real problem. ### Finding 4 — the reachability enumeration covers six of eight, and its javadoc claims it covers all `validateAllReachesEveryOneOfTodaysSixValidators` is the test the file calls "the heart of claim 2": one minimal failing config per validator, each asserted to fail through `validateAll()`. Measured on the PR head, it covers **six**: ``` // validateAuthExposure // validateCharters // validateLeadTabPrefixes // validateMembers // validateModels // validateSubscriptionProfiles ``` There are **eight** validators. The two missing are `validateLeadRollover` and `validatePanePlacementAgainstLeadTabs`. The method **name** is therefore honest — it really does reach six. What is false is the surrounding claim that those six are "today's" validators. Today there are eight. Your new validator is covered, by `FleetConfigTest.validateAllAlsoRefusesPanePlacementAgainstALeadTab` (`FleetConfigTest.java:835`). That satisfied my acceptance criterion, so this is not a coverage hole for #661. It is a placement problem: validateAll-reachability now lives in two files, and the file whose job is to enumerate it does not know about the new one. That is how the next validator gets missed. **`validateLeadRollover`'s missing case is pre-existing and is now [fleetd #668](https://git.ltms.dev/fleet/fleetd/issues/668). Do not fix it here.** ### What to change in this PR 1. **Add the pane-placement case to the enumeration** in `validateAllReachesEveryOneOfTodaysSixValidators`, in the same `assertValidateAllRefuses(dir, "...yaml", """…""", "…")` shape as the other six. You may keep or drop your `FleetConfigTest` version — your call — but the enumeration must contain it, so one file lists every validateAll-reachability case. 2. **Rename that method to drop the hardcoded number**, same rule you just applied to the canary: `validateAllReachesEveryOneOfTodaysRealValidators`, or similar. Fix every reference to the old name — last time one was left behind. 3. **Make the current-state claims truthful.** After step 1 the enumeration covers seven of eight. Say that, and say which one is missing and why, pointing at #668. The lines that assert what is covered *now* are: - the class-level `<ol>` item, around lines 47 and 49 ("reaches each of today's six real validators" / "every one of those six failures") - the Claim 2 section header, around line 212 ("reaches all six today") - that method's own javadoc and its closing sentence, around lines 267 and 271 ### Explicitly leave these alone Lines around **25-26, 30, 32, 60 and 63, and 70** also say "six", and they are **not** in scope. They record what was measured or replaced at the time — "the fix replaces the six individual `cfg.validateXxx()` calls", "a hardcoded list of today's six method calls leaves the whole suite green (measured at review: 1491 tests)". Those were true when written, and rewriting the number inside a past measurement would make them false. Changing a recorded measurement to match today is worse than leaving it dated. (The separate question of whether that much history belongs in a source comment at all is a house-style matter for the whole repo, not something to settle inside this PR.) Then re-run `mvn clean install`, report the real `Tests run:` line, and push.
ltms commented 2026-10-03 20:00:25 +02:00
Author
Owner
Copy Link
Copy Source

Merged locally as b4b7cf5 and pushed to main. PR 667 closed by hand (we merge locally, so the forge never closes one by itself).

What landed

  • FleetConfig.validatePanePlacementAgainstLeadTabs() — refuses startup when any profile is pane-placed while any fleet.leaders entry names a non-blank tab.
  • Three false comments corrected: LeadTabScanner.java, FleetConfig.validateLeadTabPrefixes() javadoc, and LeadLauncher.java:201-205.
  • The validateAll reachability enumeration gained a pane-placement case, and its test method name no longer carries a hardcoded count.

What I verified myself, not on the worker's word

  • The PR branch was 6 commits behind main. A two-dot diff made it look like the merge would revert #663 and #664. It does not: the merge base is 9417de1, the three-dot diff touches only 6 files, and the trial merge reported no conflicted path. I checked this before merging rather than after.
  • The new validator is reached in production, not inert. validateAll() is invokeAllValidators(this), and that sweep is genuinely reflective — public, no-arg, void, name starts with validate, excluding validateAll. No hardcoded list, so the new method is swept with no second step.
  • Build in a throwaway worktree, never the main clone: Tests run: 1927, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, 170 surefire report files (the same count as main — a positive control, taken after rm -rf target/surefire-reports).

One defect I fixed at merge

FleetConfig's validateAll/invokeAllValidators javadoc still counted validators — "today's six", "a seventh validator". It was already wrong on main at seven, and this change made it eight. Commit b4b7cf5 makes the wording count-free, so it cannot drift again. Measured: eight public no-arg void validate* methods, at FleetConfig.java lines 2671, 2708, 2755, 2790, 2827, 2856, 2896 and 2958.

I left the surrounding history-style prose in that javadoc alone. It breaks the project's comment rules, but it predates this change, so fixing it here would widen the scope.

Still owed, and not done by this change

  • wiki/11-Features.md:678-680 claims two guards protect the lead-tab trust boundary. Before this change only one existed. Now the second one really does exist, so that sentence needs rewriting to name the real pair rather than deleting. Lead-only work, because wiki/ is a submodule.
  • A wiki/11-Features.md feature entry for the new validator: what it does, the knob, why it exists, the gotcha.
  • fleetd #668 — validateLeadRollover has no validateAll-reachability case. A pre-existing gap, now named in the test javadoc, not fixed here.
  • A merge is not a deployment. The running daemon still holds the jar it booted with.

Closing.

Merged locally as **b4b7cf5** and pushed to `main`. PR 667 closed by hand (we merge locally, so the forge never closes one by itself). ## What landed - `FleetConfig.validatePanePlacementAgainstLeadTabs()` — refuses startup when any profile is pane-placed while any `fleet.leaders` entry names a non-blank `tab`. - Three false comments corrected: `LeadTabScanner.java`, `FleetConfig.validateLeadTabPrefixes()` javadoc, and `LeadLauncher.java:201-205`. - The `validateAll` reachability enumeration gained a pane-placement case, and its test method name no longer carries a hardcoded count. ## What I verified myself, not on the worker's word - **The PR branch was 6 commits behind `main`.** A two-dot diff made it look like the merge would revert #663 and #664. It does not: the merge base is `9417de1`, the three-dot diff touches only 6 files, and the trial merge reported no conflicted path. I checked this before merging rather than after. - **The new validator is reached in production, not inert.** `validateAll()` is `invokeAllValidators(this)`, and that sweep is genuinely reflective — public, no-arg, `void`, name starts with `validate`, excluding `validateAll`. No hardcoded list, so the new method is swept with no second step. - **Build in a throwaway worktree, never the main clone**: `Tests run: 1927, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, 170 surefire report files (the same count as `main` — a positive control, taken after `rm -rf target/surefire-reports`). ## One defect I fixed at merge `FleetConfig`'s `validateAll`/`invokeAllValidators` javadoc still counted validators — "today's six", "a seventh validator". It was already wrong on `main` at seven, and this change made it eight. Commit b4b7cf5 makes the wording count-free, so it cannot drift again. Measured: eight public no-arg `void` `validate*` methods, at `FleetConfig.java` lines 2671, 2708, 2755, 2790, 2827, 2856, 2896 and 2958. I left the surrounding history-style prose in that javadoc alone. It breaks the project's comment rules, but it predates this change, so fixing it here would widen the scope. ## Still owed, and not done by this change - `wiki/11-Features.md:678-680` claims **two** guards protect the lead-tab trust boundary. Before this change only one existed. Now the second one really does exist, so that sentence needs rewriting to name the real pair rather than deleting. Lead-only work, because `wiki/` is a submodule. - A `wiki/11-Features.md` feature entry for the new validator: what it does, the knob, why it exists, the gotcha. - fleetd #668 — `validateLeadRollover` has no `validateAll`-reachability case. A pre-existing gap, now named in the test javadoc, not fixed here. - A merge is not a deployment. The running daemon still holds the jar it booted with. Closing.
ltms closed this issue 2026-10-03 20:00:29 +02:00
ltms commented 2026-10-03 20:02:33 +02:00
Author
Owner
Copy Link
Copy Source

Correction to my close comment above. I listed what was still owed but signed off acceptance criterion 3 without running its second half — the mutation check.

I have now run it. The mutant survives. Set.of() → Set.of("fleet") at FleetdAssembly.java:265, one line changed, full suite green: Tests run: 1927, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, 170 report files. Reverted afterwards and confirmed the worktree was byte-identical to HEAD.

So AC3 was only half met: the three false source comments were corrected, but the argument at FleetdAssembly.java:265 is still unpinned. Split out as #670 with the mutant output and its own acceptance criteria.

This issue stays closed — the validator it asked for landed and works. The unmet half is a test-coverage gap on a different line, so it belongs in its own ticket rather than holding this one open.

Correction to my close comment above. I listed what was still owed but signed off acceptance criterion 3 without running its second half — the mutation check. I have now run it. **The mutant survives.** `Set.of()` → `Set.of("fleet")` at `FleetdAssembly.java:265`, one line changed, full suite green: `Tests run: 1927, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, 170 report files. Reverted afterwards and confirmed the worktree was byte-identical to `HEAD`. So AC3 was only half met: the three false source comments were corrected, but the argument at `FleetdAssembly.java:265` is still unpinned. Split out as **#670** with the mutant output and its own acceptance criteria. This issue stays closed — the validator it asked for landed and works. The unmet half is a test-coverage gap on a different line, so it belongs in its own ticket rather than holding this one open.
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#661
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?