From d4a2cd720cf76be79f8a4ae2f2021fc5741fbdf8 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 19:37:00 +0700 Subject: [PATCH 1/3] fleetd #393: deliver memberSkills to opencode members, and stop overclaiming seeding success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitWorktrees.seedSkills copies memberSkills:-seeded skill folders into every provisioned worktree's .claude/skills/ and logged "skill seeding: N of M" as if that were success — but .claude/skills/ is a Claude Code CLI convention. opencode has no such discovery, so a kind: opencode member never actually read a seeded skill even though the log said N of M succeeded. Two changes, both required: 1. Deliver it. OpenCodeLauncher.skillInstructionFiles scans /.claude/skills/*/SKILL.md at spawn time (the one point the launcher knows both the kind and the cwd) and appends each to the generated opencode.json's instructions[] array, the same channel already used for the member charter and IDE rules. A skill folder with no SKILL.md is named and skipped rather than silently dropped. 2. Stop claiming it where the claim can't be verified. GitWorktrees.seedSkills' log now says explicitly that consumption depends on the member's kind and points at the launcher's own log; OpenCodeLauncher logs its own kind-aware "skill delivery: M of N ..." line once the kind is actually known, naming any folder it could not turn into an instructions[] entry. fleetd.example.yaml's memberSkills: doc previously claimed "Claude Code members only; an opencode member reads a different path (.opencode/agent) this key does not touch" — false as of this fix, corrected to name both kinds and how each consumes it. Tests: OpenCodeLauncherTest gains two cases driving the real GitWorktrees#add seeding path (not a hand-built fixture) into an opencode-kind spawn — one asserting a seeded skill's SKILL.md lands in instructions[] plus the honest log line, one covering a skill folder without SKILL.md (delivered skills still flow, the malformed one is named in the log and excluded from instructions[]). ClaudeCodeLauncher is untouched — its native .claude/skills/ discovery already worked and is out of scope. mvn -B clean test: Tests run: 1603, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS --- fleetd/fleetd.example.yaml | 21 ++- .../ltms/fleet/member/OpenCodeLauncher.java | 94 +++++++++++- .../dev/ltms/fleet/session/GitWorktrees.java | 30 ++-- .../fleet/member/OpenCodeLauncherTest.java | 136 ++++++++++++++++++ 4 files changed, 262 insertions(+), 19 deletions(-) diff --git a/fleetd/fleetd.example.yaml b/fleetd/fleetd.example.yaml index 08e97f9..7a0dfd8 100644 --- a/fleetd/fleetd.example.yaml +++ b/fleetd/fleetd.example.yaml @@ -793,12 +793,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/member/OpenCodeLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java index e7330dc..c1aed92 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(); @@ -450,6 +527,15 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { root.putArray("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()) { // One shared mcp node for both servers — putObject would replace the node (and thus // the other server) on the second call, so build into a single get-or-create node. 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/member/OpenCodeLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java index f0d700a..755e350 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,142 @@ 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 #219: config root + discovery root under memberHerdrSocket ------------------------ /** A config with {@code memberHerdrSocket:} set, and optionally {@code worktreeRoot:}/{@code worktreeGroup:}. */ -- 2.52.0 From 9e4e423ad68b7cdd38c044142e13c60b21eae562 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 19:58:25 +0700 Subject: [PATCH 2/3] fleetd #393 follow-up: remove the instructions[] writer-ordering hazard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenCodeLauncher.writeConfig has three writers into the instructions[] array (charter, seeded skills, IDE rules). The charter writer used putArray (create-or-REPLACE) instead of withArray (get-or-create), which "worked" only because it happened to run first against a still-empty array — an undeclared ordering dependency nothing tested. Found by the fleet01 lead and verified on this branch's merge: flipping the skills writer to putArray left the full 1603-test suite green while silently deleting the charter entry, which would launch an opencode member with no role contract at all. Fix: charter's putArray -> withArray (one-word change, behavior-identical today). Add three tests asserting instructions[] CONTENT as an exact ordered list (not size) across writer combinations: charter only, charter + IDE rules, and charter + IDE rules + seeded skills. Mutation testing (see PR body) shows the skills and IDE-rules writers are each independently detectable by name; the charter writer's own mutation is not detectable by any test, because it structurally always runs first against an empty array, so putArray and withArray are equivalent there. --- .../ltms/fleet/member/OpenCodeLauncher.java | 14 ++- .../fleet/member/OpenCodeLauncherTest.java | 108 ++++++++++++++++++ 2 files changed, 121 insertions(+), 1 deletion(-) 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 c1aed92..893e7f4 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java @@ -524,7 +524,19 @@ 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 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 755e350..35d99fa 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java @@ -898,6 +898,114 @@ class OpenCodeLauncherTest { "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:}. */ -- 2.52.0 From 97e4c1d6581d1f3d2e62a5039037215aec3bb638 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 20:25:23 +0700 Subject: [PATCH 3/3] fleetd #474: ConfigRef.reload() runs the charter tool-surface gate too A charter naming an MCP tool the server does not register refused Fleetd.main at startup but slipped through ConfigRef.reload(), because reload() only ran FleetConfig.validateAll(), which never looks at what a charter's text names. CharterToolSurface stays in the mcp package (config must not depend on it), so ConfigRef now accepts the check as a Consumer extraValidation, run inside reload()'s same try/catch as validateAll(). Fleetd.main wires a new package-private adapter, Fleetd.assertChartersNameOnlyRegisteredTools, into both the startup call site and ConfigRef's constructor, so the two call sites can never check different things. Tests: ConfigRefTest (reload refuses/accepts, via a locally-built equivalent consumer since Fleetd's method is package-private to dev.ltms.fleet) and the new FleetdConfigRefCharterToolSurfaceWiringTest (same proof through the exact Fleetd::assertChartersNameOnlyRegisteredTools reference production uses). Verified deleting the new extraValidation.accept(fresh) call site fails both new "refuses" tests by name. --- .../src/main/java/dev/ltms/fleet/Fleetd.java | 33 +++++- .../java/dev/ltms/fleet/config/ConfigRef.java | 42 +++++++ .../ltms/fleet/mcp/CharterToolSurface.java | 14 +++ ...ConfigRefCharterToolSurfaceWiringTest.java | 106 ++++++++++++++++++ .../dev/ltms/fleet/config/ConfigRefTest.java | 101 +++++++++++++++++ 5 files changed, 292 insertions(+), 4 deletions(-) create mode 100644 fleetd/src/test/java/dev/ltms/fleet/FleetdConfigRefCharterToolSurfaceWiringTest.java 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/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. -- 2.52.0