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

fleet_list's members rows hand every worker another lead's roster and its owner; CallerResolver#members() has no caller #710

New Issue
Closed
opened 2026-10-04 06:41:13 +02:00 by ltms · 3 comments
ltms commented 2026-10-04 06:41:13 +02:00
Owner
Copy Link
Copy Source

Two findings from reviewing PR #709 (#703), both reported by the implementer as out-of-scope
observations and both checked by me afterwards.

A third finding in the original version of this ticket was wrong and is withdrawn. I claimed
nothing pinned the fleet_list handler to collaboratorsVisibleTo. PR #709 added exactly that
guard — FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCollaboratorsVisibleTo, control
assertion included. I had read only half of the test diff. See the first comment below for how
that happened; it is the more useful part of this ticket.

1. members rows expose every member on the daemon, and its owner, to any READ holder

SessionManager.roster() is List.copyOf(registry.values()) with no owner filter, and
FleetMcp.java:1360-1361 adds owner to every row unconditionally:

if (s.ownerTerminal() != null) {
    m.put("owner", s.ownerTerminal());
}

fleet_list is gated on READ, which a worker holds. So a worker can read every member on the
daemon — including members belonging to a different lead — and which pane owns each one. This
is the same shape #703 narrowed for collaborators: per-person data whose visibility is decided by
the action alone.

The exposure is small on a single-lead host, because a worker can already read the leads rows and
their session ids, so owner mostly repeats what it can see. It stops being small as soon as two
leads share a daemon, which is a direction the project is heading: a worker could then map another
lead's whole fleet and who owns it.

Not urgent, and I have not decided the fix. The two candidate shapes are to narrow owner by role
the way coordinator and collaborators are narrowed, or to filter the members array to the
caller's own owner when the caller is a member. Those differ in behaviour, not just in size, so it
wants a decision rather than a patch.

2. CallerResolver#members() has no caller outside its own test

The architect-terminal terminal_id → slot name map. Its only use outside the resolver is
CallerResolverTest.java:463. This is the same "accessor with no caller" shape that #703 found on
collaborators() — and there, the missing caller was the symptom of a missing feature, because
knownLeadOrCollaborator() read the field directly instead. Worth checking whether the same is
true here, or whether it is simply dead and should go.

Not measured

Nobody has probed either finding from a live worker session. Both are read from the source:
finding 1 from roster() and the payload builder, finding 2 from a grep for callers.

Two findings from reviewing PR #709 (#703), both reported by the implementer as out-of-scope observations and both checked by me afterwards. > **A third finding in the original version of this ticket was wrong and is withdrawn.** I claimed > nothing pinned the `fleet_list` handler to `collaboratorsVisibleTo`. PR #709 added exactly that > guard — `FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCollaboratorsVisibleTo`, control > assertion included. I had read only half of the test diff. See the first comment below for how > that happened; it is the more useful part of this ticket. ## 1. `members` rows expose every member on the daemon, and its owner, to any `READ` holder `SessionManager.roster()` is `List.copyOf(registry.values())` with no owner filter, and `FleetMcp.java:1360-1361` adds `owner` to every row unconditionally: ```java if (s.ownerTerminal() != null) { m.put("owner", s.ownerTerminal()); } ``` `fleet_list` is gated on `READ`, which a worker holds. So a worker can read every member on the daemon — including members belonging to a **different lead** — and which pane owns each one. This is the same shape #703 narrowed for `collaborators`: per-person data whose visibility is decided by the action alone. The exposure is small on a single-lead host, because a worker can already read the `leads` rows and their session ids, so `owner` mostly repeats what it can see. It stops being small as soon as two leads share a daemon, which is a direction the project is heading: a worker could then map another lead's whole fleet and who owns it. Not urgent, and I have not decided the fix. The two candidate shapes are to narrow `owner` by role the way `coordinator` and `collaborators` are narrowed, or to filter the `members` array to the caller's own owner when the caller is a member. Those differ in behaviour, not just in size, so it wants a decision rather than a patch. ## 2. `CallerResolver#members()` has no caller outside its own test The architect-terminal `terminal_id → slot name` map. Its only use outside the resolver is `CallerResolverTest.java:463`. This is the same "accessor with no caller" shape that #703 found on `collaborators()` — and there, the missing caller was the *symptom* of a missing feature, because `knownLeadOrCollaborator()` read the field directly instead. Worth checking whether the same is true here, or whether it is simply dead and should go. ## Not measured Nobody has probed either finding from a live worker session. Both are read from the source: finding 1 from `roster()` and the payload builder, finding 2 from a `grep` for callers.
ltms commented 2026-10-04 06:43:05 +02:00
Author
Owner
Copy Link
Copy Source

Finding 1 is wrong. Withdrawing it — the guard exists.

FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCollaboratorsVisibleTo was added in PR #709
and does exactly what I said was missing. It scrapes the listHandler block and asserts both:

assertTrue(handlerBlock.contains("callers.collaborators()"), …);
assertTrue(handlerBlock.contains("collaboratorsVisibleTo(principal(exchange))"), …);

It carries the same control assertion as its coordinator twin — that the scraped block contains a
listFleet( call at all, so a drifted anchor fails loudly instead of passing on nothing. Its
javadoc even states the reason in the terms I used against it: "the predicate above can be
perfectly correct while the one production call site never asks it."

So changing the call site to a literal true would turn a test red. There is no gap.

How I got it wrong

I read the production diff with a grep, and the test diff only for FleetMcpTest. I saw the new
FleetMcpTest test supply the boolean itself, correctly concluded that that test does not prove
the wiring, and then concluded nothing else did either — without ever looking at the
FleetMcpAuthzTest half, which was the other 52 lines of the same diff.

That is reasoning from an absence I never probed for. My own standing rule is that a zero match is
not a finding until a positive probe fires: here the probe was one grep -n "listHandler" in the
test file, and I did not run it. I wrote a ticket and a PR comment on the strength of a partial
read.

The thing that caught it was the PR body claiming a source-scrape that the member's own reply had
not itemised. I nearly dismissed that as the description overstating the work. The disagreement
between two accounts of the same change was the signal, and the right response to it is to read the
code, not to pick the account that matches what I already believed.

What this says about the review, not just about me

The member built the guard and then under-reported it: its fleet_reply listed only the role
truth-table test under the acceptance criterion, while the PR body listed both. The report was
honest, just incomplete in the direction that cost it credit. Worth noting for briefs — asking for
"each criterion with its result" got me a summary per criterion rather than a list of what was
actually added.

The ticket now covers findings 2 and 3 only

Both still stand, and I verified both myself:

  • SessionManager.roster() is List.copyOf(registry.values()) with no owner filter, and
    FleetMcp.java:1360-1361 adds owner to every member row unconditionally, so any READ holder
    — including a worker — reads every member on the daemon and which pane owns it.
  • CallerResolver#members() has no caller outside CallerResolverTest.

I have retitled the ticket accordingly.

## Finding 1 is wrong. Withdrawing it — the guard exists. `FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCollaboratorsVisibleTo` was added in PR #709 and does exactly what I said was missing. It scrapes the `listHandler` block and asserts both: ```java assertTrue(handlerBlock.contains("callers.collaborators()"), …); assertTrue(handlerBlock.contains("collaboratorsVisibleTo(principal(exchange))"), …); ``` It carries the same control assertion as its coordinator twin — that the scraped block contains a `listFleet(` call at all, so a drifted anchor fails loudly instead of passing on nothing. Its javadoc even states the reason in the terms I used against it: "the predicate above can be perfectly correct while the one production call site never asks it." So changing the call site to a literal `true` **would** turn a test red. There is no gap. ### How I got it wrong I read the production diff with a `grep`, and the test diff only for `FleetMcpTest`. I saw the new `FleetMcpTest` test supply the boolean itself, correctly concluded that *that* test does not prove the wiring, and then concluded nothing else did either — without ever looking at the `FleetMcpAuthzTest` half, which was the other 52 lines of the same diff. That is reasoning from an absence I never probed for. My own standing rule is that a zero match is not a finding until a positive probe fires: here the probe was one `grep -n "listHandler" ` in the test file, and I did not run it. I wrote a ticket and a PR comment on the strength of a partial read. The thing that caught it was the PR body claiming a source-scrape that the member's own reply had not itemised. I nearly dismissed that as the description overstating the work. The disagreement between two accounts of the same change was the signal, and the right response to it is to read the code, not to pick the account that matches what I already believed. ### What this says about the review, not just about me The member built the guard and then under-reported it: its `fleet_reply` listed only the role truth-table test under the acceptance criterion, while the PR body listed both. The report was honest, just incomplete in the direction that cost it credit. Worth noting for briefs — asking for "each criterion with its result" got me a summary per criterion rather than a list of what was actually added. ### The ticket now covers findings 2 and 3 only Both still stand, and I verified both myself: - `SessionManager.roster()` is `List.copyOf(registry.values())` with no owner filter, and `FleetMcp.java:1360-1361` adds `owner` to every member row unconditionally, so any `READ` holder — including a worker — reads every member on the daemon and which pane owns it. - `CallerResolver#members()` has no caller outside `CallerResolverTest`. I have retitled the ticket accordingly.
ltms changed title from fleet_list: the collaborators wiring is unpinned, and members rows still hand every worker another lead's roster to fleet_list's members rows hand every worker another lead's roster and its owner; CallerResolver#members() has no caller 2026-10-04 06:43:19 +02:00
ltms commented 2026-10-04 07:02:07 +02:00
Author
Owner
Copy Link
Copy Source

Decision on finding 1, settled by an architect

I was blocked on which shape to use, so I put it to an architect rather than to the operator. The
decision below is theirs; I have checked the load-bearing parts myself and I am acting on it.

Chosen: (d) — omit the whole members key for a worker, and omit leads too. Not an empty
array: the key is absent, so a caller cannot read "not allowed" as "no sessions exist". The primary
and architect views do not change.

Why not the other two

(b) narrow owner only — too weak. With owner gone a member row still carries sessionId,
paneId, profile, role, state, failure detail, worktree, branch, agentSessionId, charter facts
and live status.

(c) filter by owner — wrong meaning of ownership. ownerTerminal records who spawned the
member, not who owns the current turn; the current delegator lives in the separate
PrimaryRegistry. And an architect cannot spawn (Authz.java:105,
case SPAWN, STOP, DRAIN, HANDOVER -> caller.isPrimary()), so every member it delegates to still
records the primary as ownerTerminal. An owner filter would hide those members from the architect
that is working with them. I verified the Authz line myself.

The rule, which is the part I actually wanted

READ is permission to enter an observation tool. It is not permission to receive every field
the tool can build.

For a worker, a fleet_list key is hidden by default. Show it only when both hold: the
member turn contract names a worker-owned flow that needs the data, and the data is about
that worker itself or is daemon-wide with no other person, pane, session, path or message detail
in it.

Facts about the caller come from fleet_whoami, which already returns a worker's own session,
profile, state, worktree, branch and owner. Applying the rule leaves members, leads,
collaborators and coordinator all absent for a worker; the last two already behave that way.
An architect keeps members because it may SEND to a member; a worker may not SEND at all.

Evidence that no worker flow needs fleet_list

The architect searched for one and found none: no fleet_list in any .claude/agents/*.md; not
named in the member turn contract in CLAUDE.md; in .claude/**/*.md it appears only in
fleets-status, handover and redeploy-fleetd, all marked primary-side. The Run fleet_list
nudge in ReplyPushLoop goes to the delegating lead's pane, not to a worker.

One correction I am adding to the implementation criteria

The architect said to "reverse the worker expectations" at FleetMcpTest.java:771-797. I read that
test and it is not a policy pin. Its two lines

assertTrue(out.contains("\"leads\""), "the rest of the result must still be present: " + out);
assertTrue(out.contains("\"members\""), out);

are the controls for #439's assertFalse(out.contains("\"coordinator\"")). They exist so that
test cannot pass because the payload came back empty. Deleting them would make a security
assertion blind.

So: keep a control, move it. healthCoverage and loopHealth are put unconditionally and stay
visible to a worker, so anchor the control on one of those.

Still not measured

Nobody has called fleet_list from a live worker session, before or after. The whole decision
rests on the source path and unit tests. The architect said so plainly and I am repeating it here.

Also found, not designed

GET /members on REST has the same coarse READ gate and the same unfiltered roster
(FleetApp.java:79-80,455-478). That is the same REST-door pattern I measured on #705's ticket
gate, so it is not a one-off.

## Decision on finding 1, settled by an architect I was blocked on which shape to use, so I put it to an architect rather than to the operator. The decision below is theirs; I have checked the load-bearing parts myself and I am acting on it. **Chosen: (d) — omit the whole `members` key for a worker, and omit `leads` too.** Not an empty array: the key is absent, so a caller cannot read "not allowed" as "no sessions exist". The primary and architect views do not change. ### Why not the other two **(b) narrow `owner` only — too weak.** With `owner` gone a member row still carries `sessionId`, `paneId`, profile, role, state, failure detail, worktree, branch, `agentSessionId`, charter facts and live status. **(c) filter by owner — wrong meaning of ownership.** `ownerTerminal` records who *spawned* the member, not who owns the current turn; the current delegator lives in the separate `PrimaryRegistry`. And an architect **cannot spawn** (`Authz.java:105`, `case SPAWN, STOP, DRAIN, HANDOVER -> caller.isPrimary()`), so every member it delegates to still records the primary as `ownerTerminal`. An owner filter would hide those members from the architect that is working with them. I verified the `Authz` line myself. ### The rule, which is the part I actually wanted > `READ` is permission to enter an observation tool. It is not permission to receive every field > the tool can build. > > For a worker, a `fleet_list` key is hidden by default. Show it only when **both** hold: the > member turn contract names a worker-owned flow that needs the data, **and** the data is about > that worker itself or is daemon-wide with no other person, pane, session, path or message detail > in it. Facts about the caller come from `fleet_whoami`, which already returns a worker's own session, profile, state, worktree, branch and owner. Applying the rule leaves `members`, `leads`, `collaborators` and `coordinator` all absent for a worker; the last two already behave that way. An architect keeps `members` because it may `SEND` to a member; a worker may not `SEND` at all. ### Evidence that no worker flow needs `fleet_list` The architect searched for one and found none: no `fleet_list` in any `.claude/agents/*.md`; not named in the member turn contract in `CLAUDE.md`; in `.claude/**/*.md` it appears only in `fleets-status`, `handover` and `redeploy-fleetd`, all marked primary-side. The `Run fleet_list` nudge in `ReplyPushLoop` goes to the delegating lead's pane, not to a worker. ### One correction I am adding to the implementation criteria The architect said to "reverse the worker expectations" at `FleetMcpTest.java:771-797`. I read that test and it is not a policy pin. Its two lines ```java assertTrue(out.contains("\"leads\""), "the rest of the result must still be present: " + out); assertTrue(out.contains("\"members\""), out); ``` are the **controls** for #439's `assertFalse(out.contains("\"coordinator\""))`. They exist so that test cannot pass because the payload came back empty. Deleting them would make a security assertion blind. So: **keep a control, move it.** `healthCoverage` and `loopHealth` are put unconditionally and stay visible to a worker, so anchor the control on one of those. ### Still not measured Nobody has called `fleet_list` from a live worker session, before or after. The whole decision rests on the source path and unit tests. The architect said so plainly and I am repeating it here. ### Also found, not designed `GET /members` on REST has the same coarse `READ` gate and the same unfiltered roster (`FleetApp.java:79-80,455-478`). That is the same REST-door pattern I measured on #705's ticket gate, so it is not a one-off.
ltms commented 2026-10-04 07:50:38 +02:00
Author
Owner
Copy Link
Copy Source

Both findings are fixed. Closing.

Finding 1 — PR #717, merged as f4e0ca4. The chosen fix is wider than either candidate shape in
the ticket text: instead of narrowing owner or filtering members to the caller's own rows, the
whole array is withheld from a worker, and the key is absent rather than present-and-empty. That
subsumes both candidates, because a worker now gets no row at all rather than a trimmed one.

One thing changed during review that the ticket text does not predict. The first implementation
hid leads and members from a collaborator as well as a worker. That breaks a shipped grant: a
collaborator may only fleet_send to a lead, and leads is the one place the bridge gives it that
address. The final split follows the rule wiki/11-Features.md already states for the sibling
collaborators array — you may list what you could address:

array visible to
leads the primary, an architect, a collaborator
members the primary, an architect

A worker gets neither, which is what this finding asked for.

Finding 2 — PR #713, merged as 25d53e6. The accessor was dead and is gone.

The "not measured" caveat still stands

Neither finding was ever probed from a live worker session, and that is still true. Everything is
pinned by unit tests: truth-table tests on the two predicates, source-scrape tests proving the one
production handler asks them, and a behavioural test asserting no row fragment reaches a worker's
output. I verified the pair holds by mutation — dropping the collaborator clause from
leadsVisibleTo compiled green and then killed exactly two tests, one of each kind.

wiki/11-Features.md is updated. Its fleet_list entry had three separate errors, one of which this
change created, and it now carries the visibility table plus a gotcha saying what an absent key
means: "you may not see this", not "there are none".

## Both findings are fixed. Closing. **Finding 1** — PR #717, merged as `f4e0ca4`. The chosen fix is wider than either candidate shape in the ticket text: instead of narrowing `owner` or filtering `members` to the caller's own rows, the whole array is withheld from a worker, and the key is **absent** rather than present-and-empty. That subsumes both candidates, because a worker now gets no row at all rather than a trimmed one. One thing changed during review that the ticket text does not predict. The first implementation hid `leads` and `members` from a collaborator as well as a worker. That breaks a shipped grant: a collaborator may only `fleet_send` to a lead, and `leads` is the one place the bridge gives it that address. The final split follows the rule `wiki/11-Features.md` already states for the sibling `collaborators` array — you may list what you could address: | array | visible to | |---|---| | `leads` | the primary, an architect, a collaborator | | `members` | the primary, an architect | A worker gets neither, which is what this finding asked for. **Finding 2** — PR #713, merged as `25d53e6`. The accessor was dead and is gone. ### The "not measured" caveat still stands Neither finding was ever probed from a live worker session, and that is still true. Everything is pinned by unit tests: truth-table tests on the two predicates, source-scrape tests proving the one production handler asks them, and a behavioural test asserting no row fragment reaches a worker's output. I verified the pair holds by mutation — dropping the collaborator clause from `leadsVisibleTo` compiled green and then killed exactly two tests, one of each kind. `wiki/11-Features.md` is updated. Its `fleet_list` entry had three separate errors, one of which this change created, and it now carries the visibility table plus a gotcha saying what an absent key means: "you may not see this", not "there are none".
ltms closed this issue 2026-10-04 07:50:42 +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#710
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?