diff --git a/fleetd/fleetd.example.yaml b/fleetd/fleetd.example.yaml index cbf3240..d77befd 100644 --- a/fleetd/fleetd.example.yaml +++ b/fleetd/fleetd.example.yaml @@ -802,12 +802,21 @@ guard: # .claude/skills/, so a member spawned against ANY repo — not only one that already ships its own # copy — can load a bridge skill (e.g. implementer). Unset (the default): no worktree is touched # beyond today's behaviour. A skill folder the target repo already carries under -# .claude/skills/ is never overwritten — the repo's own copy always wins. Claude Code -# members only; an opencode member reads a different path (.opencode/agent) this key does not -# touch. Best-effort like worktreeGroup above: a missing/unreadable directory here is logged and -# skipped, never a failed spawn. Every non-hidden subdirectory of this directory is copied -# wholesale, with no per-file allowlist — don't park scratch files or drafts alongside the real -# skill folders, they will be copied into every provisioned worktree too. +# .claude/skills/ is never overwritten — the repo's own copy always wins. Best-effort like +# worktreeGroup above: a missing/unreadable directory here is logged and skipped, never a failed +# spawn. Every non-hidden subdirectory of this directory is copied wholesale, with no per-file +# allowlist — don't park scratch files or drafts alongside the real skill folders, they will be +# copied into every provisioned worktree too. +# +# fleetd #393: which member KINDS actually consume this once it is copied. kind: claude-code — +# the Claude Code CLI discovers .claude/skills/ on its own; nothing else is needed. kind: opencode +# — opencode has no such discovery, so OpenCodeLauncher reads whatever landed under +# .claude/skills/ and appends each seeded skill's SKILL.md to the generated instructions[] file +# (opencode's only channel for static guidance text; unlike Claude Code's Skill tool, the content +# is always part of the system prompt, not loaded on demand). Both kinds are covered as of #393 — +# earlier builds copied the files for every kind but only claude-code could read them, and the +# seeding log said "N of M" regardless. Check the per-spawn launcher log (not just the seeding +# log) to see what a given member actually got. # memberSkills: /path/to/fleetd/checkout/.claude/skills # Session lifecycle limits (CB-303). All knobs are opt-in; omit or set to null to keep diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 3501003..5fa48e2 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -148,7 +148,10 @@ public final class Fleetd { // 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); + // fleetd #474: pass the charter/tool-surface check in as ConfigRef's extraValidation, so + // ConfigRef#reload() runs the same gate main() runs below, without dev.ltms.fleet.config + // gaining a dependency on dev.ltms.fleet.mcp — Fleetd is the seam that already holds both. + ConfigRef config = new ConfigRef(configPath, cfg, Fleetd::assertChartersNameOnlyRegisteredTools); // The primary/host env that launched fleetd must not be tainted. SubscriptionGuard guard = new SubscriptionGuard(cfg.guard().hostSet()); @@ -172,9 +175,9 @@ public final class Fleetd { // inside FleetConfig#validateCharters() — config loads before the MCP server exists, and // must not gain a dependency on the mcp package — so it runs here instead, at the one seam // that already holds both a loaded FleetConfig and the mcp package, before anything below - // opens a socket or spawns a member. - CharterToolSurface.assertChartersNameOnlyRegisteredTools( - cfg.fleet() == null ? Map.of() : cfg.fleet().charters()); + // opens a socket or spawns a member. fleetd #474: the same check is also wired into `config` + // above as ConfigRef's extraValidation, so a reload refuses what this line refuses at startup. + assertChartersNameOnlyRegisteredTools(cfg); Path socket = cfg.herdrSocket() != null && !cfg.herdrSocket().isBlank() ? Path.of(cfg.herdrSocket()) @@ -1594,6 +1597,28 @@ public final class Fleetd { } } + /** + * fleetd #474: the one place both the startup call (right after {@code cfg.validateAll()} in + * {@link #main}) and the reload call (wired into {@code config}'s {@code extraValidation} above, + * via a method reference to this method) go through, so the two can never drift into checking + * different things. Extracted only to give {@link ConfigRef}'s {@code Consumer} + * hook a {@code FleetConfig -> void} shape to bind to — {@link CharterToolSurface} itself still + * takes the raw charter map and knows nothing about {@code ConfigRef} or {@code Fleetd}. + * + *

Package-private so a test can call it directly the same way the other startup-report + * helpers above are tested, without needing to drive {@link #main} for a unit-level check; + * {@code FleetdStartupValidationTest} proves the startup call site, and {@code + * FleetdConfigRefCharterToolSurfaceWiringTest} — by constructing {@code ConfigRef} with this + * exact method reference, the same way {@code main} does above — proves the reload call site. + * {@code dev.ltms.fleet.config.ConfigRefTest} pins the same reload behaviour too, through an + * equivalent {@code Consumer} it builds locally (it cannot see this package-private + * method from {@code dev.ltms.fleet.config}). + */ + static void assertChartersNameOnlyRegisteredTools(FleetConfig cfg) { + CharterToolSurface.assertChartersNameOnlyRegisteredTools( + cfg.fleet() == null ? Map.of() : cfg.fleet().charters()); + } + /** * Poll herdr's {@code ping} until it answers or {@link #HERDR_WAIT_SECONDS} elapses (CB-504). * diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java index 3e7e6a7..4549c4b 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java @@ -11,6 +11,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; import java.util.function.Supplier; /** @@ -211,6 +212,24 @@ import java.util.function.Supplier; *

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. + * + *

fleetd #474 — {@link FleetConfig#validateAll()} is not the only gate startup + * runs before a config takes effect: {@code Fleetd.main} also calls {@code + * dev.ltms.fleet.mcp.CharterToolSurface#assertChartersNameOnlyRegisteredTools}, right after {@code + * cfg.validateAll()}, to refuse a charter that names an MCP tool the server does not register. That + * check cannot live inside {@link FleetConfig} — {@code CharterToolSurface} lives in the {@code mcp} + * package because the canonical tool set ({@code FleetTool}) does, and config is loaded before the + * MCP server exists, so {@code FleetConfig} must not gain a dependency on {@code mcp}. {@link + * #reload} cannot import {@code mcp} either, for the same reason applied one layer up: {@code + * dev.ltms.fleet.config} is loaded before {@code dev.ltms.fleet.mcp} exists, same as {@code + * FleetConfig}. So this class accepts the check as a {@code Consumer} — + * {@link #extraValidation} — supplied by whichever caller already sits at the seam that holds both + * a loaded {@code FleetConfig} and the {@code mcp} package: {@code Fleetd.main}. It is invoked + * inside the same try/catch as {@code fresh.validateAll()}, so a charter that would have refused to + * boot refuses a reload too, and keeps the running config exactly like any other {@code + * validateAll()} failure. A ref built through the two-argument constructor (every test fixture that + * does not care about this check, and {@link #fixed}) gets a no-op consumer, so nothing outside + * {@code Fleetd.main} needs to know this hook exists. */ public final class ConfigRef implements Supplier { @@ -255,10 +274,27 @@ public final class ConfigRef implements Supplier { private final Path path; private final AtomicReference current; + private final Consumer extraValidation; + /** Equivalent to the three-argument constructor with a no-op {@code extraValidation}. */ public ConfigRef(Path path, FleetConfig initial) { + this(path, initial, cfg -> { }); + } + + /** + * @param extraValidation run on every {@link #reload} candidate, inside the same try/catch as + * {@code fresh.validateAll()} — see the class doc's fleetd #474 note. + * {@code Fleetd.main} passes {@code + * Fleetd::assertChartersNameOnlyRegisteredTools} (a package-private + * {@code FleetConfig -> void} adapter over {@code + * CharterToolSurface#assertChartersNameOnlyRegisteredTools}), so a reload + * runs the same gate startup does without this class depending on the + * {@code mcp} package. + */ + public ConfigRef(Path path, FleetConfig initial, Consumer extraValidation) { this.path = path; this.current = new AtomicReference<>(Objects.requireNonNull(initial, "initial config")); + this.extraValidation = Objects.requireNonNull(extraValidation, "extraValidation"); } /** A fixed reference that never reloads — for tests and for wiring built from a config in code. */ @@ -360,6 +396,12 @@ public final class ConfigRef implements Supplier { // at all — see FleetConfig#validateAll's javadoc for why the fix is one reflective call, // not a longer hand-maintained list. fresh.validateAll(); + // fleetd #474: validateAll() does not cover everything startup refuses on — the charter + // tool-surface check (Fleetd.main, right after cfg.validateAll()) lives outside + // FleetConfig on purpose (see this class's doc) and is supplied here as extraValidation. + // Same try/catch as validateAll() above, on purpose: either failure must refuse the whole + // reload and keep the running config the same way. + extraValidation.accept(fresh); } catch (RuntimeException e) { String msg = e.getMessage() == null ? e.toString() : e.getMessage(); log.warn("config reload from {} refused, keeping the running config: {}", path, msg); diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java index 9f4e242..779bfa3 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/CharterToolSurface.java @@ -25,6 +25,20 @@ import java.util.regex.Pattern; * {@code fleetd.yaml} could ever fail it. This class is what a real charter is actually checked * against at boot; {@code FleetdStartupValidationTest} exercises it through {@code Fleetd.main} * itself, the same way it proves every other {@code validateXxx()} still runs there. + * + *

fleetd #474 — startup was not the only door: {@code + * dev.ltms.fleet.config.ConfigRef#reload()} used to run {@code FleetConfig#validateAll()} alone, + * which does not look at what a charter's text names, so a charter naming an unregistered tool + * that could not have booted the daemon could still be installed into a running one through a + * reload. This class still knows nothing about {@code ConfigRef} — {@code Fleetd.main} wires a + * small {@code FleetConfig -> void} adapter over {@link #assertChartersNameOnlyRegisteredTools} + * ({@code Fleetd::assertChartersNameOnlyRegisteredTools}) into {@code ConfigRef}'s constructor as + * its {@code Consumer} {@code extraValidation}, run inside {@code reload()}'s same + * try/catch as {@code validateAll()}, so both call sites — {@code Fleetd.main} at startup and + * {@code ConfigRef#reload()} afterwards — go through this one method and can never check different + * things. {@code dev.ltms.fleet.config.ConfigRefTest} and {@code + * FleetdConfigRefCharterToolSurfaceWiringTest} are what prove the reload call site, the same way + * {@code FleetdStartupValidationTest} proves the startup one. */ public final class CharterToolSurface { diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java index e7330dc..893e7f4 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java @@ -331,11 +331,22 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { + "form — opencode's per-model context limit could not be applied for this profile", cfg.profile(), cfg.model()); } + // fleetd #393: memberSkills seeding (GitWorktrees#seedSkills) copies skill folders into + // EVERY provisioned worktree's .claude/skills/ regardless of which kind ultimately spawns + // into it — that copy step cannot know the kind, only the caller of GitWorktrees#add does + // (see that method's own javadoc). .claude/skills/ is a Claude Code CLI convention the CLI + // discovers on its own; opencode has no such discovery, so without this, a seeded skill + // never reaches an opencode member even though GitWorktrees logged it as seeded. Read + // whatever landed under /.claude/skills/ here — the one place in this launcher that + // knows both the kind (opencode, by construction: this IS OpenCodeLauncher) and the cwd. + List skillInstructionFiles = skillInstructionFiles(spec.cwd()); // A config file is needed for the bridge MCP mount, a member charter, the IDE MCP (+ its - // guidance overlay, CB-634), a pinned endpoint (CB-508), or a resolvable autoCompactWindow. + // guidance overlay, CB-634), a pinned endpoint (CB-508), a resolvable autoCompactWindow, or + // at least one seeded skill to deliver via instructions[] (fleetd #393). if (cfg.hasMcp() || cfg.hasIdeMcp() || spec.charter() != null || hasCustomProvider(cfg) - || wantsContextLimit) { - workerEnv.put("OPENCODE_CONFIG", writeConfig(cfg, spec.charter(), spec.cwd()).toString()); + || wantsContextLimit || !skillInstructionFiles.isEmpty()) { + workerEnv.put("OPENCODE_CONFIG", + writeConfig(cfg, spec.charter(), spec.cwd(), skillInstructionFiles).toString()); } applyGitToken(workerEnv, cfg); List argv = argvWithResume(argvWithModel(argvWithAuto(cfg), cfg), spec.resumeSessionId()); @@ -358,6 +369,65 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { return withAgent; } + /** + * fleetd #393: the {@code SKILL.md} paths under {@code /.claude/skills/} this launcher can + * turn into {@code instructions[]} entries, plus the honest log this ticket asks for — emitted + * here, at the one point a skill's fate for THIS spawn is actually known, rather than trusting + * {@code GitWorktrees#seedSkills}'s kind-blind "N of M" line to mean "and it will be read." + * + *

Every non-hidden subdirectory of {@code .claude/skills/} is a candidate, whether it got + * there via {@code memberSkills:} seeding or because the target repo ships its own copy — this + * launcher does not care which; it only cares what it can find at spawn time. A candidate with + * a {@code SKILL.md} at its top level (the same shape {@link #writeConfig} already requires for + * the charter and IDE-rules instructions entries) is delivered; anything else is a directory + * this launcher cannot turn into a flat instructions entry, named explicitly in the log rather + * than silently dropped, so a caller sees a real "cannot consume" reason and not just a smaller + * number than {@code GitWorktrees}' own count. + * + *

No candidates at all (directory absent or empty) logs nothing — the same + * no-log-when-nothing-to-say shape {@link #hasCustomProvider} and friends already follow, and + * the shape {@code GitWorktrees#seedSkills} itself uses when {@code memberSkills:} is unset. + * A failure to even list the directory is logged and treated as "nothing delivered" — best + * effort, must never fail the spawn, matching {@code GitWorktrees#seedSkills}'s own contract. + */ + private List skillInstructionFiles(String cwd) { + if (cwd == null || cwd.isBlank()) { + return List.of(); + } + Path skillsDir = Path.of(cwd, ".claude", "skills"); + if (!Files.isDirectory(skillsDir)) { + return List.of(); + } + List candidates; + try (var listing = Files.list(skillsDir)) { + candidates = listing.filter(Files::isDirectory) + .filter(p -> !p.getFileName().toString().startsWith(".")) + .sorted() + .toList(); + } catch (IOException e) { + log.warn("could not scan {} for skill folders to deliver to this opencode member: {}", + skillsDir, e.getMessage()); + return List.of(); + } + if (candidates.isEmpty()) { + return List.of(); + } + List delivered = candidates.stream() + .map(dir -> dir.resolve("SKILL.md")) + .filter(Files::isRegularFile) + .toList(); + List undeliverable = candidates.stream() + .filter(dir -> !Files.isRegularFile(dir.resolve("SKILL.md"))) + .map(dir -> dir.getFileName().toString()) + .toList(); + log.info("skill delivery: {} of {} skill folder(s) under {} reached this opencode member via " + + "instructions[] (opencode does not read .claude/skills/ natively, unlike " + + "Claude Code){}", + delivered.size(), candidates.size(), skillsDir, + undeliverable.isEmpty() ? "" : "; no SKILL.md, could not be delivered: " + undeliverable); + return delivered; + } + /** * True when this profile pins its own OpenAI-compatible endpoint (CB-508) rather than using * whatever provider opencode resolves by default. @@ -418,8 +488,15 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { * fresh per-spawn directory under {@link #configRoot}, and return the config file's path for * {@code OPENCODE_CONFIG}. The dir is unique per spawn so concurrent workers never race on it; * it is best-effort cleaned on JVM exit (worker config is disposable — regenerated every spawn). + * + * @param skillInstructionFiles fleetd #393: absolute {@code SKILL.md} paths from + * {@link #skillInstructionFiles(String)}, appended to + * {@code instructions[]} so a {@code memberSkills:}-seeded skill + * reaches this opencode member the same way the charter and IDE + * rules already do. */ - private Path writeConfig(FleetConfig.Profile cfg, String charterText, String cwd) { + private Path writeConfig(FleetConfig.Profile cfg, String charterText, String cwd, + List skillInstructionFiles) { try { Path dir = Files.createTempDirectory(configParentDir(), "fleetd-opencode-"); dir.toFile().deleteOnExit(); @@ -447,7 +524,28 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { Files.writeString(charter, charterText); charter.toFile().deleteOnExit(); - root.putArray("instructions").add(charter.toAbsolutePath().toString()); + // fleetd #393 follow-up: withArray, not putArray. putArray REPLACES whatever node + // is already at "instructions" — harmless only as long as this block runs first + // against a still-empty root, which is an ordering constraint nothing declared or + // tested. The skills writer just below, and the IDE-rules writer further down, + // both already use withArray (get-or-create) for exactly this reason; this was the + // one straggler. Proven load-bearing on the fleetd #393 merge: flipping this one + // call back to putArray left the whole suite green while silently deleting the + // charter entry whenever skills or IDE rules ran after it — an opencode member + // would launch with no role contract at all, worse than the bug #393 fixed, and + // nothing caught it. See OpenCodeLauncherTest's + // instructionsArrayHoldsCharterThenIdeRulesInOrder and + // instructionsArrayHoldsCharterThenSkillsThenIdeRulesInOrder. + root.withArray("instructions").add(charter.toAbsolutePath().toString()); + } + + // fleetd #393: each seeded skill's SKILL.md, delivered as a plain instructions[] entry + // — the only mechanism opencode has for static guidance text. Unlike Claude Code's + // Skill tool, opencode cannot load one of these on demand by name; the content is just + // always part of the system prompt from spawn. That is a real difference in HOW the + // content reaches the member, not a reason to withhold it. + for (Path skillFile : skillInstructionFiles) { + root.withArray("instructions").add(skillFile.toAbsolutePath().toString()); } if (cfg.hasMcp() || cfg.hasIdeMcp()) { diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java index af80e13..71898d2 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -690,14 +690,18 @@ public final class GitWorktrees implements Worktrees { * {@code fleet.seededSkillsNote}, readable with {@code git config --worktree --get-all * fleet.seededSkills}. * - *

Claude Code specific by construction, not by a backend check here. Only {@code - * .claude/skills//SKILL.md} is a path any launcher reads today (opencode's equivalent is a - * different shape under {@code .opencode/agent}, out of scope — see issue #362). This method - * only copies files; like {@link #isolateToolSurface} — which neutralizes BOTH {@code .mcp.json} - * and {@code opencode.json} unconditionally — it runs the same for every worktree regardless of - * which backend ultimately spawns into it, because the backend is not yet chosen at {@link #add} - * time. A seeded {@code .claude/skills/} directory in an opencode member's worktree is simply - * never read by that launcher. + *

Kind-blind by construction, not by a backend check here — this used to be a real gap + * (fleetd #393). This method only copies files; like {@link #isolateToolSurface} — which + * neutralizes BOTH {@code .mcp.json} and {@code opencode.json} unconditionally — it runs the + * same for every worktree regardless of which backend ultimately spawns into it, because no + * caller of {@link #add} hands this class a kind to consult. Before fleetd #393, that made the + * log line below a false claim of success for a {@code kind: opencode} member: opencode has no + * built-in discovery of {@code .claude/skills/}, unlike the Claude Code CLI, so a seeded skill + * never reached one. It now does — {@code OpenCodeLauncher#skillInstructionFiles} reads + * whatever this method copied into {@code .claude/skills/} and appends each {@code SKILL.md} to + * the generated {@code instructions[]} — but that delivery, and the log line that honestly + * claims it (kind-aware, unlike this one), happens at the launcher, once the kind is actually + * known, not here. */ private void seedSkills(String worktreePath) { if (memberSkillsSource == null) { @@ -739,7 +743,15 @@ public final class GitWorktrees implements Worktrees { if (detail.isEmpty()) { detail = "no skill folders found under " + source; } - log.info("skill seeding: {} of {} candidate(s) from {} into {}/.claude/skills — {}", + // fleetd #393: this only claims the copy step, deliberately — it cannot know the member + // kind that will spawn into this worktree (see this method's own javadoc), so it must not + // read as "and the member will act on it." Whether that is true depends on the kind: the + // Claude Code CLI discovers .claude/skills/ on its own; OpenCodeLauncher logs its own + // "skill delivery" line, once the kind is known, naming what it could and could not turn + // into instructions[]. + log.info("skill seeding: {} of {} candidate(s) from {} into {}/.claude/skills — {} " + + "(whether the spawned member can act on this depends on its kind — see " + + "the launcher's own log for that)", seeded.size(), seeded.size() + kept.size(), source, worktreePath, detail); if (seeded.isEmpty()) { return; diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdConfigRefCharterToolSurfaceWiringTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdConfigRefCharterToolSurfaceWiringTest.java new file mode 100644 index 0000000..e7cc966 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdConfigRefCharterToolSurfaceWiringTest.java @@ -0,0 +1,106 @@ +package dev.ltms.fleet; + +import dev.ltms.fleet.config.ConfigRef; +import dev.ltms.fleet.config.FleetConfig; +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.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * fleetd #474: proves the exact wiring {@code Fleetd.main} uses to construct its live {@code + * ConfigRef} — {@code new ConfigRef(configPath, cfg, Fleetd::assertChartersNameOnlyRegisteredTools)} + * — actually makes {@link ConfigRef#reload()} refuse a charter that names an MCP tool the server + * does not register, the same way {@code Fleetd.main} itself refuses one at startup (see {@code + * FleetdStartupValidationTest#mainRefusesACharterNamingAnUnregisteredTool}). + * + *

{@code dev.ltms.fleet.config.ConfigRefTest} pins the same behaviour through a locally-built + * {@code Consumer} adapter that calls the same production {@code CharterToolSurface} + * method, because that test lives in {@code dev.ltms.fleet.config} and cannot see {@code + * Fleetd#assertChartersNameOnlyRegisteredTools} (package-private to {@code dev.ltms.fleet}). This + * class is the companion proof that lives where the real method reference is visible, so the literal + * expression {@code Fleetd::assertChartersNameOnlyRegisteredTools} — not just an equivalent — is + * what gets exercised. {@code Fleetd.main} itself cannot be driven this far in a unit test: every + * fixture in {@code FleetdStartupValidationTest} is deliberately invalid so {@code main} throws + * before opening a socket, binding Javalin, or doing anything else with a real side effect, so a + * test cannot get {@code main} far enough to hold a running daemon it could then reload — this test + * builds the {@code ConfigRef} the same way {@code main} does and drives {@link ConfigRef#reload()} + * directly instead, the same shape {@code FleetdExhaustionDetectionArmedWiringTest} and its + * siblings already use for the rest of {@code Fleetd.main}'s wiring. + */ +class FleetdConfigRefCharterToolSurfaceWiringTest { + + private static final String BASE = """ + 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 + """; + + @Test + void reloadRefusesACharterNamingAnUnregisteredToolThroughFleetdsOwnWiring(@TempDir Path dir) + throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, BASE + """ + fleet: + charters: + dev: | + Send the final handoff through fleet_reply. + """); + ConfigRef config = new ConfigRef(f, FleetConfig.load(f), + Fleetd::assertChartersNameOnlyRegisteredTools); + FleetConfig before = config.get(); + + Files.writeString(f, BASE + """ + fleet: + charters: + dev: | + Send the final handoff through bridge_send. + """); + ConfigRef.Outcome out = config.reload(); + + assertFalse(out.applied()); + assertNotNull(out.error()); + assertTrue(out.error().contains("dev"), out.error()); + assertTrue(out.error().contains("bridge_send"), out.error()); + assertSame(before, config.get()); + } + + @Test + void reloadAcceptsACharterNamingOnlyRegisteredToolsThroughFleetdsOwnWiring(@TempDir Path dir) + throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, BASE + """ + fleet: + charters: + dev: old charter + """); + ConfigRef config = new ConfigRef(f, FleetConfig.load(f), + Fleetd::assertChartersNameOnlyRegisteredTools); + + Files.writeString(f, BASE + """ + fleet: + charters: + dev: | + Send the final handoff through fleet_reply. + """); + ConfigRef.Outcome out = config.reload(); + + assertTrue(out.applied()); + assertEquals("config reloaded", out.summary()); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java index 6382405..4cf81ba 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java @@ -1,10 +1,13 @@ package dev.ltms.fleet.config; +import dev.ltms.fleet.mcp.CharterToolSurface; 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.util.Map; +import java.util.function.Consumer; import static org.junit.jupiter.api.Assertions.*; @@ -38,6 +41,23 @@ class ConfigRefTest { return new ConfigRef(f, FleetConfig.load(f)); } + /** + * The exact {@code Consumer} {@code Fleetd.main} wires into {@code ConfigRef}'s + * constructor as {@code extraValidation} (fleetd #474) — an adapter from {@code FleetConfig} to + * the raw charter map {@link CharterToolSurface#assertChartersNameOnlyRegisteredTools} takes. + * Built here rather than referencing {@code dev.ltms.fleet.Fleetd} directly, because that method + * is package-private to {@code dev.ltms.fleet} and this test lives in {@code + * dev.ltms.fleet.config} — but it calls the SAME production {@link CharterToolSurface} method + * {@code Fleetd} calls, so this proves the real check runs on reload, not a stand-in for it. + */ + private static final Consumer CHARTER_TOOL_SURFACE = cfg -> + CharterToolSurface.assertChartersNameOnlyRegisteredTools( + cfg.fleet() == null ? Map.of() : cfg.fleet().charters()); + + private static ConfigRef refForWithCharterToolSurface(Path f) { + return new ConfigRef(f, FleetConfig.load(f), CHARTER_TOOL_SURFACE); + } + @Test void aHotChangeIsAppliedAndReadThroughGet(@TempDir Path dir) throws Exception { Path f = dir.resolve("fleetd.yaml"); @@ -124,6 +144,87 @@ class ConfigRefTest { assertSame(before, ref.get()); } + /** + * fleetd #474: {@code validateAll()} (via {@code validateCharters()}) only checks that a + * charter's KEY is a role wire name and its text is non-blank — it never looks at what the text + * names, so a charter naming {@code bridge_send} (the pre-CB-634 name, removed from the tool + * surface — #469's own motivating example) passes {@code validateAll()} and used to be applied + * on reload with nothing refusing it, even though the identical charter refuses {@code + * Fleetd.main} at startup ({@code FleetdStartupValidationTest + * #mainRefusesACharterNamingAnUnregisteredTool}). This is the reload-path proof: it drives + * {@link ConfigRef#reload()} itself (not a direct call to {@link + * CharterToolSurface#assertChartersNameOnlyRegisteredTools}), through the exact {@code + * extraValidation} wiring {@code Fleetd.main} uses, and the failure message must name both the + * charter key and the unknown tool — the same information the startup failure gives (ticket + * acceptance criterion 1). + */ + @Test + void aReloadRefusesACharterNamingAnUnregisteredTool(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(""" + fleet: + charters: + dev: | + Send the final handoff through fleet_reply. + """)); + ConfigRef ref = refForWithCharterToolSurface(f); + FleetConfig before = ref.get(); + + Files.writeString(f, yaml(""" + fleet: + charters: + dev: | + Send the final handoff through bridge_send. + """)); + ConfigRef.Outcome out = ref.reload(); + + assertFalse(out.applied()); + assertNotNull(out.error()); + assertTrue(out.error().contains("dev"), + "expected the charter key 'dev' in the refusal, got: " + out.error()); + assertTrue(out.error().contains("bridge_send"), + "expected the unknown tool 'bridge_send' in the refusal, got: " + out.error()); + assertTrue(out.summary().startsWith("config reload refused"), out.summary()); + // The running config must not move at all — half-applying this would leave the daemon in a + // state that could never have booted, exactly the outcome ConfigRef.java's class doc warns + // a cold-key refusal must avoid, and this check must avoid the same way. + assertSame(before, ref.get()); + assertEquals("Send the final handoff through fleet_reply.\n", + ref.get().fleet().charterFor(dev.ltms.fleet.peer.MemberRole.DEV)); + } + + /** + * fleetd #474 acceptance criterion 3, the positive case: a reload whose charter names only + * registered tools must still be ACCEPTED. An inverted filter (one that refuses every charter, + * or refuses on any {@code fleet_*}/{@code bridge_*} token regardless of registration) would pass + * the refusal test above alone — #469's own M5 mutation cell showed exactly that shape surviving + * a negative-only suite. This is the test that catches it. + */ + @Test + void aReloadAcceptsACharterNamingOnlyRegisteredTools(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(""" + fleet: + charters: + dev: old charter, no tool names + """)); + ConfigRef ref = refForWithCharterToolSurface(f); + + Files.writeString(f, yaml(""" + fleet: + charters: + dev: | + Send the final handoff through fleet_reply, using fleet_send to delegate. + """)); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertTrue(out.deferred().isEmpty(), out.deferred().toString()); + assertEquals("config reloaded", out.summary()); + assertEquals("Send the final handoff through fleet_reply, using fleet_send to delegate.\n", + ref.get().fleet().charterFor(dev.ltms.fleet.peer.MemberRole.DEV)); + } + /** * 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. diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java index f0d700a..35d99fa 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java @@ -762,6 +762,250 @@ class OpenCodeLauncherTest { "no IDE server when ideMcpUrl is unset"); } + // --- fleetd #393: memberSkills seeding must actually reach an opencode member ------------------- + // + // Before this fix, GitWorktrees#seedSkills copied skill folders into EVERY provisioned + // worktree's .claude/skills/ and logged "skill seeding: N of M" regardless of which kind ended + // up spawning into that worktree — a claim that held for kind: claude-code (the CLI discovers + // that directory on its own) but was a guaranteed no-op for kind: opencode, which has no such + // discovery. These tests drive the REAL GitWorktrees#add seeding path (not a hand-built + // .claude/skills/ fixture), then spawn an opencode-kind member against the seeded worktree and + // assert on what the member can actually consume — an instructions[] entry — not on the + // seeding log alone. A minimal, non-hermetic git repo is enough here: unlike + // GitWorktreesTest's own seeding tests, nothing in this file cares about core.excludesFile + // composition, only about what lands in .claude/skills/ and whether OpenCodeLauncher reads it. + + private static void git(Path cwd, String... args) throws Exception { + Process p = new ProcessBuilder(prepend("git", args)).directory(cwd.toFile()) + .redirectErrorStream(true).start(); + String out = new String(p.getInputStream().readAllBytes()); + assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git timed out: git " + String.join(" ", args)); + assertEquals(0, p.exitValue(), "git " + String.join(" ", args) + " failed:\n" + out); + } + + private static List prepend(String head, String... rest) { + List cmd = new ArrayList<>(); + cmd.add(head); + cmd.addAll(List.of(rest)); + return cmd; + } + + private static Path initRepo(Path dir) throws Exception { + Files.createDirectories(dir); + git(dir, "init", "-q", "-b", "main"); + git(dir, "config", "user.email", "test@example.invalid"); + git(dir, "config", "user.name", "Test"); + Files.writeString(dir.resolve("README.md"), "seed\n"); + git(dir, "add", "README.md"); + git(dir, "commit", "-q", "-m", "seed"); + return dir; + } + + @Test + void aSeededSkillReachesTheOpencodeMembersInstructionsArray(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + Path skillsSource = tmp.resolve("skills-src"); + Path skillFile = skillsSource.resolve("implementer").resolve("SKILL.md"); + Files.createDirectories(skillFile.getParent()); + Files.writeString(skillFile, "IMPLEMENTER PROCEDURE\n"); + + // The real seeding path (fleetd #362), not a hand-built .claude/skills/ fixture — proves + // OpenCodeLauncher reads what GitWorktrees#add actually produced. + dev.ltms.fleet.session.GitWorktrees worktrees = + new dev.ltms.fleet.session.GitWorktrees(tmp.resolve("wts").toString(), null, skillsSource.toString()); + String wt = worktrees.add(repo.toString(), "cb-393-opencode", "HEAD"); + Path seededSkillMd = Path.of(wt, ".claude", "skills", "implementer", "SKILL.md"); + assertTrue(Files.exists(seededSkillMd), + "sanity: the real seeding step must have copied the skill into the worktree"); + + Logger logger = (Logger) LoggerFactory.getLogger(OpenCodeLauncher.class); + Level original = logger.getLevel(); + logger.setLevel(Level.INFO); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + + String cfgPath; + try { + // No mcpUrl, no ideUrl, no fleet (no charter): the seeded skill alone must be enough to + // trigger OPENCODE_CONFIG — proves the gate itself was updated, not only writeConfig's body. + FakeHerdr herdr = new FakeHerdr(); + Path configRoot = Files.createDirectory(tmp.resolve("configs")); + service(herdr, configRoot, opencodeIdeCfg(null, null, wt)).spawn(); + cfgPath = startEnv(herdr).get("OPENCODE_CONFIG"); + } finally { + logger.detachAppender(appender); + logger.setLevel(original); + } + + assertNotNull(cfgPath, "a seeded skill with nothing else configured must still write a config"); + JsonNode json = new ObjectMapper().readTree(Path.of(cfgPath).toFile()); + List instructions = new ArrayList<>(); + json.path("instructions").forEach(n -> instructions.add(n.asText())); + assertTrue(instructions.contains(seededSkillMd.toAbsolutePath().toString()), + "the seeded skill's SKILL.md must be an instructions[] entry — got: " + instructions); + + List infos = appender.list.stream() + .filter(e -> e.getLevel() == Level.INFO) + .map(ILoggingEvent::getFormattedMessage) + .toList(); + assertTrue(infos.stream().anyMatch(m -> m.contains("skill delivery") && m.contains("1 of 1")), + "the launcher must log, kind-aware, that it delivered the skill — got:\n" + infos); + } + + @Test + void aSkillFolderWithoutSkillMdIsNeverDeliveredAndTheLogNamesIt(@TempDir Path tmp) throws Exception { + Path wt = Files.createDirectories(tmp.resolve("wt")); + Path goodSkill = wt.resolve(".claude").resolve("skills").resolve("implementer"); + Files.createDirectories(goodSkill); + Files.writeString(goodSkill.resolve("SKILL.md"), "GOOD\n"); + Path halfShipped = wt.resolve(".claude").resolve("skills").resolve("half-shipped"); + Files.createDirectories(halfShipped); + Files.writeString(halfShipped.resolve("README.md"), "no SKILL.md here\n"); + + Logger logger = (Logger) LoggerFactory.getLogger(OpenCodeLauncher.class); + Level original = logger.getLevel(); + logger.setLevel(Level.INFO); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + + String cfgPath; + try { + FakeHerdr herdr = new FakeHerdr(); + Path configRoot = Files.createDirectory(tmp.resolve("configs")); + service(herdr, configRoot, opencodeIdeCfg(null, null, wt.toString())).spawn(); + cfgPath = startEnv(herdr).get("OPENCODE_CONFIG"); + } finally { + logger.detachAppender(appender); + logger.setLevel(original); + } + + JsonNode json = new ObjectMapper().readTree(Path.of(cfgPath).toFile()); + List instructions = new ArrayList<>(); + json.path("instructions").forEach(n -> instructions.add(n.asText())); + assertTrue(instructions.contains(goodSkill.resolve("SKILL.md").toAbsolutePath().toString()), + "the well-formed skill is still delivered alongside the malformed one"); + assertFalse(instructions.stream().anyMatch(i -> i.contains("half-shipped")), + "a skill folder with no SKILL.md can never become an instructions[] entry"); + + List infos = appender.list.stream() + .filter(e -> e.getLevel() == Level.INFO) + .map(ILoggingEvent::getFormattedMessage) + .toList(); + assertTrue(infos.stream().anyMatch(m -> m.contains("skill delivery") && m.contains("1 of 2") + && m.contains("half-shipped") && m.contains("could not be delivered")), + "the log must say plainly which folder could not be consumed and why — got:\n" + infos); + } + + // --- fleetd #393 follow-up: instructions[] has three writers (charter, seeded skills, IDE + // rules), and no test above ever exercises more than one or two of them together. A writer + // that flips from withArray (get-or-create) to putArray (create-or-REPLACE) silently deletes + // every entry written before it — proven live on this branch's merge: switching just the + // skills writer to putArray left the entire suite (1603 tests) green while deleting the + // charter entry an opencode member needs for its role contract. That hazard was found by the + // fleet01 lead and independently verified against this branch; it is not a defect in the + // skills-delivery or logging tests above, which both hold up under their own mutations — the + // gap is that none of them combine all three writers in one config. + // + // These three tests assert instructions[] CONTENT as an exact, ordered list, not a size or a + // "contains" check: a putArray mutation can replace N entries with a different N entries of + // the same count, so only a content comparison can tell "all three paths present" apart from + // "two paths present that replaced the earlier ones". + + @Test + void instructionsArrayHoldsExactlyTheCharterWhenNothingElseWritesToIt(@TempDir Path root) throws Exception { + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Fleet fleet = new FleetConfig.Fleet(Map.of(), Map.of(), Map.of(), Map.of(), + Map.of("dev", "role rule"), null); + Path cwd = Files.createDirectory(root.resolve("checkout")); + service(herdr, root, opencodeIdeCfg(null, null, cwd.toString()), () -> fleet).spawn(); + + String cfgPath = startEnv(herdr).get("OPENCODE_CONFIG"); + assertNotNull(cfgPath, "a role charter alone still writes a config"); + JsonNode json = new ObjectMapper().readTree(Path.of(cfgPath).toFile()); + Path charter = Path.of(cfgPath).resolveSibling("member-charter.md"); + assertTrue(Files.exists(charter), "the charter file was written"); + + List instructions = new ArrayList<>(); + json.path("instructions").forEach(n -> instructions.add(n.asText())); + assertEquals(List.of(charter.toAbsolutePath().toString()), instructions, + "with only the charter writer active, instructions[] holds exactly one entry: the " + + "charter — got: " + instructions); + } + + @Test + void instructionsArrayHoldsCharterThenIdeRulesInOrder(@TempDir Path root) throws Exception { + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Fleet fleet = new FleetConfig.Fleet(Map.of(), Map.of(), Map.of(), Map.of(), + Map.of("dev", "role rule"), null); + Path cwd = Files.createDirectory(root.resolve("checkout")); + service(herdr, root, opencodeIdeCfg(null, + "http://127.0.0.1:29170/index-mcp/streamable-http", cwd.toString()), () -> fleet).spawn(); + + String cfgPath = startEnv(herdr).get("OPENCODE_CONFIG"); + assertNotNull(cfgPath, "charter + IDE rules still writes a config"); + JsonNode json = new ObjectMapper().readTree(Path.of(cfgPath).toFile()); + Path charter = Path.of(cfgPath).resolveSibling("member-charter.md"); + Path rules = Path.of(cfgPath).resolveSibling("ide-rules.md"); + assertTrue(Files.exists(charter), "the charter file was written"); + assertTrue(Files.exists(rules), "the ide-rules file was written"); + + List instructions = new ArrayList<>(); + json.path("instructions").forEach(n -> instructions.add(n.asText())); + assertEquals(List.of(charter.toAbsolutePath().toString(), rules.toAbsolutePath().toString()), + instructions, + "with charter + IDE-rules writers active, instructions[] holds both, charter first — " + + "got: " + instructions); + } + + @Test + void instructionsArrayHoldsCharterThenSkillsThenIdeRulesInOrder(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + Path skillsSource = tmp.resolve("skills-src"); + Path skillFile = skillsSource.resolve("implementer").resolve("SKILL.md"); + Files.createDirectories(skillFile.getParent()); + Files.writeString(skillFile, "IMPLEMENTER PROCEDURE\n"); + + dev.ltms.fleet.session.GitWorktrees worktrees = new dev.ltms.fleet.session.GitWorktrees( + tmp.resolve("wts").toString(), null, skillsSource.toString()); + String wt = worktrees.add(repo.toString(), "cb-393-follow-up", "HEAD"); + Path seededSkillMd = Path.of(wt, ".claude", "skills", "implementer", "SKILL.md"); + assertTrue(Files.exists(seededSkillMd), "sanity: the real seeding step copied the skill"); + + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Fleet fleet = new FleetConfig.Fleet(Map.of(), Map.of(), Map.of(), Map.of(), + Map.of("dev", "role rule"), null); + Path configRoot = Files.createDirectory(tmp.resolve("configs")); + service(herdr, configRoot, opencodeIdeCfg(null, + "http://127.0.0.1:29170/index-mcp/streamable-http", wt), () -> fleet).spawn(); + + String cfgPath = startEnv(herdr).get("OPENCODE_CONFIG"); + assertNotNull(cfgPath, "charter + skills + IDE rules still writes a config"); + JsonNode json = new ObjectMapper().readTree(Path.of(cfgPath).toFile()); + Path charter = Path.of(cfgPath).resolveSibling("member-charter.md"); + Path rules = Path.of(cfgPath).resolveSibling("ide-rules.md"); + assertTrue(Files.exists(charter), "the charter file was written"); + assertTrue(Files.exists(rules), "the ide-rules file was written"); + + List instructions = new ArrayList<>(); + json.path("instructions").forEach(n -> instructions.add(n.asText())); + // Exact ordered list, not size or "contains": a putArray mutation on any writer after the + // charter replaces every entry written before it, and the replacement can still be a + // plausible-looking array of a different shape. This is the one combination all three + // writers are active for — and per fleet01 the realistic shape on a host where weighted + // placement makes opencode the default for most members. + assertEquals(List.of(charter.toAbsolutePath().toString(), + seededSkillMd.toAbsolutePath().toString(), + rules.toAbsolutePath().toString()), + instructions, + "with all three writers active, instructions[] must hold charter, then the seeded " + + "skill, then IDE rules — in that order and with nothing replaced. A " + + "putArray mutation on any writer after the charter would silently drop " + + "earlier entries here while still producing a same-shaped array — got: " + + instructions); + } + // --- fleetd #219: config root + discovery root under memberHerdrSocket ------------------------ /** A config with {@code memberHerdrSocket:} set, and optionally {@code worktreeRoot:}/{@code worktreeGroup:}. */