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

fleet_list can report another session's agentSessionId, and fleet_spawn{resumeSessionId} will resume it #249

New Issue
Closed
opened 2026-09-03 06:59:22 +02:00 by ltms · 1 comment
ltms commented 2026-09-03 06:59:22 +02:00
Owner
Copy Link
Copy Source

Split out of #234, which is now closed. #234 fixed the two defects it was filed for: the model read-back is keyed on the session id (SELECT model FROM session WHERE id = ?), and the exhaustion sink no longer fails silently when it cannot resolve a profile. This is the half that is still live.

What is left

OpenCodeSessionDiscovery.sessionIdForDirectory is still keyed on the worker's cwd:

SELECT id FROM session WHERE directory = ? ORDER BY time_updated DESC LIMIT 1

fleet_spawn provisions a worktree only when the caller asks for one. Without a worktree the member inherits the lead's cwd, which is a long-lived directory holding many old session rows from other profiles. A brand-new member has not written its own row yet, so "most recently updated session in this directory" is somebody else's session — measured on 2026-09-03 as a row from three days earlier belonging to a different profile.

The worker who fixed #234 handled this correctly and honestly: it did not pretend to fix the heuristic, it documented the caveat in the method's javadoc, and it stopped the model check from re-deriving a second piece of evidence through the same bad path. That was the right call for that ticket's scope. Nothing here is a criticism of it.

Why it still needs fixing

The caveat is documented inside the class. The consequence is not guarded at the surface where a lead acts on it.

fleet_list's own tool description says each member carries agentSessionId — "the id to pass as fleet_spawn's resumeSessionId to relaunch onto that same conversation". That sentence is confidently wrong for any member spawned without a worktree. So:

  1. A lead reads agentSessionId from fleet_list and believes it identifies that member.
  2. The lead passes it as fleet_spawn{resumeSessionId} to bring a member back.
  3. It resumes a stranger's conversation — another profile's, days old — and the lead has no way to tell, because a resumed session looks like a resumed session.

That is worse than the original #234 log-line symptom. A wrong log line misleads; this one loads foreign context into a working member and then takes a task on it.

Suggested fix

Do not try to make the directory heuristic smarter. It cannot be: a time_updated >= spawn time filter narrows the window but still picks a sibling member sharing the same cwd, and #234's javadoc already explains why nothing else at that layer disambiguates.

Refuse to answer instead. fleetd knows at spawn time whether it provisioned the worktree — GitWorktrees is what created it, and isProvisionedWorktree(cwd) already exists and is already used for exactly this kind of gate. When the member's cwd is not a worktree fleetd provisioned, the directory is shared and the lookup is unreliable, so agentSessionId should be absent, not a guess.

Absent is the correct answer here, and it is the answer this codebase already gives elsewhere for absent evidence — #175 returns UNKNOWN rather than comparing when it has no id. A missing agentSessionId makes fleet_spawn{resumeSessionId} unavailable for that member, which is right: that member genuinely cannot be resumed reliably.

Acceptance criteria

  1. A member spawned without a worktree reports no agentSessionId in fleet_list.
  2. A member spawned with a worktree still reports its own id, unchanged. Do not regress the working case.
  3. fleet_spawn{resumeSessionId} with an id for a member that has none is refused with a message saying why, rather than silently resuming something.
  4. fleet_list's tool description is corrected — today it states the id is always safe to resume from, and that is the sentence that makes a lead act on it.
  5. Tests drive the real path, not the discovery class alone. A test that calls sessionIdForDirectory directly passes today and proves nothing about the caller (#113, #248).

Scope note

Only the reporting and the resume gate. Do not rewrite the discovery heuristic — #234 already established it cannot be made reliable at that layer, and that reasoning is recorded in the method's javadoc.

Related: #234 (the two halves already fixed) · #209 (which introduced the directory-keyed identity lookup) · #113 · #248.

Split out of #234, which is now closed. #234 fixed the two defects it was filed for: the model read-back is keyed on the session **id** (`SELECT model FROM session WHERE id = ?`), and the exhaustion sink no longer fails silently when it cannot resolve a profile. This is the half that is still live. ## What is left `OpenCodeSessionDiscovery.sessionIdForDirectory` is still keyed on the worker's cwd: ```sql SELECT id FROM session WHERE directory = ? ORDER BY time_updated DESC LIMIT 1 ``` `fleet_spawn` provisions a worktree only when the caller asks for one. **Without a worktree the member inherits the lead's cwd**, which is a long-lived directory holding many old session rows from other profiles. A brand-new member has not written its own row yet, so "most recently updated session in this directory" is somebody else's session — measured on 2026-09-03 as a row from three days earlier belonging to a different profile. The worker who fixed #234 handled this correctly and honestly: it did **not** pretend to fix the heuristic, it documented the caveat in the method's javadoc, and it stopped the model check from re-deriving a second piece of evidence through the same bad path. That was the right call for that ticket's scope. Nothing here is a criticism of it. ## Why it still needs fixing The caveat is documented **inside the class**. The consequence is not guarded at the surface where a lead acts on it. `fleet_list`'s own tool description says each member carries `agentSessionId` — *"the id to pass as fleet_spawn's resumeSessionId to relaunch onto that same conversation"*. That sentence is confidently wrong for any member spawned without a worktree. So: 1. A lead reads `agentSessionId` from `fleet_list` and believes it identifies that member. 2. The lead passes it as `fleet_spawn{resumeSessionId}` to bring a member back. 3. It resumes **a stranger's conversation** — another profile's, days old — and the lead has no way to tell, because a resumed session looks like a resumed session. That is worse than the original #234 log-line symptom. A wrong log line misleads; this one loads foreign context into a working member and then takes a task on it. ## Suggested fix Do not try to make the directory heuristic smarter. It cannot be: a `time_updated >= spawn time` filter narrows the window but still picks a sibling member sharing the same cwd, and #234's javadoc already explains why nothing else at that layer disambiguates. **Refuse to answer instead.** fleetd knows at spawn time whether it provisioned the worktree — `GitWorktrees` is what created it, and `isProvisionedWorktree(cwd)` already exists and is already used for exactly this kind of gate. When the member's cwd is not a worktree fleetd provisioned, the directory is shared and the lookup is unreliable, so `agentSessionId` should be **absent**, not a guess. Absent is the correct answer here, and it is the answer this codebase already gives elsewhere for absent evidence — #175 returns UNKNOWN rather than comparing when it has no id. A missing `agentSessionId` makes `fleet_spawn{resumeSessionId}` unavailable for that member, which is right: that member genuinely cannot be resumed reliably. ## Acceptance criteria 1. A member spawned **without** a worktree reports no `agentSessionId` in `fleet_list`. 2. A member spawned **with** a worktree still reports its own id, unchanged. Do not regress the working case. 3. `fleet_spawn{resumeSessionId}` with an id for a member that has none is refused with a message saying why, rather than silently resuming something. 4. `fleet_list`'s tool description is corrected — today it states the id is always safe to resume from, and that is the sentence that makes a lead act on it. 5. Tests drive the real path, not the discovery class alone. A test that calls `sessionIdForDirectory` directly passes today and proves nothing about the caller (#113, #248). ## Scope note Only the reporting and the resume gate. Do not rewrite the discovery heuristic — #234 already established it cannot be made reliable at that layer, and that reasoning is recorded in the method's javadoc. Related: #234 (the two halves already fixed) · #209 (which introduced the directory-keyed identity lookup) · #113 · #248.
ltms closed this issue 2026-09-03 07:57:06 +02:00
ltms commented 2026-09-03 07:59:24 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main as c4deef0 (PR #253). 1234 tests, 0 failures, 0 compile errors.

The fix is the one this ticket asked for: refuse to answer rather than guess. agentSessionId() returns null for a member whose cwd is not a fleetd-provisioned worktree, and spawn() refuses a resumeSessionId request for such a cwd outright, before anything starts. OpenCodeSessionDiscovery's SQL is untouched, as scoped — #234 already established the heuristic cannot be made reliable at that layer.

isProvisionedWorktree moved from ClaudeCodeLauncher to HerdrPeerLauncher so both adapters share it.

What I checked myself rather than taking on trust

Two claims in the delivery mattered enough to verify, because both are the kind that read as fine and are not:

The refusal has to reach the lead as a message, not a stack trace. Criterion 3 says "refused with a message saying why", and an IllegalArgumentException thrown from a launcher could easily surface as an opaque failure. It does not: FleetMcp.spawn already has catch (IllegalArgumentException e) → error(e.getMessage()), alongside the existing GuardException and PlacementException handlers. The worker also updated that catch's comment to name the new case. Criterion 3 is genuinely met.

The refusal message tells the lead what to do instead, and I checked the parameter it names is real. It says to pass fleet_spawn{worktree:<ticket-slug>}, and worktree is declared "type": "string" — "'true' or a ticket slug" — so that advice works. I nearly filed this as a defect on the assumption worktree was a boolean; it is not. Worth recording because a fix whose error message names a parameter shape that does not exist is the same defect as #201's coverage line, and it is easy to wave through.

One thing worth noting for future work

13 existing tests had to move off a placeholder "/work/dir" string onto a real provisioned-worktree fixture, because they would otherwise trip the new gate incidentally — they cover #175/#234 model-mismatch machinery, not this gate.

That is a fair change, but it is a signal: a placeholder path that no gate ever looked at is now load-bearing. Any future test that invents a cwd string will silently take the "not provisioned" branch and pass for the wrong reason. If that bites, the answer is a shared fixture helper rather than another string.

Mutation evidence, both reverted and diff -q confirmed

  • gate at OpenCodeLauncher.java:812 → if (false): theHandleNeverReportsAnIdForANonProvisionedCwdEvenAfterARowAppears RED at OpenCodeLauncherTest.java:448 — expected <null> but was <ses_someone_elses>. 0 compile errors.
  • resume refusal at OpenCodeLauncher.java:683 → prepend false &&: aResumeSpawnWithoutAProvisionedWorktreeIsRefused RED at OpenCodeLauncherTest.java:379. 0 compile errors.

Closing.

Merged to `main` as `c4deef0` (PR #253). 1234 tests, 0 failures, 0 compile errors. The fix is the one this ticket asked for: **refuse to answer rather than guess**. `agentSessionId()` returns null for a member whose cwd is not a fleetd-provisioned worktree, and `spawn()` refuses a `resumeSessionId` request for such a cwd outright, before anything starts. `OpenCodeSessionDiscovery`'s SQL is untouched, as scoped — #234 already established the heuristic cannot be made reliable at that layer. `isProvisionedWorktree` moved from `ClaudeCodeLauncher` to `HerdrPeerLauncher` so both adapters share it. ## What I checked myself rather than taking on trust Two claims in the delivery mattered enough to verify, because both are the kind that read as fine and are not: **The refusal has to reach the lead as a message, not a stack trace.** Criterion 3 says "refused with a message saying why", and an `IllegalArgumentException` thrown from a launcher could easily surface as an opaque failure. It does not: `FleetMcp.spawn` already has `catch (IllegalArgumentException e) → error(e.getMessage())`, alongside the existing `GuardException` and `PlacementException` handlers. The worker also updated that catch's comment to name the new case. Criterion 3 is genuinely met. **The refusal message tells the lead what to do instead**, and I checked the parameter it names is real. It says to pass `fleet_spawn{worktree:<ticket-slug>}`, and `worktree` is declared `"type": "string"` — *"'true' or a ticket slug"* — so that advice works. I nearly filed this as a defect on the assumption `worktree` was a boolean; it is not. Worth recording because a fix whose error message names a parameter shape that does not exist is the same defect as #201's coverage line, and it is easy to wave through. ## One thing worth noting for future work 13 existing tests had to move off a placeholder `"/work/dir"` string onto a real provisioned-worktree fixture, because they would otherwise trip the new gate incidentally — they cover #175/#234 model-mismatch machinery, not this gate. That is a fair change, but it is a signal: a placeholder path that no gate ever looked at is now load-bearing. Any *future* test that invents a cwd string will silently take the "not provisioned" branch and pass for the wrong reason. If that bites, the answer is a shared fixture helper rather than another string. ## Mutation evidence, both reverted and `diff -q` confirmed - gate at `OpenCodeLauncher.java:812` → `if (false)`: `theHandleNeverReportsAnIdForANonProvisionedCwdEvenAfterARowAppears` RED at `OpenCodeLauncherTest.java:448` — *expected `<null>` but was `<ses_someone_elses>`*. 0 compile errors. - resume refusal at `OpenCodeLauncher.java:683` → prepend `false &&`: `aResumeSpawnWithoutAProvisionedWorktreeIsRefused` RED at `OpenCodeLauncherTest.java:379`. 0 compile errors. Closing.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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#249
Delete Branch "%!s()"

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?