diff --git a/fleetd/fleetd.example.yaml b/fleetd/fleetd.example.yaml index 4e4f7ea..2097fcf 100644 --- a/fleetd/fleetd.example.yaml +++ b/fleetd/fleetd.example.yaml @@ -103,7 +103,12 @@ bind: # # handoverPath: REQUIRED when this block is present — where the handover file a fresh lead session # reads must live. No default (an operator-specific path); a present block with no -# handoverPath refuses to start. +# handoverPath refuses to start. May be relative: it then resolves against the +# CALLING lead's own fleet.leaders..cwd (falling back to the daemon's own +# working directory when that lead has none configured) — never against whatever +# directory the daemon process happens to have been started in. An absolute path is +# used unchanged. Prefer an absolute path if the daemon and the lead's pane might not +# share a working directory (fleetd #480 follow-up). # requireOperatorConfirm: true # default true — confirm() refuses unless the caller also passes # # operatorConfirmed: true # maxDocAgeSeconds: 3600 # default 3600 — refuse a handover file older than this @@ -114,8 +119,8 @@ bind: # clearSettleSeconds: 20 # default 20 — how long to wait for the pane to become injectable # # again AFTER /clear before giving up (never sends bootstrapText # # if this elapses). A separate, second wait from turnSettleSeconds. -# bootstrapText: "..." # default names handoverPath — sent to the lead once its pane -# # settles after /clear +# bootstrapText: "..." # default names the RESOLVED (absolute) handoverPath — sent to +# # the lead once its pane settles after /clear # leadRollover: # handoverPath: /path/to/handover.md # requireOperatorConfirm: true diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index ae2e929..429e1c7 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -600,7 +600,7 @@ public final class Fleetd { // layer from the connection, the same way auth/CallerResolver#resolve builds a // Principal.leader(...)), never from a single-slot lookup — see LeadRollover's class // javadoc, fleetd #480 correction 2. - LeadRollover leadRollover = leadRollover(cfg, router.leadAgents(), config); + LeadRollover leadRollover = leadRollover(cfg, router.leadAgents(), config, leads); MessageService messages = new MessageService(router, injector, rendezvous, replyInbox, pushLoop, metrics); @@ -1089,20 +1089,44 @@ public final class Fleetd { * the connection and pass it in, never take it as a request field. See {@link LeadRollover}'s * class javadoc. * - * @param cfg the startup config snapshot — read ONCE here, only to decide whether to - * construct the object at all, exactly like {@code cfg.leadHeartbeat()} - * @param leadAgents the {@link AgentControl} instance that reaches the LEAD's pane (not - * {@code memberAgents}), normally {@code router.leadAgents()} - * @param config the live {@link ConfigRef}, captured only inside the returned supplier — - * never dereferenced here + *

fleetd #480 follow-up: also builds the terminal → lead-workspace lookup + * {@link LeadRollover#open} needs to resolve a relative {@code handoverPath} against the + * CALLING lead's own {@code cwd} rather than the daemon's — the daemon and a lead's pane can + * have different working directories (this repo nests {@code fleetd/} inside its own root, so + * they already differ on this host). The lookup is terminal → lead name (via {@code + * liveLeadTerminals}) → that lead's {@code cwd} (via {@code config.get().fleet().leaders()}), + * and both hops are read LIVE on every call, never off a snapshot taken here: leads are + * discovered by a live tab scan ({@code LeadTabScanner}), so a map captured at construction + * time could be empty (no lead has been scanned yet) or stale (a lead added since). + * + * @param cfg the startup config snapshot — read ONCE here, only to decide whether + * to construct the object at all, exactly like {@code + * cfg.leadHeartbeat()} + * @param leadAgents the {@link AgentControl} instance that reaches the LEAD's pane (not + * {@code memberAgents}), normally {@code router.leadAgents()} + * @param config the live {@link ConfigRef}, captured only inside the returned + * supplier and the workspace lookup — never dereferenced here + * @param liveLeadTerminals terminal id → lead NAME for every CURRENTLY recognised lead, normally + * the same {@code leads} supplier {@code main} already builds for + * {@code HerdrRouter}/{@link #leadSeatLookup} — never a value snapshot * @return a constructed {@link LeadRollover}, or {@code null} when {@code leadRollover:} is * absent from the startup config */ - static LeadRollover leadRollover(FleetConfig cfg, AgentControl leadAgents, ConfigRef config) { + static LeadRollover leadRollover(FleetConfig cfg, AgentControl leadAgents, ConfigRef config, + Supplier> liveLeadTerminals) { if (cfg.leadRollover() == null) { return null; } - return new LeadRollover(leadAgents, () -> config.get().leadRollover()); + Function leadWorkspace = terminal -> { + String leadName = liveLeadTerminals.get().get(terminal); + if (leadName == null) { + return null; + } + FleetConfig.Fleet fleet = config.get().fleet(); + FleetConfig.Leader leader = fleet == null ? null : fleet.leaders().get(leadName); + return leader == null ? null : leader.cwd(); + }; + return new LeadRollover(leadAgents, () -> config.get().leadRollover(), leadWorkspace); } /** diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java index 6eb1483..b53707d 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java @@ -1369,7 +1369,13 @@ public record FleetConfig( * lead session reads must live. There is no sane non-null default for an * operator-specific path, so a present block with a {@code null}/blank * {@code handoverPath} is refused at config load; see - * {@link #validateLeadRollover()}. + * {@link #validateLeadRollover()}. May be relative: {@code + * dev.ltms.fleet.lead.LeadRollover#open} resolves a relative path against + * the CALLING lead's configured {@code fleet.leaders..cwd}, falling + * back to the daemon's own working directory ({@code + * System.getProperty("user.dir")}) when that lead has none configured — + * never against whatever directory the daemon process happens to have been + * started in for its own sake. An absolute path is used unchanged. * @param requireOperatorConfirm default {@code true} — {@code confirm()} refuses unless the * caller also passes {@code operatorConfirmed: true}. Set {@code false} to * let the three handover-file checks alone gate the roll. @@ -1382,9 +1388,14 @@ public record FleetConfig( * @param clearSettleSeconds default 20 — bound on how long to wait for the lead's pane to * report an injectable state again after {@code /clear} before giving up. A * roll that times out here never sends {@code bootstrapText}. - * @param bootstrapText default a sentence naming {@code handoverPath} — sent to the lead's pane - * once it settles after {@code /clear}, telling the fresh session where to - * read the handover and carry on. + * @param bootstrapText default a sentence naming the RESOLVED handover path — sent to the + * lead's pane once it settles after {@code /clear}, telling the fresh + * session where to read the handover and carry on. Left {@code null} here + * when the operator configures none: the default sentence cannot be built + * at construction time because it must name the path AFTER {@code + * dev.ltms.fleet.lead.LeadRollover#open} has resolved a relative {@code + * handoverPath} against the calling lead's workspace, which this record has + * no way to know — see {@link #bootstrapTextFor(String)}. */ @JsonIgnoreProperties(ignoreUnknown = true) public record LeadRollover(String handoverPath, Boolean requireOperatorConfirm, @@ -1395,10 +1406,24 @@ public record FleetConfig( maxDocAgeSeconds = (maxDocAgeSeconds == null || maxDocAgeSeconds <= 0) ? 3600 : maxDocAgeSeconds; turnSettleSeconds = (turnSettleSeconds == null || turnSettleSeconds <= 0) ? 20 : turnSettleSeconds; clearSettleSeconds = (clearSettleSeconds == null || clearSettleSeconds <= 0) ? 20 : clearSettleSeconds; - bootstrapText = (bootstrapText == null || bootstrapText.isBlank()) - ? "Fresh lead session: read the handover file at " + handoverPath - + " and carry on from there." - : bootstrapText; + bootstrapText = (bootstrapText == null || bootstrapText.isBlank()) ? null : bootstrapText; + } + + /** + * The text actually sent to the lead's pane once it settles after {@code /clear}: the + * operator's configured {@link #bootstrapText} when one is set, otherwise the default + * sentence built from {@code resolvedHandoverPath}. + * + * @param resolvedHandoverPath the ABSOLUTE path {@code dev.ltms.fleet.lead.LeadRollover + * #open} already resolved — never the raw configured {@link + * #handoverPath}, which may still be relative and would name a + * directory the fresh lead session's own pane cannot resolve + */ + public String bootstrapTextFor(String resolvedHandoverPath) { + return bootstrapText != null + ? bootstrapText + : "Fresh lead session: read the handover file at " + resolvedHandoverPath + + " and carry on from there."; } } diff --git a/fleetd/src/main/java/dev/ltms/fleet/lead/LeadRollover.java b/fleetd/src/main/java/dev/ltms/fleet/lead/LeadRollover.java index c22ee8e..29cbb0c 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/lead/LeadRollover.java +++ b/fleetd/src/main/java/dev/ltms/fleet/lead/LeadRollover.java @@ -15,6 +15,7 @@ import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; +import java.util.function.Function; import java.util.function.LongSupplier; import java.util.function.Supplier; @@ -52,7 +53,7 @@ import java.util.function.Supplier; *

  • {@code agents.send(lead, "/clear")}
    • *
  • wait again for the pane to report injectable, bounded by {@code clearSettleSeconds} (this * is the original, pre-correction wait — still here, just no longer the only one)
    • - *
  • {@code agents.send(lead, cfg.bootstrapText())}
    • + *
  • {@code agents.send(lead, cfg.bootstrapTextFor(p.handoverPath()))}
    • * * A {@link #confirm} that returns {@link RollDecision#approved()} therefore means "every gate * passed and the roll is scheduled", never "the pane has been cleared" — the pane may @@ -87,6 +88,21 @@ import java.util.function.Supplier; * build a {@code Principal.leader(...)} (see its use of {@code ConnectionIdentity.Caller#terminal}) * — never a value the client supplies or chooses. The later MCP-tool unit that wires * {@link #open}/{@link #confirm} must pass the resolved caller terminal, not a request field. + * + *

    A relative {@code handoverPath} resolves against the CALLING lead's workspace, never + * the daemon's own cwd — a fleetd #480 follow-up. The daemon and a lead's own pane can + * have different working directories (this repo nests {@code fleetd/} inside its own root, so the + * daemon's cwd and {@code fleet.leaders..cwd} already differ on this host). {@link #open} + * resolves {@code cfg.handoverPath()} to an ABSOLUTE path exactly once — against {@code + * leadWorkspace.apply(leadTerminal)} when that lookup returns a non-null, non-blank workspace, and + * against {@code System.getProperty("user.dir")} otherwise (the same fallback {@code + * LeadLauncher#launch} already uses for a lead with no configured {@code cwd}) — and stores only + * that absolute path on {@link PendingRollover}. Every later read of {@code + * PendingRollover#handoverPath()} (the freshness/exists/empty checks in {@link #checkHandover}, + * the value {@code FleetMcp} hands back to the lead in the {@code open} response so it knows where + * to WRITE the file, and {@link FleetConfig.LeadRollover#bootstrapTextFor} which names it in the + * text typed into the fresh session) therefore already sees the resolved absolute form and never + * needs to resolve anything itself. */ public final class LeadRollover { @@ -100,6 +116,11 @@ public final class LeadRollover { * * @param leadTerminal the lead pane that opened this request — the only terminal that may * later {@link #confirm} it (see {@link RefusalReason#NOT_YOUR_ROLLOVER}) + * @param handoverPath the ABSOLUTE, resolved handover path — never the raw configured value, + * which may have been relative. {@link #open} resolves it once, against the + * calling lead's workspace, before storing it here; see this class's + * javadoc. This is the value the MCP layer hands back to the lead as + * "write your file here", so callers may rely on it always being absolute. */ public record PendingRollover(String token, String leadTerminal, String handoverPath, long requestedAtMillis) {} @@ -148,6 +169,14 @@ public final class LeadRollover { private final AgentControl agents; private final Supplier configSupplier; + /** + * Terminal id → that lead's configured workspace directory (their {@code + * fleet.leaders..cwd}), or {@code null} when the terminal names no currently-recognised + * lead. {@link #open} calls this to resolve a relative {@code handoverPath} — see this class's + * javadoc. Required: there is no sane default that would not silently reintroduce the + * daemon-cwd bug this parameter exists to fix. + */ + private final Function leadWorkspace; private final LongSupplier nowMillis; private final Runnable settleSleeper; /** @@ -160,8 +189,9 @@ public final class LeadRollover { private final Map pending = new ConcurrentHashMap<>(); /** Production constructor — wall clock, real sleep between settle polls, a real virtual thread. */ - public LeadRollover(AgentControl agents, Supplier configSupplier) { - this(agents, configSupplier, System::currentTimeMillis, + public LeadRollover(AgentControl agents, Supplier configSupplier, + Function leadWorkspace) { + this(agents, configSupplier, leadWorkspace, System::currentTimeMillis, () -> sleepUninterruptibly(SETTLE_POLL_MS), r -> Thread.ofVirtual().name("lead-rollover-continuation-").start(r)); } @@ -174,9 +204,11 @@ public final class LeadRollover { * nanoTime} freezes while the host sleeps (fleetd #386). */ LeadRollover(AgentControl agents, Supplier configSupplier, - LongSupplier nowMillis, Runnable settleSleeper, Consumer continuationRunner) { + Function leadWorkspace, LongSupplier nowMillis, + Runnable settleSleeper, Consumer continuationRunner) { this.agents = agents; this.configSupplier = configSupplier; + this.leadWorkspace = leadWorkspace; this.nowMillis = nowMillis; this.settleSleeper = settleSleeper; this.continuationRunner = continuationRunner; @@ -215,13 +247,67 @@ public final class LeadRollover { } String token = UUID.randomUUID().toString(); long requestedAt = nowMillis.getAsLong(); - PendingRollover p = new PendingRollover(token, leadTerminal, cfg.handoverPath(), requestedAt); + String resolvedPath = resolveHandoverPath(cfg.handoverPath(), leadTerminal); + PendingRollover p = new PendingRollover(token, leadTerminal, resolvedPath, requestedAt); pending.put(token, p); - log.info("lead-rollover: open token={} lead={} handoverPath={} reason={}", - token, leadTerminal, p.handoverPath(), reason); + if (resolvedPath.equals(cfg.handoverPath())) { + log.info("lead-rollover: open token={} lead={} handoverPath={} reason={}", + token, leadTerminal, resolvedPath, reason); + } else { + // The configured value was relative (or merely un-normalized) and resolved to a + // different string — log both, so an operator reading this line can see which + // directory the daemon actually looked in, not just the value it was given. + log.info("lead-rollover: open token={} lead={} configuredHandoverPath={} " + + "resolvedHandoverPath={} reason={}", + token, leadTerminal, cfg.handoverPath(), resolvedPath, reason); + } return p; } + /** + * Resolve {@code configured} to an absolute path exactly once, here, so nothing downstream + * ({@link #checkHandover}, the MCP layer's {@code open} response, {@link + * FleetConfig.LeadRollover#bootstrapTextFor}) ever has to resolve — or worse, silently + * mis-resolve — a relative path again. + * + *

    The return value is GUARANTEED absolute, not merely usually absolute. + * {@code leadWorkspace.apply(leadTerminal)} returns an operator-configured {@code + * fleet.leaders..cwd} string, and nothing forces an operator to write an absolute one — + * a relative {@code cwd} resolved with plain {@link Path#resolve} would still yield a relative + * result, silently reopening the exact bug this class exists to fix (every later reader back to + * interpreting an ambiguous string against ITS OWN working directory). {@link + * Path#toAbsolutePath()} closes that: it resolves any remaining relative path against {@code + * user.dir} (the JVM's own cwd), which is the correct base for an operator-written path the + * daemon process itself is meant to interpret, exactly like the {@code user.dir} fallback used + * below. Applying it unconditionally on both branches means the ALREADY-absolute branch stays a + * no-op (an absolute path is unaffected by {@code toAbsolutePath()}) while the relative-{@code + * cwd} branch above is closed the same way. + * + *

    + */ + private String resolveHandoverPath(String configured, String leadTerminal) { + Path path = Path.of(configured); + if (path.isAbsolute()) { + // toAbsolutePath() is a no-op for an already-absolute path — kept here anyway so both + // branches call the exact same guarantee, rather than one branch relying on + // isAbsolute() alone to already imply what toAbsolutePath() enforces. + return path.toAbsolutePath().normalize().toString(); + } + String workspace = leadWorkspace.apply(leadTerminal); + Path base = (workspace == null || workspace.isBlank()) + ? Path.of(System.getProperty("user.dir")) + : Path.of(workspace); + return base.resolve(path).toAbsolutePath().normalize().toString(); + } + /** * Validate every gate, then — if and only if all of them pass — hand a one-shot continuation * that performs the actual roll to {@code continuationRunner} and return. This method @@ -302,7 +388,7 @@ public final class LeadRollover { lead, cfg.clearSettleSeconds(), p.token()); return; } - agents.send(lead, cfg.bootstrapText()); + agents.send(lead, cfg.bootstrapTextFor(p.handoverPath())); log.info("lead-rollover: rolled token={} lead={}", p.token(), lead); } @@ -313,7 +399,9 @@ public final class LeadRollover { /** * The three handover-file checks, in order: exists, not empty, fresh (modified after - * {@link #open}'s timestamp and not older than {@code maxDocAgeSeconds}). + * {@link #open}'s timestamp and not older than {@code maxDocAgeSeconds}). Stats {@code + * p.handoverPath()} directly — {@link #open} already resolved it to an absolute path, so this + * never has to guess which directory it means. * * @return the first failing check's refusal, or {@code null} when all three pass */ diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadRolloverWiringTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadRolloverWiringTest.java index 9e536d8..c792e53 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadRolloverWiringTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadRolloverWiringTest.java @@ -13,16 +13,31 @@ import static org.junit.jupiter.api.Assertions.assertTrue; * FleetdCompletionResolverWiringTest}'s pattern — five log-only reporters in {@code Fleetd.main} * already survived mutation batteries this exact way (fleetd #415's extraction antidote note). * - *

    No behavioural test can catch this wiring dropping out: {@code LeadRolloverTest} constructs - * its own {@code LeadRollover} directly (as every prior test of an extracted factory does), so a - * mutation that deletes the {@code leadRollover(...)} call from {@code main} — or replaces one of - * its arguments with something that silently compiles, e.g. {@code router.leadAgents()} swapped - * for {@code null}, or the whole assignment swapped for a bare {@code null} literal — leaves every - * behavioural test green. This is a plain string read, guarded by an unrelated anchor assertion so - * a broken or empty file read cannot pass as a real change. + *

    What this class still covers, and what it never claimed to. {@code LeadRolloverTest} + * constructs its own {@code LeadRollover} directly (as every prior test of an extracted factory + * does) with a hand-built lookup, so a mutation that deletes the {@code leadRollover(...)} call + * from {@code main} — or replaces one of its arguments with something that still compiles, e.g. + * {@code router.leadAgents()} swapped for {@code null}, or the whole assignment swapped for a bare + * {@code null} literal — leaves every behavioural test green. This is a plain string read, guarded + * by an unrelated anchor assertion so a broken or empty file read cannot pass as a real change. * *

    This test checks source text, not runtime behaviour. It never constructs a {@code - * LeadRollover} and never runs {@code main}. + * LeadRollover} and never runs {@code main}. It pins the {@code leadRollover(...)} CALL SITE's + * argument list — that {@code main} still passes {@code leads} at all — never what the factory + * DOES with that argument once inside its own body. + * + *

    Correction (fleetd #480 relative-handover-path follow-up): that gap used to be real, and + * now is not — but not here. This class's javadoc previously claimed "no behavioural test can + * catch this wiring dropping out" for the whole factory, including the lambda {@code + * leadRollover(...)} builds internally (terminal → lead name → {@code Leader.cwd()}). That claim + * was proven true at the time — mutating that lambda's body to {@code String leadName = null;} + * (always "no lead found", which silently reintroduces the daemon-cwd bug this ticket fixes) left + * the full suite green, {@code Tests run: 1669, Failures: 0}. It is no longer true: {@code + * FleetdLeadRolloverWorkspaceLookupTest} now calls {@code Fleetd.leadRollover(...)} directly with a + * real {@link dev.ltms.fleet.config.ConfigRef} built from a temp {@code fleetd.yaml}, and fails + * against that exact one-line mutation. So: THIS class still covers only the call site's argument + * list; {@code FleetdLeadRolloverWorkspaceLookupTest} is what now covers the lambda's body. Neither + * one subsumes the other — keep both. */ class FleetdLeadRolloverWiringTest { @@ -49,14 +64,16 @@ class FleetdLeadRolloverWiringTest { void mainStillCallsTheLeadRolloverFactory() throws Exception { String source = fleetdSource(); assertTrue(source.contains( - "LeadRollover leadRollover = leadRollover(cfg, router.leadAgents(), config);"), + "LeadRollover leadRollover = leadRollover(cfg, router.leadAgents(), config, leads);"), "Fleetd.main must still assign `LeadRollover leadRollover = leadRollover(cfg, " - + "router.leadAgents(), config);`. Dropping this call, or swapping one of its " - + "arguments for something that still compiles (e.g. null in place of " + + "router.leadAgents(), config, leads);`. Dropping this call, or swapping one of " + + "its arguments for something that still compiles (e.g. null in place of " + "router.leadAgents()), leaves every behavioural test green — this source check is " + "what must go red instead. fleetd #480 correction 2 deliberately dropped " + "primaryRegistry from this call — see LeadRollover's class javadoc for why a " - + "single-slot lookup was wrong here."); + + "single-slot lookup was wrong here. The fleetd #480 relative-handover-path " + + "follow-up added `leads` (terminal → lead name) so the factory can resolve a " + + "relative handoverPath against the calling lead's own workspace."); } @Test diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadRolloverWorkspaceLookupTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadRolloverWorkspaceLookupTest.java new file mode 100644 index 0000000..ec63553 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdLeadRolloverWorkspaceLookupTest.java @@ -0,0 +1,163 @@ +package dev.ltms.fleet; + +import dev.ltms.fleet.config.ConfigRef; +import dev.ltms.fleet.config.FleetConfig; +import dev.ltms.fleet.herdr.AgentControl; +import dev.ltms.fleet.herdr.FakeHerdr; +import dev.ltms.fleet.lead.LeadRollover; +import org.junit.jupiter.api.DisplayName; +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.HashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * fleetd #480 relative-handover-path follow-up, correction round: a BEHAVIOURAL test of {@link + * Fleetd#leadRollover}'s own body — the terminal → lead-name → {@code Leader.cwd()} lookup it + * builds — not another source-text pin. + * + *

    {@code FleetdLeadRolloverWiringTest} (a plain string read) still earns its keep: it pins the + * call site's ARGUMENT LIST, so a mutation that drops {@code leads} back out of the call, or + * swaps it for {@code Map::of}, still goes red there. But nothing before this class exercised the + * LAMBDA BODY {@code leadRollover(...)} builds — the terminal-to-workspace {@code + * Function} that {@link LeadRollover#open} actually calls. Every prior test either + * exercised {@code LeadRollover} directly with a hand-built lookup ({@code LeadRolloverTest}), or + * read source text without ever calling the factory ({@code FleetdLeadRolloverWiringTest}) — so a + * mutation that breaks the lookup ITSELF (e.g. always resolving no lead, or reading a snapshot + * instead of the live supplier) left every existing test green while the daemon's real wiring + * silently reintroduced the exact bug this whole ticket fixes: a relative {@code handoverPath} + * resolving against the daemon's own {@code cwd} instead of the calling lead's. + * + *

    {@code Fleetd.leadRollover(...)} is package-private and {@code static}, so this test — living + * in the same {@code dev.ltms.fleet} package — calls it directly, exactly the way {@code + * FleetdExhaustionDetectionArmedWiringTest} and {@code FleetdCapacitySourceWiringTest} already call + * other package-private startup factories with a real {@link ConfigRef} built from a temp {@code + * fleetd.yaml} (never the gitignored live one). + * + *

    Proved against the mutation it exists to catch. Before this class was added, mutating + * {@code leadRollover(...)}'s lambda body to {@code String leadName = null;} (always "no lead + * found", which forces every relative {@code handoverPath} onto the {@code user.dir} fallback — + * i.e. the original bug) left the full suite green: {@code Tests run: 1669, Failures: 0}. With + * {@link #relativeHandoverPathResolvesAgainstTheLeadsConfiguredCwd()} added, the same one-line + * mutation now fails that test (it asserts the resolved path equals the configured lead's {@code + * cwd}, which the mutant can never produce) — see this ticket's fleet_reply history for both runs. + */ +class FleetdLeadRolloverWorkspaceLookupTest { + + private static AgentControl fakeAgents() { + return new AgentControl(new FakeHerdr()); + } + + @Test + @DisplayName("[BEHAVIOURAL] Fleetd.leadRollover(...) resolves a relative handoverPath against " + + "the CALLING lead's configured cwd, not the daemon's own working directory") + void relativeHandoverPathResolvesAgainstTheLeadsConfiguredCwd(@TempDir Path dir) throws Exception { + Path leadCwd = dir.resolve("lead-workspace"); + Files.createDirectories(leadCwd); + Path yaml = dir.resolve("fleetd.yaml"); + Files.writeString(yaml, """ + bind: + port: 8080 + fleet: + leaders: + opus: + tab: "lead: opus" + cwd: "%s" + leadRollover: + handoverPath: handover.md + """.formatted(leadCwd.toString())); + ConfigRef config = new ConfigRef(yaml, FleetConfig.load(yaml)); + + LeadRollover rollover = Fleetd.leadRollover(config.get(), fakeAgents(), config, + () -> Map.of("term_opus", "opus")); + assertNotNull(rollover, "leadRollover: is present in the loaded config, so the factory " + + "must construct an object"); + + LeadRollover.PendingRollover pending = rollover.open("term_opus", "test"); + + String expected = leadCwd.resolve("handover.md").normalize().toString(); + assertEquals(expected, pending.handoverPath(), + "a relative handoverPath must resolve against the CALLING lead's configured cwd " + + "through the REAL Fleetd.leadRollover(...) wiring — not the daemon's own " + + "working directory. This is the exact axis that was proven uncovered: " + + "mutating the factory's lambda body to always report \"no lead found\" " + + "left every prior test green."); + } + + @Test + @DisplayName("[BEHAVIOURAL] a terminal not present in the live lead-terminal map falls back " + + "to the daemon's own user.dir") + void terminalNotInLiveMapFallsBackToUserDir(@TempDir Path dir) throws Exception { + Path yaml = dir.resolve("fleetd.yaml"); + Files.writeString(yaml, """ + bind: + port: 8080 + leadRollover: + handoverPath: handover.md + """); + ConfigRef config = new ConfigRef(yaml, FleetConfig.load(yaml)); + + // No lead has been discovered yet — exactly the real shape of a lead the live tab scan + // has not yet scanned, or one with no fleet.leaders entry at all. + LeadRollover rollover = Fleetd.leadRollover(config.get(), fakeAgents(), config, Map::of); + assertNotNull(rollover); + + LeadRollover.PendingRollover pending = rollover.open("term_unknown", "test"); + + String expected = Path.of(System.getProperty("user.dir")).resolve("handover.md") + .normalize().toString(); + assertEquals(expected, pending.handoverPath(), + "a lead not present in the live terminal→name map must fall back to the daemon's " + + "own user.dir — the same fallback LeadLauncher#launch already uses for a " + + "lead with no configured cwd. This is deliberate, pinned behaviour, not an " + + "accident of the null-check chain."); + } + + @Test + @DisplayName("[BEHAVIOURAL] the terminal→lead-name lookup is read LIVE on every open() call, " + + "never snapshotted at Fleetd.leadRollover(...) construction time") + void workspaceLookupIsReadLiveNotSnapshotted(@TempDir Path dir) throws Exception { + Path leadCwd = dir.resolve("lead-workspace"); + Files.createDirectories(leadCwd); + Path yaml = dir.resolve("fleetd.yaml"); + Files.writeString(yaml, """ + bind: + port: 8080 + fleet: + leaders: + opus: + tab: "lead: opus" + cwd: "%s" + leadRollover: + handoverPath: handover.md + """.formatted(leadCwd.toString())); + ConfigRef config = new ConfigRef(yaml, FleetConfig.load(yaml)); + + // EMPTY at the moment leadRollover(...) is called. A lookup captured (snapshotted) here + // instead of read live through the supplier on every call would never see the entry added + // below — exactly the natural mistake to make, since leads are discovered by a live tab + // scan that runs AFTER this factory is constructed at startup. + Map liveLeadTerminals = new HashMap<>(); + LeadRollover rollover = Fleetd.leadRollover(config.get(), fakeAgents(), config, + () -> liveLeadTerminals); + assertNotNull(rollover); + + // The lead is "discovered" only now — mutate the SAME backing map the supplier reads from. + liveLeadTerminals.put("term_opus", "opus"); + + LeadRollover.PendingRollover pending = rollover.open("term_opus", "test"); + + String expected = leadCwd.resolve("handover.md").normalize().toString(); + assertEquals(expected, pending.handoverPath(), + "the terminal→lead-name lookup must be read LIVE on every open() call — a lead " + + "discovered by the tab scan AFTER Fleetd.leadRollover(...) was constructed " + + "must still resolve correctly, not only one that was already live at " + + "construction time"); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/lead/LeadRolloverTest.java b/fleetd/src/test/java/dev/ltms/fleet/lead/LeadRolloverTest.java index 22213cb..8d69d92 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/lead/LeadRolloverTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/lead/LeadRolloverTest.java @@ -14,6 +14,7 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Function; import java.util.function.LongSupplier; import static org.junit.jupiter.api.Assertions.*; @@ -70,8 +71,13 @@ class LeadRolloverTest { private static LeadRollover newRollover(HerdrClient herdr, FleetConfig.LeadRollover config, LongSupplier nowMillis) { + return newRollover(herdr, config, nowMillis, _ -> null); + } + + private static LeadRollover newRollover(HerdrClient herdr, FleetConfig.LeadRollover config, + LongSupplier nowMillis, Function leadWorkspace) { AgentControl agents = new AgentControl(herdr); - return new LeadRollover(agents, () -> config, nowMillis, () -> { }, Runnable::run); + return new LeadRollover(agents, () -> config, leadWorkspace, nowMillis, () -> { }, Runnable::run); } private Path writeHandover(String content) throws IOException { @@ -395,7 +401,7 @@ class LeadRolloverTest { void openThrowsWhenNotConfigured() { FakeHerdr herdr = new FakeHerdr(); AgentControl agents = new AgentControl(herdr); - LeadRollover rollover = new LeadRollover(agents, () -> null, () -> 1_000L, () -> { }, Runnable::run); + LeadRollover rollover = new LeadRollover(agents, () -> null, _ -> null, () -> 1_000L, () -> { }, Runnable::run); assertThrows(IllegalStateException.class, () -> rollover.open(LEAD, "context is full")); } @@ -475,4 +481,199 @@ class LeadRolloverTest { LeadRollover.RollDecision again = rollover.confirm(LEAD, pending.token(), true); assertEquals(LeadRollover.RefusalReason.UNKNOWN_TOKEN, again.reason()); } + + // ---- fleetd #480 follow-up: a relative handoverPath resolves against the CALLING lead's own + // workspace, never the daemon's cwd ------------------------------------------------------ + + @Test + @DisplayName("[fleetd #480 follow-up] a relative handoverPath resolves against the lead's " + + "configured workspace, and PendingRollover carries the absolute path") + void relativeHandoverPathResolvesAgainstLeadWorkspace() { + Path workspace = tmp.resolve("lead-workspace"); + FakeHerdr herdr = new FakeHerdr(); + AtomicLong clock = new AtomicLong(1_000); + FleetConfig.LeadRollover config = cfg("handover.md"); // relative — no directory component + Function leadWorkspace = terminal -> LEAD.equals(terminal) ? workspace.toString() : null; + LeadRollover rollover = newRollover(herdr, config, fixedClock(clock), leadWorkspace); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + + String expected = workspace.resolve("handover.md").normalize().toString(); + assertEquals(expected, pending.handoverPath(), + "a relative handoverPath must resolve against the CALLING lead's own workspace, " + + "never the daemon's own working directory"); + assertTrue(Path.of(pending.handoverPath()).isAbsolute(), + "the resolved path stored on PendingRollover must always be absolute"); + } + + @Test + @DisplayName("[fleetd #480 follow-up] confirm() accepts a handover file written at the " + + "resolved absolute location for a relative handoverPath") + void confirmAcceptsAFileWrittenAtTheResolvedLocation() throws IOException { + Path workspace = tmp.resolve("lead-workspace"); + Files.createDirectories(workspace); + Path expected = workspace.resolve("handover.md"); + Files.writeString(expected, "handover contents"); + + FakeHerdr herdr = new FakeHerdr(); + AtomicLong clock = new AtomicLong(1_000); + FleetConfig.LeadRollover config = cfg("handover.md"); + Function leadWorkspace = terminal -> LEAD.equals(terminal) ? workspace.toString() : null; + LeadRollover rollover = newRollover(herdr, config, fixedClock(clock), leadWorkspace); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + assertEquals(expected.toString(), pending.handoverPath()); + + LeadRollover.RollDecision decision = rollover.confirm(LEAD, pending.token(), true); + assertTrue(decision.accepted(), "expected approval; got: " + decision.reason() + " / " + decision.detail()); + } + + @Test + @DisplayName("[fleetd #480 follow-up] a file at the same relative name under a DIFFERENT " + + "directory is NOT accepted — resolution is against the lead's own workspace only") + void relativeHandoverPathDoesNotMatchAFileUnderADifferentDirectory() throws IOException { + Path workspace = tmp.resolve("lead-workspace"); + Path decoy = tmp.resolve("decoy-directory"); + Files.createDirectories(workspace); + Files.createDirectories(decoy); + // the decoy directory holds a file with the SAME relative name — if resolution ever fell + // back to searching, or resolved against the wrong base, this would be wrongly found. + Files.writeString(decoy.resolve("handover.md"), "decoy contents — must never be read"); + + FakeHerdr herdr = new FakeHerdr(); + AtomicLong clock = new AtomicLong(1_000); + FleetConfig.LeadRollover config = cfg("handover.md"); + Function leadWorkspace = terminal -> LEAD.equals(terminal) ? workspace.toString() : null; + LeadRollover rollover = newRollover(herdr, config, fixedClock(clock), leadWorkspace); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + assertEquals(workspace.resolve("handover.md").toString(), pending.handoverPath()); + + LeadRollover.RollDecision decision = rollover.confirm(LEAD, pending.token(), true); + assertFalse(decision.accepted()); + assertEquals(LeadRollover.RefusalReason.HANDOVER_MISSING, decision.reason(), + "the file at workspace/handover.md does not exist — a decoy file at the same " + + "relative name under a different directory must never be mistaken for it"); + } + + @Test + @DisplayName("[fleetd #480 follow-up] an absolute handoverPath is used unchanged — the lead " + + "workspace lookup is never even consulted") + void absoluteHandoverPathIsUnchangedByResolution() throws IOException { + Path absolute = writeHandover("handover contents"); // tmp.resolve(...) — always absolute + FakeHerdr herdr = new FakeHerdr(); + AtomicLong clock = new AtomicLong(1_000); + FleetConfig.LeadRollover config = cfg(absolute.toString()); + // a workspace lookup that would resolve a RELATIVE path somewhere completely different — + // proving it is never consulted at all for an already-absolute handoverPath. + Function leadWorkspace = terminal -> tmp.resolve("some-other-workspace").toString(); + LeadRollover rollover = newRollover(herdr, config, fixedClock(clock), leadWorkspace); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + + assertEquals(absolute.normalize().toString(), pending.handoverPath(), + "an absolute handoverPath must be used unchanged (aside from normalization)"); + } + + @Test + @DisplayName("[fleetd #480 follow-up correction] a RELATIVE fleet.leaders..cwd still " + + "yields an ABSOLUTE PendingRollover.handoverPath") + void relativeLeadWorkspaceCwdStillYieldsAnAbsoluteHandoverPath() { + FakeHerdr herdr = new FakeHerdr(); + AtomicLong clock = new AtomicLong(1_000); + FleetConfig.LeadRollover config = cfg("handover.md"); // relative handoverPath + // Nothing in FleetConfig validates fleet.leaders..cwd, so an operator can write a + // RELATIVE one — this must still resolve to an absolute PendingRollover.handoverPath, + // never silently reopen the exact bug this class fixes. + String relativeWorkspace = "relative-lead-workspace"; + Function leadWorkspace = terminal -> LEAD.equals(terminal) ? relativeWorkspace : null; + LeadRollover rollover = newRollover(herdr, config, fixedClock(clock), leadWorkspace); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + + assertTrue(Path.of(pending.handoverPath()).isAbsolute(), + "the resolved path must be absolute even when the configured cwd itself is relative"); + + // Asserting isAbsolute() alone would also pass for a path resolved against the WRONG base + // (e.g. some unrelated absolute directory) — pin the actual value too. + String expected = Path.of(System.getProperty("user.dir")).resolve(relativeWorkspace) + .resolve("handover.md").toAbsolutePath().normalize().toString(); + assertEquals(expected, pending.handoverPath(), + "a relative cwd must be finished off against the daemon's own user.dir, exactly " + + "like a missing cwd — never left relative, which would silently reopen the " + + "exact bug this class fixes: every later reader interpreting an ambiguous " + + "path against ITS OWN working directory"); + } + + @Test + @DisplayName("[fleetd #480 follow-up] a terminal with no configured workspace (null lookup " + + "result) falls back to the daemon's own user.dir") + void noConfiguredWorkspaceFallsBackToUserDir() { + FakeHerdr herdr = new FakeHerdr(); + AtomicLong clock = new AtomicLong(1_000); + FleetConfig.LeadRollover config = cfg("some-handover.md"); + Function leadWorkspace = _ -> null; // no configured workspace for anyone + LeadRollover rollover = newRollover(herdr, config, fixedClock(clock), leadWorkspace); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + + String expected = Path.of(System.getProperty("user.dir")).resolve("some-handover.md") + .normalize().toString(); + assertEquals(expected, pending.handoverPath(), + "with no configured workspace for this lead, resolution must fall back to the " + + "daemon's own user.dir — the same fallback LeadLauncher#launch already uses " + + "for a lead with no configured cwd"); + } + + @Test + @DisplayName("[fleetd #480 follow-up] a terminal whose workspace lookup returns a blank string " + + "also falls back to the daemon's own user.dir") + void blankConfiguredWorkspaceFallsBackToUserDir() { + FakeHerdr herdr = new FakeHerdr(); + AtomicLong clock = new AtomicLong(1_000); + FleetConfig.LeadRollover config = cfg("some-handover.md"); + Function leadWorkspace = _ -> " "; + LeadRollover rollover = newRollover(herdr, config, fixedClock(clock), leadWorkspace); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + + String expected = Path.of(System.getProperty("user.dir")).resolve("some-handover.md") + .normalize().toString(); + assertEquals(expected, pending.handoverPath(), + "a blank (non-null) workspace lookup result must be treated the same as null — " + + "fall back to user.dir, never resolve against an empty base"); + } + + @Test + @DisplayName("[fleetd #480 follow-up] the default bootstrapText that is actually SENT names " + + "the RESOLVED absolute handoverPath, not the raw relative configured value") + void defaultBootstrapTextNamesTheResolvedAbsolutePath() throws IOException { + Path workspace = tmp.resolve("lead-workspace"); + Files.createDirectories(workspace); + Path expected = workspace.resolve("handover.md"); + Files.writeString(expected, "handover contents"); + + FakeHerdr herdr = new FakeHerdr(); // default agentStatus "idle" — both waits settle immediately + AtomicLong clock = new AtomicLong(1_000); + // bootstrapText left null so the DEFAULT sentence is built — from the RESOLVED path + FleetConfig.LeadRollover config = new FleetConfig.LeadRollover("handover.md", true, 3600, 20, 20, null); + Function leadWorkspace = terminal -> LEAD.equals(terminal) ? workspace.toString() : null; + LeadRollover rollover = newRollover(herdr, config, fixedClock(clock), leadWorkspace); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + LeadRollover.RollDecision decision = rollover.confirm(LEAD, pending.token(), true); + + assertTrue(decision.accepted(), "expected approval; got: " + decision.reason() + " / " + decision.detail()); + var prompts = herdr.calls.stream().filter(c -> "agent.prompt".equals(c.method())).toList(); + assertEquals(2, prompts.size(), "expected exactly two agent.prompt calls: /clear then the " + + "default bootstrapText"); + String bootstrapSent = prompts.get(1).params().toString(); + assertTrue(bootstrapSent.contains(expected.toString()), + "the default bootstrapText actually sent to the pane must name the RESOLVED " + + "absolute path (" + expected + ") — a fresh session's own pane cannot " + + "resolve a relative path against a directory it never had. Actual text " + + "sent: " + bootstrapSent); + assertFalse(bootstrapSent.contains("\"handover.md\""), + "must not name the raw relative configured value in the text actually sent"); + } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpHandoverTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpHandoverTest.java index 5ca8961..ed0b8ef 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpHandoverTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpHandoverTest.java @@ -68,7 +68,9 @@ class FleetMcpHandoverTest { } private LeadRollover newRollover(String handoverPath) { - return new LeadRollover(agents, () -> cfg(handoverPath)); + // Every handoverPath this test class uses comes from tmp.resolve(...), which is already + // absolute, so the workspace lookup is never actually consulted — a no-op lookup is enough. + return new LeadRollover(agents, () -> cfg(handoverPath), _ -> null); } /** A fully wired FleetMcp on fakes (mirrors FleetMcpAuthzTest's helper), plus a leadRollover. */