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

fleetd #677: refuse two leads that share one exact tab #691

Closed
agent wants to merge 0 commits from worker/677-fix-lead-collision-f69073-12 into main
pull from: worker/677-fix-lead-collision-f69073-12
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-10-03 22:37:09 +02:00
Member
Copy Link
Copy Source

Supersedes #686.

Starts from PR #686's commit (merged in), which already extended validateLeadTabPrefixes() to catch a member tabLabel template/override colliding with a lead's exact tab. This PR adds the one remaining case the ticket asked for: two fleet.leaders entries sharing the same exact tab.

What changed

validateLeadTabPrefixes() in FleetConfig.java now runs a second, independent pass after the existing member-template checks: it collects every leader.tab(), compares each pair case-insensitively (the same way startsWithIgnoreCase does for prefixes), and refuses with its own IllegalStateException naming both leads when two share one tab. Kept out of the first loop's else if chain since it is a different relation.

Decision on the tabPrefix-vs-sibling-tab case

The ticket's lead comment suggested also refusing when one lead's tab starts with a different lead's tabPrefix. I decided against that: tabPrefix plays no role in identity resolution (grep confirms CallerResolver never reads it — only the exact tab is matched), and the default tabPrefix is "lead:", which is also the conventional lead-tab naming scheme used throughout this codebase's own fixtures (e.g. FleetConfigTest.leadersBlockRegistersEveryPaneByName's "lead: opus-5.0" / "lead: gpt-sol-5.6"). Refusing that case would reject the standard multi-lead setup with no matching identity hazard behind it.

Tests

Two new tests in FleetConfigTest, each with its control:

  • twoLeadsSharingTheSameExactTabRefusesToStart — two leads with tab "shared tab" refuses, message names both leads
  • twoLeadsWithDistinctExactTabsAreAllowed (control) — two leads with distinct tabs loads cleanly

The template-vs-exact-tab case already has a home in aFleetTabLabelTemplateThatCanRenderAsALeadTabRefusesToStart (unchanged by this PR).

validateAll() needs no change: its reflective sweep already reaches validateLeadTabPrefixes().

Mutation check

Disabled the new pass (if (false && a.tab().equalsIgnoreCase(b.tab()))) and reran the two new tests: the collision test went RED (Expected IllegalStateException ... but nothing was thrown), the control stayed GREEN. Restored the pass, confirmed clean with git diff, reran both GREEN.

Build

mvn -o clean install from fleetd/: BUILD SUCCESS, Tests run: 1945, Failures: 0, Errors: 0, Skipped: 0 (baseline on origin/main 2eb2d61 was 1942; +3 accounts for PR #686's own new test plus these two). 173 surefire report files as a control against stale XML.

Also found, not fixed (out of scope)

Surveyed FleetConfig.java's other validators for the same shape (checks each entry against a shared template/lookup but never against its siblings). Found no other instance: validateCharters, validateSubscriptionProfiles, validateModels and the validateMembers pool checks all compare against a fixed lookup table or enum, not each other, and duplicate slot/charter keys are structurally impossible (keyed maps). validatePanePlacementAgainstLeadTabs is a one-way boolean check, not a sibling comparison.

Supersedes #686. Starts from PR #686's commit (merged in), which already extended `validateLeadTabPrefixes()` to catch a member tabLabel template/override colliding with a lead's exact tab. This PR adds the one remaining case the ticket asked for: two `fleet.leaders` entries sharing the same exact `tab`. ## What changed `validateLeadTabPrefixes()` in `FleetConfig.java` now runs a second, independent pass after the existing member-template checks: it collects every `leader.tab()`, compares each pair case-insensitively (the same way `startsWithIgnoreCase` does for prefixes), and refuses with its own `IllegalStateException` naming both leads when two share one tab. Kept out of the first loop's `else if` chain since it is a different relation. ## Decision on the tabPrefix-vs-sibling-tab case The ticket's lead comment suggested also refusing when one lead's tab starts with a *different* lead's `tabPrefix`. I decided against that: `tabPrefix` plays no role in identity resolution (grep confirms `CallerResolver` never reads it — only the exact `tab` is matched), and the default `tabPrefix` is `"lead:"`, which is also the conventional lead-tab naming scheme used throughout this codebase's own fixtures (e.g. `FleetConfigTest.leadersBlockRegistersEveryPaneByName`'s `"lead: opus-5.0"` / `"lead: gpt-sol-5.6"`). Refusing that case would reject the standard multi-lead setup with no matching identity hazard behind it. ## Tests Two new tests in `FleetConfigTest`, each with its control: - `twoLeadsSharingTheSameExactTabRefusesToStart` — two leads with tab `"shared tab"` refuses, message names both leads - `twoLeadsWithDistinctExactTabsAreAllowed` (control) — two leads with distinct tabs loads cleanly The template-vs-exact-tab case already has a home in `aFleetTabLabelTemplateThatCanRenderAsALeadTabRefusesToStart` (unchanged by this PR). `validateAll()` needs no change: its reflective sweep already reaches `validateLeadTabPrefixes()`. ## Mutation check Disabled the new pass (`if (false && a.tab().equalsIgnoreCase(b.tab()))`) and reran the two new tests: the collision test went RED (`Expected IllegalStateException ... but nothing was thrown`), the control stayed GREEN. Restored the pass, confirmed clean with `git diff`, reran both GREEN. ## Build `mvn -o clean install` from `fleetd/`: `BUILD SUCCESS`, `Tests run: 1945, Failures: 0, Errors: 0, Skipped: 0` (baseline on `origin/main` 2eb2d61 was 1942; +3 accounts for PR #686's own new test plus these two). 173 surefire report files as a control against stale XML. ## Also found, not fixed (out of scope) Surveyed `FleetConfig.java`'s other validators for the same shape (checks each entry against a shared template/lookup but never against its siblings). Found no other instance: `validateCharters`, `validateSubscriptionProfiles`, `validateModels` and the `validateMembers` pool checks all compare against a fixed lookup table or enum, not each other, and duplicate slot/charter keys are structurally impossible (keyed maps). `validatePanePlacementAgainstLeadTabs` is a one-way boolean check, not a sibling comparison.
agent added 3 commits 2026-10-03 22:37:10 +02:00
fleetd #677: guard exact lead tab collisions
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 46s
Details
CI / build (pull_request) Failing after 1m56s
Details
cb4a6869b9
Merge remote-tracking branch 'refs/remotes/pr/686' into worker/677-fix-lead-collision-f69073-12 7df7985a16
fleetd #677: refuse two leads that share one exact tab
CI / shell-tests (pull_request) Failing after 10s
Details
CI / contract (pull_request) Successful in 1m17s
Details
CI / build (pull_request) Failing after 2m17s
Details
482598e2a6 
validateLeadTabPrefixes() compared each lead's tab against the member
tabLabel template, but never against another lead's tab. Two leads
configured with the same exact tab loaded cleanly, even though exact
tab is the only thing lead identity is matched on, so only one of them
could ever be found.

Add an independent pass, case-insensitive, that refuses when two
fleet.leaders entries share one exact tab, with its own exception so
the message stays accurate for this relation.

Decided not to also refuse a lead's tab starting with a sibling's
tabPrefix: tabPrefix plays no role in identity resolution (only the
exact tab does), and the default tabPrefix is "lead:", the same
string used as the conventional lead tab prefix throughout this
codebase's own fixtures (e.g. "lead: opus" / "lead: sol"). Refusing
that case would reject the standard multi-lead setup with no matching
identity hazard.
ltms commented 2026-10-03 22:55:23 +02:00
Owner
Copy Link
Copy Source

Merged locally as bfee23a, pushed. Closing by hand.

We merge locally, so Gitea never closes a PR by itself. main went 7dec74f..bfee23a. Closes #677 and supersedes #686, which I closed unmerged.

My full verification is on #686 and #677. The short version: 1945 tests / 0 failures / BUILD SUCCESS in a throwaway worktree, merged tree hash equal to the tree I built, the commit split checked so the worker's two-file claim held up, and I accepted its tabPrefix decision on its reasoning rather than my own prior view.

Two things I found that did not block this merge, both filed:

  • #693 — equalsIgnoreCase → equals survived my mutation, so the case-insensitivity was unpinned. Now fixed and merged in PR #695 (d0688c8), with the split control: the new case test goes RED, the same-case test stays GREEN.
  • #676 — the javadoc rewrite that rode along left one stale count inside a method name, quoted into the class javadoc by an {@link}. Also fixed in PR #695.

So everything this PR carried is now complete.

## Merged locally as `bfee23a`, pushed. Closing by hand. We merge locally, so Gitea never closes a PR by itself. `main` went `7dec74f..bfee23a`. Closes #677 and supersedes #686, which I closed unmerged. **My full verification is on #686 and #677.** The short version: 1945 tests / 0 failures / BUILD SUCCESS in a throwaway worktree, merged tree hash equal to the tree I built, the commit split checked so the worker's two-file claim held up, and I accepted its `tabPrefix` decision on its reasoning rather than my own prior view. Two things I found that did **not** block this merge, both filed: - **#693** — `equalsIgnoreCase` → `equals` survived my mutation, so the case-insensitivity was unpinned. Now fixed and merged in PR #695 (`d0688c8`), with the split control: the new case test goes RED, the same-case test stays GREEN. - **#676** — the javadoc rewrite that rode along left one stale count inside a method name, quoted into the class javadoc by an `{@link}`. Also fixed in PR #695. So everything this PR carried is now complete.
ltms closed this pull request 2026-10-03 22:55:26 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 10s
Details
CI / contract (pull_request) Successful in 1m17s
Details
CI / build (pull_request) Failing after 2m17s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#691
Delete Branch "worker/677-fix-lead-collision-f69073-12"

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?