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

fleet_list's visibility flags: 4 of 5 call sites are pinned only by source-text tests, and the 5th is not pinned at all #755

New Issue
Open
opened 2026-10-05 09:10:09 +02:00 by ltms · 1 comment
ltms commented 2026-10-05 09:10:09 +02:00
Owner
Copy Link
Copy Source

Found while verifying #753 (fleetd #743). Measured on 291dc02 + that branch merged, in a throwaway worktree.

The measurement

fleet_list's handler passes five visibility flags (FleetMcp.java:595-599):

Flag Call site pinned?
collaboratorsVisibleTo yes — source-text test
coordinatorVisibleTo yes — source-text test
leadsVisibleTo yes — source-text test
membersVisibleTo yes — source-text test
panesVisibleTo (new) no

I replaced panesVisibleTo(principal(exchange)) with the literal true in the handler — so every caller, including a plain worker and an anonymous one, would receive every pane's tab label and cwd. The mutation compiled and all 2122 tests passed:

MVN_EXIT=0
xml=177 tests=2122 failures=0 errors=0 skipped=0

Positive control, so this is not a broken probe: the same mutation applied to a sibling flag (leadsVisibleTo → true) killed —

[ERROR] FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsLeadsVisibleTo:500
  the fleet_list handler must ask leadsVisibleTo(principal(exchange)) who is calling,
  not pass a literal boolean

Both mutations were reverted byte-identical (git diff --stat empty).

So the predicate and the assembly are each pinned — panesVisibleTo has 6 assertions in both directions, and listOmitsThePanesArrayWhenTheCallerMayNotSeeIt pins that listFleet honours the boolean. The wiring between them is not.

Why this is a ticket and not a review comment

The established mechanism is a source-text test: read FleetMcp.java as a string and assert the handler block contains leadsVisibleTo(principal(exchange)). There are four of them.

CLAUDE.md §"Code quality" rule 4 says "no new source-text test may be added" — shipped in 291dc02, before this PR landed. So the one pattern that would close the gap is now banned, and asking an implementer to close it forces a choice between violating the policy and inventing a seam under time pressure. That is a design decision, so it goes here.

It is also worth saying plainly that the four existing pins are weak. They assert on text, so they break on a rename or a reformat and they prove nothing about behaviour — a handler could call the right predicate and still drop the result on the floor.

The proposal

One end-to-end test in auth.mode: token, pinning all five flags at once by behaviour. Token mode resolves a principal from a Bearer header with no pid lookup, so a test can drive the real fleet_list handler as a worker, an architect, a collaborator and an observer in turn, and assert which keys each one receives. That is injection, not a production reshape, so rule 4's own prescription ("make the part injectable instead") is satisfied.

If that lands, the four source-text tests can be deleted, which takes the count of banned-pattern tests in rule 4 from 8 down to 4.

Why the gap is low-risk today, and when that stops being true

Reaching it needs someone to edit that one line to a literal. Nothing does today. But the reason to fix it is that this is the first of the five flags with no pin at all, so the bar has dropped, and the next flag added will copy the newest neighbour.

Related: #743 (the change that added the flag), #753 (the PR).

Found while verifying #753 (fleetd #743). Measured on `291dc02` + that branch merged, in a throwaway worktree. ## The measurement `fleet_list`'s handler passes **five** visibility flags (`FleetMcp.java:595-599`): | Flag | Call site pinned? | |---|---| | `collaboratorsVisibleTo` | yes — source-text test | | `coordinatorVisibleTo` | yes — source-text test | | `leadsVisibleTo` | yes — source-text test | | `membersVisibleTo` | yes — source-text test | | `panesVisibleTo` (new) | **no** | I replaced `panesVisibleTo(principal(exchange))` with the literal `true` in the handler — so every caller, including a plain worker and an anonymous one, would receive every pane's tab label and `cwd`. The mutation **compiled and all 2122 tests passed**: ``` MVN_EXIT=0 xml=177 tests=2122 failures=0 errors=0 skipped=0 ``` **Positive control**, so this is not a broken probe: the same mutation applied to a *sibling* flag (`leadsVisibleTo` → `true`) **killed** — ``` [ERROR] FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsLeadsVisibleTo:500 the fleet_list handler must ask leadsVisibleTo(principal(exchange)) who is calling, not pass a literal boolean ``` Both mutations were reverted byte-identical (`git diff --stat` empty). So the predicate and the assembly are each pinned — `panesVisibleTo` has 6 assertions in both directions, and `listOmitsThePanesArrayWhenTheCallerMayNotSeeIt` pins that `listFleet` honours the boolean. **The wiring between them is not.** ## Why this is a ticket and not a review comment The established mechanism is a **source-text test**: read `FleetMcp.java` as a string and assert the handler block contains `leadsVisibleTo(principal(exchange))`. There are four of them. `CLAUDE.md` §"Code quality" rule 4 says **"no new source-text test may be added"** — shipped in `291dc02`, before this PR landed. So the one pattern that would close the gap is now banned, and asking an implementer to close it forces a choice between violating the policy and inventing a seam under time pressure. That is a design decision, so it goes here. It is also worth saying plainly that the four existing pins are weak. They assert on *text*, so they break on a rename or a reformat and they prove nothing about behaviour — a handler could call the right predicate and still drop the result on the floor. ## The proposal **One end-to-end test in `auth.mode: token`, pinning all five flags at once by behaviour.** Token mode resolves a principal from a `Bearer` header with no pid lookup, so a test can drive the real `fleet_list` handler as a worker, an architect, a collaborator and an observer in turn, and assert which keys each one receives. That is injection, not a production reshape, so rule 4's own prescription ("make the part injectable instead") is satisfied. If that lands, the four source-text tests can be deleted, which takes the count of banned-pattern tests in rule 4 from 8 down to 4. ## Why the gap is low-risk today, and when that stops being true Reaching it needs someone to edit that one line to a literal. Nothing does today. But the reason to fix it is that **this is the first of the five flags with no pin at all**, so the bar has dropped, and the next flag added will copy the newest neighbour. Related: #743 (the change that added the flag), #753 (the PR).
ltms commented 2026-10-05 09:14:08 +02:00
Author
Owner
Copy Link
Copy Source

There is now a worked example of the proposed fix in the repo

PR #754 (the observer-SEND half of #743) contains FleetMcpObserverSendDeliveryTest, written independently of this ticket. It does exactly what the proposal above asks for, for the send path instead of the list path:

  • boots a real Jetty server and a real MCP client
  • resolves the caller as OBSERVER through the real CallerResolver (a genuine herdr pane, pid 9001 → term_shell, recognised as nothing else)
  • calls fleet_send with wait:false, drives rendezvous.isWaiting() then injector.onStatus(target, IDLE)
  • asserts the delivered agent.prompt text equals exactly "[fleet_send from observer term_shell]\nhi there"

So the seam needed for this ticket already exists and is proven. A test can drive a real handler as a chosen role without a production reshape. That removes the main open question in the proposal — whether token mode was even necessary. It may not be: #754 gets a specific role out of the real resolver by controlling the herdr pane fixture, which is simpler than a Bearer header.

And it is strictly better than the scrape it sat next to

The same PR also added a source-text scrape, theSendHandlerActuallyAttributesAnObserversContent, pinning the same call site. I mutated line 474 of FleetMcp.java to drop the attribution call and ran both:

[ERROR] FleetMcpAuthzTest.theSendHandlerActuallyAttributesAnObserversContent  -- FAILURE
[ERROR] FleetMcpObserverSendDeliveryTest.anObserversSendIsAttributedAndReachesTheRealInjector -- FAILURE
Tests run: 38, Failures: 2

Both catch it, so the scrape adds nothing. I have asked for the scrape to be removed before merge (rule 4), which takes FleetMcpAuthzTest's reads of MCP_SOURCE back from 7 to main's 6.

What this changes about the plan here

Do the same thing for fleet_list: one end-to-end test driving the real handler as a worker, an architect, a collaborator and an observer in turn, asserting which of the five arrays each receives. Model it on FleetMcpObserverSendDeliveryTest rather than designing it fresh. If that lands, the four theFleetListHandlerActuallyConsults*VisibleTo scrapes can go, and MCP_SOURCE reads drop from 6 to 2.

## There is now a worked example of the proposed fix in the repo PR #754 (the observer-`SEND` half of #743) contains `FleetMcpObserverSendDeliveryTest`, written independently of this ticket. It does exactly what the proposal above asks for, for the *send* path instead of the list path: - boots a real Jetty server and a real MCP client - resolves the caller as `OBSERVER` through the **real** `CallerResolver` (a genuine herdr pane, pid 9001 → `term_shell`, recognised as nothing else) - calls `fleet_send` with `wait:false`, drives `rendezvous.isWaiting()` then `injector.onStatus(target, IDLE)` - asserts the delivered `agent.prompt` text equals exactly `"[fleet_send from observer term_shell]\nhi there"` **So the seam needed for this ticket already exists and is proven.** A test can drive a real handler as a chosen role without a production reshape. That removes the main open question in the proposal — whether token mode was even necessary. It may not be: #754 gets a specific role out of the real resolver by controlling the herdr pane fixture, which is simpler than a Bearer header. ### And it is strictly better than the scrape it sat next to The same PR also added a source-text scrape, `theSendHandlerActuallyAttributesAnObserversContent`, pinning the same call site. I mutated line 474 of `FleetMcp.java` to drop the attribution call and ran both: ``` [ERROR] FleetMcpAuthzTest.theSendHandlerActuallyAttributesAnObserversContent -- FAILURE [ERROR] FleetMcpObserverSendDeliveryTest.anObserversSendIsAttributedAndReachesTheRealInjector -- FAILURE Tests run: 38, Failures: 2 ``` Both catch it, so the scrape adds nothing. I have asked for the scrape to be removed before merge (rule 4), which takes `FleetMcpAuthzTest`'s reads of `MCP_SOURCE` back from 7 to `main`'s 6. ### What this changes about the plan here Do the same thing for `fleet_list`: one end-to-end test driving the real handler as a worker, an architect, a collaborator and an observer in turn, asserting which of the five arrays each receives. Model it on `FleetMcpObserverSendDeliveryTest` rather than designing it fresh. If that lands, the four `theFleetListHandlerActuallyConsults*VisibleTo` scrapes can go, and `MCP_SOURCE` reads drop from 6 to 2.
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#755
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?