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

fleet_status has no owner check and returns another session's pending question text AND its turnId, so the #715 turnId never has to be guessed #721

New Issue
Closed
opened 2026-10-04 08:13:39 +02:00 by ltms · 4 comments
ltms commented 2026-10-04 08:13:39 +02:00
Owner
Copy Link
Copy Source

Found by an architect member reviewing #705 option 1. I verified every line below myself in the main
clone at 9a64d42.

The leak

fleet_status is authorized with no target, so the ownership check is skipped:

// FleetMcp.java:513-518
BiFunction<...> statusHandler = (exchange, req) -> {
    McpSchema.CallToolResult denied = deny(exchange, toolAction("fleet_status", req.arguments()), null);
    //                                                                                           ^^^^
    if (denied != null) return denied;
    return status(messages, str(req.arguments(), "sessionId"));
};

status then takes the caller-supplied sessionId and tests nothing:

// FleetMcp.java:1341-1356
static McpSchema.CallToolResult status(MessageService messages, String sessionId) {
    ...
    MessageService.PendingAsk ask = messages.pendingAsk(sessionId);
    if (ask == null) return text(base);
    return text(base + "\n\n[question — worker is waiting for your answer]\n" + ask.question()
            + "\n\nAnswer it by calling fleet_send again with turnId=\"" + ask.turnId()
            + "\" and content set to your answer; the worker resumes the same turn."
            + " (ticket " + ask.ticket() + ")");
}

And pendingAsk has no caller filter either — it matches only on the session you name:

// MessageService.java:1763-1771
public PendingAsk pendingAsk(String workerSession) {
    for (Task task : tasks.values()) {
        Reply q = task.question;
        if (q != null && workerSession.equals(task.target)) {
            return new PendingAsk(task.ticket, q.text(), q.turnId());
        }
    }
    return null;
}

The gate is TASK_READ, held by three roles:

// Authz.java:145
case TASK_READ -> caller.isPrimary() || caller.isWorker() || caller.isArchitect();

Both surfaces are affected. REST has the same shape:

// FleetApp.java:81
case "GET /sessions/{id}/status", "GET /tasks/{ticket}" -> Authz.Action.TASK_READ;

Note GET /tasks/{ticket} on that same line was fixed — #716 threaded the caller's terminal into
messages.poll. The status route beside it was not. One line, two routes, only one of them gated.

Why this is worse than it looks: it removes the guess from #715

#715 is filed as "a turnId is a guessable counter". I argued on that ticket that the guess is
harder than it sounds, because Rendezvous.java:148 mints session + "#" + askSeq.incrementAndGet(),
so you must know a target's terminal id first. That mitigation is void. fleet_status hands over
the turnId directly, together with the question text and the ticket id.

The full chain, with the grant at each step:

Step Call Grant Yields
1 fleet_list READ every member's sessionId — membersVisibleTo admits primary and architect
2 fleet_status{sessionId} TASK_READ that member's pending question text, its turnId, its ticket
3 fleet_send{turnId, content} ANSWER = primary or architect answers a turn it has no part in

So an architect can hijack any delegation's rendezvous with no guessing at all, and read the question
on the way. Step 1 is the reason this is reachable: #710 hid members and leads from a worker,
so a worker cannot enumerate session ids from the bridge, but an architect still can by design.

The content is the part that matters. A pending question is a member's own words about the work in
front of it — the Authz comment two lines above TASK_READ already states this exact principle as
the reason a collaborator is excluded from that row:

ticket ids are a sequential counter with no owner check, so a holder could walk every ticket and
read another session's delegation reply.

The narrower role got the stricter rule. The broader roles kept the open door, and the door is wider
than the ticket-id one that was just closed.

Suggested fix

Give fleet_status the same treatment #716 gave fleet_poll{ticket}: pass the resolved caller and
check it, rather than authorizing with a null target.

Two things to decide deliberately, both of which have already bitten this repo:

  1. Which record to compare against. The pending ask belongs to a Task, and Task.creatorTerminal
    already exists — #705/#716 added it. Comparing against the sender of the brief is right, and it
    keeps the architect's intended ability to answer a member it briefed. Do not compare against
    MemberSession.ownerTerminal: SPAWN is primary-only, so only a lead can ever be the spawn owner,
    and gating on it would forbid the normal architect case while permitting nothing new. (Same
    reasoning I wrote on #715.)
  2. Decide the null record case explicitly, and say which way it fails. A callerTerminal == null || callerTerminal.equals(...) test fails open and buys nothing — that is exactly #718's
    MessageService.poll(String). A strict equality against a null creator fails closed and would
    break the base fleet_status call for an unnamed primary, which is the break task-15 caught in
    #705 where gating the read alone would also have broken the documented REST fallback.

Also worth separating: the base status string and the pending-ask block are not equally
sensitive. Returning liveness for a session you do not own is close to what READ already gives;
returning the question text and turnId is not. A fix may legitimately keep the first and gate only
the second, which is a smaller and safer change than refusing the whole call.

Relationship to the other open tickets

  • #715 — this is the delivery mechanism for it. #715 should be re-scoped: the turnId is handed
    out, not guessed. Both want the same Task.creatorTerminal comparison, so they may be one change.
  • #705 — same family (a TASK_READ holder reading another session's content), and the architect
    reviewing option 1 argued this should ship before the OBSERVER floor, because it closes the one
    real content leak at a fraction of the cost and does not depend on that decision. I agree.
  • #719 — unrelated, but note ask.ticket() is also returned here, and ticket ids reset per boot.

Not measured

I have not driven the hijack from a live architect session: read members, call fleet_status on a
member with an open ask, then answer it. Everything above is read from the five code sites quoted,
each of which I opened myself. The reachability of step 1 rests on membersVisibleTo admitting an
architect, which I read in FleetMcp.java:786, not from a live architect's fleet_list output.

Found by an architect member reviewing #705 option 1. I verified every line below myself in the main clone at `9a64d42`. ## The leak `fleet_status` is authorized with **no target**, so the ownership check is skipped: ```java // FleetMcp.java:513-518 BiFunction<...> statusHandler = (exchange, req) -> { McpSchema.CallToolResult denied = deny(exchange, toolAction("fleet_status", req.arguments()), null); // ^^^^ if (denied != null) return denied; return status(messages, str(req.arguments(), "sessionId")); }; ``` `status` then takes the caller-supplied `sessionId` and tests nothing: ```java // FleetMcp.java:1341-1356 static McpSchema.CallToolResult status(MessageService messages, String sessionId) { ... MessageService.PendingAsk ask = messages.pendingAsk(sessionId); if (ask == null) return text(base); return text(base + "\n\n[question — worker is waiting for your answer]\n" + ask.question() + "\n\nAnswer it by calling fleet_send again with turnId=\"" + ask.turnId() + "\" and content set to your answer; the worker resumes the same turn." + " (ticket " + ask.ticket() + ")"); } ``` And `pendingAsk` has no caller filter either — it matches only on the session you name: ```java // MessageService.java:1763-1771 public PendingAsk pendingAsk(String workerSession) { for (Task task : tasks.values()) { Reply q = task.question; if (q != null && workerSession.equals(task.target)) { return new PendingAsk(task.ticket, q.text(), q.turnId()); } } return null; } ``` The gate is `TASK_READ`, held by three roles: ```java // Authz.java:145 case TASK_READ -> caller.isPrimary() || caller.isWorker() || caller.isArchitect(); ``` **Both surfaces are affected.** REST has the same shape: ``` // FleetApp.java:81 case "GET /sessions/{id}/status", "GET /tasks/{ticket}" -> Authz.Action.TASK_READ; ``` Note `GET /tasks/{ticket}` on that same line **was** fixed — #716 threaded the caller's terminal into `messages.poll`. The status route beside it was not. One line, two routes, only one of them gated. ## Why this is worse than it looks: it removes the guess from #715 #715 is filed as "a `turnId` is a **guessable** counter". I argued on that ticket that the guess is harder than it sounds, because `Rendezvous.java:148` mints `session + "#" + askSeq.incrementAndGet()`, so you must know a target's terminal id first. **That mitigation is void.** `fleet_status` hands over the `turnId` directly, together with the question text and the ticket id. The full chain, with the grant at each step: | Step | Call | Grant | Yields | |---|---|---|---| | 1 | `fleet_list` | `READ` | every member's `sessionId` — `membersVisibleTo` admits primary **and architect** | | 2 | `fleet_status{sessionId}` | `TASK_READ` | that member's pending **question text**, its `turnId`, its `ticket` | | 3 | `fleet_send{turnId, content}` | `ANSWER` = primary **or architect** | answers a turn it has no part in | So an architect can hijack any delegation's rendezvous with no guessing at all, and read the question on the way. Step 1 is the reason this is reachable: #710 hid `members` and `leads` from a **worker**, so a worker cannot enumerate session ids from the bridge, but an architect still can by design. The content is the part that matters. A pending question is a member's own words about the work in front of it — the `Authz` comment two lines above `TASK_READ` already states this exact principle as the reason a **collaborator** is excluded from that row: > ticket ids are a sequential counter with no owner check, so a holder could walk every ticket and > read another session's delegation reply. The narrower role got the stricter rule. The broader roles kept the open door, and the door is wider than the ticket-id one that was just closed. ## Suggested fix Give `fleet_status` the same treatment #716 gave `fleet_poll{ticket}`: pass the resolved caller and check it, rather than authorizing with a `null` target. Two things to decide deliberately, both of which have already bitten this repo: 1. **Which record to compare against.** The pending ask belongs to a `Task`, and `Task.creatorTerminal` already exists — #705/#716 added it. Comparing against the **sender of the brief** is right, and it keeps the architect's intended ability to answer a member it briefed. Do **not** compare against `MemberSession.ownerTerminal`: `SPAWN` is primary-only, so only a lead can ever be the spawn owner, and gating on it would forbid the normal architect case while permitting nothing new. (Same reasoning I wrote on #715.) 2. **Decide the `null` record case explicitly, and say which way it fails.** A `callerTerminal == null || callerTerminal.equals(...)` test fails **open** and buys nothing — that is exactly #718's `MessageService.poll(String)`. A strict equality against a `null` creator fails **closed** and would break the base `fleet_status` call for an unnamed primary, which is the break task-15 caught in #705 where gating the read alone would also have broken the documented REST fallback. Also worth separating: the **base status string** and the **pending-ask block** are not equally sensitive. Returning liveness for a session you do not own is close to what `READ` already gives; returning the question text and `turnId` is not. A fix may legitimately keep the first and gate only the second, which is a smaller and safer change than refusing the whole call. ## Relationship to the other open tickets - **#715** — this is the delivery mechanism for it. #715 should be re-scoped: the `turnId` is handed out, not guessed. Both want the same `Task.creatorTerminal` comparison, so they may be one change. - **#705** — same family (a `TASK_READ` holder reading another session's content), and the architect reviewing option 1 argued this should ship **before** the `OBSERVER` floor, because it closes the one real content leak at a fraction of the cost and does not depend on that decision. I agree. - **#719** — unrelated, but note `ask.ticket()` is also returned here, and ticket ids reset per boot. ## Not measured I have not driven the hijack from a live architect session: read `members`, call `fleet_status` on a member with an open ask, then answer it. Everything above is read from the five code sites quoted, each of which I opened myself. The reachability of step 1 rests on `membersVisibleTo` admitting an architect, which I read in `FleetMcp.java:786`, not from a live architect's `fleet_list` output.
ltms commented 2026-10-04 08:27:55 +02:00
Author
Owner
Copy Link
Copy Source

Lead, before delegating. I re-measured the fix surface in the main clone at 6e06058 (after #718 merged). Five new facts, two of which change the suggested fix above. Where this comment and the ticket body disagree, this comment is newer and it wins.

1. Exactly one Task is ever constructed, and both production callers record the creator

$ grep -rn 'new Task(' fleetd/src/main/java
MessageService.java:1304:  Task task = new Task(ticket, target, nowNanos, creatorTerminal);

One site, reached only through sendAsync. Both production callers pass a real terminal:

FleetMcp.java:1021:  messages.sendAsync(sessionId, content, onAccepted, creatorTerminal)
FleetApp.java:698:   messages.sendAsync(id, content, null, caller == null ? null : caller.terminal())

So every pending ask already has an owner recorded. Nothing new has to be stored. That settles decision 1 in the ticket: compare against Task.creatorTerminal, and the field is already populated.

2. Reuse ownsTicket — do not write a new comparison

MessageService.java:1434 already is the single place that knows task ownership:

private static boolean ownsTicket(Task task, String callerTerminal) {
    return callerTerminal == null || callerTerminal.equals(task.creatorTerminal);
}

It is private static in MessageService, which is also where the check belongs — the pending-ask scan is in that class, so the comparison stays next to the data.

I need to correct decision 2 in the ticket body, which blurs two different nulls. I wrote that a callerTerminal == null || ... test "fails open and buys nothing — that is exactly #718's MessageService.poll(String)". That is wrong as applied here. There are two nulls and they behave differently:

null Meaning ownsTicket result Right?
callerTerminal == null the unnamed primary — token or loopback trust, no herdr pane allows Yes, deliberately. Documented at MessageService.java:1428-1431, and it is what keeps the REST fallback working
task.creatorTerminal == null a task created through a short sendAsync overload denies every terminal-bearing caller Yes — fails closed in the dangerous direction

The #718 defect was a convenience overload that silently supplied the first null for a caller who did have a terminal. It was not the unnamed-primary allowance. So reusing ownsTicket unchanged is correct.

What you must not do is repeat the #718 shape: do not add a one-argument pendingAsk(String) that forwards null. That would hand the unnamed-primary bypass to every caller, which is the actual defect #718 was about.

3. You can afford to change the signature in place — poll's reason not to does not apply

poll(String) kept its overload because deleting it meant editing 44 test call sites. pendingAsk is nothing like that:

$ grep -rn 'pendingAsk' fleetd/src/test/java | wc -l
8
$ grep -rln 'pendingAsk' fleetd/src/test/java
fleetd/src/test/java/dev/ltms/fleet/msg/MessageServiceTest.java

8 call sites, all in one file. So change pendingAsk to take the caller terminal and fix all 8. No convenience overload, and therefore nothing for a future call site to reach for by mistake.

4. The REST route looks gated and is not

FleetApp.java:870 already passes the session id as the target:

if (!allow(ctx, routeAction("GET /sessions/{id}/status"), id)) {

That has no effect. The TASK_READ row ignores targetSession entirely — it is caller.isPrimary() || caller.isWorker() || caller.isArchitect(). Do not read that argument as an existing check. The REST surface at FleetApp.java:881 leaks the same three fields as the MCP one:

body.put("question", ask.question());
body.put("turnId", ask.turnId());
body.put("ticket", ask.ticket());

Two surfaces, one fix. A change that only touches FleetMcp leaves the REST route wide open.

5. The invariant is already written down, which makes it a free test case

Authz.java, the comment on the READ row:

READ is roster, profile, and identity observation — fleet_list, fleet_profiles, and fleet_whoami — and carries no secrets: no ticket reply, no pending question, and no other session's turn state. Those live under TASK_READ.

This is not a new policy. The codebase already claims a pending question is not readable without the stricter grant. The defect is that TASK_READ turned out to be role-only, with no ownership test, so moving it there enforced nothing. Treat that sentence as the specification.

What stays unchanged from the ticket body

Gate only the pending-ask block, not the whole call. The base status string (idle/working/done, and REST's ready) stays readable as it is today. That is the smaller change, it breaks no existing caller, and liveness is close to what READ already gives. Returning the question text and turnId is the part that is not.

Honest limit on what this fixes

This does not close the hijack on its own, and nobody should read the merge as if it did. #715 measured that a turnId is session + "#" + askSeq.incrementAndGet() — a counter. After this change an architect can no longer read another member's turnId, but it can still guess one, and answer() still takes no caller identity. Step 3 of the chain in the ticket body is untouched.

So the correct description of this unit is: it closes the read half. #715 is the write half, and the chain stays open until both land. I am sending #715 to an architect in parallel, because Rendezvous.AskWaiter records only the worker's session —

Rendezvous.java:63:  private record AskWaiter(String session, CompletableFuture<String> answer) {

— so there is nowhere to compare a delegator today, and where that identity should come from differs between a blocking and an async send. That is a design decision, not an implementation, so #715 is not ready to delegate yet.

Lead, before delegating. I re-measured the fix surface in the main clone at `6e06058` (after #718 merged). Five new facts, two of which **change the suggested fix above**. Where this comment and the ticket body disagree, this comment is newer and it wins. ## 1. Exactly one `Task` is ever constructed, and both production callers record the creator ``` $ grep -rn 'new Task(' fleetd/src/main/java MessageService.java:1304: Task task = new Task(ticket, target, nowNanos, creatorTerminal); ``` One site, reached only through `sendAsync`. Both production callers pass a real terminal: ``` FleetMcp.java:1021: messages.sendAsync(sessionId, content, onAccepted, creatorTerminal) FleetApp.java:698: messages.sendAsync(id, content, null, caller == null ? null : caller.terminal()) ``` So every pending ask already has an owner recorded. Nothing new has to be stored. That settles decision 1 in the ticket: compare against `Task.creatorTerminal`, and the field is already populated. ## 2. Reuse `ownsTicket` — do not write a new comparison `MessageService.java:1434` already is the single place that knows task ownership: ```java private static boolean ownsTicket(Task task, String callerTerminal) { return callerTerminal == null || callerTerminal.equals(task.creatorTerminal); } ``` It is `private static` in `MessageService`, which is also where the check belongs — the pending-ask scan is in that class, so the comparison stays next to the data. **I need to correct decision 2 in the ticket body, which blurs two different `null`s.** I wrote that a `callerTerminal == null || ...` test "fails **open** and buys nothing — that is exactly #718's `MessageService.poll(String)`". That is wrong as applied here. There are two nulls and they behave differently: | null | Meaning | `ownsTicket` result | Right? | |---|---|---|---| | `callerTerminal == null` | the **unnamed primary** — token or loopback trust, no herdr pane | allows | **Yes, deliberately.** Documented at `MessageService.java:1428-1431`, and it is what keeps the REST fallback working | | `task.creatorTerminal == null` | a task created through a short `sendAsync` overload | denies every terminal-bearing caller | **Yes** — fails closed in the dangerous direction | The #718 defect was a *convenience overload that silently supplied the first null for a caller who did have a terminal*. It was not the unnamed-primary allowance. So reusing `ownsTicket` unchanged is correct. What you must **not** do is repeat the #718 shape: **do not add a one-argument `pendingAsk(String)` that forwards `null`.** That would hand the unnamed-primary bypass to every caller, which is the actual defect #718 was about. ## 3. You can afford to change the signature in place — `poll`'s reason not to does not apply `poll(String)` kept its overload because deleting it meant editing 44 test call sites. `pendingAsk` is nothing like that: ``` $ grep -rn 'pendingAsk' fleetd/src/test/java | wc -l 8 $ grep -rln 'pendingAsk' fleetd/src/test/java fleetd/src/test/java/dev/ltms/fleet/msg/MessageServiceTest.java ``` 8 call sites, all in one file. So change `pendingAsk` to take the caller terminal and fix all 8. No convenience overload, and therefore nothing for a future call site to reach for by mistake. ## 4. The REST route *looks* gated and is not `FleetApp.java:870` already passes the session id as the target: ```java if (!allow(ctx, routeAction("GET /sessions/{id}/status"), id)) { ``` That has **no effect**. The `TASK_READ` row ignores `targetSession` entirely — it is `caller.isPrimary() || caller.isWorker() || caller.isArchitect()`. Do not read that argument as an existing check. The REST surface at `FleetApp.java:881` leaks the same three fields as the MCP one: ```java body.put("question", ask.question()); body.put("turnId", ask.turnId()); body.put("ticket", ask.ticket()); ``` **Two surfaces, one fix.** A change that only touches `FleetMcp` leaves the REST route wide open. ## 5. The invariant is already written down, which makes it a free test case `Authz.java`, the comment on the `READ` row: > READ is roster, profile, and identity observation — `fleet_list`, `fleet_profiles`, and `fleet_whoami` — and carries no secrets: **no ticket reply, no pending question**, and no other session's turn state. Those live under `TASK_READ`. This is not a new policy. The codebase already claims a pending question is not readable without the stricter grant. The defect is that `TASK_READ` turned out to be role-only, with no ownership test, so moving it there enforced nothing. Treat that sentence as the specification. ## What stays unchanged from the ticket body Gate **only the pending-ask block**, not the whole call. The base status string (`idle`/`working`/`done`, and REST's `ready`) stays readable as it is today. That is the smaller change, it breaks no existing caller, and liveness is close to what `READ` already gives. Returning the question text and `turnId` is the part that is not. ## Honest limit on what this fixes **This does not close the hijack on its own, and nobody should read the merge as if it did.** #715 measured that a `turnId` is `session + "#" + askSeq.incrementAndGet()` — a counter. After this change an architect can no longer *read* another member's `turnId`, but it can still *guess* one, and `answer()` still takes no caller identity. Step 3 of the chain in the ticket body is untouched. So the correct description of this unit is: it closes the **read** half. #715 is the **write** half, and the chain stays open until both land. I am sending #715 to an architect in parallel, because `Rendezvous.AskWaiter` records only the worker's session — ``` Rendezvous.java:63: private record AskWaiter(String session, CompletableFuture<String> answer) { ``` — so there is nowhere to compare a delegator today, and where that identity should come from differs between a blocking and an async send. That is a design decision, not an implementation, so #715 is not ready to delegate yet.
ltms commented 2026-10-04 08:33:45 +02:00
Author
Owner
Copy Link
Copy Source

Lead, a pointer rather than a correction — nothing above changes.

I checked that the acceptance criteria I set are actually satisfiable before you spend time on a harness. They are, and #716 already left you a near-exact template for every case on both surfaces. Mirror these rather than inventing anything:

REST — fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppAuthTest.java

:182  restPollRefusesADifferentWorkerButAllowsTheCreatorAndTheUnnamedPrimary
:150  theTaskStatusRouteActuallyThreadsTheCallersTerminalIntoPoll
:223  restSendAsyncRecordsTheCreatingCallersTerminalSoItCanStillPollItsOwnTicket

:182 covers P1, P2 and P3 in one test for the poll route — a different worker refused, the creator allowed, the unnamed primary allowed. That is the same three-way split you need for the status route. :150 is the "actually threads the terminal through" shape, which is what stops a fix that looks right but passes the wrong value.

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

:512  theFleetPollHandlerActuallyThreadsCallerTerminalIntoPoll
:649  pollingAnOwnTicketStaysOpenToWorkersAndArchitects

:512 is the one my own mutation run killed when I reverted FleetMcp poll's callerTerminal to null, so it is proven to fail for the right reason. That makes it the right model for your criterion 3.

Two notes on using them:

  • :223's shape matters for P4. It exists because recording and checking have to ship together: a fix that checks a creator the send never recorded refuses the ticket's own creator. You are not changing the recording side, so P4 should hold for free — but assert it rather than assuming it, because that is the half that silently inverts.
  • Keep the behaviour in the test name, not the bug. restPollRefusesADifferentWorker... is the house style: it names what is protected.

Nothing here changes the scope or the five properties. Re-read my earlier comment before you commit — that one does correct the ticket body.

Lead, a pointer rather than a correction — nothing above changes. I checked that the acceptance criteria I set are actually satisfiable before you spend time on a harness. They are, and #716 already left you a near-exact template for every case on both surfaces. Mirror these rather than inventing anything: **REST** — `fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppAuthTest.java` ``` :182 restPollRefusesADifferentWorkerButAllowsTheCreatorAndTheUnnamedPrimary :150 theTaskStatusRouteActuallyThreadsTheCallersTerminalIntoPoll :223 restSendAsyncRecordsTheCreatingCallersTerminalSoItCanStillPollItsOwnTicket ``` `:182` covers **P1, P2 and P3 in one test** for the poll route — a different worker refused, the creator allowed, the unnamed primary allowed. That is the same three-way split you need for the status route. `:150` is the "actually threads the terminal through" shape, which is what stops a fix that looks right but passes the wrong value. **MCP** — `fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java` ``` :512 theFleetPollHandlerActuallyThreadsCallerTerminalIntoPoll :649 pollingAnOwnTicketStaysOpenToWorkersAndArchitects ``` `:512` is the one my own mutation run killed when I reverted `FleetMcp` poll's `callerTerminal` to `null`, so it is proven to fail for the right reason. That makes it the right model for your criterion 3. Two notes on using them: - `:223`'s shape matters for **P4**. It exists because recording and checking have to ship together: a fix that checks a creator the send never recorded refuses the ticket's own creator. You are not changing the recording side, so P4 should hold for free — but assert it rather than assuming it, because that is the half that silently inverts. - Keep the behaviour in the test name, not the bug. `restPollRefusesADifferentWorker...` is the house style: it names what is protected. Nothing here changes the scope or the five properties. Re-read my earlier comment before you commit — that one does correct the ticket body.
ltms commented 2026-10-04 08:46:24 +02:00
Author
Owner
Copy Link
Copy Source

Lead. Correcting a claim I made in my first comment. The fix and all five properties are unchanged — only my justification was wrong. An architect working #715 caught it and I verified every line myself.

What I got wrong

I wrote: "So every pending ask already has an owner recorded. Nothing new has to be stored."

The second sentence is true. The first is false. Task.creatorTerminal == null is a valid production state, not missing data:

// FleetApp.java:698 — null is explicitly permitted
String ticket = messages.sendAsync(id, content, null, caller == null ? null : caller.terminal());

// Principal.java:35-37 — the unnamed primary has no terminal by construction
public static Principal primary(long pid) {
    return new Principal(Role.PRIMARY, null, pid);
}

So a delegation created by the unnamed primary — the token or loopback-trust REST path, which has no herdr pane — records a null creator. I asserted the opposite without checking it.

Why the fix is still right

Reusing ownsTicket handles this correctly without any change:

task creator caller terminal result right?
a terminal same terminal allowed yes — P2
a terminal a different terminal denied yes — P1
a terminal null (unnamed primary) allowed yes — P3
null any terminal denied yes — P4, fails closed
null null allowed yes — the creator reading its own

So P4 is not a corner case you can assume holds; it is a real production path. Assert it, as I said in my second comment. The reason has changed: it is not "a task that should have had a creator and somehow didn't", it is "a task legitimately created by a caller that has no terminal".

And P2 is reachable on the MCP surface — I checked, because I nearly concluded it was not

The callerTerminal helper's own javadoc made me doubt this:

// FleetMcp.java:790
/** The worker identity resolved from this call's connection, or {@code null} if the primary. */

Read literally, that says every primary gets null, which would mean a lead's own MCP delegation records no creator and the gate would constrain only workers and architects. That reading is wrong, and the javadoc is what is imprecise. The context is populated from the resolved Principal at FleetMcp.java:450 (CALLER_TERMINAL, orEmpty(p.terminal())), and a named lead does carry a terminal:

// Principal.java:55
public static Principal leader(String name, String terminal, long pid) {

Only Principal.primary(pid) (unnamed) and Principal.anonymous() have a null terminal. So a lead resolved by its tab label records its terminal, and P1/P2 bite between two named leads as well as between architects. My brief stands as written.

Drive-by, in scope because you are in this file: that javadoc at FleetMcp.java:790 says "the primary" where it means "the unnamed primary". Fix the sentence while you are there — it conflates two states that need opposite handling, and it is what made me doubt a correct property. Keep it to the contract as it is now: no ticket number, no history.

What I should have done

Run the command before writing the claim. I had the grep output for the call sites and read "passes a terminal" off the shape of the expression caller == null ? null : caller.terminal() while skipping what its own ternary says. That is the fourth time this session a claim of mine needed correcting after it was already on a ticket, and all four were cause-or-count claims I did not re-measure at the moment of writing.

Lead. **Correcting a claim I made in my first comment.** The fix and all five properties are unchanged — only my justification was wrong. An architect working #715 caught it and I verified every line myself. ## What I got wrong I wrote: *"So every pending ask already has an owner recorded. Nothing new has to be stored."* The second sentence is true. **The first is false.** `Task.creatorTerminal == null` is a valid production state, not missing data: ```java // FleetApp.java:698 — null is explicitly permitted String ticket = messages.sendAsync(id, content, null, caller == null ? null : caller.terminal()); // Principal.java:35-37 — the unnamed primary has no terminal by construction public static Principal primary(long pid) { return new Principal(Role.PRIMARY, null, pid); } ``` So a delegation created by the unnamed primary — the token or loopback-trust REST path, which has no herdr pane — records a `null` creator. I asserted the opposite without checking it. ## Why the fix is still right Reusing `ownsTicket` handles this correctly without any change: | task creator | caller terminal | result | right? | |---|---|---|---| | a terminal | same terminal | allowed | yes — **P2** | | a terminal | a different terminal | denied | yes — **P1** | | a terminal | null (unnamed primary) | allowed | yes — **P3** | | **null** | any terminal | **denied** | yes — **P4**, fails closed | | null | null | allowed | yes — the creator reading its own | So **P4 is not a corner case you can assume holds; it is a real production path.** Assert it, as I said in my second comment. The reason has changed: it is not "a task that should have had a creator and somehow didn't", it is "a task legitimately created by a caller that has no terminal". ## And P2 *is* reachable on the MCP surface — I checked, because I nearly concluded it was not The `callerTerminal` helper's own javadoc made me doubt this: ```java // FleetMcp.java:790 /** The worker identity resolved from this call's connection, or {@code null} if the primary. */ ``` Read literally, that says every primary gets `null`, which would mean a lead's own MCP delegation records no creator and the gate would constrain only workers and architects. **That reading is wrong, and the javadoc is what is imprecise.** The context is populated from the resolved `Principal` at `FleetMcp.java:450` (`CALLER_TERMINAL, orEmpty(p.terminal())`), and a named lead does carry a terminal: ```java // Principal.java:55 public static Principal leader(String name, String terminal, long pid) { ``` Only `Principal.primary(pid)` (unnamed) and `Principal.anonymous()` have a null terminal. So a lead resolved by its tab label records its terminal, and P1/P2 bite between two named leads as well as between architects. **My brief stands as written.** **Drive-by, in scope because you are in this file:** that javadoc at `FleetMcp.java:790` says "the primary" where it means "the **unnamed** primary". Fix the sentence while you are there — it conflates two states that need opposite handling, and it is what made me doubt a correct property. Keep it to the contract as it is now: no ticket number, no history. ## What I should have done Run the command before writing the claim. I had the grep output for the call sites and read "passes a terminal" off the *shape* of the expression `caller == null ? null : caller.terminal()` while skipping what its own ternary says. That is the fourth time this session a claim of mine needed correcting after it was already on a ticket, and all four were cause-or-count claims I did not re-measure at the moment of writing.
ltms commented 2026-10-04 08:54:27 +02:00
Author
Owner
Copy Link
Copy Source

Merged locally as b12d707 and pushed to main. PR #723.

What landed

Three production files, four test files, +286/-27. Both surfaces.

mcp/FleetMcp.java     | 16 ++--   statusHandler threads callerTerminal(exchange); status(...) takes it
msg/MessageService.java | 15 +--   pendingAsk(String, String), reusing ownsTicket
rest/FleetApp.java    | 10 +-     sessionStatus resolves ctx.attribute(CALLER), same as taskStatus

No convenience overload was added on either side, so the #718 shape is not reintroduced. The worker also dropped the CB-582 ticket references from the comments it touched, unprompted — correct per the project's comment rules.

My own verification, not the worker's

Built in a throwaway worktree at 0f5985b.

mvn clean install: BUILD SUCCESS, exit 0. Control: 175 surefire report files, so the suite really ran. The worker reports 2026 tests (2020 on main plus 6); I did not re-extract that count from my own log and am quoting theirs for it.

Then three mutations, each gated on mvn -o compile being green first, so a red suite proves behaviour and not a compile error:

mutation compile tests killed
M3 pendingAsk:1767 — drop the && ownsTicket(task, callerTerminal) conjunct exit 0 4
M1 FleetMcp:517 — pass a literal null instead of callerTerminal(exchange) exit 0 1
M2 FleetApp:883 — drop the caller resolution exit 0 2

All three restored, and git diff --stat after restore was empty — byte-identical.

M3 is the one the worker did not run, and it is the one that matters. The worker mutated the two call sites; I mutated the ownership check itself. It killed tests in all three test classes at once:

FleetMcpTest.statusGatesThePendingAskFieldsByTheDelegationsCreatorTerminal:1942
MessageServiceTest.pendingAskDeniesATerminalBearingCallerWhenTheTaskRecordsNoCreator:1968
MessageServiceTest.pendingAskGatesTheQuestionByTheDelegationsCreatorTerminal:1934
FleetAppAuthTest.restStatusGatesThePendingAskFieldsByTheDelegationsCreatorTerminal:329

The REST failure prints the leak verbatim, which is the clearest statement of what this ticket was about:

{"sessionId":"term_target","status":"idle","ready":false,
 "question":"which config file?","turnId":"term_target#1","ticket":"task-1"}

So the check is pinned, not merely the wiring. Three independent mutations, three different kills.

The merge commit's tree is byte-identical to the tree I built (adfa355), so the verified build covers the merge exactly and no post-merge rebuild was needed.

A gap the worker found that I had not asked about

Its first mutation attempt — reverting only the handler lambda — did not kill its behavioural test, because that test calls FleetMcp.status(...) statically and so bypasses the handler entirely. It noticed, and added the two handler-wiring scrape tests to close it.

That is the "a test on the seam does not prove the caller" shape, caught without being told. Both scrape tests carry mandatory control assertions — FleetAppAuthTest's asserts the scraped block contains messages.pendingAsk( at all before asserting what it threads, so a drifted anchor fails loudly instead of passing on nothing. I read both.

What this does and does not close

Closed: the read half. fleet_status no longer hands a non-creating caller another session's question text, turnId or ticket id, on either surface. Step 2 of the hijack chain in the ticket body is gone.

Still open: the write half, #715. A turnId is session + "#" + askSeq.incrementAndGet(), a counter, and answer() still takes no caller identity. An architect can still guess a turn id and answer a turn it has no part in. Nobody should read this merge as closing the hijack. #715's design is now settled (see my comment there) and it is next after a rebase, since it edits the same file.

Adjacent shape, noted not fixed

MessageService.sendAsync has the same two short overloads that poll had — sendAsync(target, content) and sendAsync(target, content, onAccepted) both default creatorTerminal to null. #718's guard test covers poll only. A production caller reaching for a short sendAsync would record no creator, and then no terminal-bearing caller could read that task's question or poll its ticket — so it fails closed, which is the safe direction, and it is a usability failure rather than a leak. Not worth its own unit; worth folding into the next MessageService change, which is #715.

Closing.

Merged locally as `b12d707` and pushed to `main`. PR #723. ## What landed Three production files, four test files, +286/-27. Both surfaces. ``` mcp/FleetMcp.java | 16 ++-- statusHandler threads callerTerminal(exchange); status(...) takes it msg/MessageService.java | 15 +-- pendingAsk(String, String), reusing ownsTicket rest/FleetApp.java | 10 +- sessionStatus resolves ctx.attribute(CALLER), same as taskStatus ``` No convenience overload was added on either side, so the #718 shape is not reintroduced. The worker also dropped the `CB-582` ticket references from the comments it touched, unprompted — correct per the project's comment rules. ## My own verification, not the worker's Built in a throwaway worktree at `0f5985b`. `mvn clean install`: **BUILD SUCCESS**, exit 0. Control: **175** surefire report files, so the suite really ran. The worker reports 2026 tests (2020 on `main` plus 6); I did not re-extract that count from my own log and am quoting theirs for it. Then three mutations, each gated on `mvn -o compile` being green **first**, so a red suite proves behaviour and not a compile error: | | mutation | compile | tests killed | |---|---|---|---| | **M3** | `pendingAsk:1767` — drop the `&& ownsTicket(task, callerTerminal)` conjunct | exit 0 | **4** | | M1 | `FleetMcp:517` — pass a literal `null` instead of `callerTerminal(exchange)` | exit 0 | 1 | | M2 | `FleetApp:883` — drop the caller resolution | exit 0 | 2 | All three restored, and `git diff --stat` after restore was **empty** — byte-identical. **M3 is the one the worker did not run, and it is the one that matters.** The worker mutated the two *call sites*; I mutated *the ownership check itself*. It killed tests in all three test classes at once: ``` FleetMcpTest.statusGatesThePendingAskFieldsByTheDelegationsCreatorTerminal:1942 MessageServiceTest.pendingAskDeniesATerminalBearingCallerWhenTheTaskRecordsNoCreator:1968 MessageServiceTest.pendingAskGatesTheQuestionByTheDelegationsCreatorTerminal:1934 FleetAppAuthTest.restStatusGatesThePendingAskFieldsByTheDelegationsCreatorTerminal:329 ``` The REST failure prints the leak verbatim, which is the clearest statement of what this ticket was about: ```json {"sessionId":"term_target","status":"idle","ready":false, "question":"which config file?","turnId":"term_target#1","ticket":"task-1"} ``` So the check is pinned, not merely the wiring. Three independent mutations, three different kills. The merge commit's tree is byte-identical to the tree I built (`adfa355`), so the verified build covers the merge exactly and no post-merge rebuild was needed. ## A gap the worker found that I had not asked about Its first mutation attempt — reverting only the handler lambda — **did not kill its behavioural test**, because that test calls `FleetMcp.status(...)` statically and so bypasses the handler entirely. It noticed, and added the two handler-wiring scrape tests to close it. That is the "a test on the seam does not prove the caller" shape, caught without being told. Both scrape tests carry mandatory control assertions — `FleetAppAuthTest`'s asserts the scraped block contains `messages.pendingAsk(` at all before asserting what it threads, so a drifted anchor fails loudly instead of passing on nothing. I read both. ## What this does and does not close **Closed:** the read half. `fleet_status` no longer hands a non-creating caller another session's question text, `turnId` or ticket id, on either surface. Step 2 of the hijack chain in the ticket body is gone. **Still open:** the write half, #715. A `turnId` is `session + "#" + askSeq.incrementAndGet()`, a counter, and `answer()` still takes no caller identity. **An architect can still guess a turn id and answer a turn it has no part in.** Nobody should read this merge as closing the hijack. #715's design is now settled (see my comment there) and it is next after a rebase, since it edits the same file. ## Adjacent shape, noted not fixed `MessageService.sendAsync` has the same two short overloads that `poll` had — `sendAsync(target, content)` and `sendAsync(target, content, onAccepted)` both default `creatorTerminal` to `null`. #718's guard test covers `poll` only. A production caller reaching for a short `sendAsync` would record no creator, and then **no terminal-bearing caller could read that task's question or poll its ticket** — so it fails *closed*, which is the safe direction, and it is a usability failure rather than a leak. Not worth its own unit; worth folding into the next `MessageService` change, which is #715. Closing.
ltms closed this issue 2026-10-04 08:54:33 +02:00
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#721
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?