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

fleetd #439: omit fleet_list's coordinator key for non-primary callers #462

Closed
agent wants to merge 0 commits from worker/439-coordinator-row-gate-bc032a-8 into main
pull from: worker/439-coordinator-row-gate-bc032a-8
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-09-10 13:17:36 +02:00
Member
Copy Link
Copy Source

Closes fleetd #439.

What changed

fleet_list's coordinator row (this daemon's coord-id, mailbox state, held lead-to-lead message previews, peer reachability) was returned to EVERY caller, including a worker or an architect. Lead-to-lead coordination is orchestrator-to-orchestrator traffic; a worker has no business reading it.

Option taken: gate at the call site inside listFleet, not inside coordinatorView's signature. coordinatorView(CoordinationSource) is unchanged. A new listFleet overload takes a trailing boolean callerIsPrimary and only calls coordinatorView(...) (and only puts the coordinator key into the result) when that flag is true -- so a non-primary caller gets the key fully ABSENT, never an empty or redacted object, and never pays the cost of coordinatorView probing peer mailboxes for a row it will not receive.

Why this option over threading a Principal all the way through: listFleet has 5 delegating overloads and ~30 existing unit-test call sites that construct it directly with no caller identity at all (they test the assembly logic, not the MCP dispatch). Adding the boolean only to the deepest/full overload, and keeping the old signature as a compat wrapper that passes true, means every one of those existing call sites is untouched and keeps its pre-#439 behavior (which was already correct for them, since none of them simulate a worker). Only the one real production call site -- the fleet_list MCP handler -- now computes and passes the real answer: principal(exchange).isPrimary().

heldView (the truncated-preview render used inside the held[] array) is reachable ONLY through coordinatorView -- grepped, no other caller -- so gating coordinatorView's call already blocks it for a non-primary caller. Nothing else needed there.

Authz's READ case (FleetMcp.java:80) is untouched, as instructed -- it stays shared by fleet_status, fleet_profiles and fleet_whoami. The gate is entirely inside fleet_list's own result assembly, reusing the intent behind the already-correct COORD_READ case without touching the table.

Scope: MCP surface only. I checked the REST side myself (grep -rn coordinatorView\|heldView src/main/java outside FleetMcp.java returns nothing) -- no REST handler reaches either method, so there was nothing to fix there.

Tests (FleetMcpTest)

Three new tests, all in dev.ltms.fleet.mcp.FleetMcpTest:

  • listOmitsTheCoordinatorKeyEntirelyForAWorkerEvenWhenLeadCoordinationIsOn -- drives Principal.worker(...).isPrimary() (the real production boolean) through listFleet with lead coordination fully configured; asserts the "coordinator" key AND any fragment of its content ("mac-opus") are absent, while leads/members/healthCoverage are still present.
  • listOmitsTheCoordinatorKeyEntirelyForAnArchitectToo -- same, but with Principal.architect("lead-designer", ...).isPrimary(). This is a real executed test, not just reasoning by analogy to the worker case.
  • listIsByteForByteUnchangedForThePrimaryCaller -- compares the new gated overload called with callerIsPrimary=true against the pre-#439 overload (which always assembled the row) on an IDENTICAL setup; asserts the two JSON strings are assertEquals (not just both non-empty), then separately asserts every field the ticket names (selfId, mailbox, heldCount, heldDurable, held, peers) is present.

Break-and-restore proof (acceptance criterion 4)

I replaced the gate if (callerIsPrimary) with if (true) // TEMP fleetd #439 break-test: gate disabled on purpose and reran just the three new tests:

[ERROR] Tests run: 3, Failures: 2, Errors: 0, Skipped: 0
[ERROR] FleetMcpTest.listOmitsTheCoordinatorKeyEntirelyForAWorkerEvenWhenLeadCoordinationIsOn:701
  a worker must never see the coordinator key at all: {...,"coordinator":{"selfId":"mac-opus",...}} ==> expected: <false> but was: <true>
[ERROR] FleetMcpTest.listOmitsTheCoordinatorKeyEntirelyForAnArchitectToo:729
  an architect must never see the coordinator key either: {...,"coordinator":{...}} ==> expected: <false> but was: <true>

(the byte-for-byte primary test still passed, as expected -- disabling the gate does not change the primary's output).

Restored the gate and reran the same three:

Tests run: 3, Failures: 0, Errors: 0, Skipped: 0

Confirmed the restore left no stray code (grep -n "TEMP\|if (true)" FleetMcp.java -> no matches).

Full build

mvn -B clean test in fleetd/, real (unpiped) output, redirected to a file and checked via $?:

exit=0
...
[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.497 s -- in dev.ltms.fleet.mcp.FleetMcpTest
...
[INFO] Results:
[INFO] Tests run: 1581, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

Same-shape survey (reported, not fixed -- out of scope per the brief)

Other private/static ...View(...) result-section builders in FleetMcp.java that take only a source object with no caller argument (the same structural shape as the bug, whether or not they are actually a problem -- I did not judge each one, just enumerated):

  • heldMailView(LeadMessage) (~line 950) -- used only by pollHeldPeerMail, which is already dispatch-gated to Authz.Action.COORD_READ (primary-only) BEFORE the function ever runs, so this one is not reachable by a non-primary caller. Checked, not a live instance of the bug.
  • profilesView(PeerLauncher, QuarantineSource, OutageSource) (~line 1222) -- feeds fleet_profiles, which is READ (open to primary/worker/architect) by design; not obviously sensitive, not evaluated further.
  • mailboxView(LeadChannel.MailboxState) (~line 1476), peerView(LeadChannel, String) (~line 1514) -- both reachable only through coordinatorView, so they inherit this fix's gate transitively.
  • memberCapacityView(...) (~line 1578), capacityView(...) (~line 1640), leadView(...) (~line 1689), memberView(MemberSession) (~line 1716) -- all feed fleet_list's members/leads/capacity sections, which acceptance criterion 3 requires to stay visible to every role; not flagged as a problem.

Verification notes

  • Worker case: tested directly (see above).
  • Architect case: tested directly with a real Principal.architect(...), not only reasoned about.
  • Never touched wiki/, .mcp.json, or fleetd/fleetd.yaml.
  • Never printed an environment variable value -- only checked presence as booleans when confirming GITEA_TOKEN/GITEA_HOST were set.

Update — review follow-up (M2 killed)

The lead reviewed this PR, merged the branch onto main at 56c6d14, and ran a mutation battery. Summary of what came back and what I did about it:

  • CONTROL: Tests run: 1581, Failures: 0, Errors: 0, Skipped: 0, rc=0.
  • M1 (force the gate open, if (callerIsPrimary) → if (true)) — KILLED, as expected: 2 of my existing tests failed.
  • M2 (leave the gate alone, change only what the one production call site feeds it — principal(exchange).isPrimary() → a literal true) — SURVIVED. The full suite stayed green (rc=0, Tests run: 1581, Failures: 0), because every test I had written called listFleet directly and supplied the boolean itself. The gate worked; nothing checked that the real handler actually consults it.

Fix: pin the caller, not just the gate

  1. Named the decision. Added FleetMcp.coordinatorVisibleTo(Principal caller) (return caller.isPrimary();), placed next to denyFor/recordPrimarySingleton — the same "policy predicate split out for testability" idiom already used there. The fleet_list handler (FleetMcp.java line ~430) now reads:

    coordinatorVisibleTo(principal(exchange))

    instead of inlining .isPrimary().

  2. Pinned the predicate's role table in FleetMcpAuthzTest.onlyThePrimaryMaySeeTheCoordinatorRow, using the file's existing Principal constants (PRIMARY, WORKER_A, ARCH_DESIGN, ANON): primary → true, worker/architect/anonymous → false. ANON is new coverage — the previous pass of this ticket didn't test it.

  3. Added a source-reading detector at the boundary, FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCoordinatorVisibleTo, same idiom as toolsTheServerRegisters() / everyRegisteredToolHasItsHandlerActionPinned(): it reads FleetMcp.java's own source, isolates the listHandler block (from the listHandler = declaration to the next handler's stopHandler =), asserts as a control that the block actually contains a listFleet( call (so a drifted anchor fails loudly instead of "no violation found"), then anchors on the trailing-argument position (not a bare contains("true"), since true appears many other places in that file) and asserts it is exactly coordinatorVisibleTo(principal(exchange)), not a literal true/false.

Reproduced M2 exactly, and killed it

Changed the handler's argument to a literal, matching the lead's diff:

-                            coordinatorVisibleTo(principal(exchange)));
+                            true);  // MUTANT: handler no longer asks who called

Ran FleetMcpAuthzTest:

[ERROR] Tests run: 19, Failures: 1, Errors: 0, Skipped: 0
[ERROR] FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCoordinatorVisibleTo -- Time elapsed: 0.004 s <<< FAILURE!
org.opentest4j.AssertionFailedError: the fleet_list handler must ask coordinatorVisibleTo(principal(exchange)) who is
calling, not pass a literal boolean -- found: true ==> expected: <coordinatorVisibleTo(principal(exchange))> but was: <true>

Restored the handler; reran FleetMcpAuthzTest:

Tests run: 19, Failures: 0, Errors: 0, Skipped: 0

grep -n "MUTANT" src/main/java/dev/ltms/fleet/mcp/FleetMcp.java → no matches (clean restore).

Also mutated the predicate itself (step 4)

-        return caller.isPrimary();
+        return true; // MUTANT fleetd #439 break-test: predicate always allows

Ran FleetMcpAuthzTest:

[ERROR] Tests run: 19, Failures: 1, Errors: 0, Skipped: 0
[ERROR] FleetMcpAuthzTest.onlyThePrimaryMaySeeTheCoordinatorRow -- Time elapsed: 0.016 s <<< FAILURE!
org.opentest4j.AssertionFailedError: a worker must not see lead-to-lead coordination state ==> expected: <false> but was: <true>

Restored; reran: Tests run: 19, Failures: 0, Errors: 0, Skipped: 0. grep -n "MUTANT" → no matches after restore.

Full build

mvn -B clean test in fleetd/, unpiped, redirected to a file, checked via $?:

exit=0
[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0 -- in dev.ltms.fleet.mcp.FleetMcpTest
[INFO] Tests run: 19, Failures: 0, Errors: 0, Skipped: 0 -- in dev.ltms.fleet.mcp.FleetMcpAuthzTest
[INFO] Results:
[INFO] Tests run: 1583, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

(1583 = the 1581 on main at 56c6d14 + the 2 new FleetMcpAuthzTest cases.)

On the default-true question (not changed here, per instruction)

Asked to weigh in, not to act: coordinatorVisibleTo's default only lives at one seam — the compat listFleet overload without the boolean, which every shorter overload chains through. I count 6 pre-existing test call sites in FleetMcpTest that rely on that default being true to see the coordinator row at all (listReportsThisDaemonsOwnCoordIdWhenLeadCoordinationIsOn, listReportsAnUnresolvedSelfProbeAsUnknownNeverAsAMeasuredZero, listReportsHeldMessagesWithATruncatedPreviewNeverTheFullBody, listReportsAnHonestHeldCountAndDurabilityNotJustPendingZero, listReportsHeldDurableFalseWhenTheChannelSaysMailIsNotDurable, listReportsEachDeclaredPeersLiveReachability), plus my own listIsByteForByteUnchangedForThePrimaryCaller's preExisting comparison arm. Flipping the default to false would need each of those to add an explicit true (or move to the 12-arg overload) — a bounded, mechanical fix, not a sweep.

My view: I'd lean toward false. The only real production caller (the MCP handler) never relies on the default — it always passes the computed answer — so the default exists purely for callers that don't specify one, which today is exactly the ~30 pre-#439 unit tests plus, hypothetically, any future call site someone adds without thinking about who's asking. CLAUDE.md's asymmetry argument applies cleanly here: a default of true fails open (a forgetful future caller silently discloses coordination state and nothing says so), a default of false fails closed (a forgetful future caller loses the coordinator row and, if that row was expected, a test breaks and points right at the missing argument). Six known call sites needing one added argument is a small, visible price for making the dangerous mistake the loud one. Leaving this to you as asked.

Constraints followed

  • Authz's table untouched.
  • Did not widen scope to the REST surface or to the other views from the same-shape survey.
  • Never merged; staged files by explicit path only (git add src/main/java/.../FleetMcp.java src/test/java/.../FleetMcpAuthzTest.java), never git add -A.
  • Did not touch wiki/, .mcp.json, or fleetd/fleetd.yaml.
  • Never printed an environment variable's value.
  • mvn output was always redirected to a file and checked via $?, never piped into tail/grep.
  • Pushed to the same branch (worker/439-coordinator-row-gate-bc032a-8), this PR (#462) updated in place — no new PR opened.
Closes fleetd #439. ## What changed `fleet_list`'s `coordinator` row (this daemon's coord-id, mailbox state, held lead-to-lead message previews, peer reachability) was returned to EVERY caller, including a worker or an architect. Lead-to-lead coordination is orchestrator-to-orchestrator traffic; a worker has no business reading it. **Option taken: gate at the call site inside `listFleet`, not inside `coordinatorView`'s signature.** `coordinatorView(CoordinationSource)` is unchanged. A new `listFleet` overload takes a trailing `boolean callerIsPrimary` and only calls `coordinatorView(...)` (and only puts the `coordinator` key into the result) when that flag is true -- so a non-primary caller gets the key fully ABSENT, never an empty or redacted object, and never pays the cost of `coordinatorView` probing peer mailboxes for a row it will not receive. Why this option over threading a `Principal` all the way through: `listFleet` has 5 delegating overloads and ~30 existing unit-test call sites that construct it directly with no caller identity at all (they test the assembly logic, not the MCP dispatch). Adding the boolean only to the deepest/full overload, and keeping the old signature as a compat wrapper that passes `true`, means every one of those existing call sites is untouched and keeps its pre-#439 behavior (which was already correct for them, since none of them simulate a worker). Only the one real production call site -- the `fleet_list` MCP handler -- now computes and passes the real answer: `principal(exchange).isPrimary()`. `heldView` (the truncated-preview render used inside the `held[]` array) is reachable ONLY through `coordinatorView` -- grepped, no other caller -- so gating `coordinatorView`'s call already blocks it for a non-primary caller. Nothing else needed there. `Authz`'s `READ` case (`FleetMcp.java:80`) is untouched, as instructed -- it stays shared by `fleet_status`, `fleet_profiles` and `fleet_whoami`. The gate is entirely inside `fleet_list`'s own result assembly, reusing the intent behind the already-correct `COORD_READ` case without touching the table. Scope: MCP surface only. I checked the REST side myself (`grep -rn coordinatorView\|heldView src/main/java` outside `FleetMcp.java` returns nothing) -- no REST handler reaches either method, so there was nothing to fix there. ## Tests (`FleetMcpTest`) Three new tests, all in `dev.ltms.fleet.mcp.FleetMcpTest`: - `listOmitsTheCoordinatorKeyEntirelyForAWorkerEvenWhenLeadCoordinationIsOn` -- drives `Principal.worker(...).isPrimary()` (the real production boolean) through `listFleet` with lead coordination fully configured; asserts the `"coordinator"` key AND any fragment of its content (`"mac-opus"`) are absent, while `leads`/`members`/`healthCoverage` are still present. - `listOmitsTheCoordinatorKeyEntirelyForAnArchitectToo` -- same, but with `Principal.architect("lead-designer", ...).isPrimary()`. This is a real executed test, not just reasoning by analogy to the worker case. - `listIsByteForByteUnchangedForThePrimaryCaller` -- compares the new gated overload called with `callerIsPrimary=true` against the pre-#439 overload (which always assembled the row) on an IDENTICAL setup; asserts the two JSON strings are `assertEquals` (not just both non-empty), then separately asserts every field the ticket names (`selfId`, `mailbox`, `heldCount`, `heldDurable`, `held`, `peers`) is present. ### Break-and-restore proof (acceptance criterion 4) I replaced the gate `if (callerIsPrimary)` with `if (true) // TEMP fleetd #439 break-test: gate disabled on purpose` and reran just the three new tests: ``` [ERROR] Tests run: 3, Failures: 2, Errors: 0, Skipped: 0 [ERROR] FleetMcpTest.listOmitsTheCoordinatorKeyEntirelyForAWorkerEvenWhenLeadCoordinationIsOn:701 a worker must never see the coordinator key at all: {...,"coordinator":{"selfId":"mac-opus",...}} ==> expected: <false> but was: <true> [ERROR] FleetMcpTest.listOmitsTheCoordinatorKeyEntirelyForAnArchitectToo:729 an architect must never see the coordinator key either: {...,"coordinator":{...}} ==> expected: <false> but was: <true> ``` (the byte-for-byte primary test still passed, as expected -- disabling the gate does not change the primary's output). Restored the gate and reran the same three: ``` Tests run: 3, Failures: 0, Errors: 0, Skipped: 0 ``` Confirmed the restore left no stray code (`grep -n "TEMP\|if (true)" FleetMcp.java` -> no matches). ## Full build `mvn -B clean test` in `fleetd/`, real (unpiped) output, redirected to a file and checked via `$?`: ``` exit=0 ... [INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.497 s -- in dev.ltms.fleet.mcp.FleetMcpTest ... [INFO] Results: [INFO] Tests run: 1581, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` ## Same-shape survey (reported, not fixed -- out of scope per the brief) Other private/static `...View(...)` result-section builders in `FleetMcp.java` that take only a source object with no caller argument (the same structural shape as the bug, whether or not they are actually a problem -- I did not judge each one, just enumerated): - `heldMailView(LeadMessage)` (~line 950) -- used only by `pollHeldPeerMail`, which is already dispatch-gated to `Authz.Action.COORD_READ` (primary-only) BEFORE the function ever runs, so this one is not reachable by a non-primary caller. Checked, not a live instance of the bug. - `profilesView(PeerLauncher, QuarantineSource, OutageSource)` (~line 1222) -- feeds `fleet_profiles`, which is `READ` (open to primary/worker/architect) by design; not obviously sensitive, not evaluated further. - `mailboxView(LeadChannel.MailboxState)` (~line 1476), `peerView(LeadChannel, String)` (~line 1514) -- both reachable only through `coordinatorView`, so they inherit this fix's gate transitively. - `memberCapacityView(...)` (~line 1578), `capacityView(...)` (~line 1640), `leadView(...)` (~line 1689), `memberView(MemberSession)` (~line 1716) -- all feed `fleet_list`'s `members`/`leads`/`capacity` sections, which acceptance criterion 3 requires to stay visible to every role; not flagged as a problem. ## Verification notes - Worker case: tested directly (see above). - Architect case: tested directly with a real `Principal.architect(...)`, not only reasoned about. - Never touched `wiki/`, `.mcp.json`, or `fleetd/fleetd.yaml`. - Never printed an environment variable value -- only checked presence as booleans when confirming `GITEA_TOKEN`/`GITEA_HOST` were set. --- ## Update — review follow-up (M2 killed) The lead reviewed this PR, merged the branch onto `main` at `56c6d14`, and ran a mutation battery. Summary of what came back and what I did about it: - **CONTROL**: `Tests run: 1581, Failures: 0, Errors: 0, Skipped: 0`, rc=0. - **M1** (force the gate open, `if (callerIsPrimary)` → `if (true)`) — **KILLED**, as expected: 2 of my existing tests failed. - **M2** (leave the gate alone, change only what the *one production call site* feeds it — `principal(exchange).isPrimary()` → a literal `true`) — **SURVIVED**. The full suite stayed green (`rc=0`, `Tests run: 1581, Failures: 0`), because every test I had written called `listFleet` directly and supplied the boolean itself. The gate worked; nothing checked that the real handler actually consults it. ### Fix: pin the caller, not just the gate 1. **Named the decision.** Added `FleetMcp.coordinatorVisibleTo(Principal caller)` (`return caller.isPrimary();`), placed next to `denyFor`/`recordPrimarySingleton` — the same "policy predicate split out for testability" idiom already used there. The `fleet_list` handler (`FleetMcp.java` line ~430) now reads: ```java coordinatorVisibleTo(principal(exchange)) ``` instead of inlining `.isPrimary()`. 2. **Pinned the predicate's role table** in `FleetMcpAuthzTest.onlyThePrimaryMaySeeTheCoordinatorRow`, using the file's existing `Principal` constants (`PRIMARY`, `WORKER_A`, `ARCH_DESIGN`, `ANON`): primary → true, worker/architect/anonymous → false. `ANON` is new coverage — the previous pass of this ticket didn't test it. 3. **Added a source-reading detector at the boundary**, `FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCoordinatorVisibleTo`, same idiom as `toolsTheServerRegisters()` / `everyRegisteredToolHasItsHandlerActionPinned()`: it reads `FleetMcp.java`'s own source, isolates the `listHandler` block (from the `listHandler =` declaration to the next handler's `stopHandler =`), asserts as a **control** that the block actually contains a `listFleet(` call (so a drifted anchor fails loudly instead of "no violation found"), then anchors on the trailing-argument **position** (not a bare `contains("true")`, since `true` appears many other places in that file) and asserts it is exactly `coordinatorVisibleTo(principal(exchange))`, not a literal `true`/`false`. ### Reproduced M2 exactly, and killed it Changed the handler's argument to a literal, matching the lead's diff: ```java - coordinatorVisibleTo(principal(exchange))); + true); // MUTANT: handler no longer asks who called ``` Ran `FleetMcpAuthzTest`: ``` [ERROR] Tests run: 19, Failures: 1, Errors: 0, Skipped: 0 [ERROR] FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCoordinatorVisibleTo -- Time elapsed: 0.004 s <<< FAILURE! org.opentest4j.AssertionFailedError: the fleet_list handler must ask coordinatorVisibleTo(principal(exchange)) who is calling, not pass a literal boolean -- found: true ==> expected: <coordinatorVisibleTo(principal(exchange))> but was: <true> ``` Restored the handler; reran `FleetMcpAuthzTest`: ``` Tests run: 19, Failures: 0, Errors: 0, Skipped: 0 ``` `grep -n "MUTANT" src/main/java/dev/ltms/fleet/mcp/FleetMcp.java` → no matches (clean restore). ### Also mutated the predicate itself (step 4) ```java - return caller.isPrimary(); + return true; // MUTANT fleetd #439 break-test: predicate always allows ``` Ran `FleetMcpAuthzTest`: ``` [ERROR] Tests run: 19, Failures: 1, Errors: 0, Skipped: 0 [ERROR] FleetMcpAuthzTest.onlyThePrimaryMaySeeTheCoordinatorRow -- Time elapsed: 0.016 s <<< FAILURE! org.opentest4j.AssertionFailedError: a worker must not see lead-to-lead coordination state ==> expected: <false> but was: <true> ``` Restored; reran: `Tests run: 19, Failures: 0, Errors: 0, Skipped: 0`. `grep -n "MUTANT"` → no matches after restore. ### Full build `mvn -B clean test` in `fleetd/`, unpiped, redirected to a file, checked via `$?`: ``` exit=0 [INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0 -- in dev.ltms.fleet.mcp.FleetMcpTest [INFO] Tests run: 19, Failures: 0, Errors: 0, Skipped: 0 -- in dev.ltms.fleet.mcp.FleetMcpAuthzTest [INFO] Results: [INFO] Tests run: 1583, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` (1583 = the 1581 on `main` at `56c6d14` + the 2 new `FleetMcpAuthzTest` cases.) ### On the default-true question (not changed here, per instruction) Asked to weigh in, not to act: `coordinatorVisibleTo`'s default only lives at one seam — the compat `listFleet` overload without the boolean, which every shorter overload chains through. I count **6 pre-existing test call sites** in `FleetMcpTest` that rely on that default being `true` to see the `coordinator` row at all (`listReportsThisDaemonsOwnCoordIdWhenLeadCoordinationIsOn`, `listReportsAnUnresolvedSelfProbeAsUnknownNeverAsAMeasuredZero`, `listReportsHeldMessagesWithATruncatedPreviewNeverTheFullBody`, `listReportsAnHonestHeldCountAndDurabilityNotJustPendingZero`, `listReportsHeldDurableFalseWhenTheChannelSaysMailIsNotDurable`, `listReportsEachDeclaredPeersLiveReachability`), plus my own `listIsByteForByteUnchangedForThePrimaryCaller`'s `preExisting` comparison arm. Flipping the default to `false` would need each of those to add an explicit `true` (or move to the 12-arg overload) — a bounded, mechanical fix, not a sweep. My view: I'd lean toward `false`. The only real production caller (the MCP handler) never relies on the default — it always passes the computed answer — so the default exists purely for callers that don't specify one, which today is exactly the ~30 pre-#439 unit tests plus, hypothetically, any future call site someone adds without thinking about who's asking. `CLAUDE.md`'s asymmetry argument applies cleanly here: a default of `true` fails *open* (a forgetful future caller silently discloses coordination state and nothing says so), a default of `false` fails *closed* (a forgetful future caller loses the coordinator row and, if that row was expected, a test breaks and points right at the missing argument). Six known call sites needing one added argument is a small, visible price for making the dangerous mistake the loud one. Leaving this to you as asked. ### Constraints followed - `Authz`'s table untouched. - Did not widen scope to the REST surface or to the other views from the same-shape survey. - Never merged; staged files by explicit path only (`git add src/main/java/.../FleetMcp.java src/test/java/.../FleetMcpAuthzTest.java`), never `git add -A`. - Did not touch `wiki/`, `.mcp.json`, or `fleetd/fleetd.yaml`. - Never printed an environment variable's value. - `mvn` output was always redirected to a file and checked via `$?`, never piped into `tail`/`grep`. - Pushed to the same branch (`worker/439-coordinator-row-gate-bc032a-8`), this PR (#462) updated in place — no new PR opened.
agent added 1 commit 2026-09-10 13:17:36 +02:00
fleetd #439: omit fleet_list's coordinator key for non-primary callers
CI / contract (pull_request) Successful in 56s
Details
CI / build (pull_request) Successful in 1m40s
Details
e54e3d87ea 
The coordinator row is lead-to-lead coordination state (coord-ids, mailbox
facts, held-message previews). fleet_list returned it to every caller,
including a worker or an architect, because coordinatorView() had no way to
know who was asking.

Gate at the call site inside listFleet: a new overload takes
callerIsPrimary and only assembles/attaches the coordinator row when it is
true, so the key is absent (not empty) for a worker or an architect. The
MCP handler now passes principal(exchange).isPrimary(); every other
listFleet overload keeps passing true, so callers with no caller identity
(existing unit tests, the no-op wrappers) are unaffected -- confirmed by a
byte-for-byte comparison test against the pre-fix overload.

Authz's READ case is untouched: it stays shared by fleet_status,
fleet_profiles and fleet_whoami, and the gate here is purely inside
fleet_list's own result assembly.
agent added 1 commit 2026-09-10 13:28:24 +02:00
fleetd #439: pin the caller at the fleet_list call site, not just the gate
CI / contract (pull_request) Successful in 1m29s
Details
CI / build (pull_request) Successful in 1m33s
Details
c1ca6273fc 
Review of PR #462 found M2: the coordinatorVisibleTo gate (then an inline
principal(exchange).isPrimary() check) could survive a mutation that
replaced the argument with a literal true at the one production call
site, because every existing test drove listFleet directly and supplied
the boolean itself -- nothing exercised the handler's own call.

- Name the decision: FleetMcp.coordinatorVisibleTo(Principal), a small
  package-private predicate next to denyFor/recordPrimarySingleton. The
  fleet_list handler now calls coordinatorVisibleTo(principal(exchange))
  instead of inlining .isPrimary().
- Pin the predicate's role table in FleetMcpAuthzTest
  (onlyThePrimaryMaySeeTheCoordinatorRow), covering primary/worker/
  architect and, newly, anonymous.
- Add a source-reading detector at the boundary
  (theFleetListHandlerActuallyConsultsCoordinatorVisibleTo), same idiom as
  toolsTheServerRegisters/everyRegisteredToolHasItsHandlerActionPinned: it
  reads FleetMcp.java, isolates the listHandler block, asserts (as a
  control) that the block actually contains a listFleet( call, then
  asserts the call's trailing boolean argument is exactly
  coordinatorVisibleTo(principal(exchange)) -- not a literal true/false.

Both mutations from the review were reproduced and killed by these tests,
then reverted; see the PR body for the full break-and-restore transcript.
ltms commented 2026-09-10 14:20:08 +02:00
Owner
Copy Link
Copy Source

Merged as 92a96fc. Closing by hand — a local --no-ff merge plus push did not trip the forge's auto-close.

Proof the branch is in main: git merge-base --is-ancestor origin/worker/439-coordinator-row-gate-bc032a-8 origin/main succeeds, branch tip c1ca627.

Full verification is on #439. Worth repeating one result here, because the technique is reusable: the source-reading detector this PR added was checked for vacuity by renaming its anchor (listHandler → listHandlerX, behaviour unchanged), and it failed loudly rather than passing on an empty scrape. A control inside a source-reading test proves the search ran on something; renaming the anchor proves it would notice if that something moved.

The fail-open default this PR left behind was filed as #463 and has since shipped in 235644c.

Merged as **`92a96fc`**. Closing by hand — a local `--no-ff` merge plus push did not trip the forge's auto-close. Proof the branch is in `main`: `git merge-base --is-ancestor origin/worker/439-coordinator-row-gate-bc032a-8 origin/main` succeeds, branch tip `c1ca627`. Full verification is on #439. Worth repeating one result here, because the technique is reusable: the source-reading detector this PR added was checked for vacuity by **renaming its anchor** (`listHandler` → `listHandlerX`, behaviour unchanged), and it failed loudly rather than passing on an empty scrape. A control inside a source-reading test proves the search ran on *something*; renaming the anchor proves it would notice if that something moved. The fail-open default this PR left behind was filed as #463 and has since shipped in `235644c`.
ltms closed this pull request 2026-09-10 14:20:25 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m29s
Details
CI / build (pull_request) Successful in 1m33s
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#462
Delete Branch "worker/439-coordinator-row-gate-bc032a-8"

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?