fleetd #480 follow-up correction 2: guarantee resolveHandoverPath is always absolute
LeadRollover.resolveHandoverPath's relative branch resolved the configured
handoverPath against leadWorkspace.apply(...) (fleet.leaders.<name>.cwd) but
never forced the result absolute. If an operator writes a RELATIVE cwd, the
returned path stays relative, silently breaking the "always absolute"
contract documented on PendingRollover.
Fix: call toAbsolutePath() unconditionally on both branches (the
already-absolute input branch, where it is a no-op, and the relative
branch), so neither branch trusts isAbsolute() alone to already imply what
toAbsolutePath() enforces. Method javadoc now states the absolute result is
guaranteed, not merely usual.
Added a test: a lead with a RELATIVE cwd and a relative handoverPath still
yields an absolute PendingRollover.handoverPath. Asserts both isAbsolute()
and the exact resolved value, since isAbsolute() alone would also pass for a
path resolved against the wrong base.
Proved the test discriminates: reverting the toAbsolutePath() calls (keeping
the test) made it fail with an AssertionFailedError ("expected: <true> but
was: <false>"); restoring the fix made it pass again.
Note: FleetConfig has no validation on fleet.leaders.<name>.cwd at config
load (grep across every validate* method: 0 matches for .cwd()) — a relative
cwd is silently accepted. Not adding validation here per instruction; that
is a separate ticket.
This commit is contained in:
@@ -270,24 +270,42 @@ public final class LeadRollover {
|
||||
* FleetConfig.LeadRollover#bootstrapTextFor}) ever has to resolve — or worse, silently
|
||||
* mis-resolve — a relative path again.
|
||||
*
|
||||
* <p><strong>The return value is GUARANTEED absolute, not merely usually absolute.</strong>
|
||||
* {@code leadWorkspace.apply(leadTerminal)} returns an operator-configured {@code
|
||||
* fleet.leaders.<name>.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.
|
||||
*
|
||||
* <ul>
|
||||
* <li>already absolute → returned unchanged (normalized)</li>
|
||||
* <li>relative → resolved against {@code leadWorkspace.apply(leadTerminal)} when that is
|
||||
* non-null and non-blank; otherwise against {@code System.getProperty("user.dir")} — the
|
||||
* same fallback {@code LeadLauncher#launch} uses for a lead with no configured {@code
|
||||
* cwd}</li>
|
||||
* cwd}. If {@code leadWorkspace}'s own answer is itself relative (an operator wrote a
|
||||
* relative {@code cwd:}), the result is finished off against the daemon's own
|
||||
* {@code user.dir} — see the paragraph above.</li>
|
||||
* </ul>
|
||||
*/
|
||||
private String resolveHandoverPath(String configured, String leadTerminal) {
|
||||
Path path = Path.of(configured);
|
||||
if (path.isAbsolute()) {
|
||||
return path.normalize().toString();
|
||||
// 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).normalize().toString();
|
||||
return base.resolve(path).toAbsolutePath().normalize().toString();
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -575,6 +575,36 @@ class LeadRolloverTest {
|
||||
"an absolute handoverPath must be used unchanged (aside from normalization)");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("[fleetd #480 follow-up correction] a RELATIVE fleet.leaders.<name>.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.<name>.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<String, String> 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")
|
||||
|
||||
Reference in New Issue
Block a user