fleetd #274: clean up the worktree and branch when add() fails after creating them

GitWorktrees.add() created the worktree and branch, then ran several more
steps that can throw (requireCredentialFreeHttpsOrigin — an intended
security refusal, not only an IO accident — plus the credential-helper and
tool-surface isolation steps). Any exception there meant add() never
returned, so its caller (SessionManager#acquireWithWorktree) never learned
the path: its local `path` stayed null, the `if (path != null)` cleanup
guard never ran, and the worktree directory and branch leaked on disk
forever with nothing tracking them.

Wrap those steps in try/catch; on failure, clean up via the same
`git worktree remove --force` path remove() already uses, additionally
force-delete the new branch (remove() alone deliberately leaves a
released session's branch behind, but a branch that never finished
provisioning has nothing else pointing at it), log the cleanup outcome,
and rethrow the original exception so it is never masked.

Test drives add() itself via the existing afterWorktreeAdded seam with a
mutation that trips requireCredentialFreeHttpsOrigin after the worktree
exists, then asserts both the worktree directory and the branch are gone.
Reverting the fix (git stash on GitWorktrees.java, test unchanged) turns
it red: "the worktree directory leaked after a post-creation step threw
==> expected: <false> but was: <true>". Restored afterward.

mvn clean install: BUILD SUCCESS, Tests run: 1275, Failures: 0, Errors: 0

fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java

@@ -319,10 +319,12 @@ public final class FleetMcp {
                 };
         BiFunction<McpSyncServerExchange, McpSchema.CallToolRequest, McpSchema.CallToolResult> pollHandler =
                 (exchange, req) -> {
-                    McpSchema.CallToolResult denied = deny(exchange, Authz.Action.READ, null);
-                    if (denied != null) return denied;
                     Map<String, Object> a = req.arguments();
-                    return poll(messages, str(a, "ticket"), str(a, "target"));
+                    String target = str(a, "target");
+                    // The action depends on the ARGUMENTS, not on the tool name -- see pollAction.
+                    McpSchema.CallToolResult denied = deny(exchange, pollAction(target), target);
+                    if (denied != null) return denied;
+                    return poll(messages, str(a, "ticket"), target);
                 };
         // CB-307 Increment 3: per-msgId ack (not needed in v1 but supported by the inbox).
         // Acking removes a reply from the inbox, so it is a drain, not a read.
@@ -724,6 +726,34 @@ public final class FleetMcp {
         return text("delivered to peer lead " + coordId + " (msgId " + msg.msgId() + ")");
     }
 
+    /**
+     * Which authorization action a {@code fleet_poll} call needs, decided by its arguments
+     * (fleetd #272).
+     *
+     * <p>{@code fleet_poll} is <strong>two operations behind one tool name</strong>. With {@code
+     * ticket} it observes an async delegation and changes nothing, which is a {@link
+     * Authz.Action#READ}. With {@code target} it calls {@link MessageService#drainReplies} on that
+     * session -- the replies are removed from the inbox and a second call returns nothing -- so it
+     * is a {@link Authz.Action#DRAIN}, the same gate {@code fleet_ack} already uses for removing a
+     * single message, and the same one the REST path uses at {@code FleetApp.drainReplies}.
+     *
+     * <p>Until this method existed the handler passed a constant {@code READ} for both branches.
+     * {@code READ} is open to every authenticated role, so any worker could read a peer's id out of
+     * {@code fleet_list} and destroy the replies that peer had queued for the primary. The gate
+     * failed open, and it did so because the required action is a function of the arguments while
+     * the handler chose it before looking at them.
+     *
+     * <p>The choice lives in this method, and not inline in the handler, so that a test can assert
+     * the mapping the handler actually uses. {@code FleetMcpAuthzTest} already checked every
+     * {@link Authz.Action} against every {@link Role} and passed throughout -- it tested the policy
+     * table, which was correct, while the defect was in which action the caller handed it.
+     *
+     * @param target the {@code target} argument of the call, or {@code null}/blank when absent
+     */
+    static Authz.Action pollAction(String target) {
+        return isBlank(target) ? Authz.Action.READ : Authz.Action.DRAIN;
+    }
+
     /** {@code fleet_poll}: check an async delegation by ticket, or drain a worker's inbox by target. */
     static McpSchema.CallToolResult poll(MessageService messages, String ticket, String target) {
         if (!isBlank(target)) {

fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java

@@ -167,14 +167,59 @@ public final class GitWorktrees implements Worktrees {
         log.info("adding worktree branch={} path={} base={}", branch, wt, base);
         removeUserInfoFromHttpsOrigin(repoRoot);
         exec("git", "-C", repoRoot, "worktree", "add", wt, "-b", branch, base);
-        afterWorktreeAdded.accept(wt);
-        requireCredentialFreeHttpsOrigin(wt);
-        configureEnvironmentCredentialHelper(repoRoot, wt);
-        configureHttpsUrlRewriteForSshOrigin(repoRoot, wt);
-        isolateToolSurface(wt);
+        try {
+            afterWorktreeAdded.accept(wt);
+            requireCredentialFreeHttpsOrigin(wt);
+            configureEnvironmentCredentialHelper(repoRoot, wt);
+            configureHttpsUrlRewriteForSshOrigin(repoRoot, wt);
+            isolateToolSurface(wt);
+        } catch (RuntimeException e) {
+            cleanupAfterAddFailure(repoRoot, wt, branch, e);
+            throw e;
+        }
         return wt;
     }
 
+    /**
+     * {@code add()} has already created the worktree and its branch by the time any step from
+     * {@link #afterWorktreeAdded} through {@link #isolateToolSurface} can throw — including
+     * {@link #requireCredentialFreeHttpsOrigin}, an intended security refusal, not only an IO
+     * accident. Without this, {@code add()} never returns, so its caller
+     * ({@code SessionManager#acquireWithWorktree}) never receives a path to register or clean up:
+     * its local {@code path} stays null, the {@code if (path != null)} guard in its own catch block
+     * never runs, and the worktree directory and branch leak on disk forever with nothing tracking
+     * them (fleetd #274).
+     *
+     * <p>Reuses {@link #remove} — the same {@code git worktree remove --force} path every other
+     * cleanup exit in this class already goes through — rather than a bespoke removal. It
+     * additionally deletes {@code branch}: {@link #remove} alone deliberately leaves a released
+     * session's branch behind (a worker's branch is expected to outlive its worktree, for PRs and
+     * recovery), but a branch that never finished provisioning has no session, no PR, and nothing
+     * else pointing at it, so leaving it behind would just trade one leak for a smaller one. Forced
+     * (`-D`) because the branch is new and unmerged by construction. The worktree is removed first:
+     * a branch checked out by a worktree cannot be deleted until the worktree that holds it is gone.
+     *
+     * <p>Cleanup failure must never mask {@code original} — that is the exception that explains
+     * what actually went wrong — so a failure here is only logged, matching the pattern already
+     * used in {@code SessionManager#acquireWithWorktree}'s own catch block.
+     */
+    private void cleanupAfterAddFailure(String repoRoot, String worktreePath, String branch, RuntimeException original) {
+        log.warn("provisioning failed for branch={} path={}: {} — cleaning up before rethrowing",
+                branch, worktreePath, original.getMessage());
+        try {
+            remove(repoRoot, worktreePath);
+        } catch (RuntimeException cleanup) {
+            log.warn("failed to remove leaked worktree {} after provisioning error: {}",
+                    worktreePath, cleanup.getMessage());
+        }
+        try {
+            exec("git", "-C", repoRoot, "branch", "-D", branch);
+        } catch (RuntimeException cleanup) {
+            log.warn("failed to remove leaked branch {} after provisioning error: {}",
+                    branch, cleanup.getMessage());
+        }
+    }
+
     /**
      * A linked worktree shares its primary checkout's git config. Remove HTTPS user info before
      * adding one, so a credential accidentally embedded in that config cannot reach the member.

fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java

@@ -179,6 +179,53 @@ class FleetMcpAuthzTest {
                 "no CallerResolver supplied ⇒ authorization not enforced (legacy behaviour)");
     }
 
+    // --- which action each tool hands the gate (fleetd #272) ------------------------------------
+
+    /**
+     * fleetd #272: {@code fleet_poll{target}} drains a session's reply inbox, so it needs
+     * {@link Authz.Action#DRAIN} -- not the {@link Authz.Action#READ} the handler passed for both
+     * of its branches until this ticket.
+     *
+     * <p>This asserts against {@link FleetMcp#pollAction}, the method the handler itself calls, so
+     * the handler holds no separate copy of the rule that this test could miss. Every other test in
+     * this class checks the policy table (is a worker allowed to DRAIN?) and all of them passed for
+     * the whole time the defect was live -- the table was right, the action fed to it was wrong.
+     */
+    @Test
+    void pollingByTargetIsADrainAndPollingByTicketIsARead() {
+        assertEquals(Authz.Action.DRAIN, FleetMcp.pollAction("term_b"),
+                "poll by target removes the replies — that is a drain, not an observation");
+        assertEquals(Authz.Action.READ, FleetMcp.pollAction(null),
+                "poll by ticket changes nothing");
+        assertEquals(Authz.Action.READ, FleetMcp.pollAction("  "),
+                "a blank target is an absent target");
+    }
+
+    @Test
+    void aWorkerMayNotDrainAnotherSessionsInboxByPolling() {
+        FleetMcp m = mcp(true);
+
+        assertNotNull(m.denyFor(WORKER_A, FleetMcp.pollAction("term_b"), "term_b"),
+                "a worker draining a peer's inbox would destroy replies queued for the primary");
+        assertNotNull(m.denyFor(ARCH_DESIGN, FleetMcp.pollAction("term_b"), "term_b"),
+                "an architect has no lifecycle rights either — same gate as fleet_ack");
+        assertNull(m.denyFor(PRIMARY, FleetMcp.pollAction("term_b"), "term_b"),
+                "collecting a held reply is the primary's job");
+    }
+
+    /**
+     * The tightening must not close the branch that legitimately serves non-primary callers: an
+     * architect may {@code fleet_send}, so it owns tickets and must be able to poll them.
+     */
+    @Test
+    void pollingAnOwnTicketStaysOpenToWorkersAndArchitects() {
+        FleetMcp m = mcp(true);
+
+        assertNull(m.denyFor(WORKER_A, FleetMcp.pollAction(null), null));
+        assertNull(m.denyFor(ARCH_DESIGN, FleetMcp.pollAction(null), null),
+                "an architect delegates with wait:false, so it must be able to poll its ticket");
+    }
+
     // --- identity reconstruction from the transport context ------------------------------------
 
     @Test

fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java

@@ -21,6 +21,7 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 
 import static org.junit.jupiter.api.Assertions.*;
 
@@ -362,6 +363,46 @@ class GitWorktreesTest {
         assertEquals("worktree origin contains HTTPS user info; refusing provision", error.getMessage());
     }
 
+    /**
+     * fleetd #274. {@code add()} creates the worktree and its branch, then runs several more steps
+     * that can throw — {@code requireCredentialFreeHttpsOrigin} among them, an intended security
+     * refusal, not an IO accident. Before the fix, any exception from those later steps left
+     * {@code add()} never returning, so its caller never learned the path and the worktree
+     * directory plus its branch leaked on disk forever with nothing tracking them.
+     *
+     * <p>This drives the exact same {@code afterWorktreeAdded} test seam as
+     * {@link #provisioningRefusesAWorktreeWhoseOriginStillHasHttpsUserInfo} — a mutation applied
+     * right after {@code git worktree add}, so the step that throws
+     * ({@code requireCredentialFreeHttpsOrigin}, reached moments later inside {@code add()} itself)
+     * runs strictly after the worktree and branch already exist, not downstream of {@code add()}
+     * in some other caller. {@code afterWorktreeAdded} also hands back the created path, so the
+     * assertions below don't have to guess the generated nonce.
+     */
+    @Test
+    void addCleansUpTheWorktreeAndBranchWhenAPostCreationStepThrows(@TempDir Path tmp) throws Exception {
+        Path repo = initRepo(tmp.resolve("repo"));
+        git(repo, "remote", "add", "origin", "https://git.ltms.dev/akb/kb.git");
+        String branch = "cb-274-leak";
+        AtomicReference<String> createdPath = new AtomicReference<>();
+        GitWorktrees worktrees = new GitWorktrees(tmp.resolve("wts").toString(), worktreePath -> {
+            createdPath.set(worktreePath);
+            try {
+                git(Path.of(worktreePath), "remote", "set-url", "origin",
+                        "https://synthetic-test-token@git.ltms.dev/akb/kb.git");
+            } catch (Exception e) {
+                throw new RuntimeException(e);
+            }
+        });
+
+        assertThrows(WorktreeException.class, () -> worktrees.add(repo.toString(), branch, "HEAD"));
+
+        assertNotNull(createdPath.get(), "afterWorktreeAdded must have run with the created path");
+        assertFalse(Files.exists(Path.of(createdPath.get())),
+                "the worktree directory leaked after a post-creation step threw");
+        String heads = forEachRef(repo, "refs/heads/" + branch);
+        assertTrue(heads.isBlank(), "the branch leaked after a post-creation step threw:\n" + heads);
+    }
+
     // ---- CB-189: broader remote-URL coverage — every remote, both fetch and push URLs, any
     // non-SSH scheme. Reporting only, additive to the origin/https strip-and-refuse tests above. ----