From 61944fc045499e38e470e90ff08a5e4c90266b6b Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 14 Aug 2026 16:37:29 +0200 Subject: [PATCH] CB-557: place an unqualified spawn inside its role's pool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pools were config-only until now: the launchers still received one global effectiveDefaultProfile and placement still ranged over every configured profile, so a reviewer could be placed on an architect-only backend. Three parts: SessionManager computed the role, stored it on the MemberSession, and never put it on the SpawnRequest. So the role reached the record that describes the spawn but not the call that performs it — every launcher saw DEV. Both spawn paths (plain and worktree) now carry it. CompositePeerLauncher takes the Fleet and draws its candidates from fleet. instead of from all profiles. An absent or empty pool means unconstrained, not blocked: a config that declares pools for some roles must keep spawning the rest, so it falls back to every profile. A null Fleet is the pre-CB-557 wiring and behaves exactly as before. Bridged passes cfg.fleet() to the composite and cfg.fleet().tabLabel() to both launchers. The tab-label knob was accepted by HerdrPeerLauncher but passed by nobody, so it was inert — the label only looked right because the fallback happened to match the configured template. Four tests now pin the wiring instead of the coincidence. An EXPLICIT profile stays exempt from the pool. `bridge_spawn{profile:"opus"}` carries no role, so it defaults to DEV; judging it against the dev pool would refuse a spawn the operator asked for by name. maxLoad still applies to it. Also cleared the IDE warnings in the touched files: an immediately-rethrown catch (the comment stays, the redundant block goes), unused lambda params, a javadoc link to a package-private class, two unused imports. 601 tests pass. --- .../main/java/dev/ltms/bridged/Bridged.java | 3 +- .../bridged/member/CompositePeerLauncher.java | 87 +++++++++--- .../ltms/bridged/session/SessionManager.java | 7 +- .../member/CompositePeerLauncherTest.java | 126 +++++++++++++++++- 4 files changed, 194 insertions(+), 29 deletions(-) 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()); + } }