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