From cb4a6869b9a8a2612a3f22fc08bb71ebc934ff91 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 3 Oct 2026 22:10:29 +0200 Subject: [PATCH] fleetd #677: guard exact lead tab collisions --- .../dev/ltms/fleet/config/FleetConfig.java | 95 ++++++++++--------- .../ltms/fleet/config/FleetConfigTest.java | 57 ++++++----- .../config/FleetConfigValidateAllTest.java | 76 +++------------ 3 files changed, 97 insertions(+), 131 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java index 238b3050..65440ff0 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java @@ -1129,11 +1129,8 @@ public record FleetConfig( * {@code tab} can never be discovered, launched or not * @param instances how many of this lead should be live (default 1). The daemon * launches only the shortfall, so a restart adopts rather than doubles - * @param tabPrefix no longer used to find a lead's tab — {@code tab} is matched - * exactly. Its only remaining job is the startup collision guard - * ({@link #validateLeadTabPrefixes()}), which still uses it to refuse - * a worker {@code tabLabel} template that could be misread as a lead. - * Default {@code "lead:"} + * @param tabPrefix lead-tab naming convention checked against member labels. Lead + * identity uses {@code tab}. Default {@code "lead:"} * @param scanIntervalSeconds how long a tab scan is cached before herdr is asked again; also the * worst case before a newly-labelled tab is recognised. Default 10 * @param kind which agent runs there ({@code claude}, {@code opencode}, …) @@ -1236,10 +1233,7 @@ public record FleetConfig( String tabLabel) { /** - * Role first, so the tab bar reads as the fleet and so the label shares a namespace with a - * lead's {@code tabPrefix}. Because {@code {role}} comes from a closed enum, a generated - * member label can never begin with {@code "lead:"} — the clash that - * {@link #validateLeadTabPrefixes()} used to have to check for is unrepresentable here. + * Role first, so the tab bar identifies the member's fleet role. */ public static final String DEFAULT_TAB_LABEL = "{role}: {profile} #{n}"; @@ -2685,28 +2679,11 @@ public record FleetConfig( } /** - * Reject a lead-scan convention that a worker tab would also satisfy (CB-531). + * Reject a member tab-label template that could render as a configured lead tab or match a + * lead-tab naming convention. * - *

The scan reads a tab label and concludes "a lead lives here". fleetd also writes - * tab labels — every member gets one rendered into its tab. Choose a lead {@code tabPrefix} that - * a member template matches and the daemon starts labelling its own members as leads, promoting - * the entire fleet to {@link dev.ltms.fleet.auth.Role#PRIMARY} with no message and no diff. - * {@link #validatePanePlacementAgainstLeadTabs()} is the check that stops a pane-placed member - * from landing inside a lead's tab in the first place; this check is a second, independent - * guard that catches the hazard even when every profile places members correctly, by refusing - * a label that a scan would still misread as a lead. - * - *

CB-557 shrank this check rather than removing it. The default template is - * {@code "{role}: {profile} #{n}"} and {@code {role}} comes from a closed enum, so a - * generated label can no longer collide by construction. What remains checkable is what - * an operator still writes by hand: the {@code fleet.tabLabel} template and any per-profile - * {@code tabLabel} override. - * - *

Fatal rather than a warning, unlike {@link #warnUnknownTopLevelKeys}: an unknown key means - * a feature does nothing, while this means a feature does the opposite of what it says. - * - * @throws IllegalStateException when the fleet template or any profile's {@code tabLabel} - * override starts with a configured lead prefix + * @throws IllegalStateException when the fleet template or a profile {@code tabLabel} override + * can render as a configured lead tab or match a lead-tab prefix */ public void validateLeadTabPrefixes() { if (fleet == null || fleet.leaders().isEmpty()) { @@ -2717,20 +2694,30 @@ public record FleetConfig( if (leader == null) { return; } + String tab = leader.tab(); String prefix = leader.tabPrefix(); - // The fleet-wide template is checked once per prefix: it labels every member that has no - // override, so one bad template promotes the entire fleet, not one profile. - if (startsWithIgnoreCase(fleet.tabLabel(), prefix)) { + if (templateCanRenderAs(fleet.tabLabel(), tab)) { + bad.add("fleet.tabLabel=\"" + fleet.tabLabel() + "\" can render as the tab of " + + "lead '" + leadName + "' (\"" + tab + "\")"); + } else if (startsWithIgnoreCase(fleet.tabLabel(), prefix)) { bad.add("fleet.tabLabel=\"" + fleet.tabLabel() + "\" starts with the tabPrefix of " + "lead '" + leadName + "' (\"" + prefix + "\")"); } profiles().entrySet().stream() - .filter(e -> startsWithIgnoreCase(e.getValue().tabLabel(), prefix)) .map(Map.Entry::getKey) .sorted() - .forEach(p -> bad.add("profile '" + p + "' overrides tabLabel with \"" - + profiles().get(p).tabLabel() + "\", which starts with the tabPrefix of " - + "lead '" + leadName + "' (\"" + prefix + "\")")); + .forEach(p -> { + String label = profiles().get(p).tabLabel(); + if (templateCanRenderAs(label, tab)) { + bad.add("profile '" + p + "' overrides tabLabel with \"" + label + + "\", which can render as the tab of lead '" + leadName + + "' (\"" + tab + "\")"); + } else if (startsWithIgnoreCase(label, prefix)) { + bad.add("profile '" + p + "' overrides tabLabel with \"" + label + + "\", which starts with the tabPrefix of lead '" + leadName + + "' (\"" + prefix + "\")"); + } + }); }); if (bad.isEmpty()) { return; @@ -2741,6 +2728,31 @@ public record FleetConfig( + "lead tabs cannot be confused."); } + private static boolean templateCanRenderAs(String template, String tab) { + if (template == null || template.isBlank() || tab == null || tab.isBlank()) { + return false; + } + var placeholders = Pattern.compile("\\{(?:role|profile|model|n)}").matcher(template); + StringBuilder expression = new StringBuilder("^"); + int literalStart = 0; + while (placeholders.find()) { + expression.append(Pattern.quote(template.substring(literalStart, placeholders.start()))); + expression.append(".*"); + literalStart = placeholders.end(); + } + expression.append(Pattern.quote(template.substring(literalStart))).append("$"); + return Pattern.compile(expression.toString(), Pattern.CASE_INSENSITIVE).matcher(tab).matches(); + } + + /** Case-insensitive prefix test that tolerates a null or blank label. */ + private static boolean startsWithIgnoreCase(String label, String prefix) { + if (label == null || prefix == null || prefix.isBlank()) { + return false; + } + String stripped = label.strip(); + return stripped.regionMatches(true, 0, prefix, 0, prefix.length()); + } + /** * Reject a profile that places its members by {@code "pane"} while any {@code fleet.leaders} * entry names a {@code tab}. A pane-placed member lands inside the focused tab rather than its @@ -2800,15 +2812,6 @@ public record FleetConfig( } } - /** Case-insensitive prefix test that tolerates a null/blank label. */ - private static boolean startsWithIgnoreCase(String label, String prefix) { - if (label == null || prefix == null || prefix.isBlank()) { - return false; - } - String stripped = label.strip(); - return stripped.regionMatches(true, 0, prefix, 0, prefix.length()); - } - /** * Reject a subscription profile whose {@code env:} block tries to reseat the Anthropic binding * (CB-542). diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java index 508e4609..379858a9 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java @@ -676,12 +676,8 @@ class FleetConfigTest { assertEquals(5, hb.quietNudgeCap()); } - /** - * The hazard the guard exists for: fleetd writes worker tab labels and reads lead tab labels. - * Overlap the two and every worker it spawns is read back as a lead. - */ @Test - void aLeadPrefixThatAProfileTabLabelOverrideAlsoMatchesRefusesToStart(@TempDir Path dir) + void aProfileTabLabelOverrideMatchingALeadTabRefusesToStart(@TempDir Path dir) throws Exception { Path f = dir.resolve("collide.yaml"); Files.writeString(f, """ @@ -689,32 +685,33 @@ class FleetConfigTest { port: 8080 profiles: gx10: - tabLabel: "lead: {profile} #{n}" + tabLabel: "alpha" fleet: leaders: opus: - tab: "lead: opus" - tabPrefix: "lead:" + tab: "alpha" """); FleetConfig cfg = FleetConfig.load(f); IllegalStateException e = assertThrows(IllegalStateException.class, cfg::validateLeadTabPrefixes); assertTrue(e.getMessage().contains("gx10"), "the message must name the offending profile"); + assertTrue(e.getMessage().contains("alpha"), "the message must name the offending label"); } - /** A bad fleet-wide template promotes every member, not one profile — so it is checked too. */ @Test - void aFleetTabLabelThatMatchesALeadPrefixRefusesToStart(@TempDir Path dir) throws Exception { + void aFleetTabLabelTemplateThatCanRenderAsALeadTabRefusesToStart(@TempDir Path dir) throws Exception { Path f = dir.resolve("collide-template.yaml"); Files.writeString(f, """ bind: port: 8080 + profiles: + pha: {} fleet: - tabLabel: "lead: {role} {profile}" + tabLabel: "al{profile}" leaders: opus: - tab: "lead: opus" + tab: "alpha" """); FleetConfig cfg = FleetConfig.load(f); @@ -723,12 +720,28 @@ class FleetConfigTest { assertTrue(e.getMessage().contains("fleet.tabLabel")); } - /** - * The point of making role the label's first field: {@code {role}} comes from a closed enum, so - * a generated label cannot begin with {@code "lead:"} however the fleet is configured. - */ @Test - void theDefaultTabLabelCannotCollideWithTheDefaultLeadPrefix(@TempDir Path dir) throws Exception { + void anExactFleetTabLabelCollisionRefusesToStart(@TempDir Path dir) throws Exception { + Path f = dir.resolve("exact-tab-collision.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + fleet: + tabLabel: "alpha" + leaders: + alpha: + tab: "alpha" + """); + + IllegalStateException e = assertThrows(IllegalStateException.class, + () -> FleetConfig.load(f).validateAll()); + assertTrue(e.getMessage().contains("fleet.tabLabel"), + "the message must name the offending label"); + assertTrue(e.getMessage().contains("alpha"), "the message must name the colliding lead tab"); + } + + @Test + void aFleetTabLabelTemplateThatCannotRenderAsALeadTabIsAllowed(@TempDir Path dir) throws Exception { Path f = dir.resolve("ok.yaml"); Files.writeString(f, """ bind: @@ -737,17 +750,13 @@ class FleetConfigTest { gx10: baseUrl: http://gx00.gw:8000 fleet: + tabLabel: "worker-{profile}" leaders: opus: - tab: "lead: opus" + tab: "alpha" """); - assertDoesNotThrow(() -> FleetConfig.load(f).validateLeadTabPrefixes()); - for (MemberRole role : MemberRole.values()) { - assertFalse(FleetConfig.Fleet.DEFAULT_TAB_LABEL - .replace("{role}", role.wireName()).startsWith("lead:"), - "no role renders a label that reads as a lead"); - } + assertDoesNotThrow(() -> FleetConfig.load(f).validateAll()); } @Test diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigValidateAllTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigValidateAllTest.java index 7a60fd24..eb4b0e54 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigValidateAllTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigValidateAllTest.java @@ -17,63 +17,21 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; /** - * The gap this class exists to close: mutation testing on the fleetd ticket "central allow-list - * of usable models" found that although {@link FleetConfig#validateModels()}'s own logic was well - * pinned, nothing proved either real caller ({@code Fleetd.main} and {@link ConfigRef#reload()}) - * still invoked it — deleting the call site left the full suite green (1478/0/0/0). A follow-up - * measurement (same technique — remove one call site, run the suite, not read the code) found the - * SAME gap for all five of {@link FleetConfig}'s other validators at startup, and for four of the - * six inside {@link ConfigRef#reload()}. This is a class of gap, not one line's mistake: every one - * of those thirteen tests called the validator itself directly, never the real caller that was - * supposed to. + * Tests the reflective validator sweep and {@link FleetConfig#validateAll()} reachability. * - *

The fix replaces the six individual {@code cfg.validateXxx()} calls at each of the two real - * call sites with one {@link FleetConfig#validateAll()}, which reaches every validator by - * reflection rather than by a hand-maintained list of names. A hand-maintained list of six names - * would have exactly the defect it replaces: the seventh validator someone adds next month has no - * reason to be added to it, and nothing would say so. This class proves TWO separate claims, and - * keeps them separate on purpose: - * - *

    - *
  1. {@link #theSweepMechanismIsGenericNotHardcodedToFleetConfigsSixNames()} and its neighbours - * prove the reflective sweep itself ({@link FleetConfig#invokeAllValidators}) is a general - * mechanism — it runs whatever public, no-arg, void {@code validateXxx()} methods a class - * happens to declare today, including a class with more of them than {@link FleetConfig} - * has right now. This is the proof that a future, real seventh validator on {@link - * FleetConfig} would be swept automatically, without needing to add a real (unwanted) - * seventh validator just to exercise the claim.
    2. - *
  2. {@link #validateAllReachesEveryOneOfTodaysRealValidators()} proves {@link - * FleetConfig#validateAll()} itself is wired to that same generic mechanism and genuinely - * reaches every one of today's real validators — reusing the exact minimal failing - * configurations {@code FleetConfigTest} already established for each one directly, plus a - * dedicated fixture for {@link FleetConfig#validateLeadRollover()}, which no other test - * drives through {@code validateAll()} — so a single call to {@code validateAll()} is shown - * to reproduce every one of those failures.
    3. - *
- * - *

Together with the direct-{@code Fleetd.main}-invocation tests in {@code - * FleetdStartupValidationTest} (which prove the real startup call site still calls {@code - * validateAll()}) and the {@code ConfigRefTest} reload tests (which prove the same for {@link - * ConfigRef#reload()}), removing {@code cfg.validateAll();} from either real call site now fails - * a test in this module. - * - *

What is NOT pinned, measured rather than assumed. Reverting {@link - * FleetConfig#validateAll()} to a hardcoded list of today's method calls leaves the whole - * suite green. Nothing ties {@code validateAll()} to - * the generic sweep — claim 1 proves {@link FleetConfig#invokeAllValidators} is generic, and claim - * 2 proves {@code validateAll()} reaches today's validators, and a hardcoded list satisfies both. So the - * reflective sweep is a convenience, not the guarantee. The guarantee is {@link - * #fleetConfigDeclaresExactlyTheseValidatorsToday()}: it fails the moment any validator is added - * or removed, which forces whoever changes the set to look at this file. + *

{@link #theSweepMechanismIsGenericNotHardcodedToFleetConfigsSixNames()} and its neighbours + * prove that {@link FleetConfig#invokeAllValidators} runs each public, no-arg, void + * {@code validateXxx()} method on its target. {@link #fleetConfigDeclaresExactlyTheseValidatorsToday()} + * is the canary for the validator set. {@link #validateAllReachesEveryOneOfTodaysRealValidators()} + * is the reachability check for that set. */ class FleetConfigValidateAllTest { - // ── Claim 1: the reflective sweep is a general mechanism, not six names in disguise ────────── + // ── Claim 1: the reflective sweep is a general mechanism ───────────────────────────────────── /** - * A throwaway fixture class, unrelated to {@link FleetConfig} in every way except shape: three - * public, no-arg, void methods named {@code validateXxx}. Proves the sweep works on ANY class - * with this shape, not on something special-cased to {@link FleetConfig}. + * Fixture with public, no-arg, void methods named {@code validateXxx}. It proves the sweep uses + * the target's method shape rather than special handling for {@link FleetConfig}. */ static class ThreeValidators { final List ran = new ArrayList<>(); @@ -102,12 +60,8 @@ class FleetConfigValidateAllTest { } /** - * The core of the "self-maintaining" requirement: the exact same class shape as {@link - * ThreeValidators}, plus one more method — standing in for "a developer adds a validator next - * month". Nothing about the sweep changes to pick it up; the new method is invoked purely - * because it exists and matches the shape. This is what makes adding a seventh real validator - * to {@link FleetConfig} safe without touching {@link FleetConfig#validateAll()} or either - * call site — there is no "wire it in" step left to forget. + * Fixture with an added valid method. It proves the sweep reaches a method because it matches + * the validator shape. */ static class FourValidators { final List ran = new ArrayList<>(); @@ -135,7 +89,7 @@ class FleetConfigValidateAllTest { FleetConfig.invokeAllValidators(target); assertEquals(List.of("validateAlpha", "validateBeta", "validateDelta", "validateGamma"), sorted(target.ran), - "the fourth method must be reached automatically — proving a class can grow the " + "the added method must be reached automatically — proving a class can grow the " + "set of things it validates with no change to the sweep itself"); } @@ -297,16 +251,16 @@ class FleetConfigValidateAllTest { port: 8765 """, "auth.mode: token"); - // validateLeadTabPrefixes: a fleet-wide tabLabel that starts with a lead's own tabPrefix. + // validateLeadTabPrefixes: a fleet-wide tabLabel that equals a lead tab. assertValidateAllRefuses(dir, "lead-tab-prefixes.yaml", """ bind: host: 127.0.0.1 port: 8765 fleet: - tabLabel: "lead: {role} {profile}" + tabLabel: "alpha" leaders: opus: - tab: "lead: opus" + tab: "alpha" """, "fleet.tabLabel"); // validateSubscriptionProfiles: subscription: true with env: reseating ANTHROPIC_BASE_URL.