diff --git a/bridged/bridged.example.yaml b/bridged/bridged.example.yaml index 1179042..9e18d9f 100644 --- a/bridged/bridged.example.yaml +++ b/bridged/bridged.example.yaml @@ -55,34 +55,15 @@ 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. 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 + @@ -108,11 +89,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 +150,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 +165,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 +222,105 @@ 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: +# 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:`. +# +# 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. + # + # 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. 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 + # 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..29ea236 100644 --- a/bridged/src/main/java/dev/ltms/bridged/Bridged.java +++ b/bridged/src/main/java/dev/ltms/bridged/Bridged.java @@ -1,11 +1,14 @@ 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; 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; @@ -36,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; @@ -78,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()); @@ -88,7 +95,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,26 +130,28 @@ 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(), + () -> config.get().fleet().tabLabel())); } if (!opencodeProfiles.isEmpty()) { adapters.add(new OpenCodeLauncher(agents, spaces, opencodeProfiles, cfg.effectiveDefaultProfile(), System::getenv, - cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs())); + cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs(), + () -> config.get().fleet().tabLabel())); } AtomicReference> liveCountRef = new AtomicReference<>(_ -> 0); PeerLauncher workers = new CompositePeerLauncher( adapters, cfg.effectiveDefaultProfile(), - cfg.profiles(), - PlacementPolicies.fromName(cfg.placement()), + 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 // 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(); @@ -186,33 +195,56 @@ 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; } + // 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 // 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()); } @@ -356,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. @@ -365,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/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..236c675 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,12 +71,20 @@ public record BridgedConfig( Integer spawnReadyPollMs, Broker broker, Primary primary, - Map leaders, - Map members, - LeadScan leadScan, + 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. @@ -203,7 +202,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 +329,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 +424,177 @@ 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, + 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; + } } /** - * 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(); } } @@ -514,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. @@ -528,8 +671,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 +739,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 +782,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", "configReload"); /** Load and validate config from {@code path}. */ public static BridgedConfig load(Path path) { @@ -636,38 +800,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 +850,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 +966,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 +1005,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 +1043,18 @@ 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. + // 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, configReload); } /** @@ -876,40 +1085,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 +1193,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/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/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/main/java/dev/ltms/bridged/member/ClaudeCodeLauncher.java b/bridged/src/main/java/dev/ltms/bridged/member/ClaudeCodeLauncher.java index b104136..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 @@ -79,9 +80,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, + Supplier tabLabelTemplate) { this(agents, spaces, guard, profiles, defaultProfile, env, spawnReadyTimeoutMs, - System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs)); + System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs), + tabLabelTemplate); } /** @@ -106,8 +122,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, + Supplier 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..605e0c2 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; @@ -25,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 @@ -64,10 +66,25 @@ 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-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 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 * simple wiring; it preserves the pre-CB-518 behaviour exactly. @@ -78,7 +95,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,15 +113,62 @@ 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) { + // 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<>(); @@ -121,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; @@ -151,20 +231,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.get().select(ctx); HerdrPeerLauncher d = byProfile.get(chosen.profile()); if (d == null) { @@ -174,9 +255,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); @@ -186,7 +268,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); } } @@ -200,8 +282,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 @@ -219,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; @@ -231,12 +313,36 @@ 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) { + 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. */ + 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 = profiles0().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/member/HerdrPeerLauncher.java b/bridged/src/main/java/dev/ltms/bridged/member/HerdrPeerLauncher.java index 0e79e35..eb27d8a 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; @@ -28,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; @@ -75,7 +77,32 @@ 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; 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 Supplier 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 +137,26 @@ 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, 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. + */ + protected HerdrPeerLauncher(String namePrefix, AgentControl agents, WorkspaceControl spaces, + Map profiles, String defaultProfile, + Function env, + long spawnReadyTimeoutMs, + LongSupplier nowMillis, Runnable sleeper, + Supplier tabLabelTemplate) { + this.tabLabelTemplate = tabLabelTemplate; this.namePrefix = namePrefix; this.agents = agents; this.spaces = spaces; @@ -240,6 +287,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 +301,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 +337,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 +382,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 +413,10 @@ 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 == 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 812de5d..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, @@ -112,9 +113,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, + Supplier tabLabelTemplate) { this(agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs), - defaultConfigRoot(), defaultDiscoveryRoot()); + defaultConfigRoot(), defaultDiscoveryRoot(), tabLabelTemplate); } /** @@ -142,8 +157,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, + Supplier 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/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/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/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/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"); + } +} 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..1dfa0ba 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; @@ -16,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.*; @@ -782,4 +785,97 @@ 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, 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); + 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)); + } + + /** + * 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)); + } } 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()); + } }