diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 7181b7a..f8ee3cb 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -352,7 +352,11 @@ public final class Fleetd { // pane resolves to an architect until the later spawn lifecycle binds one. The registry is // what CallerResolver resolves against and what that lifecycle will read profiles from; // nothing here spawns a slot. - MemberRegistry members = new MemberRegistry(cfg.fleet()); + // fleetd #424: MemberRegistry.live re-reads fleet.architects through `config` on every + // reserve/requireSlotFor call, so a reload that removes or adds an architect slot governs + // the next spawn with no restart — the frozen `new MemberRegistry(cfg.fleet())` this used + // to be let a "revoked" slot keep granting new architect spawns forever. + MemberRegistry members = MemberRegistry.live(() -> config.get().fleet()); sessions.setMemberLifecycle(members); if (!members.slots().isEmpty()) { log.info("member slots: {} configured {} — none bound yet (a slot is idle until the " diff --git a/fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java b/fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java index 972cf9c..3007842 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java +++ b/fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java @@ -11,6 +11,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.function.Supplier; /** * The architect-slot registry (CB-548): every gateway-local architect name and the strong-model @@ -19,16 +20,43 @@ import java.util.Objects; * *

Two halves, split by who owns each: *

* + *

The binding rule (fleetd #424): config governs what a bound slot still grants, as + * well as what may be bound next. Removing a slot from config revokes it — that is the + * ticket's entire point ("Revoking an architect slot does not revoke it"). Revoking it means an + * architect already bound to that slot loses the ARCHITECT privilege on its very next request: + * {@link #roleForSlot} and {@link #nameForSlot} read {@link #slots()} directly, with no cache, so + * the moment a slot drops out of config, {@link CallerResolver#resolve} (which calls both on every + * request from a bound pane, {@code CallerResolver.java:220}) can no longer confirm the pane's slot + * is an architect slot, and the pane falls through to {@code Principal.worker(...)}. What does + * not change is the {@code terminalToSlot} occupancy — the binding created by + * {@link #bind} is untouched by a reload, on purpose: unbinding it here would double-book the slot + * key (a second terminal could then bind to the "freed" key while the first is still the terminal + * the operator actually meant to demote) and would silently break {@link #unbind}'s compare-safe + * contract, which needs the original {@code terminal → slot} pair intact to remove it cleanly. So + * the demoted session keeps occupying its slot — {@link #slotForTerminal} and {@link #snapshot()} + * still name it — it just no longer resolves as an architect through that occupancy, and a fresh + * spawn still cannot bind to the same key while it is occupied ({@link #reserve}/ + * {@link #requireSlotFor} refuse it anyway, since it is gone from {@link #slots()}). The demoted + * session's own turn is unaffected: {@code fleet_reply}'s authorization + * ({@code Authz.Action.REPLY}) is {@code caller.ownsSession(targetSession)} — identity by terminal, + * not by role — so a demoted architect can still end its own turn normally. + * *

Spawning/lifecycle is deliberately a separate unit: this class only owns the bindings and * exposes the map the resolver resolves against plus the profile lookup lifecycle will call. * Nothing here creates or manages an architect session. @@ -55,14 +83,42 @@ public final class MemberRegistry implements MemberLifecycle { } } - private final Map slots; + private final Supplier fleet; /** Live {@code terminal_id → qualified slot key}; guarded by {@code terminalToSlot}. */ private final Map terminalToSlot = new HashMap<>(); /** Slot keys held between reservation and the terminal binding. Guarded by terminalToSlot. */ private final java.util.Set reservedSlots = new java.util.HashSet<>(); - /** Flatten every role pool in {@code fleet} into one registry. Leaders are not members. */ + /** + * Freeze the pool at construction — for tests, and for the rare case of wiring a fixed, + * code-built config. Production wiring should prefer {@link #live}, which re-reads + * {@code fleet:} on every call. + */ public MemberRegistry(FleetConfig.Fleet fleet) { + this(() -> fleet); + } + + private MemberRegistry(Supplier fleet) { + this.fleet = fleet; + } + + /** + * Live variant (fleetd #424): {@code fleet} is read fresh on every {@link #slots()} call — pass + * {@code () -> config.get().fleet()}, the same supplier shape {@code CompositePeerLauncher} + * already uses for placement — so a reload that adds or removes an architect slot governs the + * next spawn's {@link #reserve}/{@link #requireSlotFor} check with no restart. A separate, + * private constructor rather than a same-arity public overload of + * {@link #MemberRegistry(FleetConfig.Fleet)}: a {@code FleetConfig.Fleet} and a + * {@code Supplier} overload are ambiguous for a literal {@code null} — the + * same reason {@code CallerResolver.withLeads} is a static factory rather than a fourth + * constructor overload. + */ + public static MemberRegistry live(Supplier fleet) { + return new MemberRegistry(Objects.requireNonNull(fleet, "fleet")); + } + + /** Flatten every role pool in {@code fleet} into one map. Leaders are not members. */ + private static Map flatten(FleetConfig.Fleet fleet) { Map flat = new LinkedHashMap<>(); if (fleet != null) { for (MemberRole role : MemberRole.values()) { @@ -74,18 +130,22 @@ public final class MemberRegistry implements MemberLifecycle { }); } } - this.slots = Collections.unmodifiableMap(flat); + return Collections.unmodifiableMap(flat); } - /** The configured slots, keyed by qualified {@link Entry#key()}. Unmodifiable snapshot. */ + /** + * The configured slots, keyed by qualified {@link Entry#key()}. Unmodifiable snapshot of + * {@code fleet:} as of this call — see the class doc for which constructor makes that + * live versus frozen. + */ public Map slots() { - return slots; + return flatten(fleet.get()); } - /** The slots belonging to {@code role}, in definition order. */ + /** The slots belonging to {@code role}, in definition order, as of this call. */ public Map slotsFor(MemberRole role) { Map out = new LinkedHashMap<>(); - slots.forEach((key, e) -> { + slots().forEach((key, e) -> { if (e.role() == role) { out.put(key, e); } @@ -123,25 +183,35 @@ public final class MemberRegistry implements MemberLifecycle { * declares none */ public String profileForSlot(String slotName) { - Entry e = slots.get(slotName); + Entry e = slots().get(slotName); return (e == null || e.profile() == null) ? null : e.profile(); } - /** The role a qualified slot key belongs to, or {@code null} when the key is unknown. */ + /** + * The role a qualified slot key belongs to, or {@code null} when the key is not currently + * configured. Deliberately live, with no cache (fleetd #424, see the class doc's binding rule): + * removing a slot from config must make {@link CallerResolver#resolve} stop granting the + * ARCHITECT role for it on the very next request from a terminal that was bound to it, which is + * the ticket's whole point — revoking a slot must actually revoke it, not just refuse the next + * spawn. + */ public MemberRole roleForSlot(String slotName) { - Entry e = slots.get(slotName); + Entry e = slots().get(slotName); return e == null ? null : e.role(); } - /** The unqualified configured name for a slot, or {@code null} if it is unknown. */ + /** + * The unqualified configured name for a slot, or {@code null} if it is not currently configured. + * Live for the same reason as {@link #roleForSlot} — see the class doc's binding rule. + */ public String nameForSlot(String slotName) { - Entry e = slots.get(slotName); + Entry e = slots().get(slotName); return e == null ? null : e.name(); } /** True when {@code slotName} is a configured architect slot. */ public boolean isSlot(String slotName) { - return slots.containsKey(slotName); + return slots().containsKey(slotName); } /** diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java index 52d8e15..8a77496 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java @@ -32,18 +32,26 @@ import java.util.function.Supplier; * not the fact that they are config. Most of {@code fleet:} — every role pool * ({@code architects}/{@code developers}/{@code reviewers}), {@code charters}, and * {@code tabLabel} — is read the same live way, through the same supplier - * ({@code () -> config.get().fleet()}). But {@code fleet:} as a whole is NOT in this - * class: {@code fleet.leaders} inside the same key is frozen, which is exactly what - * makes {@code fleet:} split rather than hot — see below. {@code models:} (fleetd #422) joined - * this class whole: {@link FleetConfig#validateModels()} re-runs fully against the fresh - * config on every {@link #reload()} (via {@link FleetConfig#validateAll()}), refusing a bad - * edit outright rather than caching a stale copy anywhere, and the on/off half added by - * fleetd #422 is read live both by {@code CompositePeerLauncher}'s spawn gate - * ({@code enforceModelEnabled} and its candidate filter) and by {@code fleet_profiles}/ - * {@code GET /profiles} (via {@code PeerLauncher.disabledModels()}). Nothing about - * {@code models:} is baked into an object built at startup, so — unlike the deferred keys - * below — there is no frozen half left to report; it moved here from deferred rather than - * joining split. + * ({@code () -> config.get().fleet()}). {@code architects} in particular is hot for + * two independent consumers (fleetd #424): {@code CompositePeerLauncher} + * reads it live for placement (which profile an unqualified architect spawn may land on), and + * {@code MemberRegistry} separately reads it live, through its own instance of the same + * supplier shape, for identity — both which slot a spawn may bind to and what a slot + * already bound still grants. Removing an architect slot from config therefore revokes the + * {@link dev.ltms.fleet.auth.Role#ARCHITECT} role on the bound pane's very next request; only + * the slot occupancy survives, so the demoted session still holds its slot key until + * it unbinds. See {@code MemberRegistry}'s class doc for that binding rule. + * But {@code fleet:} as a whole is NOT in this class: {@code fleet.leaders} + * inside the same key is frozen, which is exactly what makes {@code fleet:} split rather than + * hot — see below. {@code models:} (fleetd #422) joined this class whole: {@link + * FleetConfig#validateModels()} re-runs fully against the fresh config on every {@link + * #reload()} (via {@link FleetConfig#validateAll()}), refusing a bad edit outright rather than + * caching a stale copy anywhere, and the on/off half added by fleetd #422 is read live both by + * {@code CompositePeerLauncher}'s spawn gate ({@code enforceModelEnabled} and its candidate + * filter) and by {@code fleet_profiles}/{@code GET /profiles} (via + * {@code PeerLauncher.disabledModels()}). Nothing about {@code models:} is baked into an + * object built at startup, so — unlike the deferred keys below — there is no frozen half left + * to report; it moved here from deferred rather than joining split. *

  • Deferred — accepted into the new snapshot, but the wiring built at startup * keeps the old value until a restart: {@code lifecycle:}, {@code leadHeartbeat:}, * {@code idleSleepGuard:} ({@code Fleetd.java} reads it once, at startup, to decide whether @@ -525,26 +533,35 @@ public final class ConfigRef implements Supplier { + "opened once and needs a restart; the broker URI env-var name kept out of a " + "member's environment is read live on every spawn and already applied"); } - // fleetd #333: unlike health/coordinator above, most of `fleet:` (architects, developers, - // reviewers, charters, tabLabel) is genuinely hot — ConfigRefTest.aHotChangeIsAppliedAndRead- + // fleetd #333: unlike health/coordinator above, most of `fleet:` (developers, reviewers, + // charters, tabLabel) is genuinely hot — ConfigRefTest.aHotChangeIsAppliedAndRead- // ThroughGet and aCharterChangeIsHotAndReachesTheLiveConfig prove it reaches the live config - // with no restart note. Only fleet.leaders is frozen (Fleetd.java:281 reads - // cfg.fleet().leaders() off the startup snapshot to build both the LeadTabScanner's - // tab-label-to-name map, wired into CallerResolver.withLeadsAndMembers at Fleetd.java:620/624, - // and — when herdr answered — LeadLauncher(...).ensureLeads() at Fleetd.java:315, which - // auto-launches each lead up to its `instances` count; neither is rebuilt on reload). So this - // compares fleet.leaders alone, not the whole Fleet record: comparing the whole record would - // report "split" for a tabLabel-only or charters-only change that is actually fully hot, - // which is the over-claim mirror of the under-claim bug this class exists to prevent. + // with no restart note. `architects` is hot too, and — since fleetd #424 — hot for BOTH of + // its consumers, not just the one this comment used to name: CompositePeerLauncher reads it + // live for PLACEMENT through the () -> config.get().fleet() supplier named in the class doc's + // Hot bullet, and MemberRegistry separately reads it live for IDENTITY (which slot a spawn + // may bind to, AND what a slot already bound still grants) through its own instance of that + // same supplier shape — see MemberRegistry.live and its class doc for the binding rule: + // removing a slot revokes ARCHITECT on the bound pane's very next request, and only the slot + // OCCUPANCY survives, so the demoted session keeps its slot key until it unbinds. Only + // fleet.leaders is frozen (Fleetd.java:281 reads cfg.fleet().leaders() off the startup + // snapshot to build both the LeadTabScanner's tab-label-to-name map, wired into + // CallerResolver.withLeadsAndMembers at Fleetd.java:620/624, and — when herdr answered — + // LeadLauncher(...).ensureLeads() at Fleetd.java:315, which auto-launches each lead up to its + // `instances` count; neither is rebuilt on reload). So this compares fleet.leaders alone, not + // the whole Fleet record: comparing the whole record would report "split" for a tabLabel-only + // or architects-only change that is actually fully hot, which is the over-claim mirror of the + // under-claim bug this class exists to prevent. if (!Objects.equals(leadersOf(old), leadersOf(fresh))) { changed.add("fleet: fleet.leaders (each lead's tab, workspace, cwd, profile and " + "instances count) is read once at startup to build the LeadTabScanner's " + "identity map and to auto-launch leads, and neither is rebuilt on reload, so a " + "lead added, removed, or given a new tab: label needs a restart — until then it " + "stays unrecognised, and a caller from its new tab resolves as a worker, not a " - + "lead; the rest of fleet: (architects, developers, reviewers, charters, " - + "tabLabel) is read live through the supplier on CompositePeerLauncher and " - + "already applied"); + + "lead; the rest of fleet: (developers, reviewers, charters, tabLabel) is read " + + "live through the supplier on CompositePeerLauncher, and architects is read " + + "live through that same supplier for placement AND through a separate supplier " + + "on MemberRegistry for spawn-time identity — both already applied"); } // Kept in step with SPLIT_KEYS the same way changedColdKeys is kept in step with COLD_KEYS — // every message here must be traceable to one of the split keys the class doc documents. diff --git a/fleetd/src/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java b/fleetd/src/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java new file mode 100644 index 0000000..ea177e1 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java @@ -0,0 +1,257 @@ +package dev.ltms.fleet.auth; + +import dev.ltms.fleet.config.ConfigRef; +import dev.ltms.fleet.config.FleetConfig; +import dev.ltms.fleet.herdr.FakeHerdr; +import dev.ltms.fleet.herdr.PaneLocator; +import dev.ltms.fleet.mcp.ConnectionIdentity; +import dev.ltms.fleet.peer.MemberRole; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * fleetd #424 — revoking (or granting) an architect slot must take effect on the next spawn with + * no restart. A session already bound to a slot keeps its binding (the {@code + * terminalToSlot} occupancy) even after that slot drops out of config, but NOT the ARCHITECT + * privilege the slot used to grant — that is revoked on the bound session's very next + * request. See {@link MemberRegistry}'s class doc for the exact rule: "config governs what a + * bound slot still grants, as well as what may be bound next." + * + *

    Every test here drives a REAL {@link ConfigRef#reload()} against a {@code @TempDir} file and + * asserts {@link ConfigRef.Outcome#applied()}, rather than comparing two frozen + * {@code MemberRegistry} instances in memory — the defect this ticket fixes is specifically that + * {@link MemberRegistry} used to ignore a live reload, so a test that never reloads cannot tell the + * fixed registry from the broken one. {@code requireSlotFor} and {@code reserve} are pinned in + * separate tests, in both directions (removed and added), so a registry that simply refuses (or + * simply allows) everything cannot pass by accident — see {@link MemberRegistryTest} for the + * registry's other invariants (bind/unbind cardinality, thread-safety), which are unaffected by + * this ticket and still exercised against the frozen constructor. + */ +class MemberRegistryLiveTest { + + private static String yaml(String fleetBlock) { + return """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet + opus: + baseUrl: http://gx00.gw:8001 + model: opus + guard: + offSubscriptionHosts: + - gx00.gw + """ + fleetBlock; + } + + private static final String WITH_SONNET_SLOT = """ + fleet: + architects: + designer: + profile: sonnet + """; + + /** No architect pool at all — developers is unrelated dead data for this registry (#424 out of scope). */ + private static final String WITHOUT_ARCHITECT_SLOTS = """ + fleet: + developers: + dev1: + profile: sonnet + """; + + private static ConfigRef refFor(Path f) { + return new ConfigRef(f, FleetConfig.load(f)); + } + + // ── requireSlotFor is live (criteria 1, 2, 4) ────────────────────────────────────────────── + + @Test + void requireSlotForRefusesAProfileWhoseSlotWasRemovedByReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + assertDoesNotThrow(() -> registry.requireSlotFor(MemberRole.ARCHITECT, "sonnet"), + "the slot is configured before the reload"); + + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + assertThrows(IllegalArgumentException.class, + () -> registry.requireSlotFor(MemberRole.ARCHITECT, "sonnet"), + "revoking the slot must refuse the NEXT spawn that names it"); + } + + @Test + void requireSlotForAllowsAProfileWhoseSlotWasAddedByReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + assertThrows(IllegalArgumentException.class, + () -> registry.requireSlotFor(MemberRole.ARCHITECT, "sonnet"), + "no architect slot is configured yet"); + + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + assertDoesNotThrow(() -> registry.requireSlotFor(MemberRole.ARCHITECT, "sonnet"), + "a slot added by reload must be usable with no restart"); + } + + // ── reserve is live too — tested separately from requireSlotFor (criteria 1, 2, 4) ──────── + + @Test + void reserveRefusesAProfileWhoseSlotWasRemovedByReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + MemberLifecycle.SlotReservation before = registry.reserve(MemberRole.ARCHITECT, "sonnet"); + assertEquals("architect:designer", before.slot()); + registry.release(before); // free it back up so the reload-side reserve below starts clean + + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + assertThrows(IllegalArgumentException.class, + () -> registry.reserve(MemberRole.ARCHITECT, "sonnet"), + "revoking the slot must refuse the NEXT reservation for it"); + } + + @Test + void reserveAllowsAProfileWhoseSlotWasAddedByReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + assertThrows(IllegalArgumentException.class, + () -> registry.reserve(MemberRole.ARCHITECT, "sonnet"), + "no architect slot is configured yet"); + + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + MemberLifecycle.SlotReservation after = registry.reserve(MemberRole.ARCHITECT, "sonnet"); + assertEquals("architect:designer", after.slot(), + "a slot added by reload must be reservable with no restart"); + } + + // ── a bound architect is demoted, but the binding itself is not touched (fleetd #424) ─────── + // The lead's corrected ruling: the PRIVILEGE a slot grants is revoked on the bound session's + // very next request, but the terminalToSlot BINDING itself is untouched by a reload — dropping + // it would double-book the slot key and break unbind's compare-safe contract. See the class + // doc's binding rule. + + /** A caller identity resolving the one canned pane (terminal {@code term_a}) in {@link FakeHerdr}. */ + private static ConnectionIdentity boundPaneIdentity() { + return new ConnectionIdentity(new PaneLocator(new FakeHerdr()), _ -> FakeHerdr.WORKER_PID); + } + + @Test + void anArchitectAlreadyBoundToASlotIsDemotedByReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + MemberLifecycle.SlotReservation reservation = registry.reserve(MemberRole.ARCHITECT, "sonnet"); + assertTrue(registry.bind(reservation, "term_a")); + assertEquals("architect:designer", registry.slotForTerminal("term_a")); + + // Drive the real caller path, not the roleForSlot seam directly: CallerResolver.resolve is + // what a live request actually goes through (CallerResolver.java:220), and a resolver that + // ignored roleForSlot entirely would still pass a test that only checked the seam. + CallerResolver resolver = CallerResolver.withLeadsAndMembers( + boundPaneIdentity(), false, null, Map::of, registry); + + Principal before = resolver.resolve("127.0.0.1", 42, null); + assertEquals(Role.ARCHITECT, before.role(), "sanity check: the harness binds term_a as an architect"); + assertEquals("designer", before.name()); + + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + Principal after = resolver.resolve("127.0.0.1", 42, null); + assertEquals(Role.WORKER, after.role(), + "removing the slot from config must demote the bound session to worker on its " + + "NEXT request — this is the ticket's whole point"); + assertEquals("term_a", after.terminal(), "same pane, same terminal — only the role changed"); + } + + @Test + void theOriginalBindingStillOccupiesTheRemovedSlotSoASecondTerminalCannotClaimIt(@TempDir Path dir) + throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + MemberLifecycle.SlotReservation reservation = registry.reserve(MemberRole.ARCHITECT, "sonnet"); + assertTrue(registry.bind(reservation, "term_a")); + + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef.Outcome removed = ref.reload(); + assertTrue(removed.applied(), "the reload must actually take effect: " + removed.summary()); + + // The binding survives the removal untouched. + assertEquals("architect:designer", registry.slotForTerminal("term_a"), + "a live binding must never be retroactively unbound by a config edit"); + assertEquals(Map.of("term_a", "architect:designer"), registry.snapshot()); + + // Bring the slot back into config. If the binding had been silently dropped by the removal + // (rather than merely losing the privilege it grants), a second terminal could now claim + // the "freed" key — the exact double-booking the class doc's binding rule rules out. + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef.Outcome restored = ref.reload(); + assertTrue(restored.applied(), "the reload must actually take effect: " + restored.summary()); + + assertFalse(registry.bind("architect:designer", "term_b"), + "the slot is still occupied by term_a — a second terminal must not bind to it"); + assertThrows(IllegalArgumentException.class, + () -> registry.reserve(MemberRole.ARCHITECT, "sonnet"), + "the slot is still occupied by term_a — a fresh reservation must not find it free"); + assertEquals("architect:designer", registry.slotForTerminal("term_a"), + "the original binding is unchanged throughout"); + } + + @Test + void unbindStillSucceedsForTheOriginalTerminalAfterItsSlotIsRemoved(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + MemberLifecycle.SlotReservation reservation = registry.reserve(MemberRole.ARCHITECT, "sonnet"); + assertTrue(registry.bind(reservation, "term_a")); + + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + assertTrue(registry.unbind("architect:designer", "term_a"), + "unbind must still work for a slot that config has since removed, or a session " + + "that outlives its slot's removal could never release it"); + assertNull(registry.slotForTerminal("term_a")); + assertEquals(Map.of(), registry.snapshot()); + } +}