diff --git a/fleetd/fleetd.example.yaml b/fleetd/fleetd.example.yaml index 79a0d0a..dbf3383 100644 --- a/fleetd/fleetd.example.yaml +++ b/fleetd/fleetd.example.yaml @@ -306,6 +306,35 @@ profiles: # GOTCHA 2 — `maxLoad` is the ONLY throttle you have here. There is no metering, no budget # and no refusal on cost; the cap on live members is the single thing standing between a # fan-out and your monthly limit. Set it deliberately and keep it small. + # + # GOTCHA 3 (fleetd #176) — `maxLoad` counts members, never the lead itself. The lead is a live + # `claude` session on this SAME account (a lead is never moved off-subscription, whatever its + # own profile says), so it already holds one seat before any member spawns. If a lead's + # `fleet.leaders..profile` names THIS profile — or ANY OTHER `subscription: true` + # profile that shares this one's account (see THE SENTINEL, just below, next to + # `credentialId:`) — `fleet_list`'s `free` for this profile subtracts that lead's live + # seat(s) automatically; see `profile:` under THE FLEET below. If no lead entry names a + # profile sharing this account, fleetd has no way to know a lead holds a seat here, and `free` + # will overstate what a fresh `fleet_spawn` actually gets by exactly the seats the lead is + # quietly holding. + # + # THE SENTINEL (fleetd #176 stage 2, correcting an inert stage 1 fix): every `subscription: + # true` profile that leaves `credentialId` unset shares ONE implicit account-wide credential + # id with every other such profile on this host — because a subscription profile doesn't + # authenticate with a credential of its own, it authenticates as the operator's own Claude + # login, and there is exactly one of those. So on a typical host, `opus` (the lead's profile) + # and `sonnet` (the members' profile) are linked automatically, with NOTHING to set here — that + # is what makes GOTCHA 3 above work without also writing matching `credentialId:` values on + # both. This linkage is not just cosmetic: it is the same key `BackendQuarantine`/cool-off use, + # so a usage-limit hit on `opus` now quarantines `sonnet` too (and vice versa) — correct, since + # they are one Claude account, but worth knowing before you wonder why an unrelated-looking + # profile went quarantined. + # + # WHEN TO OVERRIDE — set explicit, DIFFERENT `credentialId:` values on two `subscription: true` + # profiles only when they are genuinely two separate Claude logins on the same host (a real, + # if unusual, setup). An explicit `credentialId` always wins over the sentinel, so this is the + # one way to keep two subscription profiles from being treated as one account for lead-seat + # counting AND for quarantine/cool-off grouping alike. # gitTokenEnv: GITEA_TOKEN # opt-in: let this profile's workers open their own PR (CB-302) # gitHostEnv: GITEA_HOST # defaults to GITEA_HOST; injected only with gitTokenEnv # exhaustedPattern: "usage limit has been reached" # opt-in: classify a usage-limit refusal (CB-578) @@ -503,6 +532,22 @@ fleet: # recognised: give it a `profile:` and the daemon launches the shortfall when fewer than # `instances` are live. Omit `profile:` and it is recognise-only, as before. # + # `profile:` has a SECOND job as of fleetd #176, even for a recognise-only lead you never want + # auto-launched: it is also how fleetd learns which account this lead's own session shares. A + # `subscription: true` profile bills the operator's Claude account, and the lead itself is always + # a live `claude` session on that same account — `maxLoad` never counted that seat. If a lead + # entry here names a profile that shares a worker profile's account, `fleet_list`'s `free` for + # that worker profile subtracts the lead's live seat(s) automatically. "Shares the account" is + # decided by matching `effectiveCredentialId()`, which (fleetd #176 stage 2 — see THE SENTINEL, + # next to `credentialId:`, in THE WORKERS above) means: an explicit, matching `credentialId:` on + # both, OR — the common case, needing NO extra config — both being `subscription: true` with + # `credentialId` left unset, since those all share one implicit account-wide id. A lead on `opus` + # and workers on `sonnet` link automatically this way; they do NOT need the same profile name. + # Setting `profile:` on an already-running, recognise-only lead is safe — the daemon only launches + # the SHORTFALL below `instances`, so naming a profile here does not, by itself, start anything. + # Omit it and fleetd has no way to derive the sharing — there is no other reliable signal on the + # daemon's side — so that lead's seat goes uncounted, exactly as before this ticket. + # # `tab:` (CB-579) is REQUIRED and is the only field identity depends on — the exact label of the # tab hosting the lead, matched case-insensitively. Label the tab yourself and put that same # string here, and the pane is recognised on the next rescan. Reopen the tab later, or the session diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index c17db37..c397ea9 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -641,7 +641,8 @@ public final class Fleetd { new FleetMcp.OutageSource(profile -> { var configured = config.get().profiles().get(profile); return configured == null ? null : configured.effectiveCredentialId(); - }, outagePolicy)); + }, outagePolicy), + new FleetMcp.LeadSeatSource(leadSeatLookup(() -> config.get().profiles(), leaders, leads))); // CB-637: the receive half. Only constructed when a lead mailbox actually opened — with no // coordinator (or an unreachable one) there is nothing to deliver, so no scheduler is @@ -769,6 +770,68 @@ public final class Fleetd { .orElse(null); } + /** + * fleetd #176: per-profile factory for {@link FleetMcp.LeadSeatSource} — how many seats a + * profile's own live LEAD session(s) hold on the same Claude subscription. + * + *

{@code maxLoad} counts only members; the lead itself is a live {@code claude} session that + * is never moved off-subscription ({@code LeadLauncher} strips {@code ANTHROPIC_BASE_URL}/ + * {@code AUTH_TOKEN} from a lead's env whatever its profile says), so a {@code subscription: + * true} profile's real ceiling is lower than its configured {@code maxLoad} by exactly the + * number of lead seats sharing that same account. + * + *

The derivation, and why this route was chosen over a new config key. The link is + * {@code fleet.leaders..profile} — the field the operator already sets to name which + * {@code profiles:} entry a lead runs on (CB-557; see {@code fleetd.example.yaml}) — matched + * against the profile passed in here via {@link FleetConfig.Profile#effectiveCredentialId()}, + * the same grouping key {@link dev.ltms.fleet.placement.BackendQuarantine} already uses to say + * two profiles share one account. Nothing new is added to the config schema: this reuses a field + * that already exists and already means "the profile this lead's own session runs on". A lead + * entry that names no {@code profile:} (recognise-only, CB-558) says nothing about which account + * it shares, and there is no other reliable signal on the daemon's side to derive that from — so + * such a lead contributes no seats, exactly as before this ticket. Making that lead's seat count + * requires the operator to add one line (`profile: sonnet` under its {@code fleet.leaders} entry) + * — a config statement, not a code change, and the smallest one available given the field + * already exists for a closely related purpose. + * + *

Only counts leads {@code liveLeadTerminals} currently reports — CB-531's live tab scan (or + * the legacy {@code primary.terminal} pin) — never every configured lead: an entry whose + * {@code instances} nobody has actually started is not really competing for a seat, and must not + * shrink capacity for one that was never live. + * + * @param profiles the live profile map, normally {@code () -> config.get().profiles()} + * in {@code main} — hot, like every other {@code maxLoad}/ + * {@code credentialId} read {@link FleetMcp.CapacitySource} already does + * @param leaders {@code fleet.leaders}, read once at startup like the rest of that + * block ({@code fleetd.example.yaml} notes it is not hot) — passed as a + * plain map, never re-read from {@code config.get()} + * @param liveLeadTerminals terminal_id → lead name for every CURRENTLY recognised lead, normally + * the same supplier {@link dev.ltms.fleet.auth.CallerResolver#leads()} + * and {@code LeadCoordLoop} already consult + */ + static Function leadSeatLookup(Supplier> profiles, + Map leaders, Supplier> liveLeadTerminals) { + return profileName -> { + FleetConfig.Profile target = profiles.get().get(profileName); + if (target == null || !target.isSubscription()) { + return 0; + } + String targetCredential = target.effectiveCredentialId(); + int seats = 0; + for (String leadName : liveLeadTerminals.get().values()) { + FleetConfig.Leader lead = leaders.get(leadName); + if (lead == null || lead.profile() == null || lead.profile().isBlank()) { + continue; + } + FleetConfig.Profile leadProfile = profiles.get().get(lead.profile()); + if (leadProfile != null && targetCredential.equals(leadProfile.effectiveCredentialId())) { + seats++; + } + } + return seats; + }; + } + /** * fleetd #248 / fleetd#201 Unit 5: package-private factory for the per-target backend-error * pattern lookup {@link CompletionResolver} classifies a pane scrape against. Closes over the 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 81300ac..d30f365 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java @@ -92,9 +92,15 @@ import java.util.regex.PatternSyntaxException; * fleetd's own process, so fleetd's own {@code $SHELL} says nothing about what * that pane runs. There is no channel to ask herdr for another user's shell, so * this must be told, never guessed. {@code null}/blank (or a value not ending - * in {@code zsh}) is treated the same as "not zsh": the {@code - * memberCredentials.policy: allow-list} ZDOTDIR scrub is skipped in favour of - * the CB-596 sentinel overlay — a degraded control, never a refusal to spawn. + * in {@code zsh}) is treated the same as "not zsh". fleetd #155: what that means + * now depends on {@code memberCredentials.policy}. Under {@code deny-by-default} + * it stays a degraded control, never a refusal to spawn — the pane-creation + * overlay is unaffected by shell type, so the spawn proceeds with a WARN naming + * the shell. Under {@code allow-list} the spawn is REFUSED instead: that policy's + * whole point is a control a sourced file cannot undo, so silently falling back + * to the weaker overlay would be the same "control silently does nothing" defect + * #155 exists to remove — configure this field (or move the account to zsh, or + * switch policy back to {@code deny-by-default}) to unblock the spawn. * When {@code memberHerdrSocket} is NOT configured this field is never * consulted at all; fleetd keeps reading its own {@code $SHELL}, exactly as * before this field existed. @@ -644,14 +650,46 @@ public record FleetConfig( } /** - * The credential group this profile quarantines with (CB-578 stage B): the configured - * {@link #credentialId} when set, else this profile's own name — so an unconfigured profile - * quarantines alone, exactly as it did before this field existed. Two profiles that set the - * same non-blank {@code credentialId} share one quarantine: a {@code BACKEND_EXHAUSTED} - * classification on either one quarantines both. + * The shared credential id every {@code subscription: true} profile falls back to when it + * sets no explicit {@link #credentialId} (fleetd #176 stage 2, correcting an inert first cut + * of that ticket). A subscription profile has no credential of its own to fall back to its + * name for: it authenticates as the operator's own Claude login, and a host has exactly one + * of those, whatever names the operator gives the profiles running on it. Falling back to the + * profile's own name (the way an ordinary off-subscription profile does) would keep two + * subscription profiles on one login apart from each other, which is the opposite of what + * "one account" means. + * + *

Measured live and what it broke: a lead on profile {@code opus}, members on profile + * {@code sonnet}, same Claude login, neither setting {@code credentialId}. Before this + * sentinel, {@code opus.effectiveCredentialId()} was {@code "opus"} and {@code sonnet + * .effectiveCredentialId()} was {@code "sonnet"} — so fleetd #176's lead-seat matcher (and, + * this sentinel now also fixes, {@code CompositePeerLauncher}'s quarantine/cool-off spawn + * refusal and {@code BackendOutagePolicy}'s incident grouping) silently never linked them: the + * fix shipped, and stayed inert on the one host it was written for. + */ + public static final String SUBSCRIPTION_CREDENTIAL_ID = ""; + + /** + * The credential group this profile quarantines with (CB-578 stage B; extended fleetd #176 + * stage 2 — see {@link #SUBSCRIPTION_CREDENTIAL_ID}): the configured {@link #credentialId} + * when set — that always wins, so an operator with two separate Claude logins on one host can + * still keep them apart. Otherwise, a {@code subscription: true} profile falls back to + * {@link #SUBSCRIPTION_CREDENTIAL_ID} rather than its own name; an ordinary off-subscription + * profile falls back to its own name, exactly as it did before this field existed, so an + * unconfigured off-subscription profile still quarantines alone. + * + *

A {@code BACKEND_EXHAUSTED} (or repeated backend-error) classification on any profile + * sharing the result quarantines/cools off every profile that shares it — including, now, + * every {@code subscription: true} profile with no explicit {@code credentialId}. That is + * intended, not incidental: one Claude subscription hitting a usage limit really does take out + * every profile running on it, the same way {@code credentialId: openai-shared} already lets + * two OpenAI-backed profiles share one quarantine. */ public String effectiveCredentialId() { - return (credentialId == null || credentialId.isBlank()) ? profile : credentialId; + if (credentialId != null && !credentialId.isBlank()) { + return credentialId; + } + return isSubscription() ? SUBSCRIPTION_CREDENTIAL_ID : profile; } /** True when this profile's workers are granted a forge token to open their own PR (CB-302). */ @@ -946,7 +984,15 @@ public record FleetConfig( * survives restarts of the agent inside it — so identity is now the tab label alone. * * @param profile the {@code profiles:} entry to launch this lead on when one must - * be created; {@code null} ⇒ recognise-only, never create + * be created; {@code null} ⇒ recognise-only, never create. + *

fleetd #176: also the field {@code Fleetd.leadSeatLookup} reads + * to learn which account this lead's own live session shares — set it + * (safely, even on an already-running recognise-only lead: naming a + * profile here never starts anything beyond {@code instances}) so a + * {@code subscription: true} worker profile sharing its + * {@code effectiveCredentialId()} has this lead's seat subtracted from + * {@code fleet_list}'s {@code free}. {@code null} here also means this + * lead's seat cannot be derived and is not counted. * @param tab the exact tab label hosting this lead, matched case-insensitively; * the only field identity depends on. Required — a lead with no * {@code tab} can never be discovered, launched or not diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java index 22c828d..332e153 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -96,6 +96,8 @@ public final class FleetMcp { private final QuarantineSource quarantine; /** fleetd #201 Unit 5: SEPARATE from {@link #quarantine} — see {@link OutageSource}'s doc. */ private final OutageSource outage; + /** fleetd #176: SEPARATE from both of the above — see {@link LeadSeatSource}'s doc. */ + private final LeadSeatSource leadSeats; /** CB-637: this daemon's lead-to-lead channel; {@code null} when no coordinator is configured. */ private final LeadChannel leadChannel; @@ -135,6 +137,24 @@ public final class FleetMcp { } } + /** + * fleetd #176: the seats a profile's own live LEAD session(s) hold on the same Claude + * subscription — the third reason (alongside {@link QuarantineSource} and {@link OutageSource}) + * {@code free} can overstate what a fresh {@code fleet_spawn} would actually get. + * + *

{@code maxLoad} counts only members, never the lead itself. But a + * {@code subscription: true} profile bills the operator's own Claude account, and the lead is + * always a live {@code claude} session on that same account (it is never moved off-subscription + * — see {@code LeadLauncher}). So a fan-out that fills every member slot still leaves the lead's + * own seat unaccounted for, and the daemon reports a slot that was never really free. See + * {@code Fleetd.leadSeatLookup} for how the count is derived — from {@code fleet.leaders. + * .profile} and each profile's {@code effectiveCredentialId()}, never a hardcoded constant. + */ + public record LeadSeatSource(Function seatsFor) { + /** Inert source — no profile is ever reported as sharing a seat with a lead. */ + public static LeadSeatSource none() { return new LeadSeatSource(_ -> 0); } + } + /** * @param callers resolves each call's {@link Principal}; {@code null} disables authorization. * This surface needs its own enforcement: {@code /mcp} is a raw servlet on @@ -149,7 +169,7 @@ public final class FleetMcp { CallerResolver callers, Metrics metrics, CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine) { this(messages, workers, sessions, identity, presence, primaryRegistry, callers, metrics, capacity, - healthCoverage, quarantine, null, OutageSource.none()); + healthCoverage, quarantine, null, OutageSource.none(), LeadSeatSource.none()); } /** @@ -163,12 +183,12 @@ public final class FleetMcp { CallerResolver callers, Metrics metrics, CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine, LeadChannel leadChannel) { this(messages, workers, sessions, identity, presence, primaryRegistry, callers, metrics, capacity, - healthCoverage, quarantine, leadChannel, OutageSource.none()); + healthCoverage, quarantine, leadChannel, OutageSource.none(), LeadSeatSource.none()); } /** * As above, with fleetd #201 Unit 5 cool-off facts for {@code fleet_list}/{@code fleet_profiles} - * (see {@link OutageSource}). This is what {@code Fleetd.main} actually wires up. + * (see {@link OutageSource}). * * @param outage required — pass {@link OutageSource#none()} for a caller that does not want the * feature, never a defaulting overload (the same rule {@code quarantine} follows). @@ -177,10 +197,28 @@ public final class FleetMcp { ConnectionIdentity identity, MemberPresence presence, PrimaryRegistry primaryRegistry, CallerResolver callers, Metrics metrics, CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine, LeadChannel leadChannel, OutageSource outage) { + this(messages, workers, sessions, identity, presence, primaryRegistry, callers, metrics, capacity, + healthCoverage, quarantine, leadChannel, outage, LeadSeatSource.none()); + } + + /** + * As above, with fleetd #176 lead-seat facts (see {@link LeadSeatSource}). This is what + * {@code Fleetd.main} actually wires up. + * + * @param leadSeats required — pass {@link LeadSeatSource#none()} for a caller that does not want + * the feature, never a defaulting overload (the same rule {@code quarantine} and + * {@code outage} follow). + */ + public FleetMcp(MessageService messages, PeerLauncher workers, SessionManager sessions, + ConnectionIdentity identity, MemberPresence presence, PrimaryRegistry primaryRegistry, + CallerResolver callers, Metrics metrics, CapacitySource capacity, HealthCoverageSource healthCoverage, + QuarantineSource quarantine, LeadChannel leadChannel, OutageSource outage, + LeadSeatSource leadSeats) { this.leadChannel = leadChannel; this.capacity = capacity; this.quarantine = Objects.requireNonNull(quarantine, "quarantine"); this.outage = Objects.requireNonNull(outage, "outage"); + this.leadSeats = Objects.requireNonNull(leadSeats, "leadSeats"); this.healthCoverage = healthCoverage; McpJsonMapper json = new JacksonMcpJsonMapperSupplier().get(); this.transport = HttpServletStreamableServerTransportProvider.builder() @@ -310,7 +348,7 @@ public final class FleetMcp { McpSchema.CallToolResult denied = deny(exchange, Authz.Action.READ, null); if (denied != null) return denied; return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, outage, - callers == null ? Map.of() : callers.leads(), + leadSeats, callers == null ? Map.of() : callers.leads(), callerTerminal(exchange), leadChannel == null ? null : leadChannel.selfCoordId()); }; @@ -994,7 +1032,8 @@ public final class FleetMcp { CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine, OutageSource outage, Map leads, String selfTerm) { - return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, outage, leads, selfTerm, null); + return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, outage, + LeadSeatSource.none(), leads, selfTerm, null); } /** @@ -1011,7 +1050,7 @@ public final class FleetMcp { QuarantineSource quarantine, Map leads, String selfTerm, String selfCoordId) { return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, OutageSource.none(), - leads, selfTerm, selfCoordId); + LeadSeatSource.none(), leads, selfTerm, selfCoordId); } /** As above, plus fleetd #201 Unit 5 cool-off facts (see {@link OutageSource}). */ @@ -1019,6 +1058,16 @@ public final class FleetMcp { CapacitySource capacity, HealthCoverageSource healthCoverage, QuarantineSource quarantine, OutageSource outage, Map leads, String selfTerm, String selfCoordId) { + return listFleet(workers, sessions, messages, capacity, healthCoverage, quarantine, outage, + LeadSeatSource.none(), leads, selfTerm, selfCoordId); + } + + /** As above, plus fleetd #176 lead-seat facts (see {@link LeadSeatSource}). */ + static McpSchema.CallToolResult listFleet(PeerLauncher workers, SessionManager sessions, MessageService messages, + CapacitySource capacity, HealthCoverageSource healthCoverage, + QuarantineSource quarantine, OutageSource outage, + LeadSeatSource leadSeats, Map leads, String selfTerm, + String selfCoordId) { try { Map live = workers.list().stream() .map(Agent.class::cast) @@ -1045,7 +1094,7 @@ public final class FleetMcp { } if (capacity.available()) result.put("capacity", profiles.stream() .map(profile -> capacityView(profile, capacity.liveCount(), capacity.maxLoad(), roster, messages, - capacity.clock().getAsLong(), quarantine, outage)).toList()); + capacity.clock().getAsLong(), quarantine, outage, leadSeats)).toList()); return text(json(result)); } catch (HerdrException e) { return error("herdr error listing the fleet: " + e.getMessage()); @@ -1084,20 +1133,33 @@ public final class FleetMcp { * {@code credentialId}/{@code coolingOffForSeconds}, but never {@code quarantinedForSeconds} — * that key is added only when exhaustion quarantine is ALSO active for this profile, since the * two checks are independent and either, both, or neither can be true. + * + *

fleetd #176: {@code maxLoad} counts panes, not subscription seats — it never counted the + * lead's own seat on a {@code subscription: true} profile's account. {@link LeadSeatSource} + * reports that count (0 for a non-subscription profile, or when no live lead shares its + * credential), and it is subtracted from {@code free} the same way {@code live} already is — + * {@code maxLoad} itself is left untouched, so the row still reports the configured cap. The + * {@code leadSeats} key is added only when the count is positive, for the same + * byte-identical-when-unused reason as the quarantine/cool-off keys above. */ private static Map capacityView(String profile, Function liveCount, Function maxLoad, List roster, MessageService messages, long nowNanos, QuarantineSource quarantine, - OutageSource outage) { + OutageSource outage, LeadSeatSource leadSeats) { Integer cap = maxLoad.apply(profile); int live = liveCount.apply(profile); + int leadSeatCount = leadSeats.seatsFor().apply(profile); int reclaimable = (int) roster.stream().filter(s -> profile.equals(s.profile())) .filter(s -> (s.state() == MemberSession.State.READY || s.state() == MemberSession.State.DONE)) .filter(s -> messages == null || (!messages.hasAcceptedDelivery(s.terminalId()) && !messages.hasInboxMessage(s.terminalId()))) .count(); Map row = new LinkedHashMap<>(); row.put("profile", profile); row.put("maxLoad", cap); row.put("live", live); - row.put("free", cap == null ? null : Math.max(0, cap - live)); row.put("reclaimable", reclaimable); + row.put("free", cap == null ? null : Math.max(0, cap - live - leadSeatCount)); + row.put("reclaimable", reclaimable); + if (leadSeatCount > 0) { + row.put("leadSeats", leadSeatCount); + } String credentialId = quarantine.credentialIdFor().apply(profile); if (credentialId != null) { quarantine.quarantine().remainingSeconds(credentialId).ifPresent(remaining -> { diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java index 3ae40d7..990b076 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -184,7 +184,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { */ private final ConcurrentMap zdotdirByPane = new ConcurrentHashMap<>(); - /** Guards {@link #warnNonZsh} to one WARN per launcher instance, not one per spawn. */ + /** + * Guards {@link #warnNonZsh} to one WARN per launcher instance, not one per spawn. fleetd #155: + * only the {@code policy: deny-by-default} path still warns on a non-zsh shell — the {@code + * policy: allow-list} path refuses the spawn instead (see {@link #applyEnvironmentAllowListPolicy}). + */ private final AtomicBoolean nonZshShellWarned = new AtomicBoolean(); /** Live config provides URI environment names that must never enter member panes. */ private final Supplier config; @@ -1255,6 +1259,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { if (!creds.isAllowList()) { overlayBlockedCredentials(workerEnv, creds); logCredentialGap(creds, null); + // fleetd #155: this overlay itself does not depend on the shell (it lands in the + // pane-creation env map before any shell runs), so the spawn is never refused here — + // only policy=allow-list's ZDOTDIR scrub needs a login shell to run at all. Still worth + // telling the operator: the stronger post-shell control is unavailable on this shell. + warnNonZsh(memberLoginShell()); } } @@ -1307,10 +1316,16 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { * dev.ltms.fleet.session.Worktrees#shareWithGroup} already uses, reused rather than * inventing a second group key. Either one missing means the scrub cannot be guaranteed * reachable by the member, which is the same "cannot guarantee the scrub runs" case as a - * non-zsh shell, so it gets the identical fallback. + * non-zsh shell — that one still gets the fallback (see fleetd #155's javadoc note below + * on why a missing worktreeRoot/worktreeGroup is a different defect, out of that ticket's + * scope). * - * In every branch: never refuse to spawn. A degraded credential control must not become an - * outage for an opt-in feature. + * fleetd #155: the one exception to "never refuse to spawn" is a non-zsh login shell, right + * below. The operator asked for {@code policy: allow-list} specifically — its whole point is a + * control a sourced file cannot undo — so silently degrading to the weaker overlay is exactly + * the "silently does nothing" failure this ticket exists to remove. Every other branch in this + * method (worktreeRoot/worktreeGroup missing) keeps the old "degrade, never refuse" behaviour; + * that gap is real but is fleetd #213's scope, not this one. */ private Path applyEnvironmentAllowListPolicy(FleetConfig.Profile cfg, Launch launch) { FleetConfig.MemberCredentials creds = memberCredentials == null ? null : memberCredentials.get(); @@ -1324,20 +1339,21 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { // must not even be called on this path, only the explicit memberLoginShell: config can // answer it. With memberHerdrSocket absent, nothing here changes: fleetd's own $SHELL is // still the input, exactly as before this fix. - String loginShell = memberHerdrSocket ? configuredMemberLoginShell() : resolveEnv("SHELL"); + String loginShell = memberLoginShell(); boolean zsh = isZshShell(loginShell); if (!zsh) { - // A non-zsh login shell ignores ZDOTDIR entirely: NO scrub would run, so pretending - // otherwise would be worse than saying so. Warn loudly and fall back to the CB-596 - // sentinel overlay over the enumerated known: names — weaker (a sourced file can undo - // it), but strictly better than nothing. Deliberately no "allowed N of M" line here: the - // scrub this count describes does not run on this path, so printing it would tell an - // operator that a fraction of names were blocked when the real number blocked is zero. - // logCredentialGap's WARN (below) is the only signal for this path. - warnNonZsh(loginShell); - overlayBlockedCredentials(launch.env(), creds); - logCredentialGap(creds, null); - return null; + // fleetd #155: a non-zsh login shell ignores ZDOTDIR entirely — NO scrub would run. The + // operator explicitly asked for policy=allow-list's blocking control, so degrading to + // the weaker CB-596 overlay and spawning anyway would be the same "control silently does + // nothing" defect this ticket exists to close. Refuse instead — the caller (FleetMcp.spawn) + // catches IllegalArgumentException and surfaces the message to the operator. + throw new IllegalArgumentException( + "memberCredentials policy=allow-list requires the member's login shell to be " + + "zsh, so the ZDOTDIR scrub can run after it — refusing to spawn under " + + "login shell '" + (loginShell == null ? "" : loginShell) + + "'. Configure memberLoginShell: as a zsh path (only read when " + + "memberHerdrSocket is set), move the member's OS account onto zsh, or " + + "set memberCredentials.policy: deny-by-default instead."); } Path parentDir; String group = null; @@ -1392,6 +1408,20 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { return cfg == null ? null : cfg.memberLoginShell(); } + /** + * fleetd #155: the member pane's login shell, using the same {@code memberHerdrSocket} routing + * {@link #applyEnvironmentAllowListPolicy} already used — fleetd's own {@code $SHELL} decides it + * when {@code memberHerdrSocket} is absent (today's only mode); the configured {@code + * memberLoginShell:} decides it otherwise, since fleetd's own {@code $SHELL} names a different + * user's shell once member panes run under a different OS user. Shared by both {@link + * #applyEnvironmentAllowListPolicy} (allow-list: refuses on non-zsh) and {@link + * #applyMemberCredentialPolicy} (deny-by-default: warns on non-zsh) so the two policies agree on + * what "the member's shell" means. + */ + private String memberLoginShell() { + return memberHerdrSocketConfigured() ? configuredMemberLoginShell() : resolveEnv("SHELL"); + } + /** * fleetd #213: {@code worktreeRoot}, as the ZDOTDIR scrub's parent directory under {@code * memberHerdrSocket}, or {@code null} when unconfigured — the same "cannot guarantee the scrub @@ -1476,15 +1506,23 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { private static final String SSH_AUTH_SOCK = MemberEnvAllowList.SSH_AUTH_SOCK; /** - * CB-633: a non-zsh login shell means the allow-list control CANNOT run — say so once per - * launcher instance, naming the shell, instead of failing silently. + * fleetd #155: called ONLY from the {@code policy: deny-by-default} branch of {@link + * #applyMemberCredentialPolicy} — under {@code policy: allow-list}, a non-zsh shell now REFUSES + * the spawn instead (see {@link #applyEnvironmentAllowListPolicy}), since that policy's whole + * point is a control a sourced file cannot undo. deny-by-default's own overlay does not depend + * on the shell (it lands in the pane-creation env map before any shell runs), so this is a + * heads-up, not a defect report: say once per launcher instance, naming the shell, that the + * stronger allow-list control is unavailable here — never silently. */ private void warnNonZsh(String shell) { + if (isZshShell(shell)) { + return; + } if (nonZshShellWarned.compareAndSet(false, true)) { - log.warn("memberCredentials policy=allow-list: member login shell '{}' is NOT zsh — " - + "ZDOTDIR scrubbing cannot run, so members' inherited environment is " - + "UNPROTECTED beyond the enumerated known: fallback. Move herdr onto a " - + "zsh account or switch policy back to deny-by-default.", + log.warn("memberCredentials policy=deny-by-default: member login shell '{}' is NOT zsh — " + + "the pane-creation credential overlay still applies here (it does not " + + "depend on the shell), but policy=allow-list's stronger post-shell " + + "ZDOTDIR scrub is unavailable on this shell.", shell == null ? "" : shell); } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadSeatLookupTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadSeatLookupTest.java new file mode 100644 index 0000000..d81b89a --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadSeatLookupTest.java @@ -0,0 +1,156 @@ +package dev.ltms.fleet; + +import dev.ltms.fleet.config.FleetConfig; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.util.Map; +import java.util.function.Function; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * fleetd #176: {@link Fleetd#leadSeatLookup} is the factory {@code Fleetd.main} wires into {@code + * FleetMcp.LeadSeatSource} so {@code fleet_list}'s {@code free} can subtract the seat(s) a + * {@code subscription: true} profile's own live LEAD session holds on that same account — + * {@code maxLoad} never counted the lead, only members. {@code FleetdLeadSeatWiringTest} proves + * {@code main} still passes this factory's result in; this class proves the factory's own matching + * logic: subscription-only, credential-matched, and counting only CURRENTLY LIVE leads. + */ +class FleetdLeadSeatLookupTest { + + private static FleetConfig.Profile subscriptionProfile(String name, String credentialId) { + return new FleetConfig.Profile(name, null, "claude-sonnet-5", null, null, null, + "tab", "fleet", "w #{n}", null, null, null, null, null, null, null, + null, 3, true, null, credentialId, null); + } + + private static FleetConfig.Profile offSubscriptionProfile(String name, String credentialId) { + return new FleetConfig.Profile(name, "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN", + null, "tab", "fleet", "w #{n}", null, null, null, null, null, null, null, + null, 2, false, null, credentialId, null); + } + + private static FleetConfig.Leader leadOnProfile(String profile) { + return new FleetConfig.Leader(profile, "lead: primary", 1, "lead:", 10, "claude", "claude-sonnet-5"); + } + + @Test + @DisplayName("a live lead sharing the target profile's credential counts as one seat") + void liveLeadSharingCredentialCountsAsOneSeat() { + Map profiles = Map.of("sonnet", subscriptionProfile("sonnet", null)); + Map leaders = Map.of("primary", leadOnProfile("sonnet")); + Function lookup = Fleetd.leadSeatLookup(() -> profiles, leaders, + () -> Map.of("term_primary", "primary")); + + assertEquals(1, lookup.apply("sonnet")); + } + + @Test + @DisplayName("no live lead names this profile ⇒ zero seats, exactly as before this ticket") + void noLiveLeadOnTheProfileCountsAsZero() { + Map profiles = Map.of("sonnet", subscriptionProfile("sonnet", null)); + Map leaders = Map.of("primary", leadOnProfile("sonnet")); + Function lookup = Fleetd.leadSeatLookup(() -> profiles, leaders, Map::of); + + assertEquals(0, lookup.apply("sonnet")); + } + + @Test + @DisplayName("a lead entry with no `profile:` (recognise-only) contributes no seats — cannot be derived") + void recogniseOnlyLeadWithNoProfileContributesNothing() { + Map profiles = Map.of("sonnet", subscriptionProfile("sonnet", null)); + FleetConfig.Leader recogniseOnly = new FleetConfig.Leader(null, "lead: primary", 1, "lead:", 10, + "claude", "claude-sonnet-5"); + Map leaders = Map.of("primary", recogniseOnly); + Function lookup = Fleetd.leadSeatLookup(() -> profiles, leaders, + () -> Map.of("term_primary", "primary")); + + assertEquals(0, lookup.apply("sonnet")); + } + + @Test + @DisplayName("a non-subscription profile never has a lead seat subtracted, whatever the credential match") + void nonSubscriptionProfileIsNeverAdjusted() { + Map profiles = Map.of( + "terra", offSubscriptionProfile("terra", "shared-openai"), + "sonnet", subscriptionProfile("sonnet", "shared-openai")); + Map leaders = Map.of("primary", leadOnProfile("sonnet")); + Function lookup = Fleetd.leadSeatLookup(() -> profiles, leaders, + () -> Map.of("term_primary", "primary")); + + assertEquals(0, lookup.apply("terra"), "terra is not subscription:true, so it must never be adjusted"); + } + + @Test + @DisplayName("explicit, different credentialIds still separate two subscription profiles (post fleetd #176 " + + "stage 2 sentinel)") + void differentCredentialIsNotCounted() { + Map profiles = Map.of( + "sonnet", subscriptionProfile("sonnet", "claude-account-a"), + "opus", subscriptionProfile("opus", "claude-account-b")); + Map leaders = Map.of("primary", leadOnProfile("opus")); + Function lookup = Fleetd.leadSeatLookup(() -> profiles, leaders, + () -> Map.of("term_primary", "primary")); + + assertEquals(0, lookup.apply("sonnet"), "different accounts must never be conflated into one seat count " + + "— an explicit credentialId on both sides must still win over the subscription sentinel, so an " + + "operator with two separate Claude logins on one host can keep them apart"); + } + + /** + * fleetd #176 stage 2 — the exact live shape that shipped inert: a lead on subscription profile + * {@code opus}, members on a DIFFERENTLY NAMED subscription profile {@code sonnet}, same Claude + * login, and NEITHER profile sets {@code credentialId}. Every other test in this class puts the + * lead on the SAME profile name as the target, which happened to keep working even with the old + * fall-back-to-profile-name {@code effectiveCredentialId()} — this is the one that did not, and + * its absence is what let the stage-1 fix ship without ever catching the bug it was filed for. + */ + @Test + @DisplayName("[LIVE SHAPE] lead on a DIFFERENT subscription profile, same account, neither sets " + + "credentialId ⇒ still counts as a seat") + void leadOnADifferentSubscriptionProfileSameAccountStillCountsAsASeat() { + Map profiles = Map.of( + "opus", subscriptionProfile("opus", null), + "sonnet", subscriptionProfile("sonnet", null)); + Map leaders = Map.of("primary", leadOnProfile("opus")); + Function lookup = Fleetd.leadSeatLookup(() -> profiles, leaders, + () -> Map.of("term_primary", "primary")); + + assertEquals(1, lookup.apply("sonnet"), "opus and sonnet are both subscription:true with no explicit " + + "credentialId, so they share one Claude login and the lead's live seat on opus must be charged " + + "against sonnet too — this is the live host's actual shape (fleetd #176 stage 2)"); + } + + @Test + @DisplayName("two live instances of the same lead count as two seats") + void twoLiveInstancesOfTheSameLeadCountAsTwoSeats() { + Map profiles = Map.of("sonnet", subscriptionProfile("sonnet", null)); + Map leaders = Map.of("primary", leadOnProfile("sonnet")); + Function lookup = Fleetd.leadSeatLookup(() -> profiles, leaders, + () -> Map.of("term_a", "primary", "term_b", "primary")); + + assertEquals(2, lookup.apply("sonnet")); + } + + @Test + @DisplayName("an unconfigured target profile resolves to zero, not a thrown exception") + void unconfiguredTargetProfileIsZero() { + Function lookup = Fleetd.leadSeatLookup(Map::of, Map.of(), Map::of); + + assertEquals(0, lookup.apply("ghost")); + } + + @Test + @DisplayName("live leads are read through the supplier on every call, not snapshotted") + void liveLeadsAreReadThroughOnEveryCall() { + Map profiles = Map.of("sonnet", subscriptionProfile("sonnet", null)); + Map leaders = Map.of("primary", leadOnProfile("sonnet")); + java.util.Map live = new java.util.HashMap<>(); + Function lookup = Fleetd.leadSeatLookup(() -> profiles, leaders, () -> live); + + assertEquals(0, lookup.apply("sonnet")); + live.put("term_primary", "primary"); + assertEquals(1, lookup.apply("sonnet")); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadSeatWiringTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadSeatWiringTest.java new file mode 100644 index 0000000..29ecdc3 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadSeatWiringTest.java @@ -0,0 +1,43 @@ +package dev.ltms.fleet; + +import java.nio.file.Files; +import java.nio.file.Path; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * fleetd #176: {@code Fleetd.main} builds its {@code FleetMcp} from a 14-argument constructor whose + * last argument is a {@code FleetMcp.LeadSeatSource} wrapping {@link Fleetd#leadSeatLookup}. That + * argument is exactly the kind of wiring fleetd #248 warned about: dropping it (or swapping it for + * the inert {@code FleetMcp.LeadSeatSource.none()}) compiles with 0 errors and leaves every test + * that builds its own {@code FleetMcp}/{@code CapacitySource} directly — every test that predates + * this ticket — green, because none of them go through {@code main} at all. + * + *

{@link FleetdLeadSeatLookupTest} proves the factory's own matching logic; this class is the + * plain source-text assertion that proves {@code main} still passes its result in, mirroring + * {@code FleetdCompletionResolverWiringTest}'s approach for the same class of gap. + * + *

This test checks source text, not runtime behaviour. It never constructs a + * {@code FleetMcp} and never runs {@code main}. + */ +class FleetdLeadSeatWiringTest { + + private static String fleetdSource() throws Exception { + return Files.readString(Path.of("src/main/java/dev/ltms/fleet/Fleetd.java")); + } + + @Test + @DisplayName("[SOURCE TEXT] FleetMcp's construction call still passes a LeadSeatSource built from leadSeatLookup(...)") + void fleetMcpConstructionStillWiresLeadSeatLookup() throws Exception { + String source = fleetdSource(); + assertTrue(source.contains("new FleetMcp.LeadSeatSource(leadSeatLookup(() -> config.get().profiles(), " + + "leaders, leads))"), + "FleetMcp's construction call must still pass a LeadSeatSource built from " + + "Fleetd.leadSeatLookup(...). Dropping it or swapping in " + + "FleetMcp.LeadSeatSource.none() (fleetd #176's would-be silent regression, the same " + + "shape as fleetd #248's measured mutations) compiles with 0 errors and leaves every " + + "existing behavioural test green — this source check is what must go red instead."); + } +} 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 2ba37ec..bf07edf 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java @@ -6,11 +6,18 @@ import dev.ltms.fleet.peer.MemberRole; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.RecordComponent; +import java.lang.reflect.Type; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeMap; +import java.util.regex.Matcher; import java.util.regex.Pattern; import static org.junit.jupiter.api.Assertions.*; @@ -1366,9 +1373,18 @@ class FleetConfigTest { } /** - * Every optional knob the example documents must bind under the exact spelling used there. - * Keep this list in step with {@code fleetd.example.yaml}: a rename that updates the record - * but not the example (or vice versa) fails here instead of silently no-op'ing in production. + * A spot check that the knobs listed below bind under the exact spelling the example uses — + * it asserts real VALUES arrive in the record, which no name-matching guard can do. + * + *

This is NOT a coverage guard, and must not be read as one (fleetd #113). The list + * inside it is hand-written, so it only ever covers what someone remembered to add. Coverage + * — "is every key the code reads documented, and does every documented key bind?" — comes + * from {@link #everyNestedConfigKeyIsDocumentedInTheExample} and + * {@link #everyLiveKeyInTheExampleBindsToARecordComponent}, both of which derive their key + * set from the record tree and therefore cannot drift. + * + *

Adding a knob here is optional. Leaving one out is not a coverage gap, because the two + * derived guards above already fail on an undocumented or unbindable key. */ @Test void everyOptionalKnobDocumentedInTheExampleBinds(@TempDir Path dir) throws Exception { @@ -1525,6 +1541,230 @@ class FleetConfigTest { return p.matcher(yaml).find(); } + /** + * The nested half of {@link #everyKnownTopLevelKeyIsDocumentedInTheExample} (fleetd #113). + * + *

That guard walks {@link FleetConfig#KNOWN_TOP_LEVEL_KEYS} and anchors its regex at + * column 0, so it sees ONLY top-level keys. Every nested key — {@code profiles..model}, + * {@code health.paneProbeIntervalSeconds} and a hundred others — is outside its scope, and + * neither its name nor its output says so. A green run then reads as "the example documents + * the schema" when most of the schema was never looked at. + * + *

This walks the record tree rather than a name list, so a key added to any nested record + * is covered the moment it compiles, with no edit here. That is the point: a hand-maintained + * second copy of a list always drifts from the thing it mirrors. + * + *

Scope, stated on purpose (fleetd #113 criterion 3 — every check reports what it + * did and did not look at): + *

+ */ + @Test + void everyNestedConfigKeyIsDocumentedInTheExample() throws Exception { + Path example = Path.of("fleetd.example.yaml"); + assertTrue(Files.exists(example), "fleetd.example.yaml must ship next to the pom"); + String text = Files.readString(example); + + Map pathByName = configKeyPaths(); + + // The denominator. An under-counting walk passes every subset check vacuously, which is + // the exact shape fleetd #113 collects — so the walk has to prove it descended at all. + // The floor is DERIVED, not a literal: the nested walk must find substantially more keys + // than the top-level set the old guard used, or it has not gone below the first level. + int topLevel = FleetConfig.KNOWN_TOP_LEVEL_KEYS.size(); + assertTrue(pathByName.size() > topLevel * 2, + "the record walk found " + pathByName.size() + " config key(s) against " + + topLevel + " top-level key(s) — it has stopped descending into the " + + "nested records, so this guard would pass vacuously. Fix the walk " + + "before trusting a green run."); + + List undocumented = pathByName.entrySet().stream() + .filter(e -> !keyDocumentedAnywhere(text, e.getKey())) + .map(Map.Entry::getValue) + .sorted() + .toList(); + + assertTrue(undocumented.isEmpty(), () -> "checked " + pathByName.size() + + " config key(s) that FleetConfig can bind; " + undocumented.size() + + " appear nowhere in fleetd.example.yaml: " + undocumented + + " — document each one there, commented out if optional. fleetd.yaml is " + + "gitignored, so the example is the only committed description of the schema."); + } + + /** + * The other direction: a LIVE key in the example that {@link FleetConfig} cannot bind. That is + * a key an operator would copy into {@code fleetd.yaml} expecting it to do something, where it + * would be silently ignored. + * + *

Scope, stated on purpose: only live (uncommented) keys are checked. Most of the + * example is commented-out prose, and that prose contains lines like {@code # mode: token} + * that are indistinguishable from keys by text alone. Parsing them would produce false + * failures, so they are deliberately out of scope — and saying so here is the point, rather + * than letting a reader assume the whole file was validated. + */ + @Test + void everyLiveKeyInTheExampleBindsToARecordComponent() throws Exception { + Path example = Path.of("fleetd.example.yaml"); + String text = Files.readString(example); + + List> paths = liveKeyPaths(text); + assertTrue(paths.size() >= 20, + "only " + paths.size() + " live key path(s) were parsed out of the example — the " + + "parser is not seeing the file, so this guard would pass vacuously."); + + List unbindable = paths.stream() + .filter(path -> !pathBinds(path)) + .map(path -> String.join(".", path)) + .distinct() + .sorted() + .toList(); + + assertTrue(unbindable.isEmpty(), () -> "checked " + paths.size() + + " live key path(s) in fleetd.example.yaml; " + unbindable.size() + + " bind to nothing in FleetConfig: " + unbindable + + " — an operator copying one of these into fleetd.yaml gets silence, not an error."); + } + + /** + * Every configuration key {@link FleetConfig} can bind, at every depth, as + * {@code name -> a dotted path to one place it appears}. Derived from the record components, + * so it cannot drift from the code. + */ + private static Map configKeyPaths() { + Map out = new TreeMap<>(); + collectConfigKeys(FleetConfig.class, "", new HashSet<>(), out); + return out; + } + + private static void collectConfigKeys(Class type, String prefix, Set seen, + Map out) { + if (!type.isRecord() || !seen.add(type.getName())) { + return; + } + for (RecordComponent rc : type.getRecordComponents()) { + String path = prefix.isEmpty() ? rc.getName() : prefix + "." + rc.getName(); + out.putIfAbsent(rc.getName(), path); + Class nested = rc.getType(); + if (nested.isRecord()) { + collectConfigKeys(nested, path, seen, out); + } else if (Map.class.isAssignableFrom(nested) || List.class.isAssignableFrom(nested)) { + Class element = elementRecord(rc); + if (element != null) { + String childPrefix = Map.class.isAssignableFrom(nested) + ? path + "." : path + "[]"; + collectConfigKeys(element, childPrefix, seen, out); + } + } + } + } + + /** The record type inside a {@code Map} or {@code List} component, else null. */ + private static Class elementRecord(RecordComponent rc) { + if (rc.getGenericType() instanceof ParameterizedType pt) { + Type[] args = pt.getActualTypeArguments(); + if (args.length > 0 && args[args.length - 1] instanceof Class c && c.isRecord()) { + return c; + } + } + return null; + } + + /** + * True when {@code key} is documented in the example, in either of the two conventions that + * file actually uses: + *

    + *
  1. as a YAML key at any indentation, live or commented out ({@code key:}); or
    2. + *
  2. in a prose block that describes a section's sub-keys, one per line, as + * {@code # key → what it does}.
    3. + *
+ * + *

The second form is not decoration. {@code broker.uri} is documented ONLY that way, on + * purpose: writing it out as a copy-pasteable {@code uri: amqp://user:pass@host} invites an + * operator to paste a password into a file, which is the very thing {@code uriEnv} exists to + * avoid. A guard that demanded the key form would push the file toward doing that. So this + * encodes the convention the example really uses rather than imposing a new one. + */ + private static boolean keyDocumentedAnywhere(String yaml, String key) { + String quoted = Pattern.quote(key); + Pattern asYamlKey = Pattern.compile("(?m)^\\s*(?:#\\s*)?" + quoted + ":"); + Pattern asProseEntry = Pattern.compile("(?m)^\\s*#\\s*" + quoted + "\\s+\u2192"); + return asYamlKey.matcher(yaml).find() || asProseEntry.matcher(yaml).find(); + } + + /** Every live (uncommented) key in {@code yaml}, as a path from the document root. */ + private static List> liveKeyPaths(String yaml) { + Pattern keyLine = Pattern.compile("^(\\s*)([A-Za-z][A-Za-z0-9_]*):(\\s.*)?$"); + List stack = new ArrayList<>(); + List indents = new ArrayList<>(); + List> paths = new ArrayList<>(); + for (String line : yaml.split("\n", -1)) { + if (line.isBlank() || line.stripLeading().startsWith("#")) { + continue; + } + Matcher m = keyLine.matcher(line); + if (!m.matches()) { + continue; + } + int indent = m.group(1).length(); + while (!indents.isEmpty() && indents.get(indents.size() - 1) >= indent) { + indents.remove(indents.size() - 1); + stack.remove(stack.size() - 1); + } + indents.add(indent); + stack.add(m.group(2)); + paths.add(List.copyOf(stack)); + } + return paths; + } + + /** True when a dotted YAML path resolves to something {@link FleetConfig} can bind. */ + private static boolean pathBinds(List path) { + Class type = FleetConfig.class; + boolean nextSegmentIsAFreeFormName = false; + for (int i = 0; i < path.size(); i++) { + if (nextSegmentIsAFreeFormName) { + nextSegmentIsAFreeFormName = false; + continue; + } + RecordComponent rc = componentNamed(type, path.get(i)); + if (rc == null) { + return false; + } + Class t = rc.getType(); + if (t.isRecord()) { + type = t; + } else if (Map.class.isAssignableFrom(t)) { + Class element = elementRecord(rc); + if (element == null) { + return true; // Map: its entries are data, not schema + } + type = element; + nextSegmentIsAFreeFormName = true; + } else { + // A scalar or a list of scalars: nothing may legitimately nest under it. + return i == path.size() - 1; + } + } + return true; + } + + private static RecordComponent componentNamed(Class type, String name) { + if (type == null || !type.isRecord()) { + return null; + } + for (RecordComponent rc : type.getRecordComponents()) { + if (rc.getName().equals(name)) { + return rc; + } + } + return null; + } + @Test void placementDefaultsToFixedForExistingConfigs(@TempDir Path dir) throws Exception { Path f = dir.resolve("no-placement.yaml"); diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java index 600e3e7..b6237b1 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java @@ -651,6 +651,48 @@ class FleetMcpTest { assertEquals(2, out.split("\"free\":0", -1).length - 1, out); } + /** + * fleetd #176 stage 2 (correcting the inert stage 1): two {@code subscription: true} profiles, + * {@code opus} and {@code sonnet}, neither setting an explicit {@code credentialId} — the exact + * shape measured on the live Mac fleet. This is INTENDED, not a regression: a real Claude usage + * limit on the one login behind both profiles really does take out every profile running on it, + * the same way {@code credentialId: openai-shared} already lets two OpenAI-backed profiles share + * one quarantine (see {@code everyProfileSharingTheQuarantinedCredentialReportsZeroFree} above). + * The {@code credentialIdFor} function here is built the same way {@code Fleetd.main} wires it — + * {@code profile -> profiles.get(profile).effectiveCredentialId()} — so this proves the actual + * config-driven behaviour, not just {@code capacityView}'s arithmetic with a hand-picked string. + */ + @Test + void quarantiningOneSubscriptionProfileZeroesFreeOnTheOtherSharingTheSameAccount() { + FakeHerdr h = new FakeHerdr(); + SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + Map profiles = Map.of( + "opus", new FleetConfig.Profile("opus", null, "claude-opus-4", null, null, null, + "tab", "fleet", "w #{n}", null, null, null, null, null, null, null, + null, 3, true, null, null, null), + "sonnet", new FleetConfig.Profile("sonnet", null, "claude-sonnet-5", null, null, null, + "tab", "fleet", "w #{n}", null, null, null, null, null, null, null, + null, 3, true, null, null, null)); + BackendQuarantine quarantine = new BackendQuarantine(() -> 0L, TimeUnit.MINUTES.toNanos(20)); + quarantine.quarantine(FleetConfig.Profile.SUBSCRIPTION_CREDENTIAL_ID); + FleetMcp.QuarantineSource source = new FleetMcp.QuarantineSource(profile -> { + FleetConfig.Profile configured = profiles.get(profile); + return configured == null ? null : configured.effectiveCredentialId(); + }, quarantine); + + String out = textOf(FleetMcp.listFleet(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), + sessions, null, new FleetMcp.CapacitySource(profile -> 0, profile -> 3, + () -> Set.of("opus", "sonnet"), () -> 0), new FleetMcp.HealthCoverageSource(() -> "off"), + source, Map.of(), "")); + + assertEquals(2, out.split("\"free\":0", -1).length - 1, + "opus and sonnet share one Claude login with neither setting credentialId, so quarantining " + + "opus's account must also zero sonnet's free — this is intended, not a side effect: " + + out); + assertEquals(2, out.split("\"credentialId\":\"" + FleetConfig.Profile.SUBSCRIPTION_CREDENTIAL_ID + "\"", -1) + .length - 1, out); + } + /** fleetd #201 Unit 5: cool-off forces {@code free:0} but never adds {@code quarantinedForSeconds}. */ @Test void coolingOffProfileReportsZeroFreeButNeverQuarantinedForSeconds() { @@ -767,6 +809,68 @@ class FleetMcpTest { assertFalse(out.contains("quarantinedForSeconds"), out); } + /** + * fleetd #176: this is the exact shape measured on the Mac fleet — {@code maxLoad:3, live:2}, + * where one of the "free" three is really the lead's own seat on the same subscription. The old + * formula ({@code max(0, cap - live)}) reported {@code free:1}; the real ceiling is {@code 0} + * (two members plus the lead's own seat already fill all three). + */ + @Test + void leadSeatSubtractsFromFreeTheSameWayLiveDoes() { + FakeHerdr h = new FakeHerdr(); + SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + FleetMcp.LeadSeatSource leadSeats = new FleetMcp.LeadSeatSource( + profile -> "sonnet".equals(profile) ? 1 : 0); + + String out = textOf(FleetMcp.listFleet(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), + sessions, null, new FleetMcp.CapacitySource(profile -> 2, profile -> 3, + () -> Set.of("sonnet"), () -> 0), new FleetMcp.HealthCoverageSource(() -> "off"), + FleetMcp.QuarantineSource.none(), FleetMcp.OutageSource.none(), leadSeats, Map.of(), "", null)); + + assertTrue(out.contains("\"maxLoad\":3"), "maxLoad itself must be left untouched: " + out); + assertTrue(out.contains("\"live\":2"), out); + assertTrue(out.contains("\"free\":0"), "2 live + 1 lead seat fills all 3: " + out); + assertTrue(out.contains("\"leadSeats\":1"), out); + } + + /** + * fleetd #176: the OTHER measurement in the issue — a completely idle fleet still overstates + * {@code free} by the lead's own seat. {@code maxLoad:3, live:0} must report {@code free:2}, the + * real fan-out ceiling, not {@code 3}. + */ + @Test + void leadSeatLowersFreeOnAnOtherwiseIdleSubscriptionProfile() { + FakeHerdr h = new FakeHerdr(); + SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + FleetMcp.LeadSeatSource leadSeats = new FleetMcp.LeadSeatSource( + profile -> "sonnet".equals(profile) ? 1 : 0); + + String out = textOf(FleetMcp.listFleet(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), + sessions, null, new FleetMcp.CapacitySource(profile -> 0, profile -> 3, + () -> Set.of("sonnet"), () -> 0), new FleetMcp.HealthCoverageSource(() -> "off"), + FleetMcp.QuarantineSource.none(), FleetMcp.OutageSource.none(), leadSeats, Map.of(), "", null)); + + assertTrue(out.contains("\"live\":0"), out); + assertTrue(out.contains("\"free\":2"), "an idle fleet's real ceiling is 3 minus the lead's own seat: " + out); + assertTrue(out.contains("\"leadSeats\":1"), out); + } + + /** A profile with no lead seats reported must be byte-identical to before this ticket. */ + @Test + void zeroLeadSeatsOmitsTheKeyAndLeavesFreeUnchanged() { + FakeHerdr h = new FakeHerdr(); + SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw"))); + + String out = textOf(FleetMcp.listFleet(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")), + sessions, null, new FleetMcp.CapacitySource(profile -> 0, profile -> 2, + () -> Set.of("terra"), () -> 0), new FleetMcp.HealthCoverageSource(() -> "off"), + FleetMcp.QuarantineSource.none(), FleetMcp.OutageSource.none(), FleetMcp.LeadSeatSource.none(), + Map.of(), "", null)); + + assertTrue(out.contains("\"free\":2"), out); + assertFalse(out.contains("leadSeats"), "no lead shares this profile's credential: " + out); + } + @Test void listReportsLeadsAndFlagsTheCallersOwnRow() { FakeHerdr h = new FakeHerdr(); diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java index d995775..64b5101 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java @@ -1409,14 +1409,16 @@ class ClaudeCodeLauncherTest { } /** - * CB-633 follow-up (#192): the mirror of the zsh test above. On a non-zsh login shell {@code - * ZDOTDIR} is ignored, so no scrub ever runs — the report must keep the WARN wording (a name here - * really is inherited unblocked) rather than claiming a scrub protects it. This is the trap PR - * #174 fell into the other direction: keying the wording on the shell, not on {@code - * creds.isAllowList()}, is what keeps this branch correct. + * fleetd #155 (was CB-633 follow-up (#192) "allowListPolicyOnNonZshKeepsTheWarnWording"): on a + * non-zsh login shell {@code ZDOTDIR} is ignored, so no scrub could ever run there. Before #155 + * the launcher degraded to the weaker overlay and spawned anyway; that is exactly the "control + * silently does nothing" failure #155 exists to close, since {@code policy: allow-list} is the + * operator explicitly asking for a control a sourced file cannot undo. Now the spawn is REFUSED + * instead — this test asserts the refusal, naming the shell, on the real spawn path ({@link + * ClaudeCodeLauncher#spawn}), not merely on the launcher method that computes it. */ @Test - void allowListPolicyOnNonZshKeepsTheWarnWording() { + void allowListPolicyOnNonZshRefusesTheSpawn() { FakeHerdr herdr = new FakeHerdr(); FleetConfig.Profile cfg = new FleetConfig.Profile( "ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN", @@ -1430,27 +1432,12 @@ class ClaudeCodeLauncherTest { 0, System::currentTimeMillis, () -> {}, null, () -> creds, () -> Set.of("AI_GATEWAY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN")); - Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class); - ListAppender appender = new ListAppender<>(); - appender.start(); - logger.addAppender(appender); - try { - svc.spawn(); - } finally { - logger.detachAppender(appender); - } + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, svc::spawn, + "policy=allow-list on a non-zsh shell must refuse the spawn, not silently degrade " + + "to the weaker overlay"); - assertTrue(appender.list.stream().anyMatch(e -> - e.getFormattedMessage().contains("memberCredentials gap") - && e.getFormattedMessage().contains("UNBLOCKED") - && e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN")), - "no scrub runs on a non-zsh shell, so the WARN wording must be kept — got: " - + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); - assertFalse(appender.list.stream().anyMatch(e -> - e.getFormattedMessage().contains("memberCredentials gap") - && e.getFormattedMessage().toLowerCase(java.util.Locale.ROOT).contains("scrub")), - "nothing is scrubbed on this path, so the report must not claim otherwise — got: " - + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + assertTrue(e.getMessage().contains("allow-list") && e.getMessage().contains("/bin/bash"), + "the refusal must name the policy and the actual shell, got: " + e.getMessage()); } /** diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java index 2d0d69b..75f883c 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java @@ -29,6 +29,7 @@ import java.util.function.Supplier; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeTrue; @@ -99,18 +100,26 @@ class HerdrPeerLauncherAllowListWiringTest { } /** - * A non-zsh shell cannot read {@code ZDOTDIR} at all. The launcher must fall back rather than - * generate a directory nothing will ever read — a directory that would look like protection. + * fleetd #155: a non-zsh shell cannot read {@code ZDOTDIR} at all, so under {@code + * policy: allow-list} — the policy the operator picked specifically for a control a sourced file + * cannot undo — the launcher must REFUSE the spawn rather than silently generate a directory + * nothing will ever read (protection theatre) or fall back to the weaker overlay (exactly the + * "control silently does nothing" defect this ticket exists to close). Real path: through {@link + * HerdrPeerLauncher#spawn}, the method the daemon actually calls. */ @Test - void aNonZshShellGeneratesNothingAndFallsBack() { + void aNonZshShellUnderAllowListPolicyRefusesTheSpawn() { FakeHerdr herdr = new FakeHerdr(); WiringLauncher launcher = new WiringLauncher(herdr, allowList(), "/bin/bash"); - launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, + () -> launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)), + "policy=allow-list on a non-zsh shell must refuse the spawn, not silently degrade"); + assertTrue(e.getMessage().contains("allow-list") && e.getMessage().contains("/bin/bash"), + "the refusal must name the policy and the actual shell, got: " + e.getMessage()); assertFalse(launcher.env.containsKey("ZDOTDIR"), - "bash ignores ZDOTDIR; setting it would be protection theatre"); + "a refused spawn must not have generated (or wired in) a scrub directory: " + launcher.env); } private static Supplier allowList() { @@ -246,11 +255,14 @@ class HerdrPeerLauncherAllowListWiringTest { * "allowed N of M" line — which describes what the scrub does — must not be printed there either. * Before this fix the line was logged BEFORE the zsh gate, so a non-zsh host printed e.g. * "allowed 1 of 3" while blocking nothing at all, telling an operator a control ran when it did - * not. Real path: goes through {@link HerdrPeerLauncher#spawn}, same as the sibling test above, - * with the shell fixed to bash so the fallback branch is the one exercised. + * not. fleetd #155: the spawn itself is now refused on this path (see {@code + * aNonZshShellUnderAllowListPolicyRefusesTheSpawn}) rather than falling back — this test's own + * concern still holds under the refusal: the "allowed N of M" line describes a scrub that never + * ran here, so it must still never appear. Real path: goes through {@link + * HerdrPeerLauncher#spawn}, same as the sibling test above, with the shell fixed to bash. */ @Test - void noAllowedCountLineIsEmittedOnTheNonZshFallbackPath() { + void noAllowedCountLineIsEmittedOnTheNonZshRefusalPath() { FakeHerdr herdr = new FakeHerdr(); Set hostEnvNames = Set.of(INJECTED, "SOME_UNRELATED_NAME", "ANOTHER_UNRELATED_NAME"); WiringLauncher launcher = new WiringLauncher(herdr, allowList(), "/bin/bash", () -> hostEnvNames); @@ -262,7 +274,9 @@ class HerdrPeerLauncherAllowListWiringTest { appender.start(); logger.addAppender(appender); try { - launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + assertThrows(IllegalArgumentException.class, + () -> launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)), + "policy=allow-list on a non-zsh shell must refuse the spawn"); } finally { logger.detachAppender(appender); logger.setLevel(original); @@ -310,13 +324,20 @@ class HerdrPeerLauncherAllowListWiringTest { * different OS user — {@link HerdrPeerLauncher#hostEnvNames} describes fleetd's own process, not * that user's. Neither "inherits them UNBLOCKED" nor "scrub blanks them" is evidence-backed * there, so neither may print; the single unknown-environment WARN must, naming the config key. + * + *

fleetd #155: {@code memberLoginShell} is now given explicitly as zsh, so the spawn reaches + * this WARN through the (still-degrading, not refusing) missing-{@code worktreeRoot}/{@code + * worktreeGroup} fallback rather than through the zsh gate, which now refuses instead of falling + * back — see {@code aNonZshShellUnderAllowListPolicyRefusesTheSpawn}. This test's own concern + * (the unknown-environment WARN) is orthogonal to which fallback reached {@code + * logCredentialGap}, so it still holds. */ @Test void gapDetectorReportsUnknownInsteadOfAConclusionWhenMemberHerdrSocketIsConfigured() { FakeHerdr herdr = new FakeHerdr(); Set hostEnvNames = Set.of("FLEETD_WORKER_TOKEN", "SOME_UNKNOWN_SECRET_TOKEN"); WiringLauncher launcher = new WiringLauncher(herdr, allowList(), "/bin/zsh", () -> hostEnvNames, - () -> configWithMemberHerdrSocket("/tmp/other-user-herdr.sock")); + () -> configWithMemberHerdrSocket("/tmp/other-user-herdr.sock", "/bin/zsh")); List messages = spawnAndCaptureLogs(launcher); @@ -340,8 +361,9 @@ class HerdrPeerLauncherAllowListWiringTest { void theUnknownEnvironmentWarnFiresOnceNotOncePerSpawn() { FakeHerdr herdr = new FakeHerdr(); Set hostEnvNames = Set.of("FLEETD_WORKER_TOKEN", "SOME_UNKNOWN_SECRET_TOKEN"); + // fleetd #155: memberLoginShell given explicitly as zsh — see the sibling test above for why. WiringLauncher launcher = new WiringLauncher(herdr, allowList(), "/bin/zsh", () -> hostEnvNames, - () -> configWithMemberHerdrSocket("/tmp/other-user-herdr.sock")); + () -> configWithMemberHerdrSocket("/tmp/other-user-herdr.sock", "/bin/zsh")); Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class); Level original = logger.getLevel(); @@ -389,42 +411,44 @@ class HerdrPeerLauncherAllowListWiringTest { } /** - * fleetd #213 defect 1, acceptance criterion 1: {@code memberHerdrSocket} configured and {@code - * memberLoginShell} configured as non-zsh must fall back to the sentinel overlay exactly like a - * non-zsh {@code $SHELL} does today — and the "generated ZDOTDIR" INFO must not appear, since no - * scrub actually runs. The WiringLauncher's own {@code env("SHELL")} is deliberately set to - * {@code /bin/zsh} — the OPPOSITE of what {@code memberLoginShell} says — so a launcher that - * (incorrectly) fell back to fleetd's own {@code $SHELL} here would wrongly pass the gate and - * fail this test. + * fleetd #213 defect 1, acceptance criterion 1 — updated by fleetd #155: {@code + * memberHerdrSocket} configured and {@code memberLoginShell} configured as non-zsh must now + * REFUSE the spawn (not fall back to the sentinel overlay — see {@code + * aNonZshShellUnderAllowListPolicyRefusesTheSpawn}'s javadoc for why). The WiringLauncher's own + * {@code env("SHELL")} is deliberately set to {@code /bin/zsh} — the OPPOSITE of what {@code + * memberLoginShell} says — so a launcher that (incorrectly) fell back to fleetd's own {@code + * $SHELL} here would wrongly pass the gate and fail this test. */ @Test - void memberHerdrSocketWithNonZshMemberLoginShellFallsBackToTheOverlay() { + void memberHerdrSocketWithNonZshMemberLoginShellRefusesTheSpawn() { FakeHerdr herdr = new FakeHerdr(); WiringLauncher launcher = new WiringLauncher(herdr, allowListWithKnown(List.of("SOME_TOKEN")), "/bin/zsh", null, () -> configWithMemberHerdrSocket("/tmp/other-user-herdr.sock", "/bin/bash")); - List messages = spawnAndCaptureLogs(launcher); + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, + () -> launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)), + "a non-zsh configured memberLoginShell under policy=allow-list must refuse the spawn"); - assertEquals("blocked-by-fleetd-cb596-see-gitea-issue-82", launcher.env.get("SOME_TOKEN"), - "a non-zsh memberLoginShell must fall back to the CB-596 sentinel overlay, exactly " - + "like a non-zsh $SHELL does when memberHerdrSocket is absent"); + assertTrue(e.getMessage().contains("/bin/bash"), + "the refusal must name the configured memberLoginShell, got: " + e.getMessage()); + assertFalse(launcher.env.containsKey("SOME_TOKEN"), + "a refused spawn must not have touched the pane env at all: " + launcher.env); assertFalse(launcher.env.containsKey("ZDOTDIR"), "no scrub directory may be generated when the configured member login shell is not zsh"); - assertFalse(messages.stream().anyMatch(m -> m.contains("generated ZDOTDIR")), - "the 'generated ZDOTDIR' INFO must not appear when the scrub never runs — got: " + messages); } /** - * fleetd #213 defect 1, acceptance criterion 2: {@code memberHerdrSocket} configured and NO - * {@code memberLoginShell} configured must fall back exactly like criterion 1 above — AND - * fleetd's own {@code $SHELL} must never even be consulted (not merely "not decisive"). The - * fixture's {@code env} function reports {@code /bin/zsh} for {@code SHELL} — a value that would - * WRONGLY pass the zsh gate if the fix regressed to reading it — while flagging whether it was - * ever asked for at all, so this test fails loudly on either kind of regression. + * fleetd #213 defect 1, acceptance criterion 2 — updated by fleetd #155: {@code + * memberHerdrSocket} configured and NO {@code memberLoginShell} configured must now REFUSE the + * spawn exactly like criterion 1 above — AND fleetd's own {@code $SHELL} must never even be + * consulted (not merely "not decisive"). The fixture's {@code env} function reports {@code + * /bin/zsh} for {@code SHELL} — a value that would WRONGLY pass the zsh gate if the fix + * regressed to reading it — while flagging whether it was ever asked for at all, so this test + * fails loudly on either kind of regression. */ @Test - void memberHerdrSocketWithNoMemberLoginShellFallsBackAndNeverConsultsFleetdsOwnShell() { + void memberHerdrSocketWithNoMemberLoginShellRefusesAndNeverConsultsFleetdsOwnShell() { FakeHerdr herdr = new FakeHerdr(); AtomicBoolean shellQueried = new AtomicBoolean(false); Function env = name -> { @@ -437,15 +461,18 @@ class HerdrPeerLauncherAllowListWiringTest { WiringLauncher launcher = new WiringLauncher(herdr, allowListWithKnown(List.of("SOME_TOKEN")), env, () -> configWithMemberHerdrSocket("/tmp/other-user-herdr.sock", null)); - List messages = spawnAndCaptureLogs(launcher); + assertThrows(IllegalArgumentException.class, + () -> launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)), + "no configured memberLoginShell under policy=allow-list must refuse the spawn, same " + + "as an explicit non-zsh one"); assertFalse(shellQueried.get(), "fleetd's own $SHELL must never be consulted once " + "memberHerdrSocket is configured — only memberLoginShell: may decide the gate"); - assertEquals("blocked-by-fleetd-cb596-see-gitea-issue-82", launcher.env.get("SOME_TOKEN"), - "no memberLoginShell configured must fall back to the sentinel overlay, same as a " + assertFalse(launcher.env.containsKey("SOME_TOKEN"), + "a refused spawn must not have touched the pane env at all, same as a " + "configured non-zsh shell"); - assertFalse(messages.stream().anyMatch(m -> m.contains("generated ZDOTDIR")), - "no scrub may run without a configured memberLoginShell — got: " + messages); + assertFalse(launcher.env.containsKey("ZDOTDIR"), + "no scrub may run without a configured memberLoginShell"); } /** @@ -700,7 +727,8 @@ class HerdrPeerLauncherAllowListWiringTest { /** * fleetd #213: as above, plus {@code worktreeRoot:}/{@code worktreeGroup:} — both required for * the ZDOTDIR scrub to run at all once {@code memberHerdrSocket} is configured; either missing - * falls back to the sentinel overlay, same as a non-zsh {@code memberLoginShell}. + * falls back to the sentinel overlay (fleetd #155 left this branch alone — it is not a shell + * problem, so it is not this ticket's refusal). */ private static FleetConfig configWithMemberHerdrSocketRootAndGroup(String memberHerdrSocket, String memberLoginShell, String worktreeRoot, String worktreeGroup) {