From e18e002d2fef243eeaf1ae57998a6a1731bebf85 Mon Sep 17 00:00:00 2001
From: Dai Ha <dai.ha@magnolia-cms.com>
Date: Fri, 14 Aug 2026 20:37:42 +0200
Subject: [PATCH] CB-559: a profile's launch settings are deferred, not hot

The shipped docs and javadoc said a profile's `model` and `tabLabel` take effect on
the next spawn. They do not, and ConfigRef did not detect the change either, so a
reload logged a clean "config reloaded" and silently did nothing. That is the worst
outcome a reload can produce: the operator has no reason to doubt it.

What makes a key hot is who reads it and when, not that it is config. Placement
reads weight and maxLoad through a supplier on CompositePeerLauncher, so those are
genuinely hot. HerdrPeerLauncher takes Map.copyOf(profiles) at construction and
resolves every spawn out of that copy, so model, baseUrl, argv, env and the rest
cannot move until the daemon restarts.

changedDeferredKeys now compares every launch component of an existing profile,
excluding weight and maxLoad, and names the profiles that need a restart. The
javadoc and bridged.example.yaml say the same thing. Two tests pin the pair:
weight/maxLoad reports nothing deferred, a changed model reports the profile by name.
---
 bridged/bridged.example.yaml                  | 14 ++--
 .../dev/ltms/bridged/config/ConfigRef.java    | 74 ++++++++++++++++---
 .../ltms/bridged/config/ConfigRefTest.java    | 47 +++++++++++-
 3 files changed, 114 insertions(+), 21 deletions(-)

diff --git a/bridged/bridged.example.yaml b/bridged/bridged.example.yaml
index 9e18d9f..0752f19 100644
--- a/bridged/bridged.example.yaml
+++ b/bridged/bridged.example.yaml
@@ -232,13 +232,17 @@ placement: weighted
 # 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.
+#               `tabLabel`), `placement:`, and an existing profile's weight / maxLoad. Those are
+#               hot because the placement policy reads them through a supplier — being config is
+#               not by itself enough to make a key hot.
 #   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.
+#               `spawnReadyTimeoutMs` / `spawnReadyPollMs`, ADDING or REMOVING a profile (a new
+#               backend needs its own launcher, and launchers are built once), AND an existing
+#               profile's launch settings — model, baseUrl, argv, env, configDir, mcpUrl, tabLabel.
+#               The launcher takes a copy of `profiles:` at startup and resolves every spawn out of
+#               that copy, so those never reach a launch until you restart. The reload logs them 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.
diff --git a/bridged/src/main/java/dev/ltms/bridged/config/ConfigRef.java b/bridged/src/main/java/dev/ltms/bridged/config/ConfigRef.java
index 1750234..37001d5 100644
--- a/bridged/src/main/java/dev/ltms/bridged/config/ConfigRef.java
+++ b/bridged/src/main/java/dev/ltms/bridged/config/ConfigRef.java
@@ -7,6 +7,7 @@ import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
@@ -27,13 +28,18 @@ import java.util.function.Supplier;
  * <ul>
  *   <li><strong>Hot</strong> — 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}.</li>
+ *       profile's {@code weight} / {@code maxLoad}. Those three are read through a supplier on
+ *       {@code CompositePeerLauncher}, which is what makes them hot — not the fact that they are
+ *       config.</li>
  *   <li><strong>Deferred</strong> — 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 <em>adding or removing</em> a profile (a new backend needs its
- *       own launcher, which is constructed once). A reload logs these rather than pretending they
- *       applied.</li>
+ *       {@code worktreeRoot:}, adding or removing a profile (a new backend needs its own launcher,
+ *       which is constructed once), <em>and an existing profile's launch settings</em> —
+ *       {@code model}, {@code baseUrl}, {@code argv}, {@code env}, {@code mcpUrl} and the rest.
+ *       {@code HerdrPeerLauncher} takes {@code Map.copyOf(profiles)} at construction and resolves
+ *       each spawn out of that copy, so those never reach a launch until the daemon restarts. A
+ *       reload logs these rather than pretending they applied.</li>
  *   <li><strong>Cold</strong> — 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
@@ -204,16 +210,60 @@ public final class ConfigRef implements Supplier<BridgedConfig> {
                 || !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<String> before = old.profiles() == null ? Set.of() : old.profiles().keySet();
-        Set<String> after = fresh.profiles() == null ? Set.of() : fresh.profiles().keySet();
-        if (!before.equals(after)) {
-            Set<String> diff = new LinkedHashSet<>(before);
-            diff.addAll(after);
-            diff.removeIf(p -> before.contains(p) && after.contains(p));
+        Map<String, BridgedConfig.Profile> before =
+                old.profiles() == null ? Map.of() : old.profiles();
+        Map<String, BridgedConfig.Profile> after =
+                fresh.profiles() == null ? Map.of() : fresh.profiles();
+        // Adding or removing a profile is deferred: a new backend needs its own launcher, and
+        // launchers are built once at startup.
+        if (!before.keySet().equals(after.keySet())) {
+            Set<String> diff = new LinkedHashSet<>(before.keySet());
+            diff.addAll(after.keySet());
+            diff.removeIf(p -> before.containsKey(p) && after.containsKey(p));
             changed.add("profiles (added/removed: " + String.join(", ", diff) + ")");
         }
+        // An EXISTING profile's launch settings are deferred too, and this is easy to get wrong:
+        // `HerdrPeerLauncher` takes `Map.copyOf(profiles)` at construction and `spawn` resolves the
+        // profile out of that snapshot, so a reloaded model/baseUrl/argv/env never reaches a launch.
+        // Only weight and maxLoad are genuinely hot, because placement reads them through the
+        // supplier on the composite rather than from the adapter's copy. Without this check a
+        // changed model would report "config reloaded" and silently do nothing — the worst outcome
+        // a reload can produce, because the operator has no reason to doubt it.
+        List<String> relaunch = new ArrayList<>();
+        before.forEach((name, was) -> {
+            BridgedConfig.Profile now = after.get(name);
+            if (now != null && !sameLaunchSettings(was, now)) {
+                relaunch.add(name);
+            }
+        });
+        if (!relaunch.isEmpty()) {
+            changed.add("profiles." + String.join("/", relaunch) + " launch settings "
+                    + "(model, baseUrl, argv, env, …) — the launcher holds a startup snapshot");
+        }
         return changed;
     }
+
+    /**
+     * Whether two versions of a profile would launch a peer identically. Compares every component
+     * the launcher reads at spawn; {@code weight} and {@code maxLoad} are excluded because those are
+     * read live by the placement policy and really do take effect on the next spawn.
+     */
+    private static boolean sameLaunchSettings(BridgedConfig.Profile a, BridgedConfig.Profile b) {
+        return Objects.equals(a.baseUrl(), b.baseUrl())
+                && Objects.equals(a.model(), b.model())
+                && Objects.equals(a.configDir(), b.configDir())
+                && Objects.equals(a.tokenEnv(), b.tokenEnv())
+                && Objects.equals(a.argv(), b.argv())
+                && Objects.equals(a.placement(), b.placement())
+                && Objects.equals(a.workspace(), b.workspace())
+                && Objects.equals(a.tabLabel(), b.tabLabel())
+                && Objects.equals(a.mcpUrl(), b.mcpUrl())
+                && Objects.equals(a.cwd(), b.cwd())
+                && Objects.equals(a.parityOverlay(), b.parityOverlay())
+                && Objects.equals(a.gitTokenEnv(), b.gitTokenEnv())
+                && Objects.equals(a.gitHostEnv(), b.gitHostEnv())
+                && Objects.equals(a.kind(), b.kind())
+                && Objects.equals(a.env(), b.env())
+                && Objects.equals(a.subscription(), b.subscription());
+    }
 }
diff --git a/bridged/src/test/java/dev/ltms/bridged/config/ConfigRefTest.java b/bridged/src/test/java/dev/ltms/bridged/config/ConfigRefTest.java
index abba835..f9550d7 100644
--- a/bridged/src/test/java/dev/ltms/bridged/config/ConfigRefTest.java
+++ b/bridged/src/test/java/dev/ltms/bridged/config/ConfigRefTest.java
@@ -213,9 +213,12 @@ class ConfigRefTest {
         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. */
+    /**
+     * Changing an existing profile's weight or maxLoad IS hot: placement reads those live through
+     * the supplier on the composite, so the next spawn already sees them.
+     */
     @Test
-    void changingAnExistingProfilesFieldsIsHot(@TempDir Path dir) throws Exception {
+    void changingAProfilesWeightOrMaxLoadIsHot(@TempDir Path dir) throws Exception {
         Path f = dir.resolve("bridged.yaml");
         Files.writeString(f, yaml(""));
         ConfigRef ref = refFor(f);
@@ -228,8 +231,9 @@ class ConfigRefTest {
                 profiles:
                   sonnet:
                     baseUrl: http://gx00.gw:8000
-                    model: sonnet-4-5
+                    model: sonnet
                     maxLoad: 7
+                    weight: 3.0
                 guard:
                   offSubscriptionHosts:
                     - gx00.gw
@@ -238,7 +242,42 @@ class ConfigRefTest {
 
         assertTrue(out.applied());
         assertTrue(out.deferred().isEmpty(), out.deferred().toString());
-        assertEquals("sonnet-4-5", ref.get().profiles().get("sonnet").model());
+        assertEquals(7, ref.get().profiles().get("sonnet").maxLoad());
+    }
+
+    /**
+     * Changing an existing profile's MODEL is deferred, not hot — and saying so is the whole point.
+     * HerdrPeerLauncher takes Map.copyOf(profiles) at construction and resolves every spawn out of
+     * that copy, so a reloaded model never reaches a launch. Reporting it as applied would be the
+     * worst outcome a reload can produce: the operator has no reason to doubt a clean "reloaded".
+     */
+    @Test
+    void changingAProfilesLaunchSettingsIsReportedAsDeferred(@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: deepseek-v4-flash
+                guard:
+                  offSubscriptionHosts:
+                    - gx00.gw
+                """);
+        ConfigRef.Outcome out = ref.reload();
+
+        assertTrue(out.applied());
+        assertEquals(1, out.deferred().size(), out.deferred().toString());
+        assertTrue(out.deferred().getFirst().contains("sonnet"), out.deferred().toString());
+        assertTrue(out.deferred().getFirst().contains("launch settings"), out.deferred().toString());
+        // The snapshot still carries the new value — a restart is what makes it take effect.
+        assertEquals("deepseek-v4-flash", ref.get().profiles().get("sonnet").model());
     }
 
     @Test