fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fleetd #480 follow-up: resolve relative leadRollover.handoverPath against the lead's workspace #487

Closed
agent wants to merge 0 commits from worker/480-relative-handover-path-906323-1 into main
pull from: worker/480-relative-handover-path-906323-1
merge into: fleet:main
Conversation 2 Commits - Files Changed -
agent commented 2026-09-12 00:20:58 +02:00
Member
Copy Link
Copy Source

Fixes fleetd #480 follow-up: a relative leadRollover.handoverPath was resolved against the daemon's own working directory instead of the calling lead's workspace, because LeadRollover.open() stored the raw configured string on PendingRollover and every later reader (the exists/empty/fresh checks in confirm(), the value handed back to the lead in the fleet_handover open response, and the default bootstrapText sentence) inherited that bug.

What changed

  • LeadRollover.open() resolves handoverPath to an absolute path exactly once: unchanged if already absolute, otherwise resolved against leadWorkspace.apply(leadTerminal) (falling back to System.getProperty("user.dir") when that lookup returns null/blank) - the same fallback LeadLauncher#launch already uses for a lead with no configured cwd. PendingRollover.handoverPath() now always holds the resolved absolute path.
  • LeadRollover gained a required Function<String, String> leadWorkspace constructor parameter (no defaulting overload, by design - a defaulted dependency would compile and silently keep the old behaviour).
  • FleetConfig.LeadRollover.bootstrapText is no longer defaulted in the compact constructor (it would still bake in the raw, possibly-relative path). Added bootstrapTextFor(resolvedHandoverPath), used by LeadRollover.runRollover instead of the old bootstrapText() getter.
  • Fleetd.leadRollover(...) factory gained a required liveLeadTerminals supplier parameter to build the terminal -> lead name -> Leader.cwd() lookup, read live via the existing leads supplier and ConfigRef on every call (never a startup snapshot).
  • fleetd.example.yaml and Javadoc updated to document that a relative handoverPath is allowed and what it resolves against.

Tests - 8 new cases added to LeadRolloverTest (24 total, up from 16):

  • a relative handoverPath resolves against the lead's workspace; PendingRollover carries the absolute path
  • confirm() accepts a file written at the resolved absolute location
  • a same-named file under a different directory is NOT accepted (proves resolution, not a search)
  • an absolute handoverPath is unchanged, and the workspace lookup is never consulted
  • a terminal with no configured workspace (null or blank lookup result) falls back to user.dir
  • the default bootstrapText actually SENT (asserted on text captured from the fake AgentControl) names the resolved absolute path, not the raw relative value

Existing LeadRolloverTest/FleetMcpHandoverTest/FleetdLeadRolloverWiringTest tests updated only for the new required constructor/factory parameters - their assertions are unchanged, since every existing test already used an absolute handoverPath (via @TempDir).

Build: mvn clean install from fleetd/ - Tests run: 1669, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

Scope note: grepped the whole repo for bootstrapText - the only production caller was LeadRollover.runRollover (now updated); no other caller depends on it being non-null.

Out of scope, not fixed (per ticket instructions): did not touch anything beyond LeadRollover.java, FleetConfig.java, Fleetd.java, fleetd.example.yaml, and their tests. Did not run the CLAUDE.md/wiki sync check - wiki/ is not initialized in this worktree.

Fixes fleetd #480 follow-up: a relative `leadRollover.handoverPath` was resolved against the daemon's own working directory instead of the calling lead's workspace, because `LeadRollover.open()` stored the raw configured string on `PendingRollover` and every later reader (the exists/empty/fresh checks in `confirm()`, the value handed back to the lead in the `fleet_handover` open response, and the default `bootstrapText` sentence) inherited that bug. **What changed** - `LeadRollover.open()` resolves `handoverPath` to an absolute path exactly once: unchanged if already absolute, otherwise resolved against `leadWorkspace.apply(leadTerminal)` (falling back to `System.getProperty("user.dir")` when that lookup returns null/blank) - the same fallback `LeadLauncher#launch` already uses for a lead with no configured `cwd`. `PendingRollover.handoverPath()` now always holds the resolved absolute path. - `LeadRollover` gained a required `Function<String, String> leadWorkspace` constructor parameter (no defaulting overload, by design - a defaulted dependency would compile and silently keep the old behaviour). - `FleetConfig.LeadRollover.bootstrapText` is no longer defaulted in the compact constructor (it would still bake in the raw, possibly-relative path). Added `bootstrapTextFor(resolvedHandoverPath)`, used by `LeadRollover.runRollover` instead of the old `bootstrapText()` getter. - `Fleetd.leadRollover(...)` factory gained a required `liveLeadTerminals` supplier parameter to build the terminal -> lead name -> `Leader.cwd()` lookup, read live via the existing `leads` supplier and `ConfigRef` on every call (never a startup snapshot). - `fleetd.example.yaml` and Javadoc updated to document that a relative `handoverPath` is allowed and what it resolves against. **Tests** - 8 new cases added to `LeadRolloverTest` (24 total, up from 16): - a relative `handoverPath` resolves against the lead's workspace; `PendingRollover` carries the absolute path - `confirm()` accepts a file written at the resolved absolute location - a same-named file under a *different* directory is NOT accepted (proves resolution, not a search) - an absolute `handoverPath` is unchanged, and the workspace lookup is never consulted - a terminal with no configured workspace (null or blank lookup result) falls back to `user.dir` - the default `bootstrapText` actually SENT (asserted on text captured from the fake `AgentControl`) names the resolved absolute path, not the raw relative value Existing `LeadRolloverTest`/`FleetMcpHandoverTest`/`FleetdLeadRolloverWiringTest` tests updated only for the new required constructor/factory parameters - their assertions are unchanged, since every existing test already used an absolute `handoverPath` (via `@TempDir`). **Build**: `mvn clean install` from `fleetd/` - `Tests run: 1669, Failures: 0, Errors: 0, Skipped: 0`, BUILD SUCCESS. **Scope note**: grepped the whole repo for `bootstrapText` - the only production caller was `LeadRollover.runRollover` (now updated); no other caller depends on it being non-null. **Out of scope, not fixed** (per ticket instructions): did not touch anything beyond `LeadRollover.java`, `FleetConfig.java`, `Fleetd.java`, `fleetd.example.yaml`, and their tests. Did not run the CLAUDE.md/wiki sync check - `wiki/` is not initialized in this worktree.
agent added 1 commit 2026-09-12 00:20:58 +02:00
fleetd #480 follow-up: resolve a relative leadRollover.handoverPath against the calling lead's workspace
CI / contract (pull_request) Successful in 49s
Details
CI / build (pull_request) Successful in 2m4s
Details
4bfab6b718 
LeadRollover.open() now resolves handoverPath to an absolute path exactly once, against the
calling lead's fleet.leaders.<name>.cwd (falling back to the daemon's own user.dir when that
lead has none configured), matching the LeadLauncher#launch precedent. PendingRollover stores
only the resolved absolute path, so checkHandover's exists/empty/fresh checks, the path handed
back to the lead in the fleet_handover open response, and the default bootstrapText sentence
all see the same absolute location instead of a value resolved against whatever directory the
daemon process happened to start in.

FleetConfig.LeadRollover.bootstrapText is no longer defaulted in the compact constructor (it
would otherwise still bake in the raw, possibly-relative handoverPath); a new bootstrapTextFor
(resolvedHandoverPath) method builds the default sentence from the resolved path instead.

Fleetd.leadRollover(...) gains a required liveLeadTerminals parameter to build the terminal to
lead-name to Leader.cwd lookup, read live through the existing `leads` supplier and ConfigRef on
every call, never off a startup snapshot.
agent added 1 commit 2026-09-12 00:32:36 +02:00
fleetd #480 follow-up correction: cover Fleetd.leadRollover(...)'s own wiring behaviourally
CI / contract (pull_request) Successful in 48s
Details
CI / build (pull_request) Successful in 1m51s
Details
042b8c99dd 
Add FleetdLeadRolloverWorkspaceLookupTest, calling the package-private Fleetd.leadRollover(...)
factory directly (with a real ConfigRef built from a temp fleetd.yaml, never the gitignored live
one) to prove the terminal -> lead-name -> Leader.cwd() lookup it builds actually works: a relative
handoverPath resolves against the calling lead's configured cwd; a terminal absent from the live
lead-terminal map falls back to user.dir; and the lookup is read live, not snapshotted at
construction time (a lead discovered by the tab scan after leadRollover(...) was built still
resolves correctly).

Proved this closes the gap: mutating the factory's lambda body (String leadName = null;, always
"no lead found", which forces the daemon-cwd fallback this ticket exists to fix) left the full
1669-test suite green before this commit. With the new test added, the same one-line mutation now
fails 2 of its 3 cases; reverting it goes green again (3/3). Mutation applied/reverted only during
verification and is not part of this commit (git diff on Fleetd.java is empty).

FleetdLeadRolloverWiringTest's class javadoc corrected: it previously claimed no behavioural test
could catch this wiring dropping out, which was true only before this commit and only covered the
factory's own body, not its call site. Restated what each test class actually covers: the source-
text pin covers the call site's argument list; the new behavioural test covers the lambda's body.
agent commented 2026-09-12 00:32:55 +02:00
Author
Member
Copy Link
Copy Source

Correction round (per review): added a BEHAVIOURAL test of Fleetd.leadRollover(...)'s own wiring — FleetdLeadRolloverWorkspaceLookupTest, in package dev.ltms.fleet, calling the factory directly with a real ConfigRef built from a temp fleetd.yaml.

Gap found by review: mutating the factory's lambda body (String leadName = null;, always "no lead found") left the full 1669-test suite green — the existing source-text pin (FleetdLeadRolloverWiringTest) only covers the call site's argument list, and every behavioural LeadRolloverTest hands LeadRollover its own hand-built lookup, never exercising the real one Fleetd builds.

Proof (both runs):

  • Applied the exact one-line mutation from the review, confirmed applied via two search strings (grep -c for the original line -> 0, for the mutant marker -> 1). Ran FleetdLeadRolloverWorkspaceLookupTest alone: Tests run: 3, Failures: 2 (RED, on exactly the two cases that exercise the lookup; the fallback-only case correctly still passed).
  • Reverted the mutation, confirmed git diff on Fleetd.java is empty (byte-identical to the prior commit). Re-ran the same test: Tests run: 3, Failures: 0 (GREEN).

New test coverage (3 cases): relative handoverPath resolves against the configured lead's cwd through the real wiring; a terminal absent from the live lead-terminal map falls back to user.dir; the terminal->lead-name lookup is read LIVE (a lead added to the backing map only after leadRollover(...) was constructed still resolves).

Also corrected FleetdLeadRolloverWiringTest's class javadoc, which claimed no behavioural test could catch this wiring dropping out — true before this commit, not after. It now states precisely what each test class covers: the source-text pin covers the call site's argument list, the new behavioural test covers the lambda's body.

Full suite after this commit: mvn clean install from fleetd/, unpiped — Tests run: 1672, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

Correction round (per review): added a BEHAVIOURAL test of `Fleetd.leadRollover(...)`'s own wiring — `FleetdLeadRolloverWorkspaceLookupTest`, in package `dev.ltms.fleet`, calling the factory directly with a real `ConfigRef` built from a temp `fleetd.yaml`. **Gap found by review**: mutating the factory's lambda body (`String leadName = null;`, always "no lead found") left the full 1669-test suite green — the existing source-text pin (`FleetdLeadRolloverWiringTest`) only covers the call site's argument list, and every behavioural `LeadRolloverTest` hands `LeadRollover` its own hand-built lookup, never exercising the real one `Fleetd` builds. **Proof (both runs)**: - Applied the exact one-line mutation from the review, confirmed applied via two search strings (`grep -c` for the original line -> 0, for the mutant marker -> 1). Ran `FleetdLeadRolloverWorkspaceLookupTest` alone: `Tests run: 3, Failures: 2` (RED, on exactly the two cases that exercise the lookup; the fallback-only case correctly still passed). - Reverted the mutation, confirmed `git diff` on `Fleetd.java` is empty (byte-identical to the prior commit). Re-ran the same test: `Tests run: 3, Failures: 0` (GREEN). **New test coverage** (3 cases): relative `handoverPath` resolves against the configured lead's `cwd` through the real wiring; a terminal absent from the live lead-terminal map falls back to `user.dir`; the terminal->lead-name lookup is read LIVE (a lead added to the backing map only after `leadRollover(...)` was constructed still resolves). Also corrected `FleetdLeadRolloverWiringTest`'s class javadoc, which claimed no behavioural test could catch this wiring dropping out — true before this commit, not after. It now states precisely what each test class covers: the source-text pin covers the call site's argument list, the new behavioural test covers the lambda's body. **Full suite after this commit**: `mvn clean install` from `fleetd/`, unpiped — `Tests run: 1672, Failures: 0, Errors: 0, Skipped: 0`, BUILD SUCCESS.
agent added 1 commit 2026-09-12 00:41:51 +02:00
fleetd #480 follow-up correction 2: guarantee resolveHandoverPath is always absolute
CI / contract (pull_request) Successful in 1m26s
Details
CI / build (pull_request) Successful in 1m32s
Details
261aa056f9 
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.
agent commented 2026-09-12 00:42:20 +02:00
Author
Member
Copy Link
Copy Source

Correction 2: resolveHandoverPath now guarantees an absolute result

A reviewer found that the relative branch of resolveHandoverPath did not force its result absolute: base.resolve(path).normalize().toString() stays relative if leadWorkspace.apply(...) (i.e. fleet.leaders.<name>.cwd) is itself a relative string. That silently breaks the "always absolute" contract documented on PendingRollover.

Fix: both branches now call toAbsolutePath() before normalize() — the already-absolute branch treats it as a no-op rather than trusting isAbsolute() alone. The method javadoc now states the absolute result is guaranteed, not merely usual.

New test: relativeLeadWorkspaceCwdStillYieldsAnAbsoluteHandoverPath in LeadRolloverTest — a lead with a RELATIVE cwd and a relative handoverPath. It asserts isAbsolute() is true AND asserts the exact resolved value, since isAbsolute() alone would also pass if the path resolved against the wrong base.

Mutation proof (discriminates):

  • Reverted the two toAbsolutePath() calls, kept the test → mvn test -Dtest=LeadRolloverTest#relativeLeadWorkspaceCwdStillYieldsAnAbsoluteHandoverPath → RED: Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, AssertionFailedError: the resolved path must be absolute even when the configured cwd itself is relative ==> expected: <true> but was: <false>.
  • Restored the fix, same test → GREEN: Tests run: 1, Failures: 0, Errors: 0, Skipped: 0.

Full build after restoring the fix (mvn clean install from fleetd/, unpiped): BUILD SUCCESS, Tests run: 1673, Failures: 0, Errors: 0, Skipped: 0 (up from 1672 — the one new test). LeadRolloverTest itself: Tests run: 25, Failures: 0, Errors: 0, Skipped: 0.

Answering the validation question (not acting on it): no validation exists for fleet.leaders.<name>.cwd in any of FleetConfig's validate* methods — confirmed with grep -c '\.cwd()' fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java → 0 matches in the whole file. A relative cwd is silently accepted at config load. No validation added here, per instruction — that is a separate ticket.

Files changed this round: fleetd/src/main/java/dev/ltms/fleet/lead/LeadRollover.java, fleetd/src/test/java/dev/ltms/fleet/lead/LeadRolloverTest.java (2 files, 51 insertions, 3 deletions). Commit 261aa05, pushed to worker/480-relative-handover-path-906323-1.

## Correction 2: `resolveHandoverPath` now guarantees an absolute result A reviewer found that the relative branch of `resolveHandoverPath` did not force its result absolute: `base.resolve(path).normalize().toString()` stays relative if `leadWorkspace.apply(...)` (i.e. `fleet.leaders.<name>.cwd`) is itself a relative string. That silently breaks the "always absolute" contract documented on `PendingRollover`. **Fix:** both branches now call `toAbsolutePath()` before `normalize()` — the already-absolute branch treats it as a no-op rather than trusting `isAbsolute()` alone. The method javadoc now states the absolute result is guaranteed, not merely usual. **New test:** `relativeLeadWorkspaceCwdStillYieldsAnAbsoluteHandoverPath` in `LeadRolloverTest` — a lead with a RELATIVE `cwd` and a relative `handoverPath`. It asserts `isAbsolute()` is true AND asserts the exact resolved value, since `isAbsolute()` alone would also pass if the path resolved against the wrong base. **Mutation proof (discriminates):** - Reverted the two `toAbsolutePath()` calls, kept the test → `mvn test -Dtest=LeadRolloverTest#relativeLeadWorkspaceCwdStillYieldsAnAbsoluteHandoverPath` → RED: `Tests run: 1, Failures: 1, Errors: 0, Skipped: 0`, `AssertionFailedError: the resolved path must be absolute even when the configured cwd itself is relative ==> expected: <true> but was: <false>`. - Restored the fix, same test → GREEN: `Tests run: 1, Failures: 0, Errors: 0, Skipped: 0`. **Full build after restoring the fix** (`mvn clean install` from `fleetd/`, unpiped): `BUILD SUCCESS`, `Tests run: 1673, Failures: 0, Errors: 0, Skipped: 0` (up from 1672 — the one new test). `LeadRolloverTest` itself: `Tests run: 25, Failures: 0, Errors: 0, Skipped: 0`. **Answering the validation question (not acting on it):** no validation exists for `fleet.leaders.<name>.cwd` in any of `FleetConfig`'s `validate*` methods — confirmed with `grep -c '\.cwd()' fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java` → 0 matches in the whole file. A relative `cwd` is silently accepted at config load. No validation added here, per instruction — that is a separate ticket. Files changed this round: `fleetd/src/main/java/dev/ltms/fleet/lead/LeadRollover.java`, `fleetd/src/test/java/dev/ltms/fleet/lead/LeadRolloverTest.java` (2 files, 51 insertions, 3 deletions). Commit `261aa05`, pushed to `worker/480-relative-handover-path-906323-1`.
ltms closed this pull request 2026-09-12 00:47:06 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m26s
Details
CI / build (pull_request) Successful in 1m32s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#487
Delete Branch "worker/480-relative-handover-path-906323-1"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?