From d4a2cd720cf76be79f8a4ae2f2021fc5741fbdf8 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 19:37:00 +0700 Subject: [PATCH 1/2] 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:}. */ From 9e4e423ad68b7cdd38c044142e13c60b21eae562 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 19:58:25 +0700 Subject: [PATCH 2/2] 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:}. */