diff --git a/bridged/src/main/java/dev/ltms/bridged/Bridged.java b/bridged/src/main/java/dev/ltms/bridged/Bridged.java index 479b188..46b0c81 100644 --- a/bridged/src/main/java/dev/ltms/bridged/Bridged.java +++ b/bridged/src/main/java/dev/ltms/bridged/Bridged.java @@ -138,7 +138,8 @@ public final class Bridged { cfg.effectiveDefaultProfile(), cfg.profiles(), PlacementPolicies.fromName(cfg.placement()), - profileName -> liveCountRef.get().apply(profileName)); + profileName -> liveCountRef.get().apply(profileName), + cfg.fleet()); // CB-504: under supervision (launchd/systemd) bridged can start before herdr's socket // exists. The client itself is lazy — it connects per call — but the orphan reap below is // the first thing that actually talks to herdr, so without this wait a boot-order race diff --git a/bridged/src/main/java/dev/ltms/bridged/member/CompositePeerLauncher.java b/bridged/src/main/java/dev/ltms/bridged/member/CompositePeerLauncher.java index f6dac69..a8866d5 100644 --- a/bridged/src/main/java/dev/ltms/bridged/member/CompositePeerLauncher.java +++ b/bridged/src/main/java/dev/ltms/bridged/member/CompositePeerLauncher.java @@ -3,6 +3,7 @@ package dev.ltms.bridged.member; import dev.ltms.bridged.config.BridgedConfig; import dev.ltms.bridged.herdr.Agent; import dev.ltms.bridged.peer.Capability; +import dev.ltms.bridged.peer.MemberRole; import dev.ltms.bridged.peer.PeerHandle; import dev.ltms.bridged.peer.PeerLauncher; import dev.ltms.bridged.peer.PeerUnreachableException; @@ -68,6 +69,13 @@ public final class CompositePeerLauncher implements PeerLauncher { private final PlacementPolicy placementPolicy; private final Function liveCount; + /** + * CB-557: the role pools an unqualified spawn draws its candidates from. Nullable, and an empty + * pool for a role means "no pool configured" — both fall back to every configured profile, which + * is the pre-CB-557 behaviour. + */ + private final BridgedConfig.Fleet fleet; + /** * Backward-compatible constructor: fixed placement, no live-counting. Use this for tests and * simple wiring; it preserves the pre-CB-518 behaviour exactly. @@ -78,7 +86,7 @@ public final class CompositePeerLauncher implements PeerLauncher { * @throws IllegalArgumentException if {@code delegates} is empty or two adapters claim one profile */ public CompositePeerLauncher(List delegates, String defaultProfile) { - this(delegates, defaultProfile, Map.of(), PlacementPolicies.fixed(), name -> 0); + this(delegates, defaultProfile, Map.of(), PlacementPolicies.fixed(), _ -> 0); } /** @@ -96,6 +104,24 @@ public final class CompositePeerLauncher implements PeerLauncher { Map profileConfigs, PlacementPolicy placementPolicy, Function liveCount) { + this(delegates, defaultProfile, profileConfigs, placementPolicy, liveCount, null); + } + + /** + * Production constructor with role pools (CB-557). An unqualified spawn draws its candidates from + * {@code fleet.} instead of from every configured profile, so a reviewer is placed on a + * reviewer backend and never on, say, the architect-only one. + * + * @param fleet the configured role pools; {@code null} ⇒ every profile is a candidate for every + * role, which is the pre-CB-557 behaviour + */ + public CompositePeerLauncher(List delegates, + String defaultProfile, + Map profileConfigs, + PlacementPolicy placementPolicy, + Function liveCount, + BridgedConfig.Fleet fleet) { + this.fleet = fleet; if (delegates.isEmpty()) { throw new IllegalArgumentException("at least one peer adapter must be configured"); } @@ -151,20 +177,21 @@ public final class CompositePeerLauncher implements PeerLauncher { return handle; } - List candidates = candidates(); + // CB-557: an unqualified spawn is placed inside the pool of the role it asked for, not across + // the whole profile list. An EXPLICIT profile (above) is left alone on purpose — it is the + // operator overriding, and refusing it would break `bridge_spawn{profile:"opus"}`, which + // carries no role and so would be judged against the dev pool it was never meant for. + List candidates = candidates(req.role()); + String roleDefault = defaultProfileFor(req.role()); Set unreachable = new HashSet<>(); - PlacementContext ctx = new PlacementContext(defaultProfile, candidates, liveCount, unreachable); + PlacementContext ctx = new PlacementContext(roleDefault, candidates, liveCount, unreachable); int maxAttempts = candidates.isEmpty() ? 1 : candidates.size(); for (int attempt = 0; attempt < maxAttempts; attempt++) { - PlacementCandidate chosen; - try { - chosen = placementPolicy.select(ctx); - } catch (RuntimeException e) { - // No candidate left (all at cap or all unreachable). The policy already threw a clear - // message; do not wrap it in a generic PeerUnreachableException. - throw e; - } + // Deliberately uncaught: when no candidate is left (all at cap, or all unreachable) the + // policy already throws a clear message. Catching it to rethrow a generic + // PeerUnreachableException would replace a precise diagnosis with a vague one. + PlacementCandidate chosen = placementPolicy.select(ctx); HerdrPeerLauncher d = byProfile.get(chosen.profile()); if (d == null) { @@ -187,7 +214,7 @@ public final class CompositePeerLauncher implements PeerLauncher { chosen.profile(), e.getMessage()); unreachable.add(chosen.profile()); // Update the context for the next selection so the policy excludes this profile. - ctx = new PlacementContext(defaultProfile, candidates, liveCount, unreachable); + ctx = new PlacementContext(roleDefault, candidates, liveCount, unreachable); } } @@ -201,8 +228,8 @@ public final class CompositePeerLauncher implements PeerLauncher { * *

maxLoad is a documented, unconditional capacity limit (see {@code BridgedConfig.Profile#maxLoad}), * and the charter makes explicit-profile spawns the normal path — so enforcing it only in placement - * ({@link dev.ltms.bridged.placement.PlacementPolicyUtil}) would leave the cap dead config on every - * call that names a profile. Same rule as placement: {@code live >= cap} is at capacity. + * ({@code PlacementPolicyUtil}, package-private, hence not linked) would leave the cap dead config + * on every call that names a profile. Same rule as placement: {@code live >= cap} is at capacity. * *

Deliberately no fallback to another profile: the caller named {@code profile} for a cost/model * reason, and silently re-routing a paid-tier (subscription) request elsewhere is worse than @@ -232,12 +259,34 @@ public final class CompositePeerLauncher implements PeerLauncher { } } - /** Build the candidate list from the configured profiles, in definition order. */ - private List candidates() { + /** + * The profile names {@code role} may be placed on, in definition order. + * + *

An empty or absent pool means "unconstrained", not "nothing allowed": a config that declares + * no pool for a role must keep spawning, so it falls back to every configured profile. Names in a + * pool that no adapter declares are dropped here rather than thrown — config load already rejects + * a pool entry with no profile, so a survivor is a profile this particular composite does not own. + */ + private List poolFor(MemberRole role) { + List pool = (fleet == null) ? List.of() : fleet.profilesFor(role); + List known = pool.stream().filter(profileConfigs::containsKey).toList(); + return known.isEmpty() ? List.copyOf(profileConfigs.keySet()) : known; + } + + /** The profile an unqualified spawn for {@code role} falls back to under {@code fixed} placement. */ + private String defaultProfileFor(MemberRole role) { + List pool = poolFor(role); + return pool.isEmpty() ? defaultProfile : pool.getFirst(); + } + + /** Build the candidate list from {@code role}'s pool, in definition order. */ + private List candidates(MemberRole role) { List out = new ArrayList<>(); - for (Map.Entry e : profileConfigs.entrySet()) { - BridgedConfig.Profile w = e.getValue(); - out.add(new PlacementCandidate(e.getKey(), null, w.weight(), w.maxLoad())); + for (String name : poolFor(role)) { + BridgedConfig.Profile w = profileConfigs.get(name); + if (w != null) { + out.add(new PlacementCandidate(name, null, w.weight(), w.maxLoad())); + } } return out; } diff --git a/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java b/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java index a7056ac..99a8947 100644 --- a/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java +++ b/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java @@ -135,7 +135,10 @@ public final class SessionManager implements TurnListener { String ownerTerminal, WorktreeRequest wt) { MemberRole memberRole = (role == null) ? MemberRole.DEV : role; if (wt == null) { - SpawnRequest req = new SpawnRequest(profile, requestedCwd, callerCwd); + // CB-557: the role must ride on the SpawnRequest, not stay a local. The launcher needs it + // to pick the profile out of that role's pool and to label the tab; a role kept only on + // the MemberSession is recorded after the spawn it was supposed to steer. + SpawnRequest req = new SpawnRequest(profile, requestedCwd, callerCwd, null, null, memberRole); PeerHandle handle = launcher.spawn(req); String resolvedProfile = resolveProfile(handle, profile); String cwd = launcher.effectiveCwd(new SpawnRequest(resolvedProfile, requestedCwd, callerCwd)); @@ -299,7 +302,7 @@ public final class SessionManager implements TurnListener { try { path = worktrees.add(repoRoot, branch, wt.baseRef()); worktrees.overlayParity(repoRoot, path, launcher.parityOverlay(preResolvedProfile)); - handle = launcher.spawn(new SpawnRequest(profile, path, callerCwd)); + handle = launcher.spawn(new SpawnRequest(profile, path, callerCwd, null, null, memberRole)); } catch (RuntimeException e) { if (path != null) { try { diff --git a/bridged/src/test/java/dev/ltms/bridged/member/CompositePeerLauncherTest.java b/bridged/src/test/java/dev/ltms/bridged/member/CompositePeerLauncherTest.java index baa3f26..d5791dc 100644 --- a/bridged/src/test/java/dev/ltms/bridged/member/CompositePeerLauncherTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/member/CompositePeerLauncherTest.java @@ -10,6 +10,7 @@ import dev.ltms.bridged.herdr.AgentControl; import dev.ltms.bridged.herdr.FakeHerdr; import dev.ltms.bridged.herdr.WorkspaceControl; import dev.ltms.bridged.peer.Capability; +import dev.ltms.bridged.peer.MemberRole; import dev.ltms.bridged.peer.PeerHandle; import dev.ltms.bridged.peer.PeerLauncher; import dev.ltms.bridged.peer.PeerUnreachableException; @@ -21,12 +22,10 @@ import org.slf4j.LoggerFactory; import java.util.EnumSet; import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.function.Function; import static org.junit.jupiter.api.Assertions.*; @@ -313,7 +312,7 @@ class CompositePeerLauncherTest { "b", stubWorker("b", 0.25f, null)); StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of()); CompositePeerLauncher composite = new CompositePeerLauncher( - List.of(adapter), "a", profiles, PlacementPolicies.weighted(), name -> 0); + List.of(adapter), "a", profiles, PlacementPolicies.weighted(), _ -> 0); int a = 0, b = 0; for (int i = 0; i < 40; i++) { @@ -333,7 +332,7 @@ class CompositePeerLauncherTest { "b", stubWorker("b")); StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of("a")); CompositePeerLauncher composite = new CompositePeerLauncher( - List.of(adapter), "a", profiles, PlacementPolicies.weighted(), name -> 0); + List.of(adapter), "a", profiles, PlacementPolicies.weighted(), _ -> 0); PeerHandle h = composite.spawn(new SpawnRequest(null, null, null)); assertEquals("b", h.profile(), "the spawn must fail over from unreachable a to b"); @@ -369,7 +368,7 @@ class CompositePeerLauncherTest { "b", stubWorker("b")); StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of("a", "b")); CompositePeerLauncher composite = new CompositePeerLauncher( - List.of(adapter), "a", profiles, PlacementPolicies.weighted(), name -> 0); + List.of(adapter), "a", profiles, PlacementPolicies.weighted(), _ -> 0); PeerUnreachableException e = assertThrows(PeerUnreachableException.class, () -> composite.spawn(new SpawnRequest(null, null, null))); @@ -420,7 +419,7 @@ class CompositePeerLauncherTest { StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of()); // A deliberately absurd live count: an unset maxLoad means unlimited, so it must never refuse. CompositePeerLauncher composite = new CompositePeerLauncher( - List.of(adapter), "a", profiles, PlacementPolicies.fixed(), name -> 1000); + List.of(adapter), "a", profiles, PlacementPolicies.fixed(), _ -> 1000); PeerHandle h = composite.spawn(new SpawnRequest("a", null, null)); assertEquals("a", h.profile(), "a profile with no maxLoad is never capped, however many live workers"); @@ -434,10 +433,123 @@ class CompositePeerLauncherTest { "b", stubWorker("b", 1.0f, 1)); StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of()); CompositePeerLauncher composite = new CompositePeerLauncher( - List.of(adapter), "a", profiles, PlacementPolicies.weighted(), name -> 1); + List.of(adapter), "a", profiles, PlacementPolicies.weighted(), _ -> 1); PlacementException e = assertThrows(PlacementException.class, () -> composite.spawn(new SpawnRequest(null, null, null))); assertTrue(e.getMessage().contains("maxLoad"), e.getMessage()); } + + // ── CB-557: an unqualified spawn is placed inside its role's pool ───────────────────────── + + /** Three profiles in definition order — pools are carved out of this set. */ + private static Map threeProfiles() { + Map m = new LinkedHashMap<>(); + m.put("opus", stubWorker("opus", 1.0f, null)); + m.put("sonnet", stubWorker("sonnet", 1.0f, null)); + m.put("terra", stubWorker("terra", 1.0f, null)); + return m; + } + + private static Map pool(String... names) { + Map m = new LinkedHashMap<>(); + for (String n : names) { + m.put(n, new BridgedConfig.Slot(n)); + } + return m; + } + + private static CompositePeerLauncher withPools(FakeHerdr herdr, BridgedConfig.Fleet fleet) { + Map profiles = threeProfiles(); + StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "opus", Set.of()); + return new CompositePeerLauncher(List.of(adapter), "opus", profiles, + PlacementPolicies.fixed(), _ -> 0, fleet); + } + + /** + * The point of the pools: a role is placed only on a backend its pool names. Before CB-557 an + * unqualified spawn ranged over every configured profile, so a reviewer could land on the + * architect-only one. + */ + @Test + void anUnqualifiedSpawnIsPlacedInsideItsRolePool() { + FakeHerdr herdr = new FakeHerdr(); + CompositePeerLauncher composite = withPools(herdr, new BridgedConfig.Fleet( + Map.of(), pool("opus"), pool("terra"), pool("sonnet"), null)); + + assertEquals("opus", composite.spawn( + new SpawnRequest(null, null, null, null, null, MemberRole.ARCHITECT)).profile()); + assertEquals("terra", composite.spawn( + new SpawnRequest(null, null, null, null, null, MemberRole.DEV)).profile()); + assertEquals("sonnet", composite.spawn( + new SpawnRequest(null, null, null, null, null, MemberRole.REVIEWER)).profile()); + } + + /** Under `fixed`, the pool's first entry wins — not the global defaultProfile. */ + @Test + void theRolePoolOutranksTheGlobalDefaultProfile() { + FakeHerdr herdr = new FakeHerdr(); + CompositePeerLauncher composite = withPools(herdr, new BridgedConfig.Fleet( + Map.of(), Map.of(), pool("sonnet", "terra"), Map.of(), null)); + + assertEquals("sonnet", composite.spawn( + new SpawnRequest(null, null, null, null, null, MemberRole.DEV)).profile(), + "the dev pool starts at sonnet, so the global default 'opus' must not win"); + } + + /** + * A role with no pool is unconstrained, not blocked. A config that declares pools for some roles + * and not others must keep spawning the rest. + */ + @Test + void aRoleWithNoPoolFallsBackToEveryProfile() { + FakeHerdr herdr = new FakeHerdr(); + CompositePeerLauncher composite = withPools(herdr, new BridgedConfig.Fleet( + Map.of(), pool("sonnet"), Map.of(), Map.of(), null)); + + assertEquals("opus", composite.spawn( + new SpawnRequest(null, null, null, null, null, MemberRole.DEV)).profile(), + "no dev pool ⇒ all profiles are candidates, so `fixed` takes the first one"); + } + + /** No fleet at all is the pre-CB-557 wiring, and must behave exactly as it did. */ + @Test + void noFleetConfiguredKeepsTheOldWholeProfileListBehaviour() { + FakeHerdr herdr = new FakeHerdr(); + CompositePeerLauncher composite = withPools(herdr, null); + + assertEquals("opus", composite.spawn(new SpawnRequest(null, null, null)).profile()); + } + + /** + * An explicit profile is the operator overriding and is NOT judged against the pool. It must + * stay that way: an unrolled `bridge_spawn{profile:"opus"}` carries no role, so it defaults to + * DEV, and enforcing the pool here would refuse a spawn the operator asked for by name. + */ + @Test + void anExplicitProfileIsNotConfinedToTheRolePool() { + FakeHerdr herdr = new FakeHerdr(); + CompositePeerLauncher composite = withPools(herdr, new BridgedConfig.Fleet( + Map.of(), pool("opus"), pool("terra"), Map.of(), null)); + + assertEquals("opus", composite.spawn(new SpawnRequest("opus", null, null)).profile(), + "naming opus explicitly must work even though the dev pool holds only terra"); + } + + /** Placement still respects maxLoad, but only across the pool — never by escaping it. */ + @Test + void aFullPoolIsRefusedRatherThanSpilledOntoAnotherRolesProfile() { + FakeHerdr herdr = new FakeHerdr(); + Map profiles = new LinkedHashMap<>(); + profiles.put("opus", stubWorker("opus", 1.0f, null)); // architect-only, uncapped + profiles.put("terra", stubWorker("terra", 1.0f, 1)); // the sole dev, capped at 1 + StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "opus", Set.of()); + CompositePeerLauncher composite = new CompositePeerLauncher(List.of(adapter), "opus", profiles, + PlacementPolicies.weighted(), name -> "terra".equals(name) ? 1 : 0, + new BridgedConfig.Fleet(Map.of(), pool("opus"), pool("terra"), Map.of(), null)); + + PlacementException e = assertThrows(PlacementException.class, () -> composite.spawn( + new SpawnRequest(null, null, null, null, null, MemberRole.DEV))); + assertTrue(e.getMessage().contains("maxLoad"), e.getMessage()); + } }