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

fleetd #703: fleet_list reports a collaborators array #709

Closed
agent wants to merge 0 commits from worker/703-list-collaborators-9c06c2-7 into main
pull from: worker/703-list-collaborators-9c06c2-7
merge into: fleet:main
Conversation 2 Commits - Files Changed -
agent commented 2026-10-04 06:38:31 +02:00
Member
Copy Link
Copy Source

Closes fleetd #703.

A lead could not discover a collaborator's sessionId: fleet_send needs the target's sessionId, and fleet_list reported only leads and members. This threads CallerResolver#collaborators() (terminal_id -> name) into fleet_list's canonical listFleet overload and adds a new collaborators array, each row {name, sessionId}.

Visibility is narrowed by role in the payload builder (new collaboratorsVisibleTo(Principal), same shape as the existing coordinatorVisibleTo): visible to the primary, an architect, and a collaborator -- the same roles Authz.permits grants SEND to a named peer -- never a worker, which holds READ but can never SEND to a collaborator. Authz/Principal/the role table are unchanged.

Gives CallerResolver#collaborators() (auth/CallerResolver.java:269) its first real caller outside the resolver itself.

Tests (all new, all pass):

  • FleetMcpAuthzTest: collaboratorsVisibleTo's role truth table (primary/architect/collaborator true; worker/anonymous false), plus a source-scrape proving the fleet_list handler actually calls collaboratorsVisibleTo(principal(exchange)) and threads callers.collaborators(), not a literal.
  • FleetMcpTest: the collaborators row is keyed by the collaborator's terminal id (acceptance A), the key is absent/empty with none configured, and a worker gets no collaborators data while an architect does (acceptance B, both halves asserted).

Revert-proof (acceptance C, done by hand during review, not left as a lingering flag): removing only the role narrowing turned exactly 1 test red in each of FleetMcpAuthzTest and FleetMcpTest (the role-table test and the worker/architect payload test); removing only the payload wiring turned exactly 3 FleetMcpTest tests red (the row-shape test, its empty-map control, and the architect half of the worker/architect test) while FleetMcpAuthzTest stayed green. Real code restored before this PR; mvn clean install is green on the restored state.

Build: mvn clean install -> BUILD SUCCESS, Tests run: 2004, Failures: 0, Errors: 0, Skipped: 0.

Out of scope, reported not fixed (per the ticket): CallerResolver#members() (the architect-terminal map accessor) has no caller outside auth/CallerResolverTest.java, same no-caller-outside-its-class shape this ticket fixed for collaborators(). Also, fleet_list's members array (each row carries owner, the terminal that spawned it) is gated only by the READ action with no per-role narrowing, so any READ holder including a worker can see which pane owns which member -- a candidate for the same kind of role narrowing this ticket added for collaborators, not chased further.

Closes fleetd #703. A lead could not discover a collaborator's sessionId: fleet_send needs the target's sessionId, and fleet_list reported only leads and members. This threads CallerResolver#collaborators() (terminal_id -> name) into fleet_list's canonical listFleet overload and adds a new collaborators array, each row {name, sessionId}. Visibility is narrowed by role in the payload builder (new collaboratorsVisibleTo(Principal), same shape as the existing coordinatorVisibleTo): visible to the primary, an architect, and a collaborator -- the same roles Authz.permits grants SEND to a named peer -- never a worker, which holds READ but can never SEND to a collaborator. Authz/Principal/the role table are unchanged. Gives CallerResolver#collaborators() (auth/CallerResolver.java:269) its first real caller outside the resolver itself. Tests (all new, all pass): - FleetMcpAuthzTest: collaboratorsVisibleTo's role truth table (primary/architect/collaborator true; worker/anonymous false), plus a source-scrape proving the fleet_list handler actually calls collaboratorsVisibleTo(principal(exchange)) and threads callers.collaborators(), not a literal. - FleetMcpTest: the collaborators row is keyed by the collaborator's terminal id (acceptance A), the key is absent/empty with none configured, and a worker gets no collaborators data while an architect does (acceptance B, both halves asserted). Revert-proof (acceptance C, done by hand during review, not left as a lingering flag): removing only the role narrowing turned exactly 1 test red in each of FleetMcpAuthzTest and FleetMcpTest (the role-table test and the worker/architect payload test); removing only the payload wiring turned exactly 3 FleetMcpTest tests red (the row-shape test, its empty-map control, and the architect half of the worker/architect test) while FleetMcpAuthzTest stayed green. Real code restored before this PR; mvn clean install is green on the restored state. Build: mvn clean install -> BUILD SUCCESS, Tests run: 2004, Failures: 0, Errors: 0, Skipped: 0. Out of scope, reported not fixed (per the ticket): CallerResolver#members() (the architect-terminal map accessor) has no caller outside auth/CallerResolverTest.java, same no-caller-outside-its-class shape this ticket fixed for collaborators(). Also, fleet_list's members array (each row carries owner, the terminal that spawned it) is gated only by the READ action with no per-role narrowing, so any READ holder including a worker can see which pane owns which member -- a candidate for the same kind of role narrowing this ticket added for collaborators, not chased further.
agent added 1 commit 2026-10-04 06:38:32 +02:00
fleetd #703: fleet_list reports a collaborators array so a lead can discover one
CI / shell-tests (pull_request) Failing after 11s
Details
CI / contract (pull_request) Successful in 58s
Details
CI / build (pull_request) Failing after 1m54s
Details
e7a7711a4e 
Threads CallerResolver#collaborators() into fleet_list's canonical listFleet
overload and adds a collaborators array (name, sessionId), visible only to
the primary, an architect, and a collaborator -- the same roles Authz grants
SEND to a named peer -- never a worker. Gives CallerResolver#collaborators()
its first real caller outside the resolver itself.
ltms commented 2026-10-04 06:42:24 +02:00
Owner
Copy Link
Copy Source

Merged locally in 4939d40. Closing by hand, because a local merge never closes a PR here.

Verified by the lead before merging: merged onto origin/main in a throwaway worktree,
mvn clean install with output to a file rather than piped — exit code 0, BUILD SUCCESS,
Tests run: 2005, Failures: 0, Errors: 0, FleetMcpTest: 100, FleetMcpAuthzTest: 28. The
pushed tree is byte-identical to the one I built.

The test counts cross-check, which is worth stating because a mismatch here would have meant a
merge surprise: the report said 2004 from base 3e8e314 (1999 plus 5 new), and I measured 2005
because main had since gained PR #708's one extra test (2000 plus 5). Both arithmetics agree.

I checked the thing that decides whether this is live rather than merely present: the handler
really does pass collaboratorsVisibleTo(principal(exchange)) and not a literal. The revert proofs
are also properly separated, which is the part I would have got wrong — removing the predicate
killed the worker half, removing the payload wiring killed three different tests, and
FleetMcpAuthzTest correctly stayed green in the second case because it only exercises the
predicate. Two mutations killing the same set would have meant one of the two tests was redundant.

One gap, tracked rather than blocking: #710

No test proves the handler consults the predicate. The new test supplies the boolean itself:

listFleetWithCollaborators(h, collaborators,
        FleetMcp.collaboratorsVisibleTo(Principal.worker("term_w", 1)))

That proves the truth table and that listFleet honours the flag. It does not prove the call site
is not a literal. If a later edit passed true there, every test added here stays green and every
worker sees the array. The repo already decided this wiring needs pinning —
theFleetListHandlerActuallyConsultsCoordinatorVisibleTo does exactly that for the coordinator
flag, control assertion included. The code here is correct, so this is a missing future-regression
guard rather than a defect, and it is #710 finding 1.

On the 17-parameter signature

Flagged honestly in the report, along with the reason: the new parameters were inserted before
the trailing ones so the coordinator scrape's trailing-argument regex kept matching. That was the
right call — quietly breaking that guard to tidy a signature would have been the worse trade — and
raising it instead of silently refactoring was right too. A value object is a reasonable follow-up
and it is not this PR's job.

Correctly reported as not run

The canonical-block sync check: unsatisfiable from a member's worktree, where wiki/ is
uninitialized. I ran it in the main clone: in sync.

Two further observations from the report, both checked by me and filed as #710 findings 2 and 3:
roster() has no owner filter so any worker reads every member and its owner, and
CallerResolver#members() has no caller outside its own test.

Merged locally in `4939d40`. Closing by hand, because a local merge never closes a PR here. Verified by the lead before merging: merged onto `origin/main` in a throwaway worktree, `mvn clean install` with output to a file rather than piped — exit code 0, `BUILD SUCCESS`, `Tests run: 2005, Failures: 0, Errors: 0`, `FleetMcpTest: 100`, `FleetMcpAuthzTest: 28`. The pushed tree is byte-identical to the one I built. The test counts cross-check, which is worth stating because a mismatch here would have meant a merge surprise: the report said 2004 from base `3e8e314` (1999 plus 5 new), and I measured 2005 because `main` had since gained PR #708's one extra test (2000 plus 5). Both arithmetics agree. I checked the thing that decides whether this is live rather than merely present: the handler really does pass `collaboratorsVisibleTo(principal(exchange))` and not a literal. The revert proofs are also properly separated, which is the part I would have got wrong — removing the predicate killed the worker half, removing the payload wiring killed three different tests, and `FleetMcpAuthzTest` correctly stayed green in the second case because it only exercises the predicate. Two mutations killing the same set would have meant one of the two tests was redundant. ### One gap, tracked rather than blocking: #710 No test proves the **handler** consults the predicate. The new test supplies the boolean itself: ```java listFleetWithCollaborators(h, collaborators, FleetMcp.collaboratorsVisibleTo(Principal.worker("term_w", 1))) ``` That proves the truth table and that `listFleet` honours the flag. It does not prove the call site is not a literal. If a later edit passed `true` there, every test added here stays green and every worker sees the array. The repo already decided this wiring needs pinning — `theFleetListHandlerActuallyConsultsCoordinatorVisibleTo` does exactly that for the coordinator flag, control assertion included. The code here is correct, so this is a missing future-regression guard rather than a defect, and it is #710 finding 1. ### On the 17-parameter signature Flagged honestly in the report, along with the reason: the new parameters were inserted *before* the trailing ones so the coordinator scrape's trailing-argument regex kept matching. That was the right call — quietly breaking that guard to tidy a signature would have been the worse trade — and raising it instead of silently refactoring was right too. A value object is a reasonable follow-up and it is not this PR's job. ### Correctly reported as not run The canonical-block sync check: unsatisfiable from a member's worktree, where `wiki/` is uninitialized. I ran it in the main clone: in sync. Two further observations from the report, both checked by me and filed as #710 findings 2 and 3: `roster()` has no owner filter so any worker reads every member and its `owner`, and `CallerResolver#members()` has no caller outside its own test.
ltms closed this pull request 2026-10-04 06:42:27 +02:00
ltms commented 2026-10-04 06:45:31 +02:00
Owner
Copy Link
Copy Source

Correction to my review comment above

My comment on this PR said the fleet_list handler's use of collaboratorsVisibleTo was not
pinned by any test, and filed that as finding 1 of #710. That was wrong, and I withdraw it.

This PR does add the guard:
FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCollaboratorsVisibleTo. It scrapes the
listHandler block and asserts the handler passes both callers.collaborators() and
collaboratorsVisibleTo(principal(exchange)), with assertTrue(handlerBlock.contains("listFleet("))
as the control so the test cannot pass by failing to find the block. That is exactly the shape I
said was missing, and it follows the existing coordinatorVisibleTo precedent.

How I got it wrong: I read the production side of the diff with grep and read the test side only
for FleetMcpTest. I never opened the other 52 lines of the diff, which are in
FleetMcpAuthzTest, so I concluded from an absence I had not probed for. My own rule is that a
zero match is not a finding until a positive probe fires — and here the probe was a single
grep -n "listHandler" in that test file, which I did not run.

The merge itself stands: I built the merged tree myself and it was green
(Tests run: 2005, Failures: 0). Nothing in the code changes. #710 is retitled and now carries
findings 2 and 3 only.

## Correction to my review comment above My comment on this PR said the `fleet_list` handler's use of `collaboratorsVisibleTo` was not pinned by any test, and filed that as finding 1 of #710. **That was wrong, and I withdraw it.** This PR does add the guard: `FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCollaboratorsVisibleTo`. It scrapes the `listHandler` block and asserts the handler passes both `callers.collaborators()` and `collaboratorsVisibleTo(principal(exchange))`, with `assertTrue(handlerBlock.contains("listFleet("))` as the control so the test cannot pass by failing to find the block. That is exactly the shape I said was missing, and it follows the existing `coordinatorVisibleTo` precedent. How I got it wrong: I read the production side of the diff with `grep` and read the test side only for `FleetMcpTest`. I never opened the other 52 lines of the diff, which are in `FleetMcpAuthzTest`, so I concluded from an absence I had not probed for. My own rule is that a zero match is not a finding until a positive probe fires — and here the probe was a single `grep -n "listHandler"` in that test file, which I did not run. The merge itself stands: I built the merged tree myself and it was green (`Tests run: 2005, Failures: 0`). Nothing in the code changes. #710 is retitled and now carries findings 2 and 3 only.
Some checks are pending
CI / shell-tests (pull_request) Failing after 11s
Details
CI / contract (pull_request) Successful in 58s
Details
CI / build (pull_request) Failing after 1m54s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#709
Delete Branch "worker/703-list-collaborators-9c06c2-7"

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?