From 4b48d2d921c64642f11c9362160b681909901e7a Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 14 Aug 2026 16:32:54 +0200 Subject: [PATCH 1/4] =?UTF-8?q?CB-557:=20fleet=20role=20pools=20=E2=80=94?= =?UTF-8?q?=20role=20is=20the=20config=20key,=20and=20the=20tab=20label=20?= =?UTF-8?q?says=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four top-level keys (leaders:, members:, leadScan:, defaultProfile:) become one `fleet:` block, and a member's role becomes the map key that contains it rather than a `role:` field inside it. Why the key and not a field: a misspelled `role: architct` used to produce a member with no contract, which nothing rejected. A misspelled pool name declares nothing, which is a shape the loader can see. `fleet.architects/developers/reviewers` are pools of profiles a role MAY run on. That replaces the single global `defaultProfile:`, so an unqualified spawn now resolves its profile from the pool of the role it asked for. Role and profile stay orthogonal: a reviewer may run on the same profile as the dev it reviews, and one profile may appear in several pools. Tab labels are role-first — `dev: sonnet #4`. The template lives on `fleet:` because a profile cannot know the role of the member launched on it; a profile may still override it. The `{n}` counter is scoped per role+profile, so a dev and a reviewer on one profile each start at #1. Making {role} the first field also turns the lead/member namespace check into a structural guarantee: roles are a closed enum, so only hand-written templates can still collide with a lead tabPrefix. Removed keys are hard errors that name their successor. `defaultProfile:` has no single successor key, so its message explains the new model instead of pointing at a key that does not exist. Map order is kept with LinkedHashMap, deliberately not Map.copyOf — the latter salts iteration order per JVM run, which would destroy the YAML definition order that `placement: fixed` selects on. Not yet wired: SessionManager still hands the launchers one effectiveDefault- Profile, so pools are not enforced at spawn time yet, and placement still ranges over all profiles. 595 tests pass. --- bridged/bridged.example.yaml | 112 ++-- .../main/java/dev/ltms/bridged/Bridged.java | 44 +- .../dev/ltms/bridged/auth/MemberRegistry.java | 70 ++- .../ltms/bridged/config/BridgedConfig.java | 529 ++++++++++------ .../bridged/member/ClaudeCodeLauncher.java | 35 +- .../bridged/member/CompositePeerLauncher.java | 5 +- .../bridged/member/HerdrPeerLauncher.java | 71 ++- .../ltms/bridged/member/OpenCodeLauncher.java | 35 +- .../dev/ltms/bridged/peer/MemberRole.java | 34 ++ .../dev/ltms/bridged/peer/SpawnRequest.java | 36 +- .../ltms/bridged/auth/MemberRegistryTest.java | 115 ++-- .../bridged/config/BridgedConfigTest.java | 576 ++++++++++++------ .../member/ClaudeCodeLauncherTest.java | 76 +++ 13 files changed, 1243 insertions(+), 495 deletions(-) diff --git a/bridged/bridged.example.yaml b/bridged/bridged.example.yaml index 1179042..17179f7 100644 --- a/bridged/bridged.example.yaml +++ b/bridged/bridged.example.yaml @@ -55,34 +55,14 @@ bind: # IS a primary for authorization, so nothing that keys on the role breaks. # # KEEP `primary:` when adding leads: it still addresses the CB-307 push loop, which needs a single -# destination for its nudges. If both name the same terminal, the `leaders:` entry wins. -# leaders: -# opus-5.0: -# terminal: term_0123456789abcd -# kind: claude -# gpt-sol-5.6: -# terminal: term_fedcba9876543 -# kind: opencode -# model: openai/gpt-5.6-terra - -# CB-531: FIND LEADS BY TAB NAME instead of pasting terminal ids. `leaders:` above needs an id that -# only exists once the session is running, so adding a lead is: open a tab, start the agent, ask it -# bridge_whoami, edit this file, restart the daemon. This block replaces all of that with a naming -# convention — label the tab `lead: ` when you open it and the pane is recognised on the next -# rescan, with no config edit and no restart. Reopen the tab later and the id changes; the label -# does not. +# destination for its nudges. If both name the same terminal, the `fleet.leaders:` entry wins. # -# bridged NEVER writes these labels. It renames worker tabs (see `tabLabel` below) but reads lead -# tabs read-only, so what is in the tab bar is always what you typed. Two things keep the convention -# from being a way to claim leadership: the configured worker spaces are excluded from the scan, so -# nothing bridged places can land in a matching tab; and startup REFUSES a `tabPrefix` that any -# worker `tabLabel` also matches, so the two namespaces cannot overlap by accident. +# Leads are configured under `fleet.leaders:` — see THE FLEET further down. # -# Opt-in on purpose — this widens who resolves as a lead, so upgrading the daemon must never switch -# it on for you. Absent block = leads come only from `leaders:`/`primary:`, exactly as before. -# leadScan: -# tabPrefix: "lead:" # `lead: opus-5.0` ⇒ a lead named opus-5.0 (case-insensitive; default "lead:") -# intervalSeconds: 10 # rescan cadence, and the worst case before a new tab is recognised +# Two things stop the tab-name convention from becoming a way to claim leadership: the configured +# member spaces are excluded from the scan, so nothing bridged places can land in a matching tab; +# and startup REFUSES a `tabPrefix` that the fleet tabLabel template, or any per-profile `tabLabel` +# override, also matches — so the two namespaces cannot overlap by accident. # CB-551: IDLE-LEAD HEARTBEAT — nudge the single lead back to work when it has been continuously # idle (no open bridge_send driving it) past the quiet period. The fleet is one lead + architects + @@ -108,11 +88,13 @@ bind: # (${HERDR_SOCKET_PATH:-~/.config/herdr/herdr.sock}). herdrSocket: ~/.config/herdr/herdr.sock -# How worker sessions are spawned. Define one or more named profiles (backends) under -# `workers`; each key is the profile name (also the ccs profile). `defaultWorker` picks -# which one a no-argument spawn uses (bridge_spawn with no profile / POST /workers). +# How member sessions are spawned. Define one or more named profiles (backends) under +# `profiles`; each key is the profile name (also the ccs profile). A profile says only WHICH +# BACKEND — model, CLI adapter, credentials, cost. It says nothing about what a member spawned on +# it is for; that is the member's role, and roles live under `fleet:` below. Which profile an +# unqualified spawn lands on comes from that role's pool, not from a global default. # -# Shared knobs (placement/workspace/tabLabel) can be repeated per profile; they usually match. +# Shared knobs (placement/workspace) can be repeated per profile; they usually match. # placement: tab → each worker lands in its OWN tab in a dedicated worker space (default). # Use `pane` for the legacy behaviour (split the focused tab). # mcpUrl → bridged mounts the bridge MCP (--mcp-config, inline) + reply charter @@ -167,7 +149,7 @@ profiles: model: coder placement: tab workspace: bridged-workers - tabLabel: "worker: {profile} #{n}" # {profile}/{model}/{n} substituted; {n} keeps sibling tabs distinct + # tabLabel: an optional per-profile override; the fleet template usually covers it mcpUrl: http://127.0.0.1:8765/mcp tokenEnv: BRIDGED_WORKER_TOKEN argv: ["ccs", "gx10"] @@ -182,7 +164,7 @@ profiles: baseUrl: http://gx01.gw:8000 # self-hosted; ccs handles the model + token placement: tab workspace: bridged-workers - tabLabel: "worker: {profile} #{n}" + # tabLabel: an optional per-profile override; the fleet template usually covers it mcpUrl: http://127.0.0.1:8765/mcp argv: ["ccs", "gx11"] weight: 0.5 @@ -239,30 +221,64 @@ profiles: # How an unqualified spawn chooses a profile: fixed (default, reproduces pre-CB-518 behaviour), # round-robin, or weighted. Omitting this key is a strict no-op for existing configs. placement: weighted -defaultProfile: gx10 -# Named member slots. A member is anything a lead spawns. Every member has two independent -# attributes: +# THE FLEET (CB-557) — who the daemon may run, and under which role. This one block replaced four +# older keys: `leaders:`, `members:`, `leadScan:` and `defaultProfile:`. +# +# A member is anything a lead spawns, and every member has two INDEPENDENT attributes: # role — which contract: architect, dev or reviewer. It picks the launch charter, the role # file, the playbook skill and the authz row. # profile — which backend: one of the `profiles:` keys above (model, CLI adapter, cost). # They vary on their own. A reviewer may run on the same profile as the dev whose diff it reads, # which is why the two cannot be one field. # -# Only members that need a STABLE IDENTITY are declared here, because a lead addresses the same -# slot across many tickets. Architects are such members. A dev or a reviewer is anonymous and -# short-lived — the lead spawns it per task and tears it down after — so it never appears here. +# The ROLE IS THE CONTAINING KEY, not a `role:` field. That is not only tidier: a misspelled role +# used to parse into a member with no contract at all, while a misspelled pool name here simply +# declares nothing. # -# A slot is declared, not recognised: config gives no terminal, so every slot is idle at boot and -# nothing resolves to it until the spawn lifecycle binds a live terminal. Nothing here spawns one. -# members: -# architect-1: -# role: architect -# profile: opus # a strong model, on the operator's subscription -# architect-2: -# role: architect -# profile: sol # a different vendor on purpose — two architects that share a -# # model share its blind spots +# Each pool lists the profiles that role MAY run on — these are pools, not identities. That is also +# what replaced `defaultProfile:`: an unqualified spawn names a role, and that role's pool supplies +# the candidates, in definition order. A dev and a reviewer staying anonymous is exactly compatible +# with being listed here; the entry key just names the entry. +fleet: + # Optional. Template for a member tab's label; {role}, {profile}, {model} and {n} are substituted. + # {n} counts per role+profile, so `dev: sonnet #2` really is the second sonnet dev. Because {role} + # comes from a closed enum, a generated label can never begin with a lead's tabPrefix. + # tabLabel: "{role}: {profile} #{n}" + + # Panes that orchestrate rather than are orchestrated. A lead may now be CREATED as well as + # recognised: give it a `profile:` and the daemon launches the shortfall when fewer than + # `instances` are live. Give it only a `terminal:` and it is recognise-only, as before. + # + # `tabPrefix` is the naming convention that finds a lead without pasting a terminal id: label the + # tab `lead: ` when you open it and the pane is recognised on the next rescan. Reopen the + # tab later and the id changes; the label does not. bridged NEVER writes these labels — it renames + # member tabs but reads lead tabs read-only, so the tab bar always shows what you typed. + # leaders: + # opus-5.0: + # profile: opus # omit to never create this lead, only recognise it + # instances: 1 # desired live count; only the shortfall is launched + # terminal: term_0123456789abcd # optional hand-pin; usually found by tabPrefix instead + # tabPrefix: "lead:" # `lead: opus-5.0` ⇒ a lead named opus-5.0 (case-insensitive) + # scanIntervalSeconds: 10 # rescan cadence, and the worst case before a new tab is seen + # kind: claude # descriptive; reported by bridge_whoami + # gpt-sol-5.6: + # terminal: term_fedcba9876543 + # kind: opencode + # model: openai/gpt-5.6-terra + + # architects: + # architect-1: + # profile: opus # a strong model, on the operator's subscription + # architect-2: + # profile: sol # a different vendor on purpose — two architects that share a + # # model share its blind spots + developers: + gx10: + profile: gx10 + # reviewers: + # gx10: + # profile: gx10 # the same backend may serve two roles; that is the point # Subscription boundary. A worker's base_url host MUST be one of these; the primary # must carry none. Every profile above must have its host listed here. diff --git a/bridged/src/main/java/dev/ltms/bridged/Bridged.java b/bridged/src/main/java/dev/ltms/bridged/Bridged.java index f692244..479b188 100644 --- a/bridged/src/main/java/dev/ltms/bridged/Bridged.java +++ b/bridged/src/main/java/dev/ltms/bridged/Bridged.java @@ -88,7 +88,7 @@ public final class Bridged { // refuses remote connections to a loopback socket. This throws rather than warns so the // dangerous configuration cannot be reached by ignoring a log line. cfg.validateAuthExposure(); - cfg.validateLeadScan(); + cfg.validateLeadTabPrefixes(); // CB-542: a subscription:true profile whose env: reseats ANTHROPIC_BASE_URL/AUTH_TOKEN would // reach an unguarded endpoint (the launcher skips SubscriptionGuard for it). Refuse at load. cfg.validateSubscriptionProfiles(); @@ -123,12 +123,14 @@ public final class Bridged { if (!claudeProfiles.isEmpty() || opencodeProfiles.isEmpty()) { adapters.add(new ClaudeCodeLauncher(agents, spaces, guard, claudeProfiles, cfg.effectiveDefaultProfile(), System::getenv, - cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs())); + cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs(), + cfg.fleet().tabLabel())); } if (!opencodeProfiles.isEmpty()) { adapters.add(new OpenCodeLauncher(agents, spaces, opencodeProfiles, cfg.effectiveDefaultProfile(), System::getenv, - cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs())); + cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs(), + cfg.fleet().tabLabel())); } AtomicReference> liveCountRef = new AtomicReference<>(_ -> 0); PeerLauncher workers = new CompositePeerLauncher( @@ -186,20 +188,33 @@ public final class Bridged { log.info("leads: {} panes recognised {}", leadTerminals.size(), leadTerminals.values()); } // CB-531: on top of the static registry, discover leads by the tab labels the operator - // writes. Opt-in, so a config with no `leadScan:` block resolves exactly as it did under - // CB-530 — the supplier is then a constant and never touches herdr. + // writes. CB-557 moved the settings onto the lead they describe, so scanning is on whenever + // a `fleet.leaders:` entry exists — with no leads configured the supplier is a constant and + // never touches herdr, exactly as a missing `leadScan:` block used to behave. final Supplier> leads; - if (cfg.leadScan() != null) { - var scan = cfg.leadScan(); - Set workerSpaces = cfg.profiles().values().stream() + var leaders = cfg.fleet().leaders(); + if (!leaders.isEmpty()) { + // One scanner, so one prefix and one interval. Distinct per-lead prefixes would need a + // scanner each; until a config actually wants that, take the first entry's settings and + // say so, rather than silently honouring one lead's prefix and dropping another's. + var scan = leaders.values().iterator().next(); + Set memberSpaces = cfg.profiles().values().stream() .map(BridgedConfig.Profile::workspace) .filter(Objects::nonNull) .collect(Collectors.toSet()); - leads = new LeadTabScanner(herdr, scan.tabPrefix(), workerSpaces, leadTerminals, - TimeUnit.SECONDS.toNanos(scan.intervalSeconds()), System::nanoTime); - log.info("lead scan: tabs labelled '{}…' host a lead (rescan every {}s, worker spaces {} " + leads = new LeadTabScanner(herdr, scan.tabPrefix(), memberSpaces, leadTerminals, + TimeUnit.SECONDS.toNanos(scan.scanIntervalSeconds()), System::nanoTime); + log.info("lead scan: tabs labelled '{}…' host a lead (rescan every {}s, member spaces {} " + "excluded)", - scan.tabPrefix(), scan.intervalSeconds(), workerSpaces); + scan.tabPrefix(), scan.scanIntervalSeconds(), memberSpaces); + long distinctPrefixes = leaders.values().stream() + .map(BridgedConfig.Leader::tabPrefix).distinct().count(); + if (distinctPrefixes > 1) { + log.warn("fleet.leaders declares {} different tabPrefix values; only '{}' is scanned " + + "for. Give every lead the same tabPrefix, or leads under the others " + + "will not be discovered.", + distinctPrefixes, scan.tabPrefix()); + } } else { leads = () -> leadTerminals; } @@ -209,10 +224,9 @@ public final class Bridged { // 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.members() == null ? Map.of() : cfg.members()); + MemberRegistry members = new MemberRegistry(cfg.fleet()); if (!members.slots().isEmpty()) { - log.info("architect slots: {} configured {} — none bound yet (a slot is idle until the " + log.info("member slots: {} configured {} — none bound yet (a slot is idle until the " + "spawn lifecycle binds a live terminal to it)", members.slots().size(), members.slots().keySet()); } diff --git a/bridged/src/main/java/dev/ltms/bridged/auth/MemberRegistry.java b/bridged/src/main/java/dev/ltms/bridged/auth/MemberRegistry.java index 2338776..2fcdf56 100644 --- a/bridged/src/main/java/dev/ltms/bridged/auth/MemberRegistry.java +++ b/bridged/src/main/java/dev/ltms/bridged/auth/MemberRegistry.java @@ -1,8 +1,11 @@ package dev.ltms.bridged.auth; import dev.ltms.bridged.config.BridgedConfig; +import dev.ltms.bridged.peer.MemberRole; +import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.Map; /** @@ -28,19 +31,60 @@ import java.util.Map; */ public final class MemberRegistry { - private final Map slots; - /** Live {@code terminal_id → slot name}; guarded by {@code this}. */ - private final Map terminalToSlot = new HashMap<>(); - - public MemberRegistry(Map slots) { - this.slots = slots == null ? Map.of() : Map.copyOf(slots); + /** + * One flattened {@code fleet:} entry. + * + *

Flattened because a slot name is unique only within its pool — {@code sonnet} may + * legitimately be both a developer and a reviewer — while a terminal binds to exactly one thing. + * The qualified {@link #key()} is what that binding uses. + * + * @param name the slot's key inside its pool + * @param role the pool it came from + * @param profile the backend it runs on + */ + public record Entry(String name, MemberRole role, String profile) { + /** {@code "architect:opus"} — unique across pools, unlike {@link #name()}. */ + public String key() { + return role.wireName() + ":" + name; + } } - /** The configured slots, keyed by gateway-local unique name. Unmodifiable snapshot. */ - public Map slots() { + private final Map slots; + /** Live {@code terminal_id → qualified slot key}; guarded by {@code this}. */ + private final Map terminalToSlot = new HashMap<>(); + + /** Flatten every role pool in {@code fleet} into one registry. Leaders are not members. */ + public MemberRegistry(BridgedConfig.Fleet fleet) { + Map flat = new LinkedHashMap<>(); + if (fleet != null) { + for (MemberRole role : MemberRole.values()) { + fleet.pool(role).forEach((name, slot) -> { + if (slot != null) { + Entry e = new Entry(name, role, slot.profile()); + flat.put(e.key(), e); + } + }); + } + } + this.slots = Collections.unmodifiableMap(flat); + } + + /** The configured slots, keyed by qualified {@link Entry#key()}. Unmodifiable snapshot. */ + public Map slots() { return slots; } + /** The slots belonging to {@code role}, in definition order. */ + public Map slotsFor(MemberRole role) { + Map out = new LinkedHashMap<>(); + slots.forEach((key, e) -> { + if (e.role() == role) { + out.put(key, e); + } + }); + return Collections.unmodifiableMap(out); + } + /** * An immutable copy of the live {@code terminal_id → slot name} bindings. * @@ -71,8 +115,14 @@ public final class MemberRegistry { * declares none */ public String profileForSlot(String slotName) { - BridgedConfig.Member a = slots.get(slotName); - return (a == null || a.profile() == null) ? null : a.profile(); + 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. */ + public MemberRole roleForSlot(String slotName) { + Entry e = slots.get(slotName); + return e == null ? null : e.role(); } /** True when {@code slotName} is a configured architect slot. */ diff --git a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java index 568f0bc..2e6015c 100644 --- a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java +++ b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java @@ -13,6 +13,8 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashMap; @@ -33,9 +35,6 @@ import java.util.Set; * cost. It says nothing about what the member spawned on it is for; that is * the member's {@code role}. Each value's {@code profile} field is defaulted * to its key at construction, so this map is always normalized - * @param defaultProfile which {@code profiles} key a no-argument spawn uses ({@code null} → the - * sole/first profile). See {@link #effectiveDefaultProfile()} for the - * resolved value * @param guard subscription-boundary allowlist * @param worktreeRoot nullable root directory for provisioned worktrees; defaults to a sibling * of the repo root @@ -48,18 +47,11 @@ import java.util.Set; * @param primary optional pinned primary terminal config ({@code null} → derived from connection); * a non-blank {@code terminal} seeds {@code PrimaryRegistry} and prevents * connection-derived overrides, CB-307 - * @param leaders named panes that orchestrate rather than are orchestrated (CB-530), keyed by - * lead name; supersedes the singular {@code primary} pin, which stays honoured. - * See {@link #leaderTerminals()} for how the two merge - * @param members named member slots, keyed by gateway-local unique slot name; each pairs a - * {@code role} with the {@code profile} it runs on. Only members that need a - * stable identity are declared here — architects, today. A {@code dev} or - * {@code reviewer} is anonymous and short-lived, so the lead spawns it per - * task and it never appears in config. A slot is not recognised like - * a lead: config declares it only, and a live session becomes that member when - * the spawn lifecycle binds its terminal to the slot. Nothing here spawns one. - * @param leadScan opt-in discovery of leads by tab label (CB-531); {@code null} ⇒ no scanning, - * and only {@code leaders:}/{@code primary:} name a lead + * @param fleet who the daemon may run and under which role (CB-557). One block replacing + * the former {@code leaders:}, {@code members:}, {@code leadScan:} and + * {@code defaultProfile:}. Role is the containing key — {@code leaders}, + * {@code architects}, {@code developers}, {@code reviewers} — and each entry + * names the {@code profiles:} backend it runs on. See {@link Fleet} * @param leadHeartbeat opt-in idle-lead heartbeat (CB-551); {@code null} ⇒ off, and an upgraded * daemon never nudges an idle lead on its own initiative * @param placement how to choose a profile for an unqualified spawn: @@ -72,7 +64,6 @@ public record BridgedConfig( Bind bind, String herdrSocket, Map profiles, - String defaultProfile, Guard guard, String worktreeRoot, Lifecycle lifecycle, @@ -80,9 +71,7 @@ public record BridgedConfig( Integer spawnReadyPollMs, Broker broker, Primary primary, - Map leaders, - Map members, - LeadScan leadScan, + Fleet fleet, LeadHeartbeat leadHeartbeat, String placement, Auth auth) { @@ -203,7 +192,10 @@ public record BridgedConfig( tokenEnv = (tokenEnv == null || tokenEnv.isBlank()) ? "BRIDGED_WORKER_TOKEN" : tokenEnv; placement = (placement == null || placement.isBlank()) ? "tab" : placement.toLowerCase(); workspace = (workspace == null || workspace.isBlank()) ? "bridged-workers" : workspace; - tabLabel = (tabLabel == null || tabLabel.isBlank()) ? "worker: {profile} #{n}" : tabLabel; + // CB-557: no per-profile default any more. A label is generated from the member's ROLE + // ("dev: sonnet #2"), which a profile cannot know, so the template lives on `fleet:` and + // this field is only an override for a profile that wants its own. Blank ⇒ defer. + tabLabel = (tabLabel == null || tabLabel.isBlank()) ? null : tabLabel; // CB-525: .mcp.json is deliberately NOT here. Replicating the primary's MCP config gave a // worker the primary's IDE servers, which are bound to the primary's checkout — so its // navigation returned paths outside its own worktree. GitWorktrees now neutralizes that @@ -327,11 +319,24 @@ public record BridgedConfig( } /** - * Render {@link #tabLabel} for the {@code n}-th worker (substitutes - * {@code {profile}}/{@code {model}}/{@code {n}}), so sibling worker tabs are distinct. + * Render this member's tab label (CB-557): {@code {role}}, {@code {profile}}, + * {@code {model}} and {@code {n}} are substituted. + * + *

Template precedence is this profile's own {@link #tabLabel} override, then + * {@code fleetTemplate}, then {@link Fleet#DEFAULT_TAB_LABEL}. Role leads the default + * template so the tab bar reads as the fleet, and {@code n} counts per role+profile — a + * global counter left gaps in the numbering, which reads as though a sibling had died. + * + * @param fleetTemplate the {@code fleet.tabLabel} template; {@code null}/blank ⇒ the default + * @param role the member's role; {@code null} renders {@code {role}} as empty + * @param n this member's number within its role+profile pair */ - public String renderTabLabel(long n) { - return tabLabel + public String renderTabLabel(String fleetTemplate, MemberRole role, long n) { + String template = (tabLabel != null && !tabLabel.isBlank()) ? tabLabel + : (fleetTemplate != null && !fleetTemplate.isBlank()) ? fleetTemplate + : Fleet.DEFAULT_TAB_LABEL; + return template + .replace("{role}", role == null ? "" : role.wireName()) .replace("{profile}", profile == null ? "" : profile) .replace("{model}", model == null ? "" : model) .replace("{n}", Long.toString(n)); @@ -409,74 +414,155 @@ public record BridgedConfig( * lead drives a fleet, and wrong the moment two leads (say an Opus lead and an opencode lead) * work as peers — the second is silently demoted and refused every orchestration call. * - *

{@code kind} and {@code model} are descriptive only at this stage: they document what runs - * in the pane and are reported back by {@code bridge_whoami}. Nothing spawns a lead — a lead - * pre-exists, which is precisely why it must be recognised by configuration rather than created. + *

{@code kind} and {@code model} are descriptive only: they document what runs in the pane + * and are reported back by {@code bridge_whoami}. * - * @param terminal the lead's herdr {@code terminal_id}; the only field identity depends on - * @param kind which agent runs there ({@code claude}, {@code opencode}, …); descriptive - * @param model the model or selector it runs, for operators reading the roster; descriptive + *

A lead is now also creatable (CB-557). Before, nothing spawned one — a lead + * pre-existed, which is why it had to be recognised by configuration rather than created. With + * {@code profile} and {@code instances} the daemon may stand one up when none is live, so the + * pane no longer has to exist before the daemon does. Recognition still comes first: a lead + * already running under {@code tabPrefix} is adopted, and only the shortfall is launched. + * + * @param profile the {@code profiles:} entry to launch this lead on when one must + * be created; {@code null} ⇒ recognise-only, never create + * @param terminal the lead's herdr {@code terminal_id} when pinned by hand; the only + * field identity depends on. {@code null} ⇒ found by {@code tabPrefix} + * @param instances how many of this lead should be live (default 1). The daemon + * launches only the shortfall, so a restart adopts rather than doubles + * @param tabPrefix label prefix marking this lead's tab, matched case-insensitively; + * the remainder is the lead's name ({@code "lead: opus"} → + * {@code opus}). Default {@code "lead:"} + * @param scanIntervalSeconds how long a tab scan is cached before herdr is asked again; also the + * worst case before a newly-labelled tab is recognised. Default 10 + * @param kind which agent runs there ({@code claude}, {@code opencode}, …) + * @param model the model or selector it runs, for operators reading the roster */ @JsonIgnoreProperties(ignoreUnknown = true) - public record Leader(String terminal, String kind, String model) { + public record Leader(String profile, String terminal, Integer instances, String tabPrefix, + Integer scanIntervalSeconds, String kind, String model) { + public Leader { + instances = (instances == null || instances < 0) ? 1 : instances; + tabPrefix = (tabPrefix == null || tabPrefix.isBlank()) ? "lead:" : tabPrefix.strip(); + scanIntervalSeconds = + (scanIntervalSeconds == null || scanIntervalSeconds <= 0) ? 10 : scanIntervalSeconds; + } + + /** True when this lead may be launched by the daemon rather than only recognised. */ + public boolean isCreatable() { + return profile != null && !profile.isBlank() && instances > 0; + } } /** - * One entry of the {@code members:} registry — a gateway-local named slot that pairs a role - * with the profile it runs on. + * One entry of a {@code fleet:} role pool — a role paired with the backend it runs on. * - *

Role and profile are separate axes. The {@code role} answers which contract - * — it picks the launch charter, the role file, the playbook skill and the authz row. The + *

Role and profile are separate axes. The role (the pool this slot sits in) answers + * which contract — launch charter, role file, playbook skill, authz row. The * {@code profile} answers which backend — model, CLI adapter, credentials, cost. They * vary independently: a {@code reviewer} may run on the very same profile as the {@code dev} * whose diff it reviews, and that case is what proves the two are not one axis. * - *

Only members that need a stable identity are declared here. Architects are, because - * a lead addresses the same pair of them across many tickets. A {@code dev} or {@code reviewer} - * is anonymous and fungible — spawned per task, torn down after — so it never appears in config. + *

An entry declares that the role may run there; it is not an identity. The key + * names the entry for operators and for error messages, nothing more. Which of a pool's + * profiles an unqualified spawn actually lands on is the placement policy's choice, and + * definition order is the {@code fixed} policy's answer. * - *

A slot is declared, not recognised: config names the slot, its role and its - * profile, and nothing else. Unlike a lead (which config pins by herdr {@code terminal_id} and - * is recognised at startup), a member slot is idle at boot — config supplies no terminal, so no - * session resolves to one until the spawn lifecycle binds a live terminal to the slot. - * - * @param role the contract this slot runs under — {@code architect}, {@code dev} or - * {@code reviewer}; required, and validated against {@link MemberRole} * @param profile the name of the {@code profiles:} entry this slot runs on; required and - * validated against {@link #profiles()} (a stale or typo'd reference fails at - * startup rather than silently spawning the wrong backend later) + * validated against {@link #profiles()}, so a stale or typo'd reference fails at + * startup rather than silently spawning the wrong backend later */ @JsonIgnoreProperties(ignoreUnknown = true) - public record Member(String role, String profile) { + public record Slot(String profile) { } /** - * Discover leads by tab label instead of by pasted {@code terminal_id} (CB-531). + * Who the daemon may run, and under which role (CB-557). * - *

Why: a lead is not spawned, so its {@code terminal_id} exists only once a human has opened - * the tab and started the agent — which makes {@code leaders:} a three-step ritual (start it, - * ask it its id, edit config, restart) repeated per lead. Naming the tab is one step, done at - * the moment the operator is already there. The convention also survives what an id does not: - * close the tab and reopen it and the id changes, while the label is retyped as-is. + *

This one block replaced four top-level keys — {@code leaders:}, {@code members:}, + * {@code leadScan:} and {@code defaultProfile:}. The change is not only a move. Role used to be + * a field on a slot ({@code role: architect}); it is now the containing key, so the + * config states the role × profile matrix directly and a role can no longer be misspelled into + * something that parses. * - *

Deliberately opt-in ({@code null} ⇒ off). Turning it on widens who resolves as - * {@link dev.ltms.bridged.auth.Role#PRIMARY}, and a config that never asked for it must not - * acquire that by upgrading the daemon. + *

It also reverses an earlier rule. Devs and reviewers were kept out of config because they + * are anonymous and spawned per task. They belong here now because these are pools, + * not identities: a pool says which backends a role is allowed to run on, and staying anonymous + * is exactly compatible with that. The pool is also what replaced {@code defaultProfile:} — an + * unqualified spawn names a role, and the role's pool supplies the candidates. * - *

bridged never writes these labels — see {@link dev.ltms.bridged.herdr.LeadTabScanner} for - * why that one-way direction is what keeps the convention trustworthy. - * - * @param tabPrefix label prefix marking a lead's tab, matched case-insensitively; the - * remainder is the lead's name ({@code "lead: opus-5.0"} → {@code - * opus-5.0}). Default {@code "lead:"} - * @param intervalSeconds how long a scan is cached before herdr is asked again; also the worst - * case before a newly-labelled tab is recognised. Default 10 + * @param leaders panes that orchestrate rather than are orchestrated, keyed by lead name + * @param architects profiles the {@code architect} role may run on + * @param developers profiles the {@code dev} role may run on + * @param reviewers profiles the {@code reviewer} role may run on + * @param tabLabel template for a member tab's label; {@code {role}}, {@code {profile}}, + * {@code {model}} and {@code {n}} (a per role+profile counter) are + * substituted. Default {@link #DEFAULT_TAB_LABEL} */ @JsonIgnoreProperties(ignoreUnknown = true) - public record LeadScan(String tabPrefix, Integer intervalSeconds) { - public LeadScan { - tabPrefix = (tabPrefix == null || tabPrefix.isBlank()) ? "lead:" : tabPrefix.strip(); - intervalSeconds = (intervalSeconds == null || intervalSeconds <= 0) ? 10 : intervalSeconds; + public record Fleet(Map leaders, + Map architects, + Map developers, + Map reviewers, + String tabLabel) { + + /** + * Role first, so the tab bar reads as the fleet and so the label shares a namespace with a + * lead's {@code tabPrefix}. Because {@code {role}} comes from a closed enum, a generated + * member label can never begin with {@code "lead:"} — the clash that + * {@link #validateLeadTabPrefixes()} used to have to check for is unrepresentable here. + */ + public static final String DEFAULT_TAB_LABEL = "{role}: {profile} #{n}"; + + public Fleet { + leaders = unmodifiableOrEmpty(leaders); + architects = unmodifiableOrEmpty(architects); + developers = unmodifiableOrEmpty(developers); + reviewers = unmodifiableOrEmpty(reviewers); + tabLabel = (tabLabel == null || tabLabel.isBlank()) ? DEFAULT_TAB_LABEL : tabLabel; + } + + /** + * Deliberately not {@code Map.copyOf}: its iteration order is salted per JVM run, which + * would discard YAML definition order. The {@code fixed} placement policy answers with a + * pool's first entry and {@code weighted} tie-breaks on candidate order, so losing that + * order makes placement unpredictable between restarts. + */ + private static Map unmodifiableOrEmpty(Map m) { + return (m == null || m.isEmpty()) + ? Map.of() : Collections.unmodifiableMap(new LinkedHashMap<>(m)); + } + + /** The pool for {@code role}, in definition order; empty when the role has none. */ + public Map pool(MemberRole role) { + if (role == null) { + return Map.of(); + } + return switch (role) { + case ARCHITECT -> architects; + case DEV -> developers; + case REVIEWER -> reviewers; + }; + } + + /** + * The profile names {@code role} may run on, in definition order, without repeats. + * + *

These are the candidates an unqualified spawn chooses between — the per-role successor + * to the old global {@code defaultProfile}. + */ + public List profilesFor(MemberRole role) { + return pool(role).values().stream() + .filter(s -> s != null && s.profile() != null && !s.profile().isBlank()) + .map(Slot::profile) + .distinct() + .toList(); + } + + /** Every role that has at least one profile configured, in enum order. */ + public List rolesConfigured() { + return Arrays.stream(MemberRole.values()) + .filter(r -> !profilesFor(r).isEmpty()) + .toList(); } } @@ -528,8 +614,8 @@ public record BridgedConfig( */ public Map leaderTerminals() { Map byTerminal = new LinkedHashMap<>(); - if (leaders != null) { - leaders.forEach((name, leader) -> { + if (fleet != null) { + fleet.leaders().forEach((name, leader) -> { if (leader != null && leader.terminal() != null && !leader.terminal().isBlank()) { byTerminal.put(leader.terminal(), name); } @@ -596,17 +682,38 @@ public record BridgedConfig( } /** - * The profile a no-argument spawn uses: {@code defaultProfile} if set, else the sole/first - * configured profile, else {@code null}. + * The candidate profiles an unqualified spawn of {@code role} chooses between, in definition + * order (CB-557). * - *

Named {@code effective…} because the record component {@code defaultProfile()} returns the - * raw config value, which may be {@code null}. This is the resolved one. + *

This replaced the global {@code defaultProfile:}. A single default could not survive roles + * being first-class: "the profile a no-argument spawn uses" has no one answer once a reviewer + * and a dev may legitimately want different backends. Asking per role gives each one its own + * pool, and definition order is the {@code fixed} policy's answer within it. + * + *

Falls back to every configured profile when the role has no pool, so a config that + * declares {@code profiles:} but no {@code fleet:} still spawns rather than failing — the + * pre-CB-557 behaviour for a config that named no slots. + */ + public List candidateProfiles(MemberRole role) { + List pool = (fleet == null) ? List.of() : fleet.profilesFor(role); + return pool.isEmpty() ? List.copyOf(profiles.keySet()) : pool; + } + + /** + * The profile an unqualified spawn of {@code role} lands on under the {@code fixed} policy: the + * first of {@link #candidateProfiles(MemberRole)}, or {@code null} when nothing is configured. + */ + public String defaultProfileFor(MemberRole role) { + List candidates = candidateProfiles(role); + return candidates.isEmpty() ? null : candidates.getFirst(); + } + + /** + * The last-resort profile for a spawn that names no role at all — the {@code dev} pool's first + * entry, since an unqualified spawn is a unit of work rather than a review or a design. */ public String effectiveDefaultProfile() { - if (defaultProfile != null && !defaultProfile.isBlank()) { - return defaultProfile; - } - return profiles.isEmpty() ? null : profiles.keySet().iterator().next(); + return defaultProfileFor(MemberRole.DEV); } private static final Logger log = LoggerFactory.getLogger(BridgedConfig.class); @@ -618,9 +725,9 @@ public record BridgedConfig( * {@link #warnUnknownTopLevelKeys}. Keep in step with the record components. */ private static final Set KNOWN_TOP_LEVEL_KEYS = Set.of( - "bind", "herdrSocket", "profiles", "defaultProfile", "guard", "worktreeRoot", - "lifecycle", "spawnReadyTimeoutMs", "spawnReadyPollMs", "broker", "primary", "leaders", - "members", "leadScan", "leadHeartbeat", "placement", "auth"); + "bind", "herdrSocket", "profiles", "guard", "worktreeRoot", + "lifecycle", "spawnReadyTimeoutMs", "spawnReadyPollMs", "broker", "primary", "fleet", + "leadHeartbeat", "placement", "auth"); /** Load and validate config from {@code path}. */ public static BridgedConfig load(Path path) { @@ -636,38 +743,46 @@ public record BridgedConfig( } } + /** The {@code fleet:} child blocks whose direct children are slot names. */ + private static final Set FLEET_POOL_KEYS = + Set.of("leaders", "architects", "developers", "reviewers"); + /** - * Reject an {@code members:} registry whose slot names repeat (CB-548). + * Reject a {@code fleet:} role pool whose slot names repeat (CB-548, re-homed by CB-557). * - *

The registry is a {@code Map} keyed by slot name, so by the time it is read duplicate keys + *

Each pool is a {@code Map} keyed by slot name, so by the time it is read duplicate keys * have already collapsed last-wins — a duplicated slot name would silently drop one slot and the * daemon would never know. Jackson's YAML parser does not fail on duplicate mapping keys by - * default, so duplicates are caught here, at parse time, before the map is built. Only the - * top-level {@code members:} block is considered, and only its direct child keys (the - * slot names) — a nested field elsewhere, even one also named {@code members:}, is ignored, so - * parsing of the rest of the config is unaffected. + * default, so duplicates are caught here, at parse time, before the map is built. * - * @throws IllegalStateException when two {@code members:} entries share a slot name, naming it + *

Only the four pools directly under the top-level {@code fleet:} are considered, + * and only their direct child keys (the slot names). A nested field elsewhere, even one also + * named {@code developers:}, is ignored, so parsing of the rest of the config is unaffected. + * + *

Names repeat freely across pools and that is deliberate: {@code sonnet} appearing + * in both {@code developers:} and {@code reviewers:} is the role × profile matrix doing its job, + * not a mistake. Only a repeat within one pool is an error. + * + * @throws IllegalStateException when one pool has two entries sharing a slot name, naming both */ static void rejectDuplicateMemberSlots(String yaml) { try (JsonParser p = YAML.createParser(yaml)) { if (p.nextToken() != JsonToken.START_OBJECT) { return; // not a mapping at top level — readValue reports the malformed file } - // Scan the TOP-LEVEL mapping only. Every other field's value (however deep, including - // any nested field also literally named "members") is consumed whole by skipValue, so - // the loop below can only ever see the top-level field names — a nested `members:` can - // neither suppress the real block nor be misread as one. + // Scan the TOP-LEVEL mapping only. Every other field's value (however deep, including a + // nested field also literally named "fleet") is consumed whole by skipValue, so the loop + // below can only ever see top-level field names. JsonToken t; while ((t = p.nextToken()) != null && t != JsonToken.END_OBJECT) { if (t == JsonToken.FIELD_NAME) { String name = p.getCurrentName(); JsonToken value = p.nextToken(); - if ("members".equals(name)) { + if ("fleet".equals(name)) { if (value == JsonToken.START_OBJECT) { - rejectDuplicateChildSlotKeys(p); + rejectDuplicateSlotsInPools(p); } - return; // the single top-level members block is handled; nothing more to check + return; // the single top-level fleet block is handled; nothing more to check } skipValue(p, value); } @@ -678,24 +793,44 @@ public record BridgedConfig( } /** - * Reject a duplicated direct child key of the (already-positioned) {@code members:} - * mapping — i.e. a duplicated {@code slot name}. - * - *

Each slot's value is consumed whole by {@link #skipValue}, so a duplicated field inside - * a slot (e.g. two {@code profile:} keys, or a duplicate nested {@code members:}) is never seen - * here and cannot masquerade as a duplicated slot name. - * - * @throws IllegalStateException when two {@code members:} entries share a slot name, naming it + * Walk the (already-positioned) {@code fleet:} mapping and check each role pool it contains. + * Any other {@code fleet:} child — {@code tabLabel}, say — is consumed whole and ignored. */ - private static void rejectDuplicateChildSlotKeys(JsonParser p) throws IOException { + private static void rejectDuplicateSlotsInPools(JsonParser p) throws IOException { + JsonToken t; + while ((t = p.nextToken()) != null && t != JsonToken.END_OBJECT) { + if (t == JsonToken.FIELD_NAME) { + String pool = p.getCurrentName(); + JsonToken value = p.nextToken(); + if (FLEET_POOL_KEYS.contains(pool) && value == JsonToken.START_OBJECT) { + rejectDuplicateChildSlotKeys(p, pool); + } else { + skipValue(p, value); + } + } + } + } + + /** + * Reject a duplicated direct child key of an already-positioned role pool — i.e. a + * duplicated slot name within that one pool. + * + *

Each slot's value is consumed whole by {@link #skipValue}, so a duplicated field + * inside a slot (two {@code profile:} keys, say) is never seen here and cannot + * masquerade as a duplicated slot name. + * + * @param pool the pool's key, named in the error so the operator knows which one to look at + * @throws IllegalStateException when two entries in {@code pool} share a slot name + */ + private static void rejectDuplicateChildSlotKeys(JsonParser p, String pool) throws IOException { Set seen = new HashSet<>(); JsonToken t; while ((t = p.nextToken()) != null && t != JsonToken.END_OBJECT) { if (t == JsonToken.FIELD_NAME) { if (!seen.add(p.getCurrentName())) { - throw new IllegalStateException("refusing to start: duplicate member slot name '" - + p.getCurrentName() + "' — slot names must be unique; a later entry would " - + "silently overwrite the earlier one"); + throw new IllegalStateException("refusing to start: duplicate slot name '" + + p.getCurrentName() + "' in fleet." + pool + " — names must be unique " + + "within a pool; a later entry would silently overwrite the earlier one"); } skipValue(p, p.nextToken()); // the slot's entire value } @@ -774,12 +909,24 @@ public record BridgedConfig( *

We are in active development, so the old spellings are not accepted as aliases. Accepting * both would leave two names for one thing in every config and doc, which is the cost the * rename was meant to remove. + * + *

Values are the advice shown to the operator, not bare key names: several of these did not + * move to one key. {@code defaultProfile:} has no successor at all — it became per-role — and a + * message naming a single replacement key would send the reader somewhere that does not exist. */ private static final Map RENAMED_TOP_LEVEL_KEYS = Map.of( - "workers", "profiles", - "worker", "profiles", - "defaultWorker", "defaultProfile", - "architects", "members"); + "workers", "'profiles'", + "worker", "'profiles'", + "defaultWorker", "a role pool under 'fleet:' — an unqualified spawn now names a role," + + " and that role's pool supplies the candidate profiles", + "defaultProfile", "a role pool under 'fleet:' — an unqualified spawn now names a role," + + " and that role's pool supplies the candidate profiles", + "architects", "'fleet.architects'", + "members", "a role pool under 'fleet:' — 'fleet.architects', 'fleet.developers' or" + + " 'fleet.reviewers'; the role is the containing key, not a 'role:' field", + "leaders", "'fleet.leaders'", + "leadScan", "'fleet.leaders..tabPrefix' and '.scanIntervalSeconds' — lead" + + " discovery is now configured on the lead it discovers"); /** * Reject a config that still uses a pre-rename top-level key, naming its replacement. @@ -801,12 +948,13 @@ public record BridgedConfig( .map(String::valueOf) .filter(RENAMED_TOP_LEVEL_KEYS::containsKey) .sorted() - .map(k -> "'" + k + "' is now '" + RENAMED_TOP_LEVEL_KEYS.get(k) + "'") + .map(k -> "'" + k + "' is now " + RENAMED_TOP_LEVEL_KEYS.get(k)) .toList(); if (!bad.isEmpty()) { throw new IllegalStateException("refusing to start: this config uses renamed top-level " + "keys — " + String.join("; ", bad) - + ". A profile says which backend to run; a member says which role runs on it."); + + ". A profile says which backend to run; a fleet role pool says which role may" + + " run on it."); } } @@ -838,14 +986,15 @@ public record BridgedConfig( String placementOrDefault = (placement != null && !placement.isBlank()) ? placement : "fixed"; // broker is left as-is: null (or an empty/blank uri) keeps the in-memory soft-state inbox. // primary is left as-is: null defaults to connection-derived identity. - // leadScan is left as-is: null is "off", and LeadScan's own compact constructor defaults the - // fields of a block that IS present. Defaulting it here would switch the feature on for - // every config that never mentioned it. - // leadHeartbeat is left as-is for the same reason (CB-551): null is "off", and LeadHeartbeat's - // own compact constructor defaults the fields of a block that IS present. - // members is left as-is: null is "none configured", and Member's fields have no - // defaults to fill. Defaulting it here would change nothing, so leave the call natural. - return new BridgedConfig(b, herdrSocket, profiles, defaultProfile, g, worktreeRoot, l, timeout, pollMs, broker, primary, leaders, members, leadScan, leadHeartbeat, placementOrDefault, a); + // fleet IS defaulted, unlike the leadScan: block it replaced, because an empty Fleet is not + // the same as an enabled one: every pool is empty, so no lead is scanned for or created and + // no role has a pool. Constructing it saves every reader a null check for no behaviour change. + Fleet f = (fleet != null) ? fleet : new Fleet(null, null, null, null, null); + // leadHeartbeat is left as-is (CB-551): null is "off", and LeadHeartbeat's own compact + // constructor defaults the fields of a block that IS present. Defaulting it here would + // switch the feature on for every config that never mentioned it. + return new BridgedConfig(b, herdrSocket, profiles, g, worktreeRoot, l, timeout, pollMs, + broker, primary, f, leadHeartbeat, placementOrDefault, a); } /** @@ -876,40 +1025,65 @@ public record BridgedConfig( * Reject a lead-scan convention that a worker tab would also satisfy (CB-531). * *

The scan reads a tab label and concludes "a lead lives here". bridged also writes - * tab labels — every worker gets {@code tabLabel} rendered into its tab. Choose a - * {@code leadScan.tabPrefix} that a worker template matches and the daemon starts labelling its - * own workers as leads, promoting the entire fleet to {@link dev.ltms.bridged.auth.Role#PRIMARY} - * with no message and no diff. The worker-space exclusion in - * {@link dev.ltms.bridged.herdr.LeadTabScanner} already blocks the realistic path, but defence - * that depends on one workspace label holding is not defence enough for a privilege boundary. + * tab labels — every member gets one rendered into its tab. Choose a lead {@code tabPrefix} that + * a member template matches and the daemon starts labelling its own members as leads, promoting + * the entire fleet to {@link dev.ltms.bridged.auth.Role#PRIMARY} with no message and no diff. + * The member-space exclusion in {@link dev.ltms.bridged.herdr.LeadTabScanner} already blocks the + * realistic path, but defence that depends on one workspace label holding is not defence enough + * for a privilege boundary. + * + *

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

Fatal rather than a warning, unlike {@link #warnUnknownTopLevelKeys}: an unknown key means * a feature does nothing, while this means a feature does the opposite of what it says. * - * @throws IllegalStateException when any worker profile's {@code tabLabel} starts with the - * configured lead prefix + * @throws IllegalStateException when the fleet template or any profile's {@code tabLabel} + * override starts with a configured lead prefix */ - public void validateLeadScan() { - if (leadScan == null) { + public void validateLeadTabPrefixes() { + if (fleet == null || fleet.leaders().isEmpty()) { return; } - String prefix = leadScan.tabPrefix(); - List clashing = profiles().entrySet().stream() - .filter(e -> e.getValue().tabLabel() != null - && e.getValue().tabLabel().strip() - .regionMatches(true, 0, prefix, 0, prefix.length())) - .map(Map.Entry::getKey) - .sorted() - .toList(); - if (clashing.isEmpty()) { + List bad = new ArrayList<>(); + fleet.leaders().forEach((leadName, leader) -> { + if (leader == null) { + return; + } + String prefix = leader.tabPrefix(); + // The fleet-wide template is checked once per prefix: it labels every member that has no + // override, so one bad template promotes the entire fleet, not one profile. + if (startsWithIgnoreCase(fleet.tabLabel(), prefix)) { + bad.add("fleet.tabLabel=\"" + fleet.tabLabel() + "\" starts with the tabPrefix of " + + "lead '" + leadName + "' (\"" + prefix + "\")"); + } + profiles().entrySet().stream() + .filter(e -> startsWithIgnoreCase(e.getValue().tabLabel(), prefix)) + .map(Map.Entry::getKey) + .sorted() + .forEach(p -> bad.add("profile '" + p + "' overrides tabLabel with \"" + + profiles().get(p).tabLabel() + "\", which starts with the tabPrefix of " + + "lead '" + leadName + "' (\"" + prefix + "\")")); + }); + if (bad.isEmpty()) { return; } - throw new IllegalStateException( - "refusing to start: leadScan.tabPrefix=\"" + prefix + "\" also matches the tabLabel " - + "of worker profile(s) " + clashing + ". Every worker spawned under them " - + "would be read back as a lead and granted spawn/stop/send on the whole " - + "fleet. Change one of the two so worker tabs and lead tabs cannot be " - + "confused."); + throw new IllegalStateException("refusing to start: " + String.join("; ", bad) + + ". Every member labelled that way would be read back as a lead and granted " + + "spawn/stop/send on the whole fleet. Change one of the two so member tabs and " + + "lead tabs cannot be confused."); + } + + /** Case-insensitive prefix test that tolerates a null/blank label. */ + private static boolean startsWithIgnoreCase(String label, String prefix) { + if (label == null || prefix == null || prefix.isBlank()) { + return false; + } + String stripped = label.strip(); + return stripped.regionMatches(true, 0, prefix, 0, prefix.length()); } /** @@ -959,39 +1133,52 @@ public record BridgedConfig( * when something later tries to use it. Validating at startup names the mistake then, rather * than leaving it to be discovered months later by a spawn that quietly has no backend. * - *

The {@code role} is checked the same way and for the same reason: a typo'd role would - * otherwise pick no charter at all, and the member would run with no contract. + *

The role itself needs no check any more (CB-557): it is the containing key, so an + * unrecognised pool name is simply not a pool and cannot become a member with no contract. That + * is the main thing the pool shape bought over the old {@code role:} field. * - *

Slot-name uniqueness needs no check here: the registry is a {@code Map} keyed by name, so - * duplicates are unrepresentable by construction once loaded — and {@link #load(Path)} already - * rejects a duplicated slot name at parse time, before the map collapses. + *

Slot-name uniqueness needs no check here either: each pool is a {@code Map} keyed by name, + * so duplicates are unrepresentable by construction once loaded — and {@link #load(Path)} + * already rejects a duplicated slot name at parse time, before the map collapses. * - * @throws IllegalStateException when any member slot is missing or names an unknown role or - * profile, naming the slot and the offending reference + * @throws IllegalStateException when a slot names no profile or an unknown one, or when a lead + * can be neither found nor created, naming the offending entry */ public void validateMembers() { - if (members == null) { + if (fleet == null) { return; } - List bad = new java.util.ArrayList<>(); - members.forEach((name, m) -> { - if (m == null || m.profile() == null || m.profile().isBlank()) { - bad.add("member slot '" + name + "' has no profile: — give it the name of a " - + "profiles: entry (the backend it runs on)."); - } else if (!profiles.containsKey(m.profile())) { - bad.add("member slot '" + name + "' references profile '" + m.profile() + List bad = new ArrayList<>(); + for (MemberRole role : MemberRole.values()) { + String pool = "fleet." + role.configKey(); + fleet.pool(role).forEach((name, slot) -> { + if (slot == null || slot.profile() == null || slot.profile().isBlank()) { + bad.add(pool + "." + name + " has no profile: — give it the name of a profiles: " + + "entry (the backend this role runs on)."); + } else if (!profiles.containsKey(slot.profile())) { + bad.add(pool + "." + name + " references profile '" + slot.profile() + + "', which is not a configured profiles: entry (have: " + + profiles.keySet() + ")."); + } + }); + } + fleet.leaders().forEach((name, leader) -> { + if (leader == null) { + return; + } + // A lead's profile is optional: without one the lead is recognised but never created, + // which is the pre-CB-557 behaviour and still a legitimate choice. A profile that IS + // named must resolve, or the shortfall launch fails at the worst possible moment. + if (leader.profile() != null && !leader.profile().isBlank() + && !profiles.containsKey(leader.profile())) { + bad.add("fleet.leaders." + name + " references profile '" + leader.profile() + "', which is not a configured profiles: entry (have: " + profiles.keySet() + ")."); } - if (m == null || m.role() == null || m.role().isBlank()) { - bad.add("member slot '" + name + "' has no role: — give it one of architect, dev, " - + "reviewer."); - } else { - try { - MemberRole.parse(m.role()); - } catch (IllegalArgumentException e) { - bad.add("member slot '" + name + "': " + e.getMessage() + "."); - } + if (!leader.isCreatable() && (leader.terminal() == null || leader.terminal().isBlank())) { + bad.add("fleet.leaders." + name + " can neither be found nor created — it pins no " + + "terminal: and names no profile: to launch one on. Give it one or the " + + "other, or drop the entry."); } }); if (!bad.isEmpty()) { diff --git a/bridged/src/main/java/dev/ltms/bridged/member/ClaudeCodeLauncher.java b/bridged/src/main/java/dev/ltms/bridged/member/ClaudeCodeLauncher.java index b104136..1d66286 100644 --- a/bridged/src/main/java/dev/ltms/bridged/member/ClaudeCodeLauncher.java +++ b/bridged/src/main/java/dev/ltms/bridged/member/ClaudeCodeLauncher.java @@ -79,9 +79,24 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { Map profiles, String defaultProfile, Function env, long spawnReadyTimeoutMs, long spawnReadyPollMs) { + this(agents, spaces, guard, profiles, defaultProfile, env, + spawnReadyTimeoutMs, spawnReadyPollMs, null); + } + + /** + * Production constructor carrying the fleet-wide tab-label template (CB-557). The template comes + * from {@code fleet.tabLabel}, which a profile cannot know because it names the member's + * role; a profile may still override it with its own {@code tabLabel}. + */ + public ClaudeCodeLauncher(AgentControl agents, WorkspaceControl spaces, SubscriptionGuard guard, + Map profiles, String defaultProfile, + Function env, + long spawnReadyTimeoutMs, long spawnReadyPollMs, + String tabLabelTemplate) { this(agents, spaces, guard, profiles, defaultProfile, env, spawnReadyTimeoutMs, - System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs)); + System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs), + tabLabelTemplate); } /** @@ -106,8 +121,24 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { Function env, long spawnReadyTimeoutMs, LongSupplier nowMillis, Runnable sleeper) { + this(agents, spaces, guard, profiles, defaultProfile, env, + spawnReadyTimeoutMs, nowMillis, sleeper, null); + } + + /** + * Full testability constructor, plus the fleet-wide tab-label template (CB-557). + * + * @param tabLabelTemplate {@code fleet.tabLabel}; {@code null}/blank ⇒ + * {@link BridgedConfig.Fleet#DEFAULT_TAB_LABEL} + */ + public ClaudeCodeLauncher(AgentControl agents, WorkspaceControl spaces, SubscriptionGuard guard, + Map profiles, String defaultProfile, + Function env, + long spawnReadyTimeoutMs, + LongSupplier nowMillis, Runnable sleeper, + String tabLabelTemplate) { super(NAME_PREFIX, agents, spaces, profiles, defaultProfile, env, - spawnReadyTimeoutMs, nowMillis, sleeper); + spawnReadyTimeoutMs, nowMillis, sleeper, tabLabelTemplate); this.guard = guard; } 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 612b804..f6dac69 100644 --- a/bridged/src/main/java/dev/ltms/bridged/member/CompositePeerLauncher.java +++ b/bridged/src/main/java/dev/ltms/bridged/member/CompositePeerLauncher.java @@ -174,9 +174,10 @@ public final class CompositePeerLauncher implements PeerLauncher { } // CB-547a: route the chosen profile but keep the caller's session identity — dropping it - // here would silently sever the resume handle on every policy-routed spawn. + // here would silently sever the resume handle on every policy-routed spawn. CB-557: the + // role rides along for the same reason, or a routed spawn would be labelled as a dev. SpawnRequest routedReq = new SpawnRequest(chosen.profile(), req.requestedCwd(), req.callerCwd(), - req.sessionName(), req.resumeSessionId()); + req.sessionName(), req.resumeSessionId(), req.role()); try { PeerHandle handle = d.spawn(routedReq); spawnedBy.put(handle.id(), d); diff --git a/bridged/src/main/java/dev/ltms/bridged/member/HerdrPeerLauncher.java b/bridged/src/main/java/dev/ltms/bridged/member/HerdrPeerLauncher.java index 0e79e35..2ce7d9b 100644 --- a/bridged/src/main/java/dev/ltms/bridged/member/HerdrPeerLauncher.java +++ b/bridged/src/main/java/dev/ltms/bridged/member/HerdrPeerLauncher.java @@ -7,6 +7,7 @@ import dev.ltms.bridged.herdr.HerdrException; import dev.ltms.bridged.herdr.Tab; import dev.ltms.bridged.herdr.Workspace; import dev.ltms.bridged.herdr.WorkspaceControl; +import dev.ltms.bridged.peer.MemberRole; import dev.ltms.bridged.peer.PeerHandle; import dev.ltms.bridged.peer.PeerLauncher; import dev.ltms.bridged.peer.PeerUnreachableException; @@ -75,7 +76,27 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { /** Host env lookup (injectable for tests); adapters read it in {@link #buildLaunch}. */ protected final Function env; - private final AtomicLong nameSeq = new AtomicLong(); // per-peer counter (also the tab #) + private final AtomicLong nameSeq = new AtomicLong(); // per-peer counter (herdr agent names only) + + /** + * The {@code fleet.tabLabel} template; {@code null} ⇒ {@link BridgedConfig.Fleet#DEFAULT_TAB_LABEL}. + * A profile's own {@code tabLabel} still overrides it. + */ + private final String tabLabelTemplate; + + /** + * Tab numbers, counted per {@code role/profile} pair (CB-557). + * + *

Deliberately not {@link #nameSeq}. That counter is shared by every profile this launcher + * serves, because its job is to make herdr agent names unique. Reusing it for the tab + * label made the numbers global, so sibling tabs read {@code #4}, {@code #9}, {@code #17} — gaps + * that look like a member died. Counting per role+profile makes {@code dev: sonnet #2} mean the + * second sonnet dev, which is what a reader assumes it means. + * + *

Resets when the daemon restarts, and that is fine: the label is a human-facing hint, not an + * identity. Identity is {@link PeerHandle#id()}. + */ + private final ConcurrentMap labelSeq = new ConcurrentHashMap<>(); private final long spawnReadyTimeoutMs; // 0 = disable gate (legacy non-blocking spawn) private final LongSupplier nowMillis; // monotonic clock (injectable for tests) @@ -110,6 +131,25 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { Function env, long spawnReadyTimeoutMs, LongSupplier nowMillis, Runnable sleeper) { + this(namePrefix, agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, + nowMillis, sleeper, null); + } + + /** + * As above, plus the {@code fleet.tabLabel} template (CB-557). + * + * @param tabLabelTemplate fleet-wide tab-label template; {@code null}/blank ⇒ + * {@link BridgedConfig.Fleet#DEFAULT_TAB_LABEL}. A separate constructor + * rather than a new parameter on the one above, so every existing call + * site keeps the default without an edit. + */ + protected HerdrPeerLauncher(String namePrefix, AgentControl agents, WorkspaceControl spaces, + Map profiles, String defaultProfile, + Function env, + long spawnReadyTimeoutMs, + LongSupplier nowMillis, Runnable sleeper, + String tabLabelTemplate) { + this.tabLabelTemplate = tabLabelTemplate; this.namePrefix = namePrefix; this.agents = agents; this.spaces = spaces; @@ -240,6 +280,13 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { return spawnInternal(profileName, requestedCwd, callerCwd, null, null).agent(); } + /** Pre-CB-557 shape: no explicit role, so the tab is labelled as a {@code dev}. */ + protected Spawned spawnInternal(String profileName, String requestedCwd, String callerCwd, + String sessionName, String resumeSessionId) { + return spawnInternal(profileName, requestedCwd, callerCwd, sessionName, resumeSessionId, + MemberRole.DEV); + } + /** * Spawn a peer with session identity (CB-547a). {@code sessionName} and {@code resumeSessionId} * are threaded from the {@link SpawnRequest} into {@link #buildLaunch(BridgedConfig.Profile, @@ -247,16 +294,27 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { * so the caller can put it on the {@link PeerHandle}. */ protected Spawned spawnInternal(String profileName, String requestedCwd, String callerCwd, - String sessionName, String resumeSessionId) { + String sessionName, String resumeSessionId, MemberRole role) { BridgedConfig.Profile cfg = requireProfile(profileName); Launch launch = buildLaunch(cfg, sessionName, resumeSessionId); String cwd = resolveCwd(requestedCwd, cfg, callerCwd); Agent agent = cfg.tabPlacement() - ? spawnInTab(cfg, launch.env(), launch.argv(), cwd) + ? spawnInTab(cfg, launch.env(), launch.argv(), cwd, role) : spawnAsPane(cfg, launch.env(), launch.argv(), cwd); return new Spawned(agent, launch.agentSessionId()); } + /** + * The next tab number for {@code role} on {@code profile}, starting at 1. + * + *

Starts at 1 rather than 0 because the number is read by a person: {@code "dev: sonnet #1"} + * is the first one, and {@code #0} invites the question of where {@code #1} went. + */ + private long nextLabelSeq(MemberRole role, String profile) { + String key = (role == null ? "" : role.wireName()) + "/" + profile; + return labelSeq.computeIfAbsent(key, _ -> new AtomicLong()).incrementAndGet(); + } + /** * {@inheritDoc} * @@ -272,7 +330,7 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { @Override public PeerHandle spawn(SpawnRequest req) { Spawned spawned = spawnInternal(req.profileName(), req.requestedCwd(), req.callerCwd(), - req.sessionName(), req.resumeSessionId()); + req.sessionName(), req.resumeSessionId(), req.role()); Agent agent = spawned.agent(); String paneId = agent.paneId(); if (spawnReadyTimeoutMs > 0) { @@ -317,7 +375,7 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { /** Dedicated worker space → own tab (carrying cwd+env) → start the peer into the seed pane. */ private Agent spawnInTab(BridgedConfig.Profile cfg, Map workerEnv, - List argv, String cwd) { + List argv, String cwd, MemberRole role) { Workspace space = spaces.ensureWorkspace(cfg.workspace()); Tab.Created tab = spaces.createTab(space.workspaceId(), cwd, workerEnv); log.info("spawning {} profile={} space={} tab={} cwd={}", @@ -348,7 +406,8 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { // Labelling is cosmetic: it must not fail the spawn or orphan the running peer — on error // we log and still return it so the caller gets its paneId and can tear it down. tidy("label tab " + tab.tab().tabId(), - () -> spaces.renameTab(tab.tab().tabId(), cfg.renderTabLabel(started.seq()))); + () -> spaces.renameTab(tab.tab().tabId(), + cfg.renderTabLabel(tabLabelTemplate, role, nextLabelSeq(role, cfg.profile())))); log.info("{} started pane={} tab={} terminal={}", namePrefix, started.agent().paneId(), started.agent().tabId(), started.agent().terminalId()); return started.agent(); diff --git a/bridged/src/main/java/dev/ltms/bridged/member/OpenCodeLauncher.java b/bridged/src/main/java/dev/ltms/bridged/member/OpenCodeLauncher.java index 812de5d..cef1d9f 100644 --- a/bridged/src/main/java/dev/ltms/bridged/member/OpenCodeLauncher.java +++ b/bridged/src/main/java/dev/ltms/bridged/member/OpenCodeLauncher.java @@ -112,9 +112,23 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { Map profiles, String defaultProfile, Function env, long spawnReadyTimeoutMs, long spawnReadyPollMs) { + this(agents, spaces, profiles, defaultProfile, env, + spawnReadyTimeoutMs, spawnReadyPollMs, null); + } + + /** + * Production constructor carrying the fleet-wide tab-label template (CB-557). The template comes + * from {@code fleet.tabLabel}, which a profile cannot know because it names the member's + * role; a profile may still override it with its own {@code tabLabel}. + */ + public OpenCodeLauncher(AgentControl agents, WorkspaceControl spaces, + Map profiles, String defaultProfile, + Function env, + long spawnReadyTimeoutMs, long spawnReadyPollMs, + String tabLabelTemplate) { this(agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs), - defaultConfigRoot(), defaultDiscoveryRoot()); + defaultConfigRoot(), defaultDiscoveryRoot(), tabLabelTemplate); } /** @@ -142,8 +156,25 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { long spawnReadyTimeoutMs, LongSupplier nowMillis, Runnable sleeper, Path configRoot, Path discoveryRoot) { + this(agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, + nowMillis, sleeper, configRoot, discoveryRoot, null); + } + + /** + * Full testability constructor, plus the fleet-wide tab-label template (CB-557). + * + * @param tabLabelTemplate {@code fleet.tabLabel}; {@code null}/blank ⇒ + * {@link BridgedConfig.Fleet#DEFAULT_TAB_LABEL} + */ + public OpenCodeLauncher(AgentControl agents, WorkspaceControl spaces, + Map profiles, String defaultProfile, + Function env, + long spawnReadyTimeoutMs, + LongSupplier nowMillis, Runnable sleeper, + Path configRoot, Path discoveryRoot, + String tabLabelTemplate) { super(NAME_PREFIX, agents, spaces, profiles, defaultProfile, env, - spawnReadyTimeoutMs, nowMillis, sleeper); + spawnReadyTimeoutMs, nowMillis, sleeper, tabLabelTemplate); this.configRoot = configRoot; this.discovery = new OpenCodeSessionDiscovery(discoveryRoot); } diff --git a/bridged/src/main/java/dev/ltms/bridged/peer/MemberRole.java b/bridged/src/main/java/dev/ltms/bridged/peer/MemberRole.java index 7185c3f..ebf902f 100644 --- a/bridged/src/main/java/dev/ltms/bridged/peer/MemberRole.java +++ b/bridged/src/main/java/dev/ltms/bridged/peer/MemberRole.java @@ -57,6 +57,40 @@ public enum MemberRole { return name().toLowerCase(Locale.ROOT); } + /** + * The {@code fleet:} block that holds this role's pool — {@code architects}, + * {@code developers}, {@code reviewers}. + * + *

Plural, and not always the wire name: the pool of things a {@code dev} may run on reads + * naturally as {@code developers:}. The wire name stays the singular {@code dev}, because that + * is what a tab label and a roster row say. + */ + public String configKey() { + return switch (this) { + case ARCHITECT -> "architects"; + case DEV -> "developers"; + case REVIEWER -> "reviewers"; + }; + } + + /** + * The role owning the {@code fleet:} pool named {@code key}, or {@code null} when the key is not + * a role pool ({@code leaders}, {@code tabLabel}, …). Null rather than a throw: callers use this + * to sort a fleet block's children, where a non-pool key is normal rather than an error. + */ + public static MemberRole fromConfigKey(String key) { + if (key == null) { + return null; + } + String k = key.trim().toLowerCase(Locale.ROOT); + for (MemberRole r : values()) { + if (r.configKey().equals(k)) { + return r; + } + } + return null; + } + /** * Parse a config/wire spelling, case-insensitively. * diff --git a/bridged/src/main/java/dev/ltms/bridged/peer/SpawnRequest.java b/bridged/src/main/java/dev/ltms/bridged/peer/SpawnRequest.java index ce63676..4cb60b8 100644 --- a/bridged/src/main/java/dev/ltms/bridged/peer/SpawnRequest.java +++ b/bridged/src/main/java/dev/ltms/bridged/peer/SpawnRequest.java @@ -1,24 +1,32 @@ package dev.ltms.bridged.peer; /** - * Parameters for a {@link PeerLauncher#spawn(SpawnRequest)} call — the peer-neutral - * aggregation of what the core knows at delegation time: which profile to use, the caller's - * requested working directory, and the caller's own cwd (to inherit when no other cwd is set). + * What a caller asks for when spawning a peer. * - *

A null or blank {@code profileName} means "use the launcher's default profile." - * A null or blank {@code requestedCwd} means "inherit from config or caller." - * A null {@code callerCwd} means "the request came from the daemon itself (not a primary)." - * - *

{@code sessionName} and {@code resumeSessionId} carry the session's durable identity (CB-547a): - * the bridge's LOGICAL name for the session (stable across restarts, meaningful to an operator) - * and the peer's OWN prior session id to resume, respectively. Both are opted in — either - * may be null/blank, in which case the launcher derives a display name and mints a fresh session. + * @param profileName the {@code profiles:} entry to spawn on; {@code null}/blank ⇒ the caller + * did not choose, and the role's pool supplies the candidates + * @param requestedCwd working directory asked for by the caller ({@code null} ⇒ unset) + * @param callerCwd the caller's own working directory, used when nothing else pins one + * @param sessionName agent session name (CB-547a); {@code null} ⇒ the launcher mints one + * @param resumeSessionId prior agent session to resume; {@code null} ⇒ a fresh session + * @param role the contract this member runs under (CB-557). Picks the tab label and, + * with the profile, the counter its tab number comes from. {@code null} is + * read as {@link MemberRole#DEV} — an unqualified spawn is a unit of work */ public record SpawnRequest(String profileName, String requestedCwd, String callerCwd, - String sessionName, String resumeSessionId) { + String sessionName, String resumeSessionId, MemberRole role) { + + public SpawnRequest { + role = (role == null) ? MemberRole.DEV : role; + } - /** Back-compat: a spawn with no session identity (fresh session, launcher-derived name). */ public SpawnRequest(String profileName, String requestedCwd, String callerCwd) { - this(profileName, requestedCwd, callerCwd, null, null); + this(profileName, requestedCwd, callerCwd, null, null, null); + } + + /** Pre-CB-557 shape: session identity without an explicit role (defaults to {@code dev}). */ + public SpawnRequest(String profileName, String requestedCwd, String callerCwd, + String sessionName, String resumeSessionId) { + this(profileName, requestedCwd, callerCwd, sessionName, resumeSessionId, null); } } diff --git a/bridged/src/test/java/dev/ltms/bridged/auth/MemberRegistryTest.java b/bridged/src/test/java/dev/ltms/bridged/auth/MemberRegistryTest.java index 53ea497..d3b8487 100644 --- a/bridged/src/test/java/dev/ltms/bridged/auth/MemberRegistryTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/auth/MemberRegistryTest.java @@ -1,12 +1,14 @@ package dev.ltms.bridged.auth; import dev.ltms.bridged.config.BridgedConfig; +import dev.ltms.bridged.peer.MemberRole; import org.junit.jupiter.api.Test; import java.util.ArrayList; -import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -22,23 +24,52 @@ import static org.junit.jupiter.api.Assertions.*; */ class MemberRegistryTest { - private static final Map SLOTS = Map.of( - "lead-designer", new BridgedConfig.Member("architect", "sonnet"), - "reviewer", new BridgedConfig.Member("architect", "gx10")); + /** + * Slot keys are qualified by role (CB-557): a bare name is unique only within its pool, so + * {@code sonnet} can be both a developer and a reviewer, while a terminal binds to exactly one. + */ + private static final String DESIGNER = "architect:lead-designer"; + private static final String REVIEWER = "architect:code-reviewer"; - private final MemberRegistry registry = new MemberRegistry(SLOTS); + private static BridgedConfig.Fleet fleetWith(Map architects) { + return new BridgedConfig.Fleet(Map.of(), architects, Map.of(), Map.of(), null); + } + + private static Map architects() { + Map pool = new LinkedHashMap<>(); + pool.put("lead-designer", new BridgedConfig.Slot("sonnet")); + pool.put("code-reviewer", new BridgedConfig.Slot("gx10")); + return pool; + } + + private final MemberRegistry registry = new MemberRegistry(fleetWith(architects())); @Test - void exposesTheConfiguredSlots() { - assertEquals(SLOTS.keySet(), registry.slots().keySet()); - assertTrue(registry.isSlot("reviewer")); + void exposesTheConfiguredSlotsQualifiedByRole() { + assertEquals(Set.of(DESIGNER, REVIEWER), registry.slots().keySet()); + assertTrue(registry.isSlot(REVIEWER)); assertFalse(registry.isSlot("nope")); + assertFalse(registry.isSlot("lead-designer"), + "the bare name is not the key — it is unique only inside its pool"); + } + + /** The case the pool shape exists for: one profile serving two roles is not a duplicate. */ + @Test + void oneProfileMayServeTwoRolesUnderTheSameSlotName() { + Map devs = Map.of("sonnet", new BridgedConfig.Slot("sonnet")); + Map revs = Map.of("sonnet", new BridgedConfig.Slot("sonnet")); + MemberRegistry r = new MemberRegistry( + new BridgedConfig.Fleet(Map.of(), Map.of(), devs, revs, null)); + + assertEquals(Set.of("dev:sonnet", "reviewer:sonnet"), r.slots().keySet()); + assertEquals(MemberRole.DEV, r.roleForSlot("dev:sonnet")); + assertEquals(MemberRole.REVIEWER, r.roleForSlot("reviewer:sonnet")); } @Test void theSpawnLifecycleReadsTheProfileBackFromASlot() { - assertEquals("sonnet", registry.profileForSlot("lead-designer")); - assertEquals("gx10", registry.profileForSlot("reviewer")); + assertEquals("sonnet", registry.profileForSlot(DESIGNER)); + assertEquals("gx10", registry.profileForSlot(REVIEWER)); assertNull(registry.profileForSlot("unknown"), "an unknown slot has no profile"); } @@ -54,9 +85,9 @@ class MemberRegistryTest { @Test void bindResolvesTheTerminalToTheSlot() { - assertTrue(registry.bind("lead-designer", "term_design")); - assertEquals("lead-designer", registry.slotForTerminal("term_design")); - assertEquals(Map.of("term_design", "lead-designer"), registry.snapshot()); + assertTrue(registry.bind(DESIGNER, "term_design")); + assertEquals(DESIGNER, registry.slotForTerminal("term_design")); + assertEquals(Map.of("term_design", DESIGNER), registry.snapshot()); } @Test @@ -68,27 +99,27 @@ class MemberRegistryTest { @Test void bindRefusesATerminalInTwoSlots() { - assertTrue(registry.bind("lead-designer", "term_design")); - assertFalse(registry.bind("reviewer", "term_design"), + assertTrue(registry.bind(DESIGNER, "term_design")); + assertFalse(registry.bind(REVIEWER, "term_design"), "a terminal may occupy at most one slot"); - assertEquals("lead-designer", registry.slotForTerminal("term_design"), + assertEquals(DESIGNER, registry.slotForTerminal("term_design"), "the first binding survives the refused second"); } @Test void bindRefusesASlotWithTwoTerminals() { - assertTrue(registry.bind("lead-designer", "term_design")); - assertFalse(registry.bind("lead-designer", "term_other"), + assertTrue(registry.bind(DESIGNER, "term_design")); + assertFalse(registry.bind(DESIGNER, "term_other"), "a slot may host at most one terminal"); - assertEquals("lead-designer", registry.slotForTerminal("term_design"), + assertEquals(DESIGNER, registry.slotForTerminal("term_design"), "the first binding survives the refused second"); assertNull(registry.slotForTerminal("term_other")); } @Test void rebindingTheSamePairIsAnIdempotentNoOp() { - assertTrue(registry.bind("lead-designer", "term_design")); - assertTrue(registry.bind("lead-designer", "term_design"), + assertTrue(registry.bind(DESIGNER, "term_design")); + assertTrue(registry.bind(DESIGNER, "term_design"), "the same terminal → slot is harmless to repeat"); assertEquals(1, registry.snapshot().size()); } @@ -97,8 +128,8 @@ class MemberRegistryTest { @Test void unbindRemovesTheExactBinding() { - assertTrue(registry.bind("lead-designer", "term_design")); - assertTrue(registry.unbind("lead-designer", "term_design")); + assertTrue(registry.bind(DESIGNER, "term_design")); + assertTrue(registry.unbind(DESIGNER, "term_design")); assertNull(registry.slotForTerminal("term_design")); assertTrue(registry.snapshot().isEmpty()); } @@ -106,32 +137,32 @@ class MemberRegistryTest { @Test void aStaleUnbindDoesNotRemoveAReplacement() { // Bind, tear down, and stand the slot back up with a NEW terminal. - assertTrue(registry.bind("lead-designer", "term_design")); - registry.unbind("lead-designer", "term_design"); - assertTrue(registry.bind("lead-designer", "term_new")); + assertTrue(registry.bind(DESIGNER, "term_design")); + registry.unbind(DESIGNER, "term_design"); + assertTrue(registry.bind(DESIGNER, "term_new")); // A late unbind naming the OLD terminal must not remove the replacement binding. - assertFalse(registry.unbind("lead-designer", "term_design")); - assertEquals("lead-designer", registry.slotForTerminal("term_new"), + assertFalse(registry.unbind(DESIGNER, "term_design")); + assertEquals(DESIGNER, registry.slotForTerminal("term_new"), "the replacement terminal stays bound"); } @Test void aStaleUnbindForATerminalThatMovedSlotsDoesNothing() { // term_design starts in lead-designer, is torn down, and stands back up in a FREE slot. - assertTrue(registry.bind("lead-designer", "term_design")); - registry.unbind("lead-designer", "term_design"); - assertTrue(registry.bind("reviewer", "term_design")); + assertTrue(registry.bind(DESIGNER, "term_design")); + registry.unbind(DESIGNER, "term_design"); + assertTrue(registry.bind(REVIEWER, "term_design")); // Unbinding against the slot it no longer occupies is refused; the new binding is intact. - assertFalse(registry.unbind("lead-designer", "term_design"), + assertFalse(registry.unbind(DESIGNER, "term_design"), "the old slot must not unbind a terminal that moved elsewhere"); - assertEquals("reviewer", registry.slotForTerminal("term_design")); + assertEquals(REVIEWER, registry.slotForTerminal("term_design")); } @Test void unbindOfNothingIsAFalseNoOp() { - assertFalse(registry.unbind("lead-designer", "term_design"), + assertFalse(registry.unbind(DESIGNER, "term_design"), "nothing was bound, so nothing is removed"); } @@ -139,14 +170,14 @@ class MemberRegistryTest { @Test void theSnapshotIsAnImmutableCopyNotAliveState() { - assertTrue(registry.bind("lead-designer", "term_design")); + assertTrue(registry.bind(DESIGNER, "term_design")); Map snap = registry.snapshot(); assertThrows(UnsupportedOperationException.class, () -> snap.put("x", "y"), "a handed-out snapshot cannot be mutated in place"); // Later binds must not leak into an earlier snapshot. - assertTrue(registry.bind("reviewer", "term_review")); + assertTrue(registry.bind(REVIEWER, "term_review")); assertFalse(snap.containsKey("term_review"), "a snapshot is a point-in-time copy, not a live view"); } @@ -164,7 +195,7 @@ class MemberRegistryTest { final String term = "term_" + i; // every thread races for the SAME slot results.add(pool.submit(() -> { go.await(); - return registry.bind("lead-designer", term); + return registry.bind(DESIGNER, term); })); } go.countDown(); @@ -191,7 +222,7 @@ class MemberRegistryTest { CountDownLatch go = new CountDownLatch(1); List> results = new ArrayList<>(); for (int i = 0; i < n; i++) { - final String slot = (i % 2 == 0) ? "lead-designer" : "reviewer"; // all race for ONE terminal + final String slot = (i % 2 == 0) ? DESIGNER : REVIEWER; // all race for ONE terminal results.add(pool.submit(() -> { go.await(); return registry.bind(slot, "shared_term") @@ -228,11 +259,11 @@ class MemberRegistryTest { /** A handed-over slot map is snapshotted at construction, not offered as live state. */ @Test void theSlotSnapshotIsFixedByConstruction() { - Map mutable = new HashMap<>(SLOTS); - MemberRegistry r = new MemberRegistry(mutable); + Map mutable = architects(); + MemberRegistry r = new MemberRegistry(fleetWith(mutable)); - mutable.put("hijack", new BridgedConfig.Member("architect", "gx10")); + mutable.put("hijack", new BridgedConfig.Slot("gx10")); - assertFalse(r.isSlot("hijack"), "a handed-over map is not offered as live state"); + assertFalse(r.isSlot("architect:hijack"), "a handed-over map is not offered as live state"); } } diff --git a/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java b/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java index cb10a51..49a853a 100644 --- a/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java @@ -1,6 +1,7 @@ package dev.ltms.bridged.config; import dev.ltms.bridged.auth.MemberRegistry; +import dev.ltms.bridged.peer.MemberRole; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -63,13 +64,12 @@ class BridgedConfigTest { BridgedConfig cfg = BridgedConfig.load(f); assertEquals(Set.of("ltms-local"), cfg.profiles().keySet()); - assertNull(cfg.defaultProfile(), "nothing named a default"); assertEquals("ltms-local", cfg.effectiveDefaultProfile(), "with one profile configured there is nothing to choose between"); } @Test - void loadsMultipleWorkerProfilesWithADefault(@TempDir Path dir) throws Exception { + void loadsMultipleProfilesAndPicksADevDefaultFromTheirPool(@TempDir Path dir) throws Exception { Path f = dir.resolve("multi.yaml"); Files.writeString(f, """ profiles: @@ -79,7 +79,12 @@ class BridgedConfigTest { ollama: baseUrl: http://ollama.ltms.dev argv: ["ccs", "ollama"] - defaultProfile: gx10 + fleet: + developers: + gx10: + profile: gx10 + ollama: + profile: ollama guard: offSubscriptionHosts: [gx10.gw, ollama.ltms.dev] """); @@ -91,7 +96,10 @@ class BridgedConfigTest { assertEquals(java.util.List.of("gx10", "ollama"), java.util.List.copyOf(cfg.profiles().keySet()), "profiles must preserve YAML definition order"); - assertEquals("gx10", cfg.defaultProfile()); + assertEquals(List.of("gx10", "ollama"), cfg.candidateProfiles(MemberRole.DEV), + "the dev pool supplies the candidates, in definition order"); + assertEquals("gx10", cfg.effectiveDefaultProfile(), + "the fixed policy answers with the pool's first entry"); assertEquals("ollama", cfg.profiles().get("ollama").profile(), "profile defaults to its map key"); assertEquals("http://gx10.gw:8000", cfg.profiles().get("gx10").baseUrl()); } @@ -124,7 +132,6 @@ class BridgedConfigTest { port: 8080 herdrSocket: /tmp/s profiles: {} - defaultProfile: a guard: {} worktreeRoot: /tmp lifecycle: {} @@ -132,9 +139,7 @@ class BridgedConfigTest { spawnReadyPollMs: 1 broker: {} primary: {} - leaders: {} - members: {} - leadScan: {} + fleet: {} leadHeartbeat: {} placement: fixed auth: {} @@ -150,38 +155,93 @@ class BridgedConfigTest { // ── CB-531: lead discovery by tab label ───────────────────────────────────────────────────── @Test - void leadScanIsOffUnlessTheBlockIsPresent(@TempDir Path dir) throws Exception { + void leadScanIsOffUnlessALeadIsConfigured(@TempDir Path dir) throws Exception { Path f = dir.resolve("no-scan.yaml"); Files.writeString(f, "bind:\n port: 8080\n"); - assertNull(BridgedConfig.load(f).leadScan(), + assertTrue(BridgedConfig.load(f).fleet().leaders().isEmpty(), "turning this on widens who resolves as PRIMARY — upgrading the daemon must not do that"); } @Test - void leadScanDefaultsItsFieldsWhenTheBlockIsPresentButBare(@TempDir Path dir) throws Exception { + void aLeadDefaultsItsScanFieldsWhenPresentButBare(@TempDir Path dir) throws Exception { Path f = dir.resolve("bare-scan.yaml"); - Files.writeString(f, "bind:\n port: 8080\nleadScan: {}\n"); + Files.writeString(f, """ + bind: + port: 8080 + fleet: + leaders: + opus: + terminal: term_opus + """); - BridgedConfig.LeadScan scan = BridgedConfig.load(f).leadScan(); - assertEquals("lead:", scan.tabPrefix()); - assertEquals(10, scan.intervalSeconds()); + BridgedConfig.Leader lead = BridgedConfig.load(f).fleet().leaders().get("opus"); + assertEquals("lead:", lead.tabPrefix()); + assertEquals(10, lead.scanIntervalSeconds()); + assertEquals(1, lead.instances(), "one of a lead is the assumption worth defaulting to"); } @Test - void leadScanReadsAnExplicitPrefixAndInterval(@TempDir Path dir) throws Exception { + void aLeadReadsAnExplicitPrefixAndInterval(@TempDir Path dir) throws Exception { Path f = dir.resolve("scan.yaml"); Files.writeString(f, """ bind: port: 8080 - leadScan: - tabPrefix: "drive:" - intervalSeconds: 30 + fleet: + leaders: + opus: + terminal: term_opus + tabPrefix: "drive:" + scanIntervalSeconds: 30 """); - BridgedConfig.LeadScan scan = BridgedConfig.load(f).leadScan(); - assertEquals("drive:", scan.tabPrefix()); - assertEquals(30, scan.intervalSeconds()); + BridgedConfig.Leader lead = BridgedConfig.load(f).fleet().leaders().get("opus"); + assertEquals("drive:", lead.tabPrefix()); + assertEquals(30, lead.scanIntervalSeconds()); + } + + /** + * The pane no longer has to exist before the daemon does (CB-557): a lead naming a profile may + * be launched, while one that names only a terminal is recognised and never created. + */ + @Test + void aLeadIsCreatableOnlyWhenItNamesAProfile(@TempDir Path dir) throws Exception { + Path f = dir.resolve("creatable.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + profiles: + opus: + subscription: true + fleet: + leaders: + launched: + profile: opus + pinned: + terminal: term_opus + """); + + var leaders = BridgedConfig.load(f).fleet().leaders(); + assertTrue(leaders.get("launched").isCreatable()); + assertFalse(leaders.get("pinned").isCreatable(), + "no profile to launch on ⇒ recognise-only, the pre-CB-557 behaviour"); + } + + @Test + void aLeadThatCanBeNeitherFoundNorCreatedRefusesToStart(@TempDir Path dir) throws Exception { + Path f = dir.resolve("useless-lead.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + fleet: + leaders: + ghost: + tabPrefix: "lead:" + """); + BridgedConfig cfg = BridgedConfig.load(f); + + IllegalStateException e = assertThrows(IllegalStateException.class, cfg::validateMembers); + assertTrue(e.getMessage().contains("ghost"), "the message must name the useless entry"); } // ── CB-551: the idle-lead heartbeat ───────────────────────────────────────────────────────── @@ -234,7 +294,8 @@ class BridgedConfigTest { * Overlap the two and every worker it spawns is read back as a lead. */ @Test - void aLeadPrefixThatAWorkerTabLabelAlsoMatchesRefusesToStart(@TempDir Path dir) throws Exception { + void aLeadPrefixThatAProfileTabLabelOverrideAlsoMatchesRefusesToStart(@TempDir Path dir) + throws Exception { Path f = dir.resolve("collide.yaml"); Files.writeString(f, """ bind: @@ -242,17 +303,45 @@ class BridgedConfigTest { profiles: gx10: tabLabel: "lead: {profile} #{n}" - leadScan: - tabPrefix: "lead:" + fleet: + leaders: + opus: + terminal: term_opus + tabPrefix: "lead:" """); BridgedConfig cfg = BridgedConfig.load(f); - IllegalStateException e = assertThrows(IllegalStateException.class, cfg::validateLeadScan); + IllegalStateException e = + assertThrows(IllegalStateException.class, cfg::validateLeadTabPrefixes); assertTrue(e.getMessage().contains("gx10"), "the message must name the offending profile"); } + /** A bad fleet-wide template promotes every member, not one profile — so it is checked too. */ @Test - void theDefaultWorkerTabLabelDoesNotCollideWithTheDefaultLeadPrefix(@TempDir Path dir) throws Exception { + void aFleetTabLabelThatMatchesALeadPrefixRefusesToStart(@TempDir Path dir) throws Exception { + Path f = dir.resolve("collide-template.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + fleet: + tabLabel: "lead: {role} {profile}" + leaders: + opus: + terminal: term_opus + """); + BridgedConfig cfg = BridgedConfig.load(f); + + IllegalStateException e = + assertThrows(IllegalStateException.class, cfg::validateLeadTabPrefixes); + assertTrue(e.getMessage().contains("fleet.tabLabel")); + } + + /** + * The point of making role the label's first field: {@code {role}} comes from a closed enum, so + * a generated label cannot begin with {@code "lead:"} however the fleet is configured. + */ + @Test + void theDefaultTabLabelCannotCollideWithTheDefaultLeadPrefix(@TempDir Path dir) throws Exception { Path f = dir.resolve("ok.yaml"); Files.writeString(f, """ bind: @@ -260,14 +349,22 @@ class BridgedConfigTest { profiles: gx10: baseUrl: http://gx00.gw:8000 - leadScan: {} + fleet: + leaders: + opus: + terminal: term_opus """); - assertDoesNotThrow(() -> BridgedConfig.load(f).validateLeadScan()); + assertDoesNotThrow(() -> BridgedConfig.load(f).validateLeadTabPrefixes()); + for (MemberRole role : MemberRole.values()) { + assertFalse(BridgedConfig.Fleet.DEFAULT_TAB_LABEL + .replace("{role}", role.wireName()).startsWith("lead:"), + "no role renders a label that reads as a lead"); + } } @Test - void theCollisionGuardIsANoOpWhenScanningIsOff(@TempDir Path dir) throws Exception { + void theCollisionGuardIsANoOpWhenNoLeadIsConfigured(@TempDir Path dir) throws Exception { Path f = dir.resolve("off.yaml"); Files.writeString(f, """ bind: @@ -277,7 +374,7 @@ class BridgedConfigTest { tabLabel: "lead: {profile}" """); - assertDoesNotThrow(() -> BridgedConfig.load(f).validateLeadScan(), + assertDoesNotThrow(() -> BridgedConfig.load(f).validateLeadTabPrefixes(), "a label that collides with a convention nobody reads is not a problem"); } @@ -289,21 +386,23 @@ class BridgedConfigTest { Files.writeString(f, """ bind: port: 8080 - leaders: - opus-5.0: - terminal: term_opus - kind: claude - gpt-sol-5.6: - terminal: term_sol - kind: opencode - model: openai/gpt-5.6-terra + fleet: + leaders: + opus-5.0: + terminal: term_opus + kind: claude + gpt-sol-5.6: + terminal: term_sol + kind: opencode + model: openai/gpt-5.6-terra """); BridgedConfig cfg = BridgedConfig.load(f); + var leaders = cfg.fleet().leaders(); - assertEquals(Set.of("opus-5.0", "gpt-sol-5.6"), cfg.leaders().keySet()); - assertEquals("opencode", cfg.leaders().get("gpt-sol-5.6").kind()); - assertEquals("openai/gpt-5.6-terra", cfg.leaders().get("gpt-sol-5.6").model()); + assertEquals(Set.of("opus-5.0", "gpt-sol-5.6"), leaders.keySet()); + assertEquals("opencode", leaders.get("gpt-sol-5.6").kind()); + assertEquals("openai/gpt-5.6-terra", leaders.get("gpt-sol-5.6").model()); // The whole point: BOTH panes resolve as leads, so neither is demoted to worker. assertEquals(Map.of("term_opus", "opus-5.0", "term_sol", "gpt-sol-5.6"), cfg.leaderTerminals()); @@ -326,9 +425,10 @@ class BridgedConfigTest { port: 8080 primary: terminal: term_shared - leaders: - opus-5.0: - terminal: term_shared + fleet: + leaders: + opus-5.0: + terminal: term_shared """); assertEquals(Map.of("term_shared", "opus-5.0"), BridgedConfig.load(f).leaderTerminals(), @@ -343,9 +443,10 @@ class BridgedConfigTest { port: 8080 primary: terminal: term_pinned - leaders: - gpt-sol-5.6: - terminal: term_sol + fleet: + leaders: + gpt-sol-5.6: + terminal: term_sol """); assertEquals(Map.of("term_pinned", "primary", "term_sol", "gpt-sol-5.6"), @@ -367,11 +468,12 @@ class BridgedConfigTest { Files.writeString(f, """ bind: port: 8080 - leaders: - sketch: - kind: opencode - real: - terminal: term_real + fleet: + leaders: + sketch: + kind: opencode + real: + terminal: term_real """); assertEquals(Map.of("term_real", "real"), BridgedConfig.load(f).leaderTerminals()); @@ -380,7 +482,7 @@ class BridgedConfigTest { // ── CB-548: the architects registry ──────────────────────────────────────────────────────── @Test - void architectsBlockDeclaresSlotsByNameAndProfileOnly(@TempDir Path dir) throws Exception { + void aRolePoolDeclaresSlotsByNameAndProfileOnly(@TempDir Path dir) throws Exception { Path f = dir.resolve("architects.yaml"); Files.writeString(f, """ bind: @@ -388,28 +490,81 @@ class BridgedConfigTest { profiles: sonnet: baseUrl: http://gx10.gw:8000 - members: - lead-designer: - role: architect - profile: sonnet - reviewer: - role: architect - profile: sonnet + fleet: + architects: + lead-designer: + profile: sonnet + second-opinion: + profile: sonnet """); BridgedConfig cfg = BridgedConfig.load(f); - assertEquals(Set.of("lead-designer", "reviewer"), cfg.members().keySet(), - "slot names are the keys — gateway-local unique by construction"); - assertEquals("sonnet", cfg.members().get("lead-designer").profile(), - "each slot carries its strong-model profile reference"); - assertEquals("sonnet", cfg.members().get("reviewer").profile()); + var pool = cfg.fleet().pool(MemberRole.ARCHITECT); + assertEquals(Set.of("lead-designer", "second-opinion"), pool.keySet(), + "slot names are the keys — unique within their pool by construction"); + assertEquals("sonnet", pool.get("lead-designer").profile(), + "each slot carries its backend reference"); + assertEquals("sonnet", pool.get("second-opinion").profile()); + } + + /** + * The role is the containing key now (CB-557), so it cannot be misspelled into a member with no + * contract. A pool name that is not a role is simply not a pool. + */ + @Test + void theRoleIsTheContainingKeyNotAField(@TempDir Path dir) throws Exception { + Path f = dir.resolve("role-by-key.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + profiles: + sonnet: + baseUrl: http://gx10.gw:8000 + fleet: + developers: + a: + profile: sonnet + reviewers: + b: + profile: sonnet + """); + + BridgedConfig cfg = BridgedConfig.load(f); + assertEquals(List.of("sonnet"), cfg.fleet().profilesFor(MemberRole.DEV)); + assertEquals(List.of("sonnet"), cfg.fleet().profilesFor(MemberRole.REVIEWER)); + assertTrue(cfg.fleet().profilesFor(MemberRole.ARCHITECT).isEmpty()); + assertEquals(List.of(MemberRole.DEV, MemberRole.REVIEWER), cfg.fleet().rolesConfigured()); + } + + /** The case the two axes exist for: one backend, two roles, and neither is a duplicate. */ + @Test + void oneProfileMayServeSeveralRoles(@TempDir Path dir) throws Exception { + Path f = dir.resolve("shared.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + profiles: + sonnet: + baseUrl: http://gx10.gw:8000 + fleet: + developers: + sonnet: + profile: sonnet + reviewers: + sonnet: + profile: sonnet + """); + + BridgedConfig cfg = BridgedConfig.load(f); + assertDoesNotThrow(cfg::validateMembers); + assertEquals("sonnet", cfg.defaultProfileFor(MemberRole.DEV)); + assertEquals("sonnet", cfg.defaultProfileFor(MemberRole.REVIEWER)); } @Test - void anArchitectCarriesNoConfigTerminalSoNothingIsRecognisedYet(@TempDir Path dir) throws Exception { - // The corrected CB-548 premise: config declares slots (name + profile) only. A `terminal:` - // key left over from the earlier premise is ignored — an architect is NOT recognised from - // config the way a lead is, so it binds nothing at startup and resolves no architect. + void aSlotCarriesNoConfigTerminalSoNothingIsRecognisedYet(@TempDir Path dir) throws Exception { + // Config declares slots (name + profile) only. A `terminal:` key is ignored — a member is + // NOT recognised from config the way a lead is, so it binds nothing at startup. Path f = dir.resolve("arch-stale-terminal.yaml"); Files.writeString(f, """ bind: @@ -417,35 +572,38 @@ class BridgedConfigTest { profiles: sonnet: baseUrl: http://gx10.gw:8000 - members: - lead-designer: - terminal: term_design - profile: sonnet + fleet: + architects: + lead-designer: + terminal: term_design + profile: sonnet """); BridgedConfig cfg = BridgedConfig.load(f); - assertEquals("sonnet", cfg.members().get("lead-designer").profile(), + assertEquals("sonnet", cfg.fleet().pool(MemberRole.ARCHITECT).get("lead-designer").profile(), "the profile is still read even when a stray terminal is ignored"); // The registry built from this config owns no bindings: the slot is idle at startup. - MemberRegistry r = new MemberRegistry(cfg.members()); + MemberRegistry r = new MemberRegistry(cfg.fleet()); assertTrue(r.snapshot().isEmpty()); assertNull(r.slotForTerminal("term_design"), - "a config terminal must not resolve an architect — slots start idle"); + "a config terminal must not resolve a member — slots start idle"); } @Test - void noArchitectsBlockLeavesNothingConfigured(@TempDir Path dir) throws Exception { + void noFleetBlockLeavesEveryPoolEmpty(@TempDir Path dir) throws Exception { Path f = dir.resolve("no-arch.yaml"); Files.writeString(f, "bind:\n port: 8080\n"); - assertNull(BridgedConfig.load(f).members(), - "no members: block ⇒ no architect identity, exactly as before CB-548"); + BridgedConfig cfg = BridgedConfig.load(f); + assertNotNull(cfg.fleet(), "an absent block is an empty fleet, not a null one"); + assertTrue(cfg.fleet().rolesConfigured().isEmpty(), + "no fleet: block ⇒ no member identity, exactly as before CB-548"); } @Test - void anArchitectSlotMayResolveToTheSoleProfileWithoutPrivileging(@TempDir Path dir) throws Exception { - // Even a single unqualified worker profile can back an architect slot — the reference is - // by name, not by position, so an explicit name is required. + void aSlotMayResolveToTheSoleProfileWithoutPrivileging(@TempDir Path dir) throws Exception { + // Even a single unqualified profile must be named explicitly — the reference is by name, + // not by position. Path f = dir.resolve("arch-single.yaml"); Files.writeString(f, """ bind: @@ -453,15 +611,16 @@ class BridgedConfigTest { profiles: ltms-local: baseUrl: http://gx10.gw:8000 - members: - lead-designer: - role: architect - profile: ltms-local + fleet: + architects: + lead-designer: + profile: ltms-local """); BridgedConfig cfg = BridgedConfig.load(f); assertDoesNotThrow(cfg::validateMembers); - assertEquals("ltms-local", cfg.members().get("lead-designer").profile()); + assertEquals("ltms-local", + cfg.fleet().pool(MemberRole.ARCHITECT).get("lead-designer").profile()); } @Test @@ -473,10 +632,10 @@ class BridgedConfigTest { profiles: gx10: baseUrl: http://gx10.gw:8000 - members: - lead-designer: - role: architect - profile: sonnet + fleet: + architects: + lead-designer: + profile: sonnet """); BridgedConfig cfg = BridgedConfig.load(f); @@ -494,10 +653,10 @@ class BridgedConfigTest { profiles: gx10: baseUrl: http://gx10.gw:8000 - members: - lead-designer: - role: architect - profile: "" + fleet: + architects: + lead-designer: + profile: "" """); BridgedConfig cfg = BridgedConfig.load(f); @@ -516,13 +675,13 @@ class BridgedConfigTest { baseUrl: http://gx10.gw:8000 gx10: baseUrl: http://gx10.gw:8000 - members: - lead-designer: - role: architect - profile: sonnet - reviewer: - role: architect - profile: gx10 + fleet: + architects: + lead-designer: + profile: sonnet + reviewers: + second-pair-of-eyes: + profile: gx10 """); assertDoesNotThrow(() -> BridgedConfig.load(f).validateMembers()); @@ -537,7 +696,7 @@ class BridgedConfigTest { } @Test - void duplicateArchitectSlotNamesAreRejectedAtParseTime(@TempDir Path dir) throws Exception { + void duplicateSlotNamesInOnePoolAreRejectedAtParseTime(@TempDir Path dir) throws Exception { Path f = dir.resolve("arch-dup.yaml"); Files.writeString(f, """ bind: @@ -545,26 +704,52 @@ class BridgedConfigTest { profiles: sonnet: baseUrl: http://gx10.gw:8000 - members: - lead-designer: - role: architect - profile: sonnet - lead-designer: - role: architect - profile: sonnet + fleet: + architects: + lead-designer: + profile: sonnet + lead-designer: + profile: sonnet """); IllegalStateException e = assertThrows(IllegalStateException.class, () -> BridgedConfig.load(f)); assertTrue(e.getMessage().contains("lead-designer"), "the refusal names the duplicated slot, was: " + e.getMessage()); - assertTrue(e.getMessage().contains("duplicate member"), - "the refusal says the slot name is duplicated"); + assertTrue(e.getMessage().contains("fleet.architects"), + "the refusal names the pool the duplicate is in, was: " + e.getMessage()); + } + + /** + * The same name in two different pools is the role × profile matrix, not a mistake — only a + * repeat within one pool loses an entry. + */ + @Test + void theSameSlotNameInTwoPoolsIsNotADuplicate(@TempDir Path dir) throws Exception { + Path f = dir.resolve("cross-pool.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + profiles: + sonnet: + baseUrl: http://gx10.gw:8000 + fleet: + developers: + sonnet: + profile: sonnet + reviewers: + sonnet: + profile: sonnet + """); + + BridgedConfig cfg = assertDoesNotThrow(() -> BridgedConfig.load(f)); + assertEquals(Set.of("sonnet"), cfg.fleet().pool(MemberRole.DEV).keySet()); + assertEquals(Set.of("sonnet"), cfg.fleet().pool(MemberRole.REVIEWER).keySet()); } @Test - void duplicateKeysOutsideArchitectsAreUnaffected(@TempDir Path dir) throws Exception { - // The duplicate check is scoped to the architects block — a duplicate elsewhere is not this + void duplicateKeysOutsideTheFleetPoolsAreUnaffected(@TempDir Path dir) throws Exception { + // The duplicate check is scoped to the fleet pools — a duplicate elsewhere is not this // guard's concern and must not change parsing of the rest of the config. Path f = dir.resolve("dup-other.yaml"); Files.writeString(f, """ @@ -576,47 +761,47 @@ class BridgedConfigTest { sonnet: baseUrl: http://gx10.gw:8000 """); - // Last-wins for a non-architect duplicate is untouched: only the architects block is walked. + // Last-wins for a non-pool duplicate is untouched: only the fleet pools are walked. assertEquals(Set.of("sonnet"), BridgedConfig.load(f).profiles().keySet()); } @Test - void aNestedArchitectsFieldDoesNotSuppressRealDuplicateDetection(@TempDir Path dir) throws Exception { - // A field ALSO named `architects` nested under another block carries its own duplicate and - // sits BEFORE the real top-level block. Only the top-level block is ever inspected: the - // refusal must name the real slot (lead-designer), not the nested one (nested-slot). + void aNestedFleetFieldDoesNotSuppressRealDuplicateDetection(@TempDir Path dir) throws Exception { + // A field ALSO named `fleet` nested under another block carries its own duplicate and sits + // BEFORE the real top-level block. Only the top-level block is ever inspected: the refusal + // must name the real slot (lead-designer), not the nested one (nested-slot). Path f = dir.resolve("nested-arch.yaml"); Files.writeString(f, """ bind: port: 8080 - members: - nested-slot: - k: v - nested-slot: - k: v - members: - lead-designer: - role: architect - profile: sonnet - lead-designer: - role: architect - profile: sonnet + fleet: + architects: + nested-slot: + k: v + nested-slot: + k: v + fleet: + architects: + lead-designer: + profile: sonnet + lead-designer: + profile: sonnet """); IllegalStateException e = assertThrows(IllegalStateException.class, () -> BridgedConfig.load(f)); assertTrue(e.getMessage().contains("lead-designer"), "the real top-level duplicate must be reported, was: " + e.getMessage()); - assertTrue(e.getMessage().contains("duplicate member"), - "the refusal says the slot name is duplicated"); + assertFalse(e.getMessage().contains("nested-slot"), + "the nested block must not be inspected, was: " + e.getMessage()); } @Test void nestedDuplicateFieldsInsideASlotAreNotDuplicateSlotNames(@TempDir Path dir) throws Exception { // A duplicated field nested inside one slot's own value (here inside an ignored `extra:` // sub-block) is not a duplicate SLOT name — it must not be rejected as one. Only the direct - // child keys of the architects mapping are slot names; whatever is deeper is the slot's - // business and must not masquerade as a duplicate slot. + // child keys of a pool mapping are slot names; whatever is deeper is the slot's business and + // must not masquerade as a duplicate slot. Path f = dir.resolve("nested-dup-inside-slot.yaml"); Files.writeString(f, """ bind: @@ -624,20 +809,21 @@ class BridgedConfigTest { profiles: sonnet: baseUrl: http://gx10.gw:8000 - members: - lead-designer: - role: architect - profile: sonnet - extra: - a: 1 - a: 1 + fleet: + architects: + lead-designer: + profile: sonnet + extra: + a: 1 + a: 1 """); BridgedConfig cfg = assertDoesNotThrow(() -> BridgedConfig.load(f)); assertDoesNotThrow(cfg::validateMembers, "a nested duplicate inside a slot is not a duplicate slot and must not refuse startup"); - assertEquals(Set.of("lead-designer"), cfg.members().keySet()); - assertEquals("sonnet", cfg.members().get("lead-designer").profile()); + var pool = cfg.fleet().pool(MemberRole.ARCHITECT); + assertEquals(Set.of("lead-designer"), pool.keySet()); + assertEquals("sonnet", pool.get("lead-designer").profile()); } @Test @@ -853,7 +1039,7 @@ class BridgedConfigTest { BridgedConfig cfg = BridgedConfig.load(example); assertEquals(8765, cfg.bind().port(), "example binds the documented default port"); assertTrue(cfg.profiles().containsKey("gx10"), "example documents the gx10 profile"); - assertEquals("gx10", cfg.defaultProfile(), "example's defaultWorker resolves"); + assertEquals("gx10", cfg.effectiveDefaultProfile(), "example's dev pool resolves"); assertTrue(cfg.guard().hostSet().contains("gx01.gw"), "every example profile's base_url host must be in the example allowlist"); } @@ -1095,8 +1281,8 @@ class BridgedConfigTest { """); IllegalStateException e = assertThrows(IllegalStateException.class, () -> BridgedConfig.load(f)); - assertTrue(e.getMessage().contains("'architects' is now 'members'"), e.getMessage()); - assertTrue(e.getMessage().contains("'defaultWorker' is now 'defaultProfile'"), e.getMessage()); + assertTrue(e.getMessage().contains("'architects' is now 'fleet.architects'"), e.getMessage()); + assertTrue(e.getMessage().contains("'defaultWorker' is now a role pool"), e.getMessage()); assertTrue(e.getMessage().contains("'workers' is now 'profiles'"), e.getMessage()); } @@ -1109,20 +1295,21 @@ class BridgedConfigTest { baseUrl: http://gx00.gw:8000 opus: baseUrl: http://gx00.gw:8000 - members: - architect-1: - role: architect - profile: opus - reviewer-1: - role: reviewer - profile: gx10 + fleet: + architects: + architect-1: + profile: opus + reviewers: + reviewer-1: + profile: gx10 """); BridgedConfig cfg = BridgedConfig.load(f); assertDoesNotThrow(cfg::validateMembers); - assertEquals("architect", cfg.members().get("architect-1").role()); - assertEquals("opus", cfg.members().get("architect-1").profile()); - assertEquals("reviewer", cfg.members().get("reviewer-1").role()); + assertEquals("opus", cfg.fleet().pool(MemberRole.ARCHITECT).get("architect-1").profile()); + assertEquals("gx10", cfg.fleet().pool(MemberRole.REVIEWER).get("reviewer-1").profile()); + assertEquals(List.of(MemberRole.ARCHITECT, MemberRole.REVIEWER), + cfg.fleet().rolesConfigured(), "the pool a slot sits in IS its role"); } /** @@ -1136,56 +1323,77 @@ class BridgedConfigTest { profiles: gx10: baseUrl: http://gx00.gw:8000 - members: - dev-1: - role: dev - profile: gx10 - reviewer-1: - role: reviewer - profile: gx10 + fleet: + developers: + dev-1: + profile: gx10 + reviewers: + reviewer-1: + profile: gx10 """); BridgedConfig cfg = BridgedConfig.load(f); assertDoesNotThrow(cfg::validateMembers); - assertEquals(cfg.members().get("dev-1").profile(), cfg.members().get("reviewer-1").profile()); - assertNotEquals(cfg.members().get("dev-1").role(), cfg.members().get("reviewer-1").role()); + assertEquals(cfg.fleet().pool(MemberRole.DEV).get("dev-1").profile(), + cfg.fleet().pool(MemberRole.REVIEWER).get("reviewer-1").profile(), + "one backend, two roles — the axes are independent"); + assertEquals("gx10", cfg.defaultProfileFor(MemberRole.DEV)); + assertEquals("gx10", cfg.defaultProfileFor(MemberRole.REVIEWER)); } + /** + * What the pool shape bought over the old {@code role:} field. A misspelled role used to parse + * into a member with no contract, so it needed catching by name. Now it is a pool name nobody + * reads: the slot simply does not exist, and no member can run under a role that is not one. + */ @Test - void aMemberSlotWithAnUnknownRoleIsRejectedAndListsTheValidRoles(@TempDir Path dir) throws Exception { + void aMisspelledPoolNameDeclaresNoMembersRatherThanRolelessOnes(@TempDir Path dir) + throws Exception { Path f = dir.resolve("bridged.yaml"); Files.writeString(f, """ profiles: gx10: baseUrl: http://gx00.gw:8000 - members: - slot-1: - role: archtiect - profile: gx10 + fleet: + archtiects: + slot-1: + profile: gx10 """); BridgedConfig cfg = BridgedConfig.load(f); - IllegalStateException e = assertThrows(IllegalStateException.class, cfg::validateMembers); - assertTrue(e.getMessage().contains("archtiect"), e.getMessage()); - assertTrue(e.getMessage().contains("architect, dev, reviewer"), - "the error must list the valid spellings so the typo is fixable from it: " + e.getMessage()); + assertDoesNotThrow(cfg::validateMembers); + assertTrue(cfg.fleet().rolesConfigured().isEmpty(), + "a pool name that is not a role declares nothing at all"); + assertNull(MemberRole.fromConfigKey("archtiects")); + } + + /** Every role's pool key must round-trip, or a correctly-spelled block would be dropped. */ + @Test + void everyRolePoolKeyRoundTrips() { + for (MemberRole role : MemberRole.values()) { + assertEquals(role, MemberRole.fromConfigKey(role.configKey()), + role + " must be readable back from the key it is written under"); + } + assertNull(MemberRole.fromConfigKey("leaders"), "a lead is not a member role"); + assertNull(MemberRole.fromConfigKey("tabLabel"), "a non-pool fleet key is not a role"); } @Test - void aMemberSlotWithNoRoleIsRejected(@TempDir Path dir) throws Exception { + void aSlotWithNoProfileIsRejectedAndNamesItsPool(@TempDir Path dir) throws Exception { Path f = dir.resolve("bridged.yaml"); Files.writeString(f, """ profiles: gx10: baseUrl: http://gx00.gw:8000 - members: - slot-1: - profile: gx10 + fleet: + developers: + slot-1: {} """); BridgedConfig cfg = BridgedConfig.load(f); IllegalStateException e = assertThrows(IllegalStateException.class, cfg::validateMembers); - assertTrue(e.getMessage().contains("has no role:"), e.getMessage()); + assertTrue(e.getMessage().contains("fleet.developers.slot-1"), e.getMessage()); + assertTrue(e.getMessage().contains("has no profile:"), e.getMessage()); } @Test @@ -1221,7 +1429,9 @@ class BridgedConfigTest { """); BridgedConfig cfg = BridgedConfig.load(f); - assertNull(cfg.defaultProfile(), "the raw config value is absent"); - assertEquals("gx10", cfg.effectiveDefaultProfile(), "the resolved value is the first profile"); + assertTrue(cfg.fleet().profilesFor(MemberRole.DEV).isEmpty(), "no dev pool is configured"); + assertEquals("gx10", cfg.effectiveDefaultProfile(), + "with no pool to choose from, every configured profile is a candidate and the " + + "first one wins"); } } diff --git a/bridged/src/test/java/dev/ltms/bridged/member/ClaudeCodeLauncherTest.java b/bridged/src/test/java/dev/ltms/bridged/member/ClaudeCodeLauncherTest.java index 62f8876..b421ffc 100644 --- a/bridged/src/test/java/dev/ltms/bridged/member/ClaudeCodeLauncherTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/member/ClaudeCodeLauncherTest.java @@ -7,6 +7,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.PeerUnreachableException; import dev.ltms.bridged.peer.SpawnRequest; @@ -782,4 +783,79 @@ class ClaudeCodeLauncherTest { assertEquals("/opt/jdk", env.get("JAVA_HOME"), "only the Anthropic binding keys are stripped; the rest of env: still applies"); } + + // ── CB-557: role-aware tab labels ───────────────────────────────────────────────────────── + + /** The {@code label} of every {@code tab.rename}, in call order. */ + private List tabLabels(FakeHerdr herdr) { + return herdr.calls.stream() + .filter(c -> c.method().equals("tab.rename")) + .map(c -> (String) ((Map) c.params()).get("label")) + .toList(); + } + + /** A profile with no {@code tabLabel:} of its own — the fleet template decides. */ + private ClaudeCodeLauncher labelService(FakeHerdr herdr, String fleetTemplate) { + BridgedConfig.Profile cfg = new BridgedConfig.Profile( + "sonnet", "http://gx00.gw:8000", "sonnet", null, "BRIDGED_WORKER_TOKEN", + List.of("claude"), "tab", "bridged-workers", null, null, null, null); + return new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), + _ -> null, 0, 0L, fleetTemplate); + } + + /** + * The knob must reach the rename call. It was inert once — {@code HerdrPeerLauncher} accepted a + * template while {@code Bridged} passed none, and the label stayed right only because the + * fallback happened to match. Pin the wiring, not the coincidence. + */ + @Test + void theFleetTemplateNamesTheRoleTheMemberWasSpawnedFor() { + FakeHerdr herdr = new FakeHerdr(); + ClaudeCodeLauncher svc = labelService(herdr, "{role}: {profile} #{n}"); + + svc.spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.REVIEWER)); + + assertEquals(List.of("reviewer: sonnet #1"), tabLabels(herdr)); + } + + /** The counter is per role+profile, so a dev and a reviewer on one profile both start at #1. */ + @Test + void theCounterRunsPerRoleAndProfileNotPerFleet() { + FakeHerdr herdr = new FakeHerdr(); + ClaudeCodeLauncher svc = labelService(herdr, "{role}: {profile} #{n}"); + + svc.spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.DEV)); + svc.spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.REVIEWER)); + svc.spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.DEV)); + + assertEquals(List.of("dev: sonnet #1", "reviewer: sonnet #1", "dev: sonnet #2"), + tabLabels(herdr)); + } + + /** No fleet template configured ⇒ the built-in default, still role-first. */ + @Test + void aBlankFleetTemplateFallsBackToTheRoleFirstDefault() { + FakeHerdr herdr = new FakeHerdr(); + labelService(herdr, null).spawn( + new SpawnRequest("sonnet", null, null, null, null, MemberRole.ARCHITECT)); + + assertEquals(List.of("architect: sonnet #1"), tabLabels(herdr)); + assertEquals("{role}: {profile} #{n}", BridgedConfig.Fleet.DEFAULT_TAB_LABEL); + } + + /** A profile that wants its own label still outranks the fleet template. */ + @Test + void aProfileTabLabelOverridesTheFleetTemplate() { + FakeHerdr herdr = new FakeHerdr(); + BridgedConfig.Profile cfg = new BridgedConfig.Profile( + "sonnet", "http://gx00.gw:8000", "sonnet", null, "BRIDGED_WORKER_TOKEN", + List.of("claude"), "tab", "bridged-workers", "pinned {profile}", null, null, null); + new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), + _ -> null, 0, 0L, "{role}: {profile} #{n}") + .spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.REVIEWER)); + + assertEquals(List.of("pinned sonnet"), tabLabels(herdr)); + } } From 61944fc045499e38e470e90ff08a5e4c90266b6b Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 14 Aug 2026 16:37:29 +0200 Subject: [PATCH 2/4] 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()); + } } From 57f8fa257af7976b07fe0934a519982299c07a5b Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 14 Aug 2026 16:47:57 +0200 Subject: [PATCH 3/4] CB-558: launch a declared lead at startup when none is live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fleet.leaders..instances` was descriptive. Now the daemon reads it: a lead that names a `profile:` is started when fewer than `instances` are running. A lead with only a `terminal:` stays recognise-only, as before. A lead is not a member, and LeadLauncher exists to keep it that way. Every other spawn path goes through HerdrPeerLauncher, which does three things a lead must never get: it appends the worker reply charter ("you are an off-subscription worker … end every turn with bridge_reply" — the opposite of an orchestrator); it registers the session with SessionManager, whose idle reaper would kill a lead for being idle, which is a lead's normal state; and it can move a peer off the subscription. So this launcher talks to AgentControl/WorkspaceControl directly. The duplicated argv/env assembly is the cheaper half of that trade. Not double-spawning is the safety property, so liveness needs two pieces of evidence. A running agent in a tab labelled `lead: ` finds an auto-launched lead. A running agent on a pinned `terminal:` finds one the operator opened by hand — without it, a pinned lead whose tab carries no matching label would be relaunched on every boot. Member workspaces are excluded, so a member in a matching tab is never counted. If herdr cannot be reached, nothing is started: a second orchestrator is worse than none. Liveness deliberately requires the AGENT, not just the label. LeadTabScanner used to promise that bridged never writes a lead label, so there was no round-trip from the daemon's own rename back into its next decision. That is no longer true, and its javadoc now says so. The trust direction is unaffected — a label is a name, not a capability — but staleness becomes real: a label left by a crashed session would otherwise read as a live lead forever and disable auto-launch permanently. Two new knobs. `workspace:` (default "leads") is where a launched lead's tab goes; it must not be a member workspace, because those are excluded from the scan and a lead placed in one would never be found again. `cwd:` defaults to bridged's own working directory. Also: WorkspaceControl.listTabs, and a FakeHerdr tab seeder that leaves the canned response byte-identical when no tab is seeded. 617 tests pass (16 new), IDE-clean. --- bridged/bridged.example.yaml | 24 +- .../main/java/dev/ltms/bridged/Bridged.java | 15 +- .../ltms/bridged/config/BridgedConfig.java | 24 +- .../ltms/bridged/herdr/LeadTabScanner.java | 17 +- .../ltms/bridged/herdr/WorkspaceControl.java | 10 + .../dev/ltms/bridged/lead/LeadLauncher.java | 304 ++++++++++++++++++ .../dev/ltms/bridged/herdr/FakeHerdr.java | 30 +- .../ltms/bridged/lead/LeadLauncherTest.java | 255 +++++++++++++++ 8 files changed, 666 insertions(+), 13 deletions(-) create mode 100644 bridged/src/main/java/dev/ltms/bridged/lead/LeadLauncher.java create mode 100644 bridged/src/test/java/dev/ltms/bridged/lead/LeadLauncherTest.java diff --git a/bridged/bridged.example.yaml b/bridged/bridged.example.yaml index 17179f7..49a88dc 100644 --- a/bridged/bridged.example.yaml +++ b/bridged/bridged.example.yaml @@ -62,7 +62,8 @@ bind: # Two things stop the tab-name convention from becoming a way to claim leadership: the configured # member spaces are excluded from the scan, so nothing bridged places can land in a matching tab; # and startup REFUSES a `tabPrefix` that the fleet tabLabel template, or any per-profile `tabLabel` -# override, also matches — so the two namespaces cannot overlap by accident. +# override, also matches — so the two namespaces cannot overlap by accident. The label is a NAME, +# never a capability: what a pane may do is decided by the role the daemon resolves for it. # CB-551: IDLE-LEAD HEARTBEAT — nudge the single lead back to work when it has been continuously # idle (no open bridge_send driving it) past the quiet period. The fleet is one lead + architects + @@ -252,15 +253,28 @@ fleet: # # `tabPrefix` is the naming convention that finds a lead without pasting a terminal id: label the # tab `lead: ` when you open it and the pane is recognised on the next rescan. Reopen the - # tab later and the id changes; the label does not. bridged NEVER writes these labels — it renames - # member tabs but reads lead tabs read-only, so the tab bar always shows what you typed. + # tab later and the id changes; the label does not. + # + # A lead the daemon launches is labelled BY the daemon, using the same convention, so it is found + # by the same scan. A lead counts as live only when herdr also reports a running agent in that + # tab — a label left behind by a session that died does not block the relaunch. + # + # An auto-launched lead is NOT a member: it gets no worker reply charter, is never registered with + # the session lifecycle (the idle reaper would kill your orchestrator), and stays on the + # subscription — ANTHROPIC_BASE_URL/AUTH_TOKEN are stripped from its env whatever the profile says. # leaders: # opus-5.0: # profile: opus # omit to never create this lead, only recognise it - # instances: 1 # desired live count; only the shortfall is launched - # terminal: term_0123456789abcd # optional hand-pin; usually found by tabPrefix instead + # instances: 1 # desired live count; only the shortfall is launched. 0 = off + # terminal: term_0123456789abcd # optional hand-pin; usually found by tabPrefix instead. + # # A running agent on this terminal also counts as live, so a + # # lead you opened by hand is not relaunched under you. # tabPrefix: "lead:" # `lead: opus-5.0` ⇒ a lead named opus-5.0 (case-insensitive) # scanIntervalSeconds: 10 # rescan cadence, and the worst case before a new tab is seen + # workspace: leads # where a launched lead's tab is created (default "leads"). + # # MUST NOT be a member workspace — those are excluded from the + # # scan, so a lead placed in one is never found again. + # cwd: /path/to/repo # the launched lead's working directory (default: bridged's own) # kind: claude # descriptive; reported by bridge_whoami # gpt-sol-5.6: # terminal: term_fedcba9876543 diff --git a/bridged/src/main/java/dev/ltms/bridged/Bridged.java b/bridged/src/main/java/dev/ltms/bridged/Bridged.java index 46b0c81..31678a9 100644 --- a/bridged/src/main/java/dev/ltms/bridged/Bridged.java +++ b/bridged/src/main/java/dev/ltms/bridged/Bridged.java @@ -6,6 +6,7 @@ import dev.ltms.bridged.herdr.AgentControl; import dev.ltms.bridged.herdr.HerdrClient; import dev.ltms.bridged.herdr.HerdrException; import dev.ltms.bridged.herdr.LeadTabScanner; +import dev.ltms.bridged.lead.LeadLauncher; import dev.ltms.bridged.herdr.PaneLocator; import dev.ltms.bridged.herdr.UnixSocketHerdrClient; import dev.ltms.bridged.herdr.WorkspaceControl; @@ -145,7 +146,8 @@ public final class Bridged { // the first thing that actually talks to herdr, so without this wait a boot-order race // would crash the daemon into a restart loop. Wait, then degrade rather than die: serving // with /healthz reporting "degraded" is strictly more useful than exiting. - if (awaitHerdr(herdr)) { + boolean herdrUp = awaitHerdr(herdr); + if (herdrUp) { // CB-117: herdr keeps worker panes alive across a daemon restart, and their ids died // with the previous process — reap those leaked orphans now, before we start serving. workers.reapOrphanWorkers(); @@ -220,6 +222,17 @@ public final class Bridged { leads = () -> leadTerminals; } + // CB-558: start any declared lead that is not already running. After the scanner is built, + // because both read the same tab labels and the ordering makes that dependency visible; and + // only when herdr answered, because the launcher's whole safety property is that it can + // count live leads first — it must never guess and risk a second orchestrator. + if (herdrUp && !leaders.isEmpty()) { + int launched = new LeadLauncher(agents, spaces, cfg).ensureLeads(); + if (launched > 0) { + log.info("lead auto-launch: {} lead(s) started", launched); + } + } + // CB-548: config-declared architect slots. Config supplies only the stable name → profile // map; the terminal → slot binding is owned by the registry and is empty at startup, so no // pane resolves to an architect until the later spawn lifecycle binds one. The registry is diff --git a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java index 2e6015c..b513bc8 100644 --- a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java +++ b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java @@ -439,18 +439,40 @@ public record BridgedConfig( */ @JsonIgnoreProperties(ignoreUnknown = true) public record Leader(String profile, String terminal, Integer instances, String tabPrefix, - Integer scanIntervalSeconds, String kind, String model) { + Integer scanIntervalSeconds, String kind, String model, + String workspace, String cwd) { + + /** + * Where an auto-launched lead's tab is created (CB-558). It must NOT be a member workspace: + * {@code LeadTabScanner} excludes those wholesale, so a lead placed in one would never be + * discovered and the daemon would relaunch it on every boot. + */ + public static final String DEFAULT_WORKSPACE = "leads"; + public Leader { instances = (instances == null || instances < 0) ? 1 : instances; tabPrefix = (tabPrefix == null || tabPrefix.isBlank()) ? "lead:" : tabPrefix.strip(); scanIntervalSeconds = (scanIntervalSeconds == null || scanIntervalSeconds <= 0) ? 10 : scanIntervalSeconds; + workspace = (workspace == null || workspace.isBlank()) + ? DEFAULT_WORKSPACE : workspace.strip(); + } + + /** Back-compat 7-arg form — no workspace or cwd, so both take their defaults. */ + public Leader(String profile, String terminal, Integer instances, String tabPrefix, + Integer scanIntervalSeconds, String kind, String model) { + this(profile, terminal, instances, tabPrefix, scanIntervalSeconds, kind, model, null, null); } /** True when this lead may be launched by the daemon rather than only recognised. */ public boolean isCreatable() { return profile != null && !profile.isBlank() && instances > 0; } + + /** The tab label an auto-launched instance of this lead gets — what the scanner reads back. */ + public String tabLabel(String name) { + return tabPrefix + " " + name; + } } /** diff --git a/bridged/src/main/java/dev/ltms/bridged/herdr/LeadTabScanner.java b/bridged/src/main/java/dev/ltms/bridged/herdr/LeadTabScanner.java index 92deac6..522d581 100644 --- a/bridged/src/main/java/dev/ltms/bridged/herdr/LeadTabScanner.java +++ b/bridged/src/main/java/dev/ltms/bridged/herdr/LeadTabScanner.java @@ -26,15 +26,26 @@ import java.util.function.Supplier; *

Direction of trust. The label names the lead; it never grants * anything a pane could take for itself. Three properties keep that honest: *

    - *
  1. bridged never renames a lead tab. The operator's label is read-only input, so what is in - * the tab bar is always what the human wrote — no round-trip where the daemon's own rename - * becomes the evidence for its next decision.
    2. *
  2. Worker spaces are excluded wholesale ({@code excludedWorkspaceLabels}), so a worker cannot * become a lead by being placed — as a split, say — inside a matching tab.
    3. *
  3. A worker cannot rename a tab: {@code tab.rename} is reachable only through * {@link WorkspaceControl}, which no {@code bridge_*} tool exposes. The label is writable by * the human at the terminal and by nobody the bridge is defending against.
    4. + *
  4. The label is a name, not a capability. What a pane may do is decided by + * {@code Authz} against the role {@code CallerResolver} returns; a tab that calls itself a + * lead still cannot act as one unless the daemon's own registry agrees.
    5. *
+ * + *

CB-558 — bridged now writes lead labels too. This class used to be able to say + * that bridged never renames a lead tab, so the label was always the human's own writing and there + * was no round-trip from the daemon's rename back into its next decision. + * {@code dev.ltms.bridged.lead.LeadLauncher} ends that: an auto-launched lead is labelled by the + * daemon and found again by this scan. The trust direction above is unaffected — bridged writing a + * name for a lead it just started is not a pane promoting itself — but staleness becomes + * real: a label left behind by a session that has since died would read as a live lead forever. + * This scanner does not solve that (its job is naming, and a stale name costs nothing here); the + * launcher does, by requiring a running agent in the tab before it counts the lead as live. If you + * ever make a decision that removes something based on this map, add the same check. * The remaining hazard is an operator one — a worker {@code tabLabel} template that * happens to start with the same prefix would promote the whole fleet — and that is refused at * startup by {@code BridgedConfig.validateLeadScan} rather than documented here. diff --git a/bridged/src/main/java/dev/ltms/bridged/herdr/WorkspaceControl.java b/bridged/src/main/java/dev/ltms/bridged/herdr/WorkspaceControl.java index 69ed9a5..ce2c1fc 100644 --- a/bridged/src/main/java/dev/ltms/bridged/herdr/WorkspaceControl.java +++ b/bridged/src/main/java/dev/ltms/bridged/herdr/WorkspaceControl.java @@ -41,6 +41,16 @@ public final class WorkspaceControl { return out; } + /** Every tab in {@code workspaceId}, in herdr's order. */ + public List listTabs(String workspaceId) { + JsonNode result = herdr.call("tab.list", Map.of("workspace_id", workspaceId)); + List out = new ArrayList<>(); + for (JsonNode t : result.path("tabs")) { + out.add(Tab.from(t)); + } + return out; + } + /** The first workspace with this exact label, if any. */ public Optional findByLabel(String label) { return listWorkspaces().stream() diff --git a/bridged/src/main/java/dev/ltms/bridged/lead/LeadLauncher.java b/bridged/src/main/java/dev/ltms/bridged/lead/LeadLauncher.java new file mode 100644 index 0000000..0693893 --- /dev/null +++ b/bridged/src/main/java/dev/ltms/bridged/lead/LeadLauncher.java @@ -0,0 +1,304 @@ +package dev.ltms.bridged.lead; + +import dev.ltms.bridged.config.BridgedConfig; +import dev.ltms.bridged.herdr.Agent; +import dev.ltms.bridged.herdr.AgentControl; +import dev.ltms.bridged.herdr.HerdrException; +import dev.ltms.bridged.herdr.Tab; +import dev.ltms.bridged.herdr.Workspace; +import dev.ltms.bridged.herdr.WorkspaceControl; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * Starts the leads {@code fleet.leaders:} declares, when none is already running (CB-558). + * + *

A lead is not a member, and this class exists to keep it that way. Every other + * spawn path in the daemon goes through {@code HerdrPeerLauncher}, which does three things a lead + * must never receive: + *

    + *
  1. it appends the reply charter — "you are an off-subscription worker … end every turn + * with {@code bridge_reply}". A lead is the orchestrator; telling it that it is a worker is + * exactly backwards.
    2. + *
  2. it registers the session with {@code SessionManager}, which subjects it to the idle reaper, + * the context cap and the shutdown drain. An idle lead is the normal state of a lead, so the + * reaper would kill the orchestrator for doing its job.
    3. + *
  3. it can move a peer off the subscription via {@code ANTHROPIC_BASE_URL}. A lead stays on the + * operator's subscription, always.
    4. + *
+ * So this launcher talks to {@link AgentControl}/{@link WorkspaceControl} directly. The duplication + * with the member launchers (argv and env assembly) is deliberate and is the cheaper half of the + * trade: entangling the worker path with a not-a-worker case is how the three rules above get + * broken later, quietly. + * + *

Liveness, and the label round-trip. {@code LeadTabScanner} used to be able to + * promise that bridged never writes a lead label. That is no longer true — an auto-launched lead is + * labelled by this class, and the scanner reads that label back. The risk this opens is not + * privilege escalation (the tab label never granted anything a pane could take for itself; see that + * class's javadoc), but staleness: a label left behind by a crashed session would otherwise + * read as a live lead forever, and the lead would never be relaunched. So a lead counts as live only + * when herdr also reports a running agent in that tab — see {@link #liveLeads}. A labelled + * tab with no agent in it is not a lead. + */ +public final class LeadLauncher { + + private static final Logger log = LoggerFactory.getLogger(LeadLauncher.class); + + private final AgentControl agents; + private final WorkspaceControl spaces; + private final BridgedConfig cfg; + + /** + * @param agents herdr agent control (start, list) + * @param spaces workspace / tab control (ensure, create, label, list) + * @param cfg the loaded config — {@code fleet.leaders}, {@code profiles} and the lead pins + */ + public LeadLauncher(AgentControl agents, WorkspaceControl spaces, BridgedConfig cfg) { + this.agents = agents; + this.spaces = spaces; + this.cfg = cfg; + } + + /** + * Bring every declared lead up to its {@code instances} count, and return how many were started. + * + *

Never throws: a daemon that cannot start a lead must still serve. herdr being unreachable, + * a profile that does not exist, a failed {@code agent.start} — each is logged and skipped, and + * the remaining leads are still attempted. + */ + public int ensureLeads() { + Map leaders = cfg.fleet().leaders(); + if (leaders.isEmpty()) { + return 0; + } + + Map live; + try { + live = liveLeads(leaders); + } catch (HerdrException e) { + // Counting is the whole safety mechanism against double-spawning. If we cannot count, we + // must not guess — spawning a second orchestrator is worse than starting none. + log.warn("lead auto-launch skipped — cannot tell which leads are live: {}", e.getMessage()); + return 0; + } + + int started = 0; + for (Map.Entry e : leaders.entrySet()) { + String name = e.getKey(); + BridgedConfig.Leader lead = e.getValue(); + int running = live.getOrDefault(name, 0); + int wanted = lead.instances(); + + if (running >= wanted) { + log.info("lead '{}': {} live, {} wanted — nothing to start", name, running, wanted); + continue; + } + if (!lead.isCreatable()) { + // A lead with a `terminal:` pin and no `profile:` is recognise-only by design: the + // operator opens it by hand. Say so once rather than looking like a silent failure. + log.info("lead '{}' is not live, and names no profile — it can be recognised but not " + + "launched. Add `profile:` under fleet.leaders.{} to have bridged start it.", + name, name); + continue; + } + + BridgedConfig.Profile profile = cfg.profiles().get(lead.profile()); + if (profile == null) { + log.warn("lead '{}' names profile '{}', which is not configured — not launching", + name, lead.profile()); + continue; + } + + for (int i = running; i < wanted; i++) { + if (launch(name, lead, profile)) { + started++; + } + } + } + return started; + } + + /** + * How many live leads exist per configured name. + * + *

Two independent pieces of evidence, because either alone double-spawns: + *

    + *
  • a running agent in a tab labelled {@code " "} — how an auto-launched + * lead, or an operator following the labelling convention, is found;
    • + *
  • a running agent on a terminal the config pins in {@code fleet.leaders..terminal} — + * how a lead the operator opened and pinned by hand is found. Without this, a pinned lead + * whose tab carries no matching label would be relaunched on every boot.
    • + *
+ * Member workspaces are excluded, exactly as the scanner excludes them: a member must not be + * counted as a lead because it happens to sit in a matching tab. + */ + private Map liveLeads(Map leaders) { + Set memberSpaces = cfg.profiles().values().stream() + .map(BridgedConfig.Profile::workspace) + .filter(w -> w != null && !w.isBlank()) + .collect(Collectors.toSet()); + + // tabId → the lead name its label declares. + Map nameByTab = new LinkedHashMap<>(); + for (Workspace ws : spaces.listWorkspaces()) { + if (ws.workspaceId() == null || memberSpaces.contains(ws.label())) { + continue; + } + for (Tab tab : spaces.listTabs(ws.workspaceId())) { + String declared = leadNameOf(tab.label(), leaders); + if (declared != null && tab.tabId() != null) { + nameByTab.put(tab.tabId(), declared); + } + } + } + + // terminalId → the lead name the config pins it to. + Map nameByPinnedTerminal = new LinkedHashMap<>(); + leaders.forEach((name, lead) -> { + if (lead.terminal() != null && !lead.terminal().isBlank()) { + nameByPinnedTerminal.put(lead.terminal().strip(), name); + } + }); + + Map counts = new LinkedHashMap<>(); + for (Agent a : agents.list()) { + String name = nameByTab.get(a.tabId()); + if (name == null) { + name = nameByPinnedTerminal.get(a.terminalId()); + } + if (name != null) { + counts.merge(name, 1, Integer::sum); + } + } + return counts; + } + + /** + * The configured lead a tab label names, or {@code null} for a label that names none. + * + *

Matched against the declared lead names rather than by splitting on the prefix, so an + * operator's {@code "lead: something-else"} tab is not mistaken for a configured lead. + */ + private String leadNameOf(String label, Map leaders) { + if (label == null) { + return null; + } + String l = label.strip(); + for (Map.Entry e : leaders.entrySet()) { + if (l.equalsIgnoreCase(e.getValue().tabLabel(e.getKey()).strip())) { + return e.getKey(); + } + } + return null; + } + + /** Start one lead. Returns false (having logged) rather than throwing on any failure. */ + private boolean launch(String name, BridgedConfig.Leader lead, BridgedConfig.Profile profile) { + String label = lead.tabLabel(name); + String cwd = (lead.cwd() == null || lead.cwd().isBlank()) + ? System.getProperty("user.dir") : lead.cwd(); + + Tab.Created tab = null; + try { + Workspace ws = spaces.ensureWorkspace(lead.workspace()); + tab = spaces.createTab(ws.workspaceId(), cwd, leadEnv(profile)); + if (tab.rootPaneId() == null) { + throw new IllegalStateException("tab " + tab.tab().tabId() + + " came back with no seed pane — nowhere to start the lead"); + } + // Same shape as the member launchers: herdr resolves the executable from `kind`, so + // argv[0] (the configured launcher, e.g. `ccs`) is dropped and only the rest is passed. + List argv = leadArgv(profile); + Agent started = agents.start("lead-" + name, herdrKind(profile), + argv.isEmpty() ? argv : argv.subList(1, argv.size()), tab.rootPaneId()); + + // Label AFTER the start succeeds. A label written before would survive a failed start + // and then read back as a live lead on the next boot, which is the exact staleness the + // agent-liveness check exists to prevent — no need to create the case ourselves. + spaces.renameTab(tab.tab().tabId(), label); + + log.info("lead '{}' launched: profile={} tab={} pane={} terminal={} label='{}' cwd={}", + name, profile.profile(), tab.tab().tabId(), started.paneId(), + started.terminalId(), label, cwd); + return true; + } catch (RuntimeException e) { + log.warn("lead '{}' failed to launch on profile '{}': {}", + name, profile.profile(), e.getMessage()); + if (tab != null && tab.tab() != null && tab.tab().tabId() != null) { + try { + spaces.closeTab(tab.tab().tabId()); + } catch (RuntimeException cleanup) { + log.warn("could not close the orphaned lead tab {}: {}", + tab.tab().tabId(), cleanup.getMessage()); + } + } + return false; + } + } + + /** + * The herdr agent kind for this profile — the same value the matching member adapter passes, so + * herdr resolves the same executable for a lead as it does for a member on that backend. + */ + private static String herdrKind(BridgedConfig.Profile profile) { + return profile.isOpenCode() ? "opencode" : "claude"; + } + + /** + * The lead's argv: the profile's own command, the model pin, and the bridge MCP mount. + * + *

No {@code --append-system-prompt}. That flag carries the worker reply charter, and a lead + * is not a worker — it reads its orchestration rules from the project's {@code CLAUDE.md} like + * any other primary. This is the single most important difference from the member launchers; + * do not "unify" it back. + */ + private List leadArgv(BridgedConfig.Profile profile) { + List argv = new ArrayList<>(profile.argv()); + if (profile.hasMcp()) { + argv.add("--mcp-config"); + argv.add("{\"mcpServers\":{\"bridge\":{\"type\":\"http\",\"url\":\"" + + profile.mcpUrl() + "\"}}}"); + } + // Appended last, for the same reason the member launcher does it (CB-533): the argv is + // usually a wrapper such as `ccs `, which exports its own model family over + // whatever it inherited, and --model outranks the environment. + if (profile.model() != null && !profile.model().isBlank()) { + argv.add("--model"); + argv.add(profile.model()); + } + return argv; + } + + /** + * The lead's environment: the profile's {@code env:} block, and nothing that could move it off + * the subscription. + * + *

{@code ANTHROPIC_BASE_URL} and {@code ANTHROPIC_AUTH_TOKEN} are stripped unconditionally — + * not defaulted, not guarded, stripped. A lead runs on the operator's subscription by + * definition, so there is no configuration under which pointing it elsewhere is correct, and a + * profile that carries them (a member profile reused as a lead's backend) must not leak them in. + */ + private Map leadEnv(BridgedConfig.Profile profile) { + Map out = new LinkedHashMap<>(); + if (profile.env() != null) { + out.putAll(profile.env()); + } + out.remove("ANTHROPIC_BASE_URL"); + out.remove("ANTHROPIC_AUTH_TOKEN"); + if (profile.configDir() != null && !profile.configDir().isBlank()) { + out.put("CLAUDE_CONFIG_DIR", profile.configDir()); + } + // Deliberately no git token: a lead reviews and merges through the operator's own + // credentials, and never needs the scoped write:repository token a member is granted. + out.values().removeIf(Objects::isNull); + return out; + } +} diff --git a/bridged/src/test/java/dev/ltms/bridged/herdr/FakeHerdr.java b/bridged/src/test/java/dev/ltms/bridged/herdr/FakeHerdr.java index fe57395..2ec0c5a 100644 --- a/bridged/src/test/java/dev/ltms/bridged/herdr/FakeHerdr.java +++ b/bridged/src/test/java/dev/ltms/bridged/herdr/FakeHerdr.java @@ -4,7 +4,9 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; /** * Recording fake {@link HerdrClient} for unit/acceptance tests. Returns canned frames @@ -24,6 +26,8 @@ public final class FakeHerdr implements HerdrClient { private boolean healthy = true; private final List extraWorkspaces = new ArrayList<>(); private final List extraAgents = new ArrayList<>(); + /** workspaceId → extra tabs that {@code tab.list} reports for it (CB-558 lead scans). */ + private final Map> extraTabs = new LinkedHashMap<>(); private int agentNameTakenFor = 0; private int agentPaneBusyFor = 0; private int workerTabPaneCount = 1; @@ -109,6 +113,18 @@ public final class FakeHerdr implements HerdrClient { return this; } + /** + * Seed a labelled tab into {@code tab.list} for one workspace (e.g. an existing {@code lead: x} + * tab). Pair it with {@link #withAgent} on the same {@code tabId} to make the lead live; + * seeding the tab alone models the stale-label case. + */ + public FakeHerdr withTab(String workspaceId, String tabId, String label) { + extraTabs.computeIfAbsent(workspaceId, _ -> new ArrayList<>()) + .add(("{\"tab_id\":\"%s\",\"workspace_id\":\"%s\",\"label\":\"%s\",\"pane_count\":1}") + .formatted(tabId, workspaceId, label)); + return this; + } + /** Seed an additional workspace into {@code workspace.list} (e.g. a pre-existing worker space). */ public FakeHerdr withWorkspace(String id, String label) { extraWorkspaces.add(("{\"workspace_id\":\"%s\",\"label\":\"%s\",\"focused\":false," @@ -228,11 +244,19 @@ public final class FakeHerdr implements HerdrClient { case "tab.rename" -> mapper.readTree(""" {"type":"tab_info","tab":{"tab_id":"w9:t2","workspace_id":"w9", "label":"worker: ltms-local","pane_count":1}}"""); - case "tab.list" -> mapper.readTree((""" + case "tab.list" -> { + // The base pair is returned for every workspace, exactly as before. Seeded tabs + // are appended only for the workspace they were registered against, so a test + // that seeds none sees the historical response byte for byte. + Object wsId = params instanceof java.util.Map m ? m.get("workspace_id") : null; + List seeded = extraTabs.getOrDefault(String.valueOf(wsId), List.of()); + yield mapper.readTree((""" {"type":"tab_list","tabs":[ {"tab_id":"w9:t1","workspace_id":"w9","label":"1","pane_count":1}, - {"tab_id":"w9:t2","workspace_id":"w9","label":"worker: ltms-local","pane_count":%d}]}""") - .formatted(workerTabPaneCount)); + {"tab_id":"w9:t2","workspace_id":"w9","label":"worker: ltms-local","pane_count":%d}%s]}""") + .formatted(workerTabPaneCount, + seeded.isEmpty() ? "" : "," + String.join(",", seeded))); + } case "tab.close" -> mapper.readTree("{\"type\":\"ok\"}"); case "pane.get" -> mapper.readTree(""" {"type":"pane_info","pane":{"pane_id":"w9:pW","workspace_id":"w9", diff --git a/bridged/src/test/java/dev/ltms/bridged/lead/LeadLauncherTest.java b/bridged/src/test/java/dev/ltms/bridged/lead/LeadLauncherTest.java new file mode 100644 index 0000000..03f8bb6 --- /dev/null +++ b/bridged/src/test/java/dev/ltms/bridged/lead/LeadLauncherTest.java @@ -0,0 +1,255 @@ +package dev.ltms.bridged.lead; + +import dev.ltms.bridged.config.BridgedConfig; +import dev.ltms.bridged.herdr.AgentControl; +import dev.ltms.bridged.herdr.FakeHerdr; +import dev.ltms.bridged.herdr.WorkspaceControl; +import org.junit.jupiter.api.Test; + +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * CB-558 — the daemon starts a declared lead when none is running. + * + *

Two properties carry the whole feature. It must not double-spawn (a second orchestrator is + * worse than none), and what it starts must be a lead and not a member: no reply charter, + * no off-subscription env, and never registered with the session lifecycle. + */ +class LeadLauncherTest { + + /** The lead's backend: a subscription profile with the bridge mounted, as `opus` really is. */ + private static BridgedConfig.Profile opusProfile() { + return new BridgedConfig.Profile( + "opus", null, "claude-opus-5", null, "BRIDGED_WORKER_TOKEN", + List.of("ccs", "ltms"), "tab", "bridged-workers", null, + "http://127.0.0.1:8765/mcp", null, null, + null, null, null, + Map.of("CLAUDE_CODE_AUTO_COMPACT_WINDOW", "300000"), null, null, true); + } + + private static BridgedConfig configWith(BridgedConfig.Leader lead) { + Map leaders = new LinkedHashMap<>(); + leaders.put("opus", lead); + BridgedConfig.Fleet fleet = + new BridgedConfig.Fleet(leaders, Map.of(), Map.of(), Map.of(), null); + return new BridgedConfig( + null, null, Map.of("opus", opusProfile()), null, null, null, null, null, + null, null, fleet, null, "fixed", null).withDefaults(); + } + + private static BridgedConfig.Leader lead(String profile, String terminal, int instances) { + return new BridgedConfig.Leader(profile, terminal, instances, "lead:", 10, null, null, + "leads", "/repo"); + } + + private static LeadLauncher launcher(FakeHerdr herdr, BridgedConfig cfg) { + return new LeadLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), cfg); + } + + @SuppressWarnings("unchecked") + private static List startedArgs(FakeHerdr herdr) { + return (List) ((Map) herdr.lastCall("agent.start").params()).get("args"); + } + + private static String startedName(FakeHerdr herdr) { + return (String) ((Map) herdr.lastCall("agent.start").params()).get("name"); + } + + @SuppressWarnings("unchecked") + private static Map tabEnv(FakeHerdr herdr) { + return (Map) ((Map) herdr.lastCall("tab.create").params()).get("env"); + } + + // ── it starts a lead when none is live ──────────────────────────────────────────────────── + + @Test + void startsTheDeclaredLeadWhenNoneIsRunning() { + FakeHerdr herdr = new FakeHerdr(); + + assertEquals(1, launcher(herdr, configWith(lead("opus", null, 1))).ensureLeads()); + assertTrue(herdr.called("agent.start"), "a lead must actually be started"); + assertEquals("lead-opus", startedName(herdr)); + } + + /** The tab is labelled so the scanner finds the lead on the next resolve. */ + @Test + void labelsTheTabWithThePrefixTheScannerReadsBack() { + FakeHerdr herdr = new FakeHerdr(); + launcher(herdr, configWith(lead("opus", null, 1))).ensureLeads(); + + assertEquals("lead: opus", + ((Map) herdr.lastCall("tab.rename").params()).get("label")); + } + + /** `instances: 2` with none live means two starts, not one. */ + @Test + void startsAsManyInstancesAsAreDeclared() { + FakeHerdr herdr = new FakeHerdr(); + + assertEquals(2, launcher(herdr, configWith(lead("opus", null, 2))).ensureLeads()); + assertEquals(2, herdr.calls.stream().filter(c -> c.method().equals("agent.start")).count()); + } + + // ── it must not double-spawn ────────────────────────────────────────────────────────────── + + /** A labelled tab WITH a running agent in it is a live lead — leave it alone. */ + @Test + void doesNotStartASecondLeadWhenOneIsAlreadyRunning() { + FakeHerdr herdr = new FakeHerdr() + .withWorkspace("wL", "leads") + .withTab("wL", "wL:t1", "lead: opus") + .withAgent("lead-opus", "term_lead", "wL:p1", "wL:t1"); + + assertEquals(0, launcher(herdr, configWith(lead("opus", null, 1))).ensureLeads()); + assertFalse(herdr.called("agent.start"), "the live lead must not be duplicated"); + } + + /** + * The reason liveness is not "does the label exist". A tab left labelled by a session that has + * since died must not block the relaunch, or one crash disables auto-launch permanently. + */ + @Test + void aLabelledTabWithNoRunningAgentIsNotALiveLead() { + FakeHerdr herdr = new FakeHerdr() + .withWorkspace("wL", "leads") + .withTab("wL", "wL:t1", "lead: opus"); // label only — nothing running in it + + assertEquals(1, launcher(herdr, configWith(lead("opus", null, 1))).ensureLeads(), + "a stale label is not a lead; the lead must be relaunched"); + } + + /** + * A lead the operator opened by hand and pinned with `terminal:` is live even though its tab + * carries no matching label. Counting labels alone would relaunch it on every boot. + */ + @Test + void aPinnedTerminalWithARunningAgentCountsAsLive() { + FakeHerdr herdr = new FakeHerdr() + .withAgent("hand-opened", "term_pinned", "wX:p1", "wX:t1"); + + assertEquals(0, launcher(herdr, configWith(lead("opus", "term_pinned", 1))).ensureLeads()); + assertFalse(herdr.called("agent.start")); + } + + /** A member sitting in a matching tab must never be counted — or spawn a lead — as one. */ + @Test + void aMemberWorkspaceIsNeverScannedForLeads() { + FakeHerdr herdr = new FakeHerdr() + .withWorkspace("wM", "bridged-workers") // a configured member space + .withTab("wM", "wM:t1", "lead: opus") // a member tab that looks like a lead + .withAgent("claude-opus-x", "term_m", "wM:p1", "wM:t1"); + + assertEquals(1, launcher(herdr, configWith(lead("opus", null, 1))).ensureLeads(), + "a member in a lead-labelled tab is not a lead, so the real lead is still missing"); + } + + /** If herdr cannot be counted, start nothing: guessing risks a second orchestrator. */ + @Test + void anUncountableHerdrStartsNothing() { + FakeHerdr herdr = new FakeHerdr().healthy(false); + + assertEquals(0, launcher(herdr, configWith(lead("opus", null, 1))).ensureLeads()); + assertFalse(herdr.called("agent.start")); + } + + // ── what it starts is a LEAD, not a member ──────────────────────────────────────────────── + + /** + * The single most important assertion here. The worker charter tells its reader it is an + * off-subscription worker that must end every turn with bridge_reply — the opposite of what an + * orchestrator is. A lead must never receive it. + */ + @Test + void theLeadNeverReceivesTheWorkerReplyCharter() { + FakeHerdr herdr = new FakeHerdr(); + launcher(herdr, configWith(lead("opus", null, 1))).ensureLeads(); + + List args = startedArgs(herdr); + assertFalse(args.contains("--append-system-prompt"), + "the reply charter is a worker contract and must not be injected into a lead"); + assertTrue(args.stream().noneMatch(a -> a.contains("bridge_reply")), args.toString()); + } + + /** It still mounts the bridge — a lead that cannot orchestrate is pointless. */ + @Test + void theLeadMountsTheBridgeMcpAndPinsItsModel() { + FakeHerdr herdr = new FakeHerdr(); + launcher(herdr, configWith(lead("opus", null, 1))).ensureLeads(); + + List args = startedArgs(herdr); + assertTrue(args.contains("--mcp-config")); + assertTrue(args.stream().anyMatch(a -> a.contains("http://127.0.0.1:8765/mcp")), args.toString()); + assertEquals("claude-opus-5", args.get(args.indexOf("--model") + 1)); + assertTrue(args.indexOf("--model") > args.indexOf("--mcp-config"), + "--model is appended last so it outranks the ccs wrapper (CB-533)"); + } + + /** A lead runs on the operator's subscription. Nothing may move it off. */ + @Test + void theLeadEnvCarriesNoAnthropicBinding() { + FakeHerdr herdr = new FakeHerdr(); + launcher(herdr, configWith(lead("opus", null, 1))).ensureLeads(); + + Map env = tabEnv(herdr); + assertNull(env.get("ANTHROPIC_BASE_URL")); + assertNull(env.get("ANTHROPIC_AUTH_TOKEN")); + assertEquals("300000", env.get("CLAUDE_CODE_AUTO_COMPACT_WINDOW"), + "the profile's own env: still applies"); + } + + /** The lead's tab goes in its own workspace, never a member one — the scanner skips those. */ + @Test + void theLeadTabIsCreatedOutsideEveryMemberWorkspace() { + FakeHerdr herdr = new FakeHerdr(); + launcher(herdr, configWith(lead("opus", null, 1))).ensureLeads(); + + String label = (String) ((Map) herdr.lastCall("workspace.create").params()).get("label"); + assertEquals("leads", label); + assertNotEquals("bridged-workers", label); + } + + // ── recognise-only and misconfiguration ─────────────────────────────────────────────────── + + /** A lead with a pin but no profile is recognise-only by design — not an error, not a launch. */ + @Test + void aLeadThatNamesNoProfileIsRecognisedButNeverLaunched() { + FakeHerdr herdr = new FakeHerdr(); + + assertEquals(0, launcher(herdr, configWith(lead(null, "term_dead", 1))).ensureLeads()); + assertFalse(herdr.called("agent.start")); + } + + /** `instances: 0` is a deliberate off switch. */ + @Test + void zeroInstancesLaunchesNothing() { + FakeHerdr herdr = new FakeHerdr(); + + assertEquals(0, launcher(herdr, configWith(lead("opus", null, 0))).ensureLeads()); + assertFalse(herdr.called("agent.start")); + } + + /** A profile name with no matching profile is logged and skipped, never a daemon crash. */ + @Test + void anUnknownProfileIsSkippedRatherThanThrown() { + FakeHerdr herdr = new FakeHerdr(); + + assertEquals(0, launcher(herdr, configWith(lead("nope", null, 1))).ensureLeads()); + assertFalse(herdr.called("agent.start")); + } + + /** No leads declared at all: not a herdr call in sight. */ + @Test + void noLeadersConfiguredTouchesHerdrNotAtAll() { + FakeHerdr herdr = new FakeHerdr(); + BridgedConfig cfg = new BridgedConfig( + null, null, Map.of("opus", opusProfile()), null, null, null, null, null, + null, null, null, null, "fixed", null).withDefaults(); + + assertEquals(0, launcher(herdr, cfg).ensureLeads()); + assertTrue(herdr.calls.isEmpty(), "nothing declared ⇒ nothing scanned"); + } +} From a2108a8a14cdad4e7fface7b138dd24c91e18171 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 14 Aug 2026 18:23:32 +0200 Subject: [PATCH 4/4] CB-559: re-read bridged.yaml without restarting the daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tuning a fleet meant restarting bridged, and a restart tears down every lead and worker it owns. Changing one pool's weight cost the whole fleet's state, so in practice nobody changed it. ConfigRef holds the live BridgedConfig in an AtomicReference. Consumers read it at the point of use, so a change reaches the next spawn with nothing rebuilt. The launchers that used to capture config into fields now take suppliers: the fleet tabLabel template, the profile map, the placement policy and the fleet block. Keys fall into three classes, and the difference is what already exists when the reload happens: hot fleet: (pools + tabLabel), placement:, and an existing profile's weight / maxLoad / model / tabLabel — live on the next spawn. deferred lifecycle:, leadHeartbeat:, guard:, worktreeRoot:, spawnReady*, and adding/removing a profile — accepted, but the startup wiring keeps the old value. The reload logs these by name. cold bind:, herdrSocket:, broker:, auth: — refuses the WHOLE reload. A cold change refuses everything rather than applying the hot half. A half-applied reload leaves the daemon matching no file on disk, which is the worst thing a reload can do to an operator reading that file to work out what the daemon is doing. Refusing keeps the invariant that the live config is always some version of the file. A parse failure or a failed startup validator is refused the same way, and the running config stays live: a file being saved is sometimes read mid-write, and degrading a working daemon over a half-written file is a bad trade. The same four validators startup runs are re-run, so a config that could not have booted cannot slip in through a reload. ConfigWatcher polls the modified time on a daemon thread, opt-in through configReload.enabled (default off, so an upgraded daemon is unchanged). It stamps the timestamp BEFORE reloading, so a refused file is not retried every tick — the next save earns a fresh attempt. A missing file is skipped silently, because editors unlink briefly mid-save. MicroProfile Config was the first idea and does not fit: @ConfigMapping needs interfaces, resolves once at bootstrap, and reload would still mean rebuild and swap. The port would also lose the raw-YAML duplicate-key detection, since duplicates have already collapsed once the tree is flattened to properties. 634 tests. --- bridged/bridged.example.yaml | 28 ++ .../main/java/dev/ltms/bridged/Bridged.java | 29 +- .../ltms/bridged/config/BridgedConfig.java | 44 ++- .../dev/ltms/bridged/config/ConfigRef.java | 219 +++++++++++++++ .../ltms/bridged/config/ConfigWatcher.java | 96 +++++++ .../bridged/member/ClaudeCodeLauncher.java | 5 +- .../bridged/member/CompositePeerLauncher.java | 88 ++++-- .../bridged/member/HerdrPeerLauncher.java | 21 +- .../ltms/bridged/member/OpenCodeLauncher.java | 5 +- .../ltms/bridged/config/ConfigRefTest.java | 256 ++++++++++++++++++ .../bridged/config/ConfigWatcherTest.java | 162 +++++++++++ .../member/ClaudeCodeLauncherTest.java | 30 +- 12 files changed, 942 insertions(+), 41 deletions(-) create mode 100644 bridged/src/main/java/dev/ltms/bridged/config/ConfigRef.java create mode 100644 bridged/src/main/java/dev/ltms/bridged/config/ConfigWatcher.java create mode 100644 bridged/src/test/java/dev/ltms/bridged/config/ConfigRefTest.java create mode 100644 bridged/src/test/java/dev/ltms/bridged/config/ConfigWatcherTest.java diff --git a/bridged/bridged.example.yaml b/bridged/bridged.example.yaml index 49a88dc..9e18d9f 100644 --- a/bridged/bridged.example.yaml +++ b/bridged/bridged.example.yaml @@ -223,6 +223,34 @@ profiles: # round-robin, or weighted. Omitting this key is a strict no-op for existing configs. placement: weighted +# Re-read this file without restarting the daemon (CB-559). Off unless you add this block, so an +# upgraded bridged keeps the old behaviour: the file is read once at boot and never again. +# enabled → turn the watch on. bridged checks the file's modified time on a timer and +# reloads when it moves. +# intervalSeconds → how often to check (default 10). One `stat` per tick, so this is cheap. +# +# Not every key can move under a running daemon, and the difference is about what already exists +# when the reload happens — not about how important the key is: +# HOT → takes effect on the next spawn: the whole `fleet:` block (every role pool and +# `tabLabel`), `placement:`, and an existing profile's weight / maxLoad / model / +# tabLabel. +# DEFERRED → accepted into the new config, but the wiring built at startup keeps the old value +# until you restart: `lifecycle:`, `leadHeartbeat:`, `guard:`, `worktreeRoot:`, +# `spawnReadyTimeoutMs` / `spawnReadyPollMs`, and ADDING or REMOVING a profile (a new +# backend needs its own launcher, and launchers are built once). The reload logs +# these by name rather than pretending they applied. +# COLD → cannot change at all: `bind:`, `herdrSocket:`, `broker:` and `auth:`. The socket is +# bound, the broker connection is open, and the auth mode decides who may reach the +# port that is already listening. +# +# A changed COLD key refuses the WHOLE reload — not the hot half applied and the cold half warned +# about. A half-applied reload would leave the daemon matching no file on disk, which is the worst +# thing a reload can do to an operator debugging one. A file that fails to parse or fails a startup +# validator is refused the same way, and the running config stays live. +# configReload: +# enabled: true +# intervalSeconds: 10 + # THE FLEET (CB-557) — who the daemon may run, and under which role. This one block replaced four # older keys: `leaders:`, `members:`, `leadScan:` and `defaultProfile:`. # diff --git a/bridged/src/main/java/dev/ltms/bridged/Bridged.java b/bridged/src/main/java/dev/ltms/bridged/Bridged.java index 31678a9..29ea236 100644 --- a/bridged/src/main/java/dev/ltms/bridged/Bridged.java +++ b/bridged/src/main/java/dev/ltms/bridged/Bridged.java @@ -1,6 +1,8 @@ package dev.ltms.bridged; import dev.ltms.bridged.config.BridgedConfig; +import dev.ltms.bridged.config.ConfigRef; +import dev.ltms.bridged.config.ConfigWatcher; import dev.ltms.bridged.guard.SubscriptionGuard; import dev.ltms.bridged.herdr.AgentControl; import dev.ltms.bridged.herdr.HerdrClient; @@ -37,7 +39,6 @@ import dev.ltms.bridged.session.SessionManager; import dev.ltms.bridged.peer.PeerLauncher; import dev.ltms.bridged.session.SessionReaper; import dev.ltms.bridged.member.ClaudeCodeLauncher; -import dev.ltms.bridged.placement.PlacementPolicies; import dev.ltms.bridged.member.CompositePeerLauncher; import dev.ltms.bridged.member.HerdrPeerLauncher; import dev.ltms.bridged.member.OpenCodeLauncher; @@ -79,6 +80,11 @@ public final class Bridged { static void main(String[] args) { Path configPath = Path.of(args.length > 0 ? args[0] : "bridged.yaml"); BridgedConfig cfg = BridgedConfig.load(configPath); + // CB-559: `cfg` stays the startup snapshot — every validation and every piece of one-time + // wiring below reads it, and must, because those decisions cannot be unmade. `config` is the + // live reference the hot paths read per use. Which keys can actually move is ConfigRef's + // contract; adding a reader here does not make a key reloadable by itself. + ConfigRef config = new ConfigRef(configPath, cfg); // The primary/host env that launched bridged must not be tainted. SubscriptionGuard guard = new SubscriptionGuard(cfg.guard().hostSet()); @@ -125,22 +131,20 @@ public final class Bridged { adapters.add(new ClaudeCodeLauncher(agents, spaces, guard, claudeProfiles, cfg.effectiveDefaultProfile(), System::getenv, cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs(), - cfg.fleet().tabLabel())); + () -> config.get().fleet().tabLabel())); } if (!opencodeProfiles.isEmpty()) { adapters.add(new OpenCodeLauncher(agents, spaces, opencodeProfiles, cfg.effectiveDefaultProfile(), System::getenv, cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs(), - cfg.fleet().tabLabel())); + () -> config.get().fleet().tabLabel())); } AtomicReference> liveCountRef = new AtomicReference<>(_ -> 0); PeerLauncher workers = new CompositePeerLauncher( adapters, cfg.effectiveDefaultProfile(), - cfg.profiles(), - PlacementPolicies.fromName(cfg.placement()), - profileName -> liveCountRef.get().apply(profileName), - cfg.fleet()); + config, + profileName -> liveCountRef.get().apply(profileName)); // 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 @@ -384,6 +388,16 @@ public final class Bridged { BridgeMcp mcp = new BridgeMcp(messages, workers, sessions, identity, presence, primaryRegistry, callers, metrics); + // CB-559: opt-in config reload. With no `configReload:` block nothing is constructed, so an + // upgraded daemon behaves exactly as before — the file is read once at boot and never again. + final ConfigWatcher configWatcher; + if (cfg.configReload() != null && cfg.configReload().isEnabled()) { + configWatcher = new ConfigWatcher(config, cfg.configReload().intervalSeconds()); + configWatcher.start(); + } else { + configWatcher = null; + } + // CB-303 part 3: single ordered shutdown hook. Drain sessions first while herdr is still // open (so releases reach the daemon), then stop poller/message/mcp/reaper, and close herdr // last. This replaces the earlier independent hooks that could race and close herdr early. @@ -393,6 +407,7 @@ public final class Bridged { messages.close(); pushLoop.close(); if (heartbeat != null) heartbeat.close(); // CB-551: stop the idle-lead heartbeat scheduler + if (configWatcher != null) configWatcher.stop(); // CB-559: stop polling the config file mcp.close(); if (reaper != null) reaper.stop(); // Release the broker connection last among message resources (no-op for the in-memory inbox). diff --git a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java index b513bc8..236c675 100644 --- a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java +++ b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java @@ -74,7 +74,17 @@ public record BridgedConfig( Fleet fleet, LeadHeartbeat leadHeartbeat, String placement, - Auth auth) { + Auth auth, + ConfigReload configReload) { + + /** Back-compat 14-arg form — no {@code configReload:} block, so file watching stays off. */ + public BridgedConfig(Bind bind, String herdrSocket, Map profiles, Guard guard, + String worktreeRoot, Lifecycle lifecycle, Integer spawnReadyTimeoutMs, + Integer spawnReadyPollMs, Broker broker, Primary primary, Fleet fleet, + LeadHeartbeat leadHeartbeat, String placement, Auth auth) { + this(bind, herdrSocket, profiles, guard, worktreeRoot, lifecycle, spawnReadyTimeoutMs, + spawnReadyPollMs, broker, primary, fleet, leadHeartbeat, placement, auth, null); + } /** * Normalize {@code profiles} once, at construction, so every reader sees the same map. @@ -622,6 +632,31 @@ public record BridgedConfig( } } + /** + * Watch {@code bridged.yaml} and re-read it when it changes (CB-559). + * + *

Opt-in, like every other block that acts on its own initiative. A daemon that reloads + * whenever a file is saved would apply a half-finished edit the moment an editor writes it, and + * an operator who did not ask for that has no reason to expect it. Absent block = off, and the + * config is read exactly once at startup as it always was. + * + *

Which keys a reload can actually change — and which refuse it — is + * {@link ConfigRef}'s contract, not this block's. This only decides when to look. + * + * @param enabled false (or an absent block) leaves the startup-only behaviour + * @param intervalSeconds how often the file's modified time is checked; defaults to 10 + */ + public record ConfigReload(Boolean enabled, Integer intervalSeconds) { + public ConfigReload { + enabled = enabled != null && enabled; + intervalSeconds = (intervalSeconds == null || intervalSeconds <= 0) ? 10 : intervalSeconds; + } + + public boolean isEnabled() { + return Boolean.TRUE.equals(enabled); + } + } + /** * The terminal → lead-name map that {@link dev.ltms.bridged.auth.CallerResolver} resolves * against, merging the {@code leaders:} registry with the legacy singular {@code primary:} pin. @@ -749,7 +784,7 @@ public record BridgedConfig( private static final Set KNOWN_TOP_LEVEL_KEYS = Set.of( "bind", "herdrSocket", "profiles", "guard", "worktreeRoot", "lifecycle", "spawnReadyTimeoutMs", "spawnReadyPollMs", "broker", "primary", "fleet", - "leadHeartbeat", "placement", "auth"); + "leadHeartbeat", "placement", "auth", "configReload"); /** Load and validate config from {@code path}. */ public static BridgedConfig load(Path path) { @@ -1015,8 +1050,11 @@ public record BridgedConfig( // leadHeartbeat is left as-is (CB-551): null is "off", and LeadHeartbeat's own compact // constructor defaults the fields of a block that IS present. Defaulting it here would // switch the feature on for every config that never mentioned it. + // configReload is left as-is: null is "off", and ConfigReload's own compact constructor + // defaults the fields of a block that IS present. Defaulting it here would start watching + // the file for every config that never asked to be watched. return new BridgedConfig(b, herdrSocket, profiles, g, worktreeRoot, l, timeout, pollMs, - broker, primary, f, leadHeartbeat, placementOrDefault, a); + broker, primary, f, leadHeartbeat, placementOrDefault, a, configReload); } /** diff --git a/bridged/src/main/java/dev/ltms/bridged/config/ConfigRef.java b/bridged/src/main/java/dev/ltms/bridged/config/ConfigRef.java new file mode 100644 index 0000000..1750234 --- /dev/null +++ b/bridged/src/main/java/dev/ltms/bridged/config/ConfigRef.java @@ -0,0 +1,219 @@ +package dev.ltms.bridged.config; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; + +/** + * The daemon's live configuration, re-readable without a restart (CB-559). + * + *

Consumers hold this, not a {@link BridgedConfig}, and read through {@link #get()} at the point + * of use. A component that captures {@code ref.get()} into a field at construction has opted out of + * reload — which is sometimes right (see deferred below), but it must then be a deliberate + * choice rather than an accident of where the field was initialised. + * + *

Not every key can change under a running daemon

+ * Keys fall into three classes, and the difference is about what already exists when the reload + * happens — not about how important the key is. + * + *
    + *
  • Hot — re-read per use, so a reload takes effect on the next spawn: + * {@code fleet:} (every role pool and {@code tabLabel}), {@code placement:}, and an existing + * profile's {@code weight} / {@code maxLoad} / {@code model} / {@code tabLabel}.
    • + *
  • Deferred — accepted into the new snapshot, but the wiring built at startup + * keeps the old value until a restart: {@code lifecycle:}, {@code leadHeartbeat:}, + * {@code spawnReadyTimeoutMs} / {@code spawnReadyPollMs}, {@code guard:}, + * {@code worktreeRoot:}, and adding or removing a profile (a new backend needs its + * own launcher, which is constructed once). A reload logs these rather than pretending they + * applied.
    • + *
  • Cold — cannot change at all under a running daemon: {@code bind:}, + * {@code herdrSocket:}, {@code broker:} and {@code auth:}. The socket is bound, the broker + * connection is open, and the auth mode decides who may reach the port that is already + * listening.
    • + *
+ * + *

A cold change refuses the whole reload. Not the hot half applied and the cold + * half warned about: that would leave the running daemon in a state matching no file on disk, which + * is the worst thing a reload can do to an operator debugging one. Refusing keeps the invariant that + * the live config is always some version of the file, and the message names the keys that must + * change through a restart. + * + *

A reload that fails to parse or fails validation is also refused, and the previous config keeps + * running. A config file being edited is normally read once mid-save; degrading a working daemon + * because it caught a half-written file would be a bad trade. + */ +public final class ConfigRef implements Supplier { + + private static final Logger log = LoggerFactory.getLogger(ConfigRef.class); + + /** Keys that cannot change under a running daemon — see the class doc. */ + private static final Set COLD_KEYS = + Set.of("bind", "herdrSocket", "broker", "auth"); + + private final Path path; + private final AtomicReference current; + + public ConfigRef(Path path, BridgedConfig initial) { + this.path = path; + this.current = new AtomicReference<>(Objects.requireNonNull(initial, "initial config")); + } + + /** A fixed reference that never reloads — for tests and for wiring built from a config in code. */ + public static ConfigRef fixed(BridgedConfig cfg) { + return new ConfigRef(null, cfg); + } + + /** The live configuration. Read this per use; do not cache it in a field. */ + @Override + public BridgedConfig get() { + return current.get(); + } + + /** The file this ref reloads from, or {@code null} for a {@link #fixed} ref. */ + public Path path() { + return path; + } + + /** + * What a reload attempt did. + * + * @param applied true when the new config is now live + * @param coldKeys cold keys whose value changed, which is why an unapplied reload was refused + * @param deferred keys that changed and were accepted, but whose effect waits for a restart + * @param error the parse or validation failure that refused the reload, else {@code null} + */ + public record Outcome(boolean applied, List coldKeys, List deferred, + String error) { + + public Outcome { + coldKeys = List.copyOf(coldKeys); + deferred = List.copyOf(deferred); + } + + static Outcome refusedCold(List keys) { + return new Outcome(false, keys, List.of(), null); + } + + static Outcome failed(String error) { + return new Outcome(false, List.of(), List.of(), error); + } + + /** A one-line summary for the operator — the reason, not just the verdict. */ + public String summary() { + if (error != null) { + return "config reload refused — " + error; + } + if (!applied) { + return "config reload refused — these keys cannot change under a running daemon: " + + String.join(", ", coldKeys) + ". Restart bridged to apply them."; + } + if (!deferred.isEmpty()) { + return "config reloaded; these changes need a restart to take effect: " + + String.join(", ", deferred); + } + return "config reloaded"; + } + } + + /** + * Re-read the file, validate it, and swap it in when nothing cold changed. + * + *

Never throws: a reload is a best-effort operation on a daemon that is already serving, and + * a bad edit must not take it down. Every failure path leaves the previous config live and is + * reported through the returned {@link Outcome}. + */ + public Outcome reload() { + if (path == null) { + return Outcome.failed("this config was built in code and has no file to reload from"); + } + BridgedConfig old = current.get(); + BridgedConfig fresh; + try { + fresh = BridgedConfig.load(path); + // The same gate startup runs. A config that would have refused to boot must not be able + // to slip in through a reload — that is how a daemon ends up in a state it could never + // have started in, which is the hardest kind to debug. + fresh.validateAuthExposure(); + fresh.validateLeadTabPrefixes(); + fresh.validateSubscriptionProfiles(); + fresh.validateMembers(); + } catch (RuntimeException e) { + String msg = e.getMessage() == null ? e.toString() : e.getMessage(); + log.warn("config reload from {} refused, keeping the running config: {}", path, msg); + return Outcome.failed(msg); + } + + List cold = changedColdKeys(old, fresh); + if (!cold.isEmpty()) { + Outcome out = Outcome.refusedCold(cold); + log.warn(out.summary()); + return out; + } + + List deferred = changedDeferredKeys(old, fresh); + current.set(fresh); + Outcome out = new Outcome(true, List.of(), deferred, null); + log.info(out.summary()); + return out; + } + + /** Cold keys whose value differs between the running config and the candidate. */ + private static List changedColdKeys(BridgedConfig old, BridgedConfig fresh) { + List changed = new ArrayList<>(); + if (!Objects.equals(old.bind(), fresh.bind())) { + changed.add("bind"); + } + if (!Objects.equals(old.herdrSocket(), fresh.herdrSocket())) { + changed.add("herdrSocket"); + } + if (!Objects.equals(old.broker(), fresh.broker())) { + changed.add("broker"); + } + if (!Objects.equals(old.auth(), fresh.auth())) { + changed.add("auth"); + } + // Kept in step with COLD_KEYS so the doc and the code cannot drift apart silently. + assert COLD_KEYS.containsAll(changed) : "a cold key was reported that COLD_KEYS omits"; + return changed; + } + + /** Changed keys that were accepted but whose effect waits for a restart. */ + private static List changedDeferredKeys(BridgedConfig old, BridgedConfig fresh) { + List changed = new ArrayList<>(); + if (!Objects.equals(old.lifecycle(), fresh.lifecycle())) { + changed.add("lifecycle"); + } + if (!Objects.equals(old.leadHeartbeat(), fresh.leadHeartbeat())) { + changed.add("leadHeartbeat"); + } + if (!Objects.equals(old.guard(), fresh.guard())) { + changed.add("guard"); + } + if (!Objects.equals(old.worktreeRoot(), fresh.worktreeRoot())) { + changed.add("worktreeRoot"); + } + if (!Objects.equals(old.spawnReadyTimeoutMs(), fresh.spawnReadyTimeoutMs()) + || !Objects.equals(old.spawnReadyPollMs(), fresh.spawnReadyPollMs())) { + changed.add("spawnReady*"); + } + // Only the profile SET is deferred: a new backend needs a launcher, and launchers are built + // once at startup. An existing profile's fields are read per spawn and so are hot. + Set before = old.profiles() == null ? Set.of() : old.profiles().keySet(); + Set after = fresh.profiles() == null ? Set.of() : fresh.profiles().keySet(); + if (!before.equals(after)) { + Set diff = new LinkedHashSet<>(before); + diff.addAll(after); + diff.removeIf(p -> before.contains(p) && after.contains(p)); + changed.add("profiles (added/removed: " + String.join(", ", diff) + ")"); + } + return changed; + } +} diff --git a/bridged/src/main/java/dev/ltms/bridged/config/ConfigWatcher.java b/bridged/src/main/java/dev/ltms/bridged/config/ConfigWatcher.java new file mode 100644 index 0000000..7cde2d2 --- /dev/null +++ b/bridged/src/main/java/dev/ltms/bridged/config/ConfigWatcher.java @@ -0,0 +1,96 @@ +package dev.ltms.bridged.config; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; + +/** + * Polls {@code bridged.yaml}'s modified time and asks {@link ConfigRef} to reload when it moves + * (CB-559). Opt-in through {@code configReload.enabled}. + * + *

Why polling and not a filesystem watch. {@code WatchService} on macOS has no + * native backend — it falls back to polling internally anyway, at an interval this code does not + * control — and editors save config files in ways that produce a different event mix per editor + * (write-in-place, write-and-rename, write-temp-and-swap). A modified-time check treats all of them + * the same and is a single {@code stat} per tick, which at a ten-second cadence costs nothing worth + * measuring. + * + *

A missing or unreadable file is not a reason to act. Many editors briefly + * unlink the file during a save. Reloading on "it vanished" would mean reloading from a file that no + * longer exists; reporting an error every tick would bury the log. So an unreadable file is skipped + * silently and the next tick tries again — the running config stays live, which is the correct + * outcome either way. + */ +public final class ConfigWatcher { + + private static final Logger log = LoggerFactory.getLogger(ConfigWatcher.class); + + private final ConfigRef ref; + private final long intervalSeconds; + private final ScheduledExecutorService scheduler; + + private volatile long lastSeenMillis; + + public ConfigWatcher(ConfigRef ref, long intervalSeconds) { + this.ref = ref; + this.intervalSeconds = intervalSeconds; + this.lastSeenMillis = modifiedMillis(ref.path()); + this.scheduler = Executors.newSingleThreadScheduledExecutor(r -> { + Thread t = new Thread(r, "config-watcher"); + // A daemon thread: an operator's config watch must never be the reason the JVM refuses + // to exit after everything else has shut down. + t.setDaemon(true); + return t; + }); + } + + /** Begin watching. A ref with no file (a fixed one) is a no-op rather than an error. */ + public void start() { + if (ref.path() == null) { + log.debug("config watch not started — this config has no file behind it"); + return; + } + scheduler.scheduleWithFixedDelay(this::tick, intervalSeconds, intervalSeconds, + TimeUnit.SECONDS); + log.info("config watch: {} re-read when it changes (every {}s)", ref.path(), intervalSeconds); + } + + /** One poll. Never throws — an exception here would silently cancel the schedule. */ + void tick() { + try { + long now = modifiedMillis(ref.path()); + if (now == 0 || now == lastSeenMillis) { + return; + } + // Stamp BEFORE reloading. A file whose reload is refused (a bad edit, or a cold key) + // must not be retried every tick — that would log the same refusal forever. The next + // save moves the timestamp again and earns a fresh attempt. + lastSeenMillis = now; + ref.reload(); + } catch (RuntimeException e) { + log.warn("config watch tick failed, still watching: {}", e.getMessage()); + } + } + + private static long modifiedMillis(Path path) { + if (path == null) { + return 0; + } + try { + return Files.getLastModifiedTime(path).toMillis(); + } catch (IOException e) { + return 0; // mid-save, or gone: say nothing and try again next tick + } + } + + /** Stop polling. Called from the daemon's ordered shutdown hook, alongside the other loops. */ + public void stop() { + scheduler.shutdownNow(); + } +} diff --git a/bridged/src/main/java/dev/ltms/bridged/member/ClaudeCodeLauncher.java b/bridged/src/main/java/dev/ltms/bridged/member/ClaudeCodeLauncher.java index 1d66286..5d68d7f 100644 --- a/bridged/src/main/java/dev/ltms/bridged/member/ClaudeCodeLauncher.java +++ b/bridged/src/main/java/dev/ltms/bridged/member/ClaudeCodeLauncher.java @@ -16,6 +16,7 @@ import java.util.Set; import java.util.UUID; import java.util.function.Function; import java.util.function.LongSupplier; +import java.util.function.Supplier; /** * The {@link HerdrPeerLauncher} adapter for Claude Code — the safe path from a @@ -92,7 +93,7 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { Map profiles, String defaultProfile, Function env, long spawnReadyTimeoutMs, long spawnReadyPollMs, - String tabLabelTemplate) { + Supplier tabLabelTemplate) { this(agents, spaces, guard, profiles, defaultProfile, env, spawnReadyTimeoutMs, System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs), @@ -136,7 +137,7 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { Function env, long spawnReadyTimeoutMs, LongSupplier nowMillis, Runnable sleeper, - String tabLabelTemplate) { + Supplier tabLabelTemplate) { super(NAME_PREFIX, agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, nowMillis, sleeper, tabLabelTemplate); this.guard = guard; 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 a8866d5..605e0c2 100644 --- a/bridged/src/main/java/dev/ltms/bridged/member/CompositePeerLauncher.java +++ b/bridged/src/main/java/dev/ltms/bridged/member/CompositePeerLauncher.java @@ -26,6 +26,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; +import java.util.function.Supplier; /** * The {@link PeerLauncher} the core actually holds when more than one adapter is configured — a thin @@ -65,16 +66,24 @@ public final class CompositePeerLauncher implements PeerLauncher { /** paneId → the delegate that spawned it, so {@link #stop} tears down through the right adapter. */ private final Map spawnedBy = new ConcurrentHashMap<>(); - private final Map profileConfigs; - 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. + * CB-559: the placement inputs are read per spawn, not captured at construction, so a + * config reload changes where the next member lands without a restart. These are the hot keys — + * role pools, an existing profile's weight/maxLoad, and the placement policy. What cannot change + * this way is the set of adapters ({@link #byProfile}), because a new backend needs a launcher + * and launchers are built once; {@code ConfigRef} classifies that as deferred and says so. */ - private final BridgedConfig.Fleet fleet; + private final Supplier> profileConfigs; + private final Supplier placementPolicy; + + /** + * CB-557: the role pools an unqualified spawn draws its candidates from. A supplier that yields + * {@code null}, and an empty pool for a role, both fall back to every configured profile — the + * pre-CB-557 behaviour. + */ + private final Supplier fleet; /** * Backward-compatible constructor: fixed placement, no live-counting. Use this for tests and @@ -121,16 +130,45 @@ public final class CompositePeerLauncher implements PeerLauncher { PlacementPolicy placementPolicy, Function liveCount, BridgedConfig.Fleet fleet) { + // LinkedHashMap, not Map.copyOf: candidates() promises definition order and the weighted + // policy breaks exact-weight ties on it, so a salted iteration order would make placement + // differ from one JVM run to the next. + this(delegates, defaultProfile, + constant(Collections.unmodifiableMap(new LinkedHashMap<>(profileConfigs))), + constant(placementPolicy), liveCount, constant(fleet)); + } + + /** + * Production constructor that re-reads its placement inputs per spawn (CB-559), so a config + * reload retargets the next member without a restart. + * + * @param config the live configuration — read at every spawn, never captured + */ + public CompositePeerLauncher(List delegates, + String defaultProfile, + Supplier config, + Function liveCount) { + this(delegates, defaultProfile, + () -> config.get().profiles(), + () -> PlacementPolicies.fromName(config.get().placement()), + liveCount, + () -> config.get().fleet()); + } + + /** The all-suppliers form every other constructor funnels into. */ + private CompositePeerLauncher(List delegates, + String defaultProfile, + Supplier> profileConfigs, + Supplier placementPolicy, + Function liveCount, + Supplier fleet) { this.fleet = fleet; if (delegates.isEmpty()) { throw new IllegalArgumentException("at least one peer adapter must be configured"); } this.delegates = List.copyOf(delegates); this.defaultProfile = defaultProfile; - // LinkedHashMap, not Map.copyOf: candidates() promises definition order and the weighted - // policy breaks exact-weight ties on it, so a salted iteration order would make placement - // differ from one JVM run to the next. - this.profileConfigs = Collections.unmodifiableMap(new LinkedHashMap<>(profileConfigs)); + this.profileConfigs = profileConfigs; this.placementPolicy = placementPolicy; this.liveCount = liveCount; Map index = new LinkedHashMap<>(); @@ -147,6 +185,22 @@ public final class CompositePeerLauncher implements PeerLauncher { this.byProfile = Collections.unmodifiableMap(index); } + /** A supplier of a value fixed at construction — how the non-reloading constructors funnel in. */ + private static Supplier constant(T value) { + return () -> value; + } + + /** + * The currently-configured profiles, never null. + * + *

Read fresh on every call so a reload is visible; a caller that needs two consistent reads + * takes one local, as {@link #poolFor} does. + */ + private Map profiles0() { + Map m = profileConfigs.get(); + return m == null ? Map.of() : m; + } + /** The adapter owning {@code profileName} (null/blank → the default). Throws on an unknown profile. */ private HerdrPeerLauncher route(String profileName) { String resolved = (profileName == null || profileName.isBlank()) ? defaultProfile : profileName; @@ -191,7 +245,7 @@ public final class CompositePeerLauncher implements PeerLauncher { // 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); + PlacementCandidate chosen = placementPolicy.get().select(ctx); HerdrPeerLauncher d = byProfile.get(chosen.profile()); if (d == null) { @@ -247,7 +301,7 @@ public final class CompositePeerLauncher implements PeerLauncher { private void enforceMaxLoad(String profile) { // Absent config, or a config whose maxLoad normalized to null (non-positive ⇒ unlimited at // load), means no cap — never cap what wasn't configured. - BridgedConfig.Profile cfg = profileConfigs.get(profile); + BridgedConfig.Profile cfg = profiles0().get(profile); Integer cap = (cfg == null) ? null : cfg.maxLoad(); if (cap == null) { return; @@ -268,9 +322,11 @@ public final class CompositePeerLauncher implements PeerLauncher { * 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; + Map configured = profiles0(); + BridgedConfig.Fleet f = fleet.get(); + List pool = (f == null) ? List.of() : f.profilesFor(role); + List known = pool.stream().filter(configured::containsKey).toList(); + return known.isEmpty() ? List.copyOf(configured.keySet()) : known; } /** The profile an unqualified spawn for {@code role} falls back to under {@code fixed} placement. */ @@ -283,7 +339,7 @@ public final class CompositePeerLauncher implements PeerLauncher { private List candidates(MemberRole role) { List out = new ArrayList<>(); for (String name : poolFor(role)) { - BridgedConfig.Profile w = profileConfigs.get(name); + BridgedConfig.Profile w = profiles0().get(name); if (w != null) { out.add(new PlacementCandidate(name, null, w.weight(), w.maxLoad())); } diff --git a/bridged/src/main/java/dev/ltms/bridged/member/HerdrPeerLauncher.java b/bridged/src/main/java/dev/ltms/bridged/member/HerdrPeerLauncher.java index 2ce7d9b..eb27d8a 100644 --- a/bridged/src/main/java/dev/ltms/bridged/member/HerdrPeerLauncher.java +++ b/bridged/src/main/java/dev/ltms/bridged/member/HerdrPeerLauncher.java @@ -29,6 +29,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Function; import java.util.function.LongSupplier; +import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -79,10 +80,15 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { private final AtomicLong nameSeq = new AtomicLong(); // per-peer counter (herdr agent names only) /** - * The {@code fleet.tabLabel} template; {@code null} ⇒ {@link BridgedConfig.Fleet#DEFAULT_TAB_LABEL}. - * A profile's own {@code tabLabel} still overrides it. + * The {@code fleet.tabLabel} template; a {@code null} supplier or a {@code null}/blank value ⇒ + * {@link BridgedConfig.Fleet#DEFAULT_TAB_LABEL}. A profile's own {@code tabLabel} still + * overrides it. + * + *

CB-559: a supplier rather than a String, so a config reload renames the next tab + * without a restart. Existing tabs keep the label they were given — bridged does not rewrite a + * label it already wrote. */ - private final String tabLabelTemplate; + private final Supplier tabLabelTemplate; /** * Tab numbers, counted per {@code role/profile} pair (CB-557). @@ -138,7 +144,8 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { /** * As above, plus the {@code fleet.tabLabel} template (CB-557). * - * @param tabLabelTemplate fleet-wide tab-label template; {@code null}/blank ⇒ + * @param tabLabelTemplate fleet-wide tab-label template, read per spawn (CB-559); {@code null}, + * or a supplier yielding {@code null}/blank ⇒ * {@link BridgedConfig.Fleet#DEFAULT_TAB_LABEL}. A separate constructor * rather than a new parameter on the one above, so every existing call * site keeps the default without an edit. @@ -148,7 +155,7 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { Function env, long spawnReadyTimeoutMs, LongSupplier nowMillis, Runnable sleeper, - String tabLabelTemplate) { + Supplier tabLabelTemplate) { this.tabLabelTemplate = tabLabelTemplate; this.namePrefix = namePrefix; this.agents = agents; @@ -407,7 +414,9 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { // we log and still return it so the caller gets its paneId and can tear it down. tidy("label tab " + tab.tab().tabId(), () -> spaces.renameTab(tab.tab().tabId(), - cfg.renderTabLabel(tabLabelTemplate, role, nextLabelSeq(role, cfg.profile())))); + cfg.renderTabLabel( + tabLabelTemplate == null ? null : tabLabelTemplate.get(), + role, nextLabelSeq(role, cfg.profile())))); log.info("{} started pane={} tab={} terminal={}", namePrefix, started.agent().paneId(), started.agent().tabId(), started.agent().terminalId()); return started.agent(); diff --git a/bridged/src/main/java/dev/ltms/bridged/member/OpenCodeLauncher.java b/bridged/src/main/java/dev/ltms/bridged/member/OpenCodeLauncher.java index cef1d9f..50527ad 100644 --- a/bridged/src/main/java/dev/ltms/bridged/member/OpenCodeLauncher.java +++ b/bridged/src/main/java/dev/ltms/bridged/member/OpenCodeLauncher.java @@ -20,6 +20,7 @@ import java.util.Map; import java.util.Set; import java.util.function.Function; import java.util.function.LongSupplier; +import java.util.function.Supplier; /** * The {@link HerdrPeerLauncher} adapter for opencode — an open-source, @@ -125,7 +126,7 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { Map profiles, String defaultProfile, Function env, long spawnReadyTimeoutMs, long spawnReadyPollMs, - String tabLabelTemplate) { + Supplier tabLabelTemplate) { this(agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs), defaultConfigRoot(), defaultDiscoveryRoot(), tabLabelTemplate); @@ -172,7 +173,7 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { long spawnReadyTimeoutMs, LongSupplier nowMillis, Runnable sleeper, Path configRoot, Path discoveryRoot, - String tabLabelTemplate) { + Supplier tabLabelTemplate) { super(NAME_PREFIX, agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, nowMillis, sleeper, tabLabelTemplate); this.configRoot = configRoot; diff --git a/bridged/src/test/java/dev/ltms/bridged/config/ConfigRefTest.java b/bridged/src/test/java/dev/ltms/bridged/config/ConfigRefTest.java new file mode 100644 index 0000000..abba835 --- /dev/null +++ b/bridged/src/test/java/dev/ltms/bridged/config/ConfigRefTest.java @@ -0,0 +1,256 @@ +package dev.ltms.bridged.config; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * CB-559: re-reading {@code bridged.yaml} under a running daemon. + * + *

The tests that matter here are the refusals. A reload that applies a good file is the easy + * half; the half that protects an operator is the one that keeps the running config when the new + * file is bad, and the one that refuses a change the running daemon cannot honour. + */ +class ConfigRefTest { + + /** A minimal file that loads and passes every startup validator. */ + private static String yaml(String extra) { + return """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet + guard: + offSubscriptionHosts: + - gx00.gw + """ + extra; + } + + private static ConfigRef refFor(Path f) { + return new ConfigRef(f, BridgedConfig.load(f)); + } + + @Test + void aHotChangeIsAppliedAndReadThroughGet(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + Files.writeString(f, yaml(""" + fleet: + tabLabel: "{role}: {profile} #{n}" + developers: + a: + profile: sonnet + """)); + ConfigRef ref = refFor(f); + assertEquals("{role}: {profile} #{n}", ref.get().fleet().tabLabel()); + + Files.writeString(f, yaml(""" + fleet: + tabLabel: "[{profile}] {role}" + developers: + a: + profile: sonnet + """)); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertTrue(out.deferred().isEmpty()); + assertEquals("config reloaded", out.summary()); + assertEquals("[{profile}] {role}", ref.get().fleet().tabLabel()); + } + + /** + * The point of the whole class: a consumer holding the ref sees the new value without being + * rebuilt. A component that captured {@code get()} into a field would still show the old one. + */ + @Test + void aConsumerHoldingTheRefSeesTheNewValue(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + Files.writeString(f, yaml("placement: weighted\n")); + ConfigRef ref = refFor(f); + java.util.function.Supplier reader = () -> ref.get().placement(); + assertEquals("weighted", reader.get()); + + Files.writeString(f, yaml("placement: fixed\n")); + assertTrue(ref.reload().applied()); + + assertEquals("fixed", reader.get()); + } + + @Test + void aChangedColdKeyRefusesTheWholeReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + Files.writeString(f, yaml("placement: weighted\n")); + ConfigRef ref = refFor(f); + + // Two changes in one file: a cold one (the port) and a hot one (placement). + Files.writeString(f, yaml("placement: fixed\n").replace("port: 8765", "port: 9999")); + ConfigRef.Outcome out = ref.reload(); + + assertFalse(out.applied()); + assertEquals(java.util.List.of("bind"), out.coldKeys()); + assertTrue(out.summary().contains("Restart bridged"), out.summary()); + // The hot half must NOT have leaked in. A half-applied reload leaves the daemon matching no + // file on disk, which is worse for an operator than no reload at all. + assertEquals("weighted", ref.get().placement()); + assertEquals(8765, ref.get().bind().port()); + } + + @Test + void aFileThatNoLongerParsesKeepsTheRunningConfig(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + Files.writeString(f, yaml("placement: weighted\n")); + ConfigRef ref = refFor(f); + BridgedConfig before = ref.get(); + + Files.writeString(f, "profiles:\n sonnet:\n baseUrl: \"unclosed\n"); + ConfigRef.Outcome out = ref.reload(); + + assertFalse(out.applied()); + assertNotNull(out.error()); + assertTrue(out.summary().startsWith("config reload refused"), out.summary()); + assertSame(before, ref.get()); + } + + /** A file that would have refused to boot must not be able to slip in through a reload. */ + @Test + void aFileThatFailsAValidatorKeepsTheRunningConfig(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + Files.writeString(f, yaml("placement: weighted\n")); + ConfigRef ref = refFor(f); + BridgedConfig before = ref.get(); + + // A member slot naming a profile that does not exist — validateMembers refuses this at + // startup, so it must refuse it here too. + Files.writeString(f, yaml(""" + fleet: + developers: + a: + profile: no-such-profile + """)); + ConfigRef.Outcome out = ref.reload(); + + assertFalse(out.applied()); + assertNotNull(out.error()); + assertSame(before, ref.get()); + } + + @Test + void aDeletedFileIsRefusedRatherThanCrashing(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + Files.writeString(f, yaml("")); + ConfigRef ref = refFor(f); + BridgedConfig before = ref.get(); + + Files.delete(f); + ConfigRef.Outcome out = ref.reload(); + + assertFalse(out.applied()); + assertNotNull(out.error()); + assertSame(before, ref.get()); + } + + /** A deferred change applies to the snapshot but the operator is told it needs a restart. */ + @Test + void aDeferredChangeIsAppliedAndReported(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + Files.writeString(f, yaml(""" + lifecycle: + drainTimeoutSeconds: 30 + """)); + ConfigRef ref = refFor(f); + + Files.writeString(f, yaml(""" + lifecycle: + drainTimeoutSeconds: 60 + """)); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertEquals(java.util.List.of("lifecycle"), out.deferred()); + assertTrue(out.summary().contains("needs a restart") || out.summary().contains("need a restart"), + out.summary()); + assertEquals(60, ref.get().lifecycle().drainTimeoutSeconds()); + } + + /** + * Adding a profile is deferred, not hot: a new backend needs its own launcher, and launchers are + * built once at startup. The snapshot carries it so a restart picks it up. + */ + @Test + void addingAProfileIsReportedAsDeferred(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + Files.writeString(f, yaml("")); + ConfigRef ref = refFor(f); + + Files.writeString(f, """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet + haiku: + baseUrl: http://gx00.gw:8000 + model: haiku + guard: + offSubscriptionHosts: + - gx00.gw + """); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertEquals(1, out.deferred().size()); + assertTrue(out.deferred().getFirst().contains("haiku"), out.deferred().toString()); + } + + /** Changing an existing profile's fields is hot — no launcher has to be rebuilt for it. */ + @Test + void changingAnExistingProfilesFieldsIsHot(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + Files.writeString(f, yaml("")); + ConfigRef ref = refFor(f); + + Files.writeString(f, """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet-4-5 + maxLoad: 7 + guard: + offSubscriptionHosts: + - gx00.gw + """); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertTrue(out.deferred().isEmpty(), out.deferred().toString()); + assertEquals("sonnet-4-5", ref.get().profiles().get("sonnet").model()); + } + + @Test + void aFixedRefHasNoFileAndRefusesToReload() { + BridgedConfig cfg = new BridgedConfig(null, null, null, null, null, null, + null, null, null, null, null, null, null, null, null).withDefaults(); + ConfigRef ref = ConfigRef.fixed(cfg); + + assertNull(ref.path()); + assertSame(cfg, ref.get()); + ConfigRef.Outcome out = ref.reload(); + assertFalse(out.applied()); + assertNotNull(out.error()); + } +} diff --git a/bridged/src/test/java/dev/ltms/bridged/config/ConfigWatcherTest.java b/bridged/src/test/java/dev/ltms/bridged/config/ConfigWatcherTest.java new file mode 100644 index 0000000..753e0c4 --- /dev/null +++ b/bridged/src/test/java/dev/ltms/bridged/config/ConfigWatcherTest.java @@ -0,0 +1,162 @@ +package dev.ltms.bridged.config; + +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.nio.file.attribute.FileTime; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * CB-559: the mtime poller behind {@code configReload:}. The tests drive {@link + * ConfigWatcher#tick()} directly rather than the scheduler, so nothing here sleeps. + */ +class ConfigWatcherTest { + + private static String yaml(String placement) { + return """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet + guard: + offSubscriptionHosts: + - gx00.gw + """ + "placement: " + placement + "\n"; + } + + /** Write and stamp an mtime, so a test never depends on the filesystem's clock resolution. */ + private static void write(Path f, String content, long millis) throws Exception { + Files.writeString(f, content); + Files.setLastModifiedTime(f, FileTime.fromMillis(millis)); + } + + @Test + void aChangedMtimeTriggersAReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + write(f, yaml("weighted"), 1_000L); + ConfigRef ref = new ConfigRef(f, BridgedConfig.load(f)); + + ConfigWatcher watcher = new ConfigWatcher(ref, 10); + try { + write(f, yaml("fixed"), 2_000L); + watcher.tick(); + + assertEquals("fixed", ref.get().placement()); + } finally { + watcher.stop(); + } + } + + @Test + void anUnchangedFileIsNotReloaded(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + write(f, yaml("weighted"), 1_000L); + ConfigRef ref = new ConfigRef(f, BridgedConfig.load(f)); + BridgedConfig before = ref.get(); + + ConfigWatcher watcher = new ConfigWatcher(ref, 10); + try { + watcher.tick(); + watcher.tick(); + + // Same instance, so no reload happened — a reload always swaps in a fresh object. + assertSame(before, ref.get()); + } finally { + watcher.stop(); + } + } + + /** + * A refused reload must not be retried every tick. Without the stamp-before-reload order the + * same refusal would be logged forever, which buries the log an operator needs. + */ + @Test + void aRefusedReloadIsNotRetriedUntilTheFileChangesAgain(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + write(f, yaml("weighted"), 1_000L); + ConfigRef ref = new ConfigRef(f, BridgedConfig.load(f)); + BridgedConfig before = ref.get(); + + ConfigWatcher watcher = new ConfigWatcher(ref, 10); + try { + // A cold change: refused, and the running config stays. + write(f, yaml("fixed").replace("port: 8765", "port: 9999"), 2_000L); + watcher.tick(); + assertSame(before, ref.get()); + + // The next tick sees the same mtime, so it does nothing at all. + watcher.tick(); + assertSame(before, ref.get()); + + // A fresh save earns a fresh attempt — and this one is hot, so it applies. + write(f, yaml("fixed"), 3_000L); + watcher.tick(); + assertEquals("fixed", ref.get().placement()); + } finally { + watcher.stop(); + } + } + + /** + * Editors briefly unlink the file mid-save. A missing file is skipped, not an error and not a + * reload — the running config stays live, which is right either way. + */ + @Test + void aMissingFileIsSkippedAndTheNextTickTriesAgain(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + write(f, yaml("weighted"), 1_000L); + ConfigRef ref = new ConfigRef(f, BridgedConfig.load(f)); + BridgedConfig before = ref.get(); + + ConfigWatcher watcher = new ConfigWatcher(ref, 10); + try { + Files.delete(f); + assertDoesNotThrow(watcher::tick); + assertSame(before, ref.get()); + + write(f, yaml("fixed"), 2_000L); + watcher.tick(); + assertEquals("fixed", ref.get().placement()); + } finally { + watcher.stop(); + } + } + + /** A ref with no file behind it must not start a scheduler that could never do anything. */ + @Test + void aFixedRefStartsNoWatch() { + BridgedConfig cfg = new BridgedConfig(null, null, null, null, null, null, + null, null, null, null, null, null, null, null, null).withDefaults(); + ConfigWatcher watcher = new ConfigWatcher(ConfigRef.fixed(cfg), 10); + try { + assertDoesNotThrow(watcher::start); + assertDoesNotThrow(watcher::tick); + } finally { + watcher.stop(); + } + } + + @Test + void configReloadIsOffUnlessTheBlockSaysOtherwise(@TempDir Path dir) throws Exception { + Path f = dir.resolve("bridged.yaml"); + write(f, yaml("weighted"), 1_000L); + assertNull(BridgedConfig.load(f).configReload()); + + write(f, yaml("weighted") + "configReload:\n enabled: true\n", 2_000L); + BridgedConfig.ConfigReload on = BridgedConfig.load(f).configReload(); + assertTrue(on.isEnabled()); + assertEquals(10, on.intervalSeconds(), "an absent interval defaults to 10s"); + + write(f, yaml("weighted") + "configReload:\n intervalSeconds: 30\n", 3_000L); + BridgedConfig.ConfigReload off = BridgedConfig.load(f).configReload(); + assertFalse(off.isEnabled(), "a block that only sets the interval does not enable the watch"); + assertEquals(30, off.intervalSeconds()); + } +} diff --git a/bridged/src/test/java/dev/ltms/bridged/member/ClaudeCodeLauncherTest.java b/bridged/src/test/java/dev/ltms/bridged/member/ClaudeCodeLauncherTest.java index b421ffc..1dfa0ba 100644 --- a/bridged/src/test/java/dev/ltms/bridged/member/ClaudeCodeLauncherTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/member/ClaudeCodeLauncherTest.java @@ -17,7 +17,9 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.UUID; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; +import java.util.function.Supplier; import static org.junit.jupiter.api.Assertions.*; @@ -795,7 +797,7 @@ class ClaudeCodeLauncherTest { } /** A profile with no {@code tabLabel:} of its own — the fleet template decides. */ - private ClaudeCodeLauncher labelService(FakeHerdr herdr, String fleetTemplate) { + private ClaudeCodeLauncher labelService(FakeHerdr herdr, Supplier fleetTemplate) { BridgedConfig.Profile cfg = new BridgedConfig.Profile( "sonnet", "http://gx00.gw:8000", "sonnet", null, "BRIDGED_WORKER_TOKEN", List.of("claude"), "tab", "bridged-workers", null, null, null, null); @@ -812,7 +814,7 @@ class ClaudeCodeLauncherTest { @Test void theFleetTemplateNamesTheRoleTheMemberWasSpawnedFor() { FakeHerdr herdr = new FakeHerdr(); - ClaudeCodeLauncher svc = labelService(herdr, "{role}: {profile} #{n}"); + ClaudeCodeLauncher svc = labelService(herdr, () -> "{role}: {profile} #{n}"); svc.spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.REVIEWER)); @@ -823,7 +825,7 @@ class ClaudeCodeLauncherTest { @Test void theCounterRunsPerRoleAndProfileNotPerFleet() { FakeHerdr herdr = new FakeHerdr(); - ClaudeCodeLauncher svc = labelService(herdr, "{role}: {profile} #{n}"); + ClaudeCodeLauncher svc = labelService(herdr, () -> "{role}: {profile} #{n}"); svc.spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.DEV)); svc.spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.REVIEWER)); @@ -837,7 +839,7 @@ class ClaudeCodeLauncherTest { @Test void aBlankFleetTemplateFallsBackToTheRoleFirstDefault() { FakeHerdr herdr = new FakeHerdr(); - labelService(herdr, null).spawn( + labelService(herdr, () -> null).spawn( new SpawnRequest("sonnet", null, null, null, null, MemberRole.ARCHITECT)); assertEquals(List.of("architect: sonnet #1"), tabLabels(herdr)); @@ -853,9 +855,27 @@ class ClaudeCodeLauncherTest { List.of("claude"), "tab", "bridged-workers", "pinned {profile}", null, null, null); new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), - _ -> null, 0, 0L, "{role}: {profile} #{n}") + _ -> null, 0, 0L, () -> "{role}: {profile} #{n}") .spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.REVIEWER)); assertEquals(List.of("pinned sonnet"), tabLabels(herdr)); } + + /** + * CB-559: the template is read per spawn, not captured at construction. This is what makes + * {@code fleet.tabLabel} a hot key — a launcher built at boot must see an edit made an hour later + * without being rebuilt. + */ + @Test + void theTemplateIsReadOnEverySpawnSoAnEditTakesEffect() { + FakeHerdr herdr = new FakeHerdr(); + AtomicReference template = new AtomicReference<>("{role}: {profile} #{n}"); + ClaudeCodeLauncher svc = labelService(herdr, template::get); + + svc.spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.DEV)); + template.set("[{profile}] {role} {n}"); + svc.spawn(new SpawnRequest("sonnet", null, null, null, null, MemberRole.DEV)); + + assertEquals(List.of("dev: sonnet #1", "[sonnet] dev 2"), tabLabels(herdr)); + } }