Add authorization entry-point audit

AUDIT.md

@@ -1,69 +1,76 @@
-# Teardown/cleanup audit — dev.ltms.fleet.session
+# Authorization entry-point audit
 
-Scope: `SessionManager.java`, `GitWorktrees.java`, `SessionReaper.java`, `MemberSession.java`,
-`Worktrees.java` (interface). Read-only; no code changed.
+Scope reviewed: every route registered in `FleetApp.build`, every tool handler
+registered in `FleetMcp`, and their shared `CallerResolver` and `Authz` gate.
 
-## Main finding
+## Result
 
-```
-1. SessionManager.java:336-338
-2. issue: the final worktree removal in release() is the one step in the whole method
-   that is not wrapped in try/catch. Every other cleanup step here (hasUncommitted check,
-   snapshot, listener notification) is defended because a `git` call can throw — exec()'s
-   own javadoc documents both a non-zero exit and its 30-second timeout as normal failure
-   modes, and every sibling worktrees.* call in this class is guarded against exactly that.
-   By the time this line runs, registry.remove(paneId) and handles.remove(paneId) have
-   already happened and launcher.stop(paneId) has already run, so if worktrees.remove()
-   throws here (e.g. `git worktree remove --force` times out on a stale lock file or a
-   slow/network filesystem, or exits non-zero), the exception escapes release() with no
-   way to retry: the paneId is already gone from the registry, so a second stop call is a
-   no-op and never re-attempts the removal. The worktree directory is now leaked forever,
-   invisible to `fleet_list`. The caller sees a stop failure — FleetMcp.stop() only catches
-   HerdrException, and FleetApp.stopMember() catches nothing — even though the session was
-   in fact fully torn down (pane stopped, deregistered, listeners notified).
-3. fix: wrap the `worktrees.remove(...)` call at the end of release() in a try/catch that
-   logs a warning, matching the pattern already used for every other cleanup step in this
-   method (e.g. cleanupAfterAddFailure's own worktree/branch removal, or the dirty-check
-   catch above it).
-4. severity: medium
-```
+No authorization-action mismatch was found. The only handler whose operation
+changes with its arguments is `fleet_poll`. It selects `DRAIN` when `target` is
+present and `READ` when it is absent, before it calls either service branch.
 
-## Secondary findings
+`Fleetd` creates one `CallerResolver` and passes that same instance to both
+`FleetMcp` and `FleetApp` (`Fleetd.java:615-650`, `720-722`). REST resolves it
+in the Javalin pre-handler. MCP resolves it in the transport context extractor.
 
-```
-1. SessionManager.java:503-514 (acquireWithWorktree's catch block)
-2. issue: after worktrees.add() succeeds, if overlayParity(), shareWithGroup(), or
-   launcher.spawn() then throws, the catch block removes only the worktree
-   (worktrees.remove(repoRoot, path)) and never deletes the branch `git worktree add`
-   created. GitWorktrees.cleanupAfterAddFailure — the sibling cleanup for failures inside
-   add() itself — explicitly deletes the branch too, with a `-D` and a documented reason
-   ("a branch that never finished provisioning has no session, no PR, nothing else
-   pointing at it"). That reasoning applies equally here, but this later catch block (the
-   one covering the three post-add() steps) omits it. Since spawn failures are a normal,
-   recurring event (this very branch already logs "spawn failed for profile=..."), this
-   leaks an orphan `worker/<slug>-<nonce>` branch in the shared repo on every such failure,
-   with nothing pointing at it once the (failed) session is never registered.
-3. fix: after worktrees.remove(...) succeeds in this catch, also delete the branch with
-   `git branch -D branch` (best-effort, log-only on failure), matching
-   cleanupAfterAddFailure's own two-step cleanup.
-4. severity: low
-```
+## REST routes
 
-No other issue in this scope survived a read of every exit of `add()`, `remove()`,
-`snapshot()`, `overlayParity()`, `shareWithGroup()`/`shareRootWithGroup()`, `release()`,
-`reapIdle()`, `drainAll()`, and the `SessionReaper` loop. Two shapes I checked and ruled
-out as not reachable / not defects:
+| Route | Gate and choice location | Branch/target review | Verdict |
+|---|---|---|---|
+| `GET /healthz` | None | Liveness probe only; intentionally open. | ok |
+| `GET /metrics` | `METRICS`, start of `metrics` | No branch or target. | ok |
+| `GET /sessions` | `READ`, start of `sessions` | No caller-selected target. | ok |
+| `GET /agents` | `READ`, start of `agents` | No caller-selected target. | ok |
+| `GET /members` | `READ`, start of `listMembers` | No caller-selected target. | ok |
+| `GET /profiles` | `READ`, start of `profiles` | No caller-selected target. | ok |
+| `GET /member-credentials` | `READ`, start of `memberCredentials` | No caller-selected target. The view exposes policy names and counts, not values. | ok |
+| `POST /members` | `SPAWN`, before query/body parsing in `spawnMember` | Arguments select role, profile, cwd, and worktree. They do not select a different authority type. | ok |
+| `DELETE /members/{paneId}` | `STOP`, after reading `paneId` in `stopMember` | Caller can select another pane, but only a primary has `STOP`. | ok |
+| `POST /sessions/{id}/message` | `SEND`, after reading path `id` in `sendMessage` | Normal send, async send, and `turnId` answer all deliver a turn/message. `turnId` does not widen the roles allowed to send. | ok |
+| `POST /sessions/{id}/reply` | `REPLY`, after reading path `id` in `replyMessage` | Caller can name a target, and `Authz` requires it to equal the connection-resolved terminal. | ok |
+| `GET /sessions/{id}/replies` | `DRAIN`, after reading path `id` in `drainReplies` | Removes inbox entries; only a primary has `DRAIN`. | ok |
+| `POST /sessions/{id}/ask` | `ASK`, after reading path `id` in `askMessage` | Caller can name a target, and `Authz` requires it to equal the connection-resolved terminal. | ok |
+| `GET /sessions/{id}/status` | `READ`, after reading path `id` in `sessionStatus` | Any authenticated role may observe any session. This matches the `READ` policy, which intentionally does not use target ownership. | ok |
+| `GET /tasks/{ticket}` | `READ`, start of `taskStatus` | Ticket polling is read-only; no service branch changes the action. | ok |
 
-- `release()`'s `worktrees.repoRoot(removed.cwd())` looked suspicious because `cwd` for a
-  worktree session is the worktree path itself, so `repoRoot` would equal `worktreePath` —
-  but I verified with a live git repo (`git --version` 2.53.0) that
-  `git -C <worktree> worktree remove --force <same worktree>` works correctly: git
-  resolves `-C` against the common git dir regardless of which linked worktree it's given,
-  so this is not a bug.
-- `git worktree remove --force` on a worktree containing a nested `.git` directory: I
-  expected this to need a double `--force` per older git docs, but tested it live and a
-  single `--force` succeeds on git 2.53.0. Not a live failure mode on this stack.
+## MCP tools
 
-The `if (x != null)` guard-in-catch shape from fleetd #274 (guard assigned only at the end)
-does not recur elsewhere in this scope: every `catch` block that guards on a local now
-assigns that local before the risky call it protects, not after.
+| Tool | Gate and choice location | Branch/target review | Verdict |
+|---|---|---|---|
+| `fleet_send` | `SEND`, at the start of `sendHandler` | `coordId`, `turnId`, synchronous, and async forms all deliver a message or a turn answer. `sessionId` is read before the branch for audit target only. | ok |
+| `fleet_reply` | `REPLY`, after deriving the connection terminal in `replyHandler` | No target argument exists. The service always receives the caller's own terminal. | ok |
+| `fleet_ask` | `ASK`, after deriving the connection terminal in `askHandler` | No target argument exists. The service always receives the caller's own terminal. | ok |
+| `fleet_status` | `READ`, start of `statusHandler` | Any authenticated role may query any session. This is the same intentional `READ` policy as the REST route. | ok |
+| `fleet_poll` with `ticket` | `READ`, `pollAction(target)` before dispatch in `pollHandler` | Reads a task only. | ok |
+| `fleet_poll` with `target` | `DRAIN`, `pollAction(target)` before dispatch in `pollHandler` | Drains and removes a target inbox. Only a primary has `DRAIN`. | ok |
+| `fleet_ack` | `DRAIN`, start of `ackHandler` | Removes one target inbox entry. Only a primary has `DRAIN`. | ok |
+| `fleet_spawn` | `SPAWN`, start of `spawnHandler` | Arguments select member configuration only. | ok |
+| `fleet_list` | `READ`, start of `listHandler` | No caller-selected target. | ok |
+| `fleet_stop` | `STOP`, after reading `paneId` in `stopHandler` | Caller can select another pane, but only a primary has `STOP`. | ok |
+| `fleet_profiles` | `READ`, start of `profilesHandler` | No caller-selected target. | ok |
+| `fleet_whoami` | `READ`, start of `whoamiHandler` | Reports the connection-resolved caller, not an argument. | ok |
+
+## Surface parity
+
+The matching route/tool pairs use the same action:
+
+| Operation | REST | MCP | Result |
+|---|---|---|---|
+| spawn | `SPAWN` | `SPAWN` | match |
+| stop | `STOP` | `STOP` | match |
+| send and ask answer | `SEND` | `SEND` | match |
+| reply | `REPLY` | `REPLY` | match |
+| ask | `ASK` | `ASK` | match |
+| drain replies | `DRAIN` | `DRAIN` for `fleet_poll{target}` and `fleet_ack` | match |
+| status | `READ` | `READ` | match |
+| task/ticket polling | `READ` | `READ` | match |
+| list/profiles | `READ` | `READ` | match |
+
+## Test risk
+
+`FleetMcpAuthzTest` has a direct regression test for the argument-dependent
+`fleet_poll` action. It tests the shared role table for the other tools, but it
+does not pin each handler's chosen action. This is not a current defect because
+the reviewed handlers choose the matching action. A future change that adds an
+argument-dependent operation should add a handler-level action-selection test,
+like `pollingByTargetIsADrainAndPollingByTicketIsARead`.