fleetd #737 unit 6: stop the unnamed primary sharing the internal bypass #744

2026-10-05T05:25:23+02:00

agent commented

2026-10-05 05:25:23 +02:00

Fixes the unit 6 scope of #737: MessageService.ownsTicket treated a null callerOwner as a universal bypass, conflating the internal no-check test seam (poll(String)'s single-arg overload) with a real unnamed primary's owner key. Now INTERNAL_NO_OWNER_CHECK is a distinct marker string, and null is just another owner key compared by equality (including null==null, so an unnamed primary still owns tickets it created itself). Applied to both ownsTicket call sites: poll and pendingAsk.

Inverted (not weakened) the two tests named in the ticket's unit-6 addendum, plus three more that pinned the same wildcard at the REST and MCP layers once the build surfaced them:

MessageServiceTest.unnamedPrimaryStillReadsAnyTicket -> unnamedPrimaryIsRefusedFromANamedLeadsTicket (now asserts refusal) + added unnamedPrimaryReadsItsOwnTicket (positive control) + theOneArgPollOverloadBypassesOwnershipEntirely (bypass test)
FleetAppAuthTest.restPollRefusesADifferentWorkerButAllowsTheCreatorAndTheUnnamedPrimary -> restPollRefusesADifferentWorkerAndAnUnnamedPrimaryOnANamedWorkersTicket (now asserts refusal) + added restPollAllowsAnUnnamedPrimaryItsOwnTicket (positive control)
MessageServiceTest.pendingAskGatesTheQuestionByTheDelegationsCreatorOwner: inverted its unnamed-primary assertion, added unnamedPrimarySeesItsOwnDelegationsPendingQuestion (positive control) and pendingAskInternalBypassSeesAnyDelegationsPendingQuestion (bypass test, driven via the now package-private INTERNAL_NO_OWNER_CHECK since pendingAsk has no no-check overload)
FleetAppAuthTest.restStatusGatesThePendingAskFieldsByTheDelegationsCreatorOwner and FleetMcpTest.statusGatesThePendingAskFieldsByTheDelegationsCreatorOwner: inverted their unnamed-primary assertions the same way

Two tests that must keep pinning the null-to-null match (AuthzTest:43, CallerResolverTest:367) were left untouched -- they test terminal/Authz plumbing, not ownsTicket, and were unaffected.

Also fixed three test-plumbing poll() calls that used the 2-arg overload with an explicit null just to wait for a phase transition on a ticket created by a named caller -- switched to the 1-arg bypass overload since they are not testing ownership.

Updated three javadoc comments that described the old null-is-wildcard rule (Principal.ownerKey, MessageService.poll(String,String), FleetMcp.status) to describe the new equality rule.

Mutation evidence (full red/green runs in the handoff):

Reintroducing callerOwner == null as an extra bypass branch kills 5 tests including the inverted named-lead-ticket test. Reverted, green.
Requiring callerOwner non-null before comparing (so null != null) kills 9 tests including the positive-control own-ticket test. Reverted, green.

Full mvn clean install: Tests run: 2097, Failures: 0, Errors: 0, Skipped: 0 -- BUILD SUCCESS.

Fixes the unit 6 scope of #737: MessageService.ownsTicket treated a null callerOwner as a universal bypass, conflating the internal no-check test seam (poll(String)'s single-arg overload) with a real unnamed primary's owner key. Now INTERNAL_NO_OWNER_CHECK is a distinct marker string, and null is just another owner key compared by equality (including null==null, so an unnamed primary still owns tickets it created itself). Applied to both ownsTicket call sites: poll and pendingAsk. Inverted (not weakened) the two tests named in the ticket's unit-6 addendum, plus three more that pinned the same wildcard at the REST and MCP layers once the build surfaced them: - MessageServiceTest.unnamedPrimaryStillReadsAnyTicket -> unnamedPrimaryIsRefusedFromANamedLeadsTicket (now asserts refusal) + added unnamedPrimaryReadsItsOwnTicket (positive control) + theOneArgPollOverloadBypassesOwnershipEntirely (bypass test) - FleetAppAuthTest.restPollRefusesADifferentWorkerButAllowsTheCreatorAndTheUnnamedPrimary -> restPollRefusesADifferentWorkerAndAnUnnamedPrimaryOnANamedWorkersTicket (now asserts refusal) + added restPollAllowsAnUnnamedPrimaryItsOwnTicket (positive control) - MessageServiceTest.pendingAskGatesTheQuestionByTheDelegationsCreatorOwner: inverted its unnamed-primary assertion, added unnamedPrimarySeesItsOwnDelegationsPendingQuestion (positive control) and pendingAskInternalBypassSeesAnyDelegationsPendingQuestion (bypass test, driven via the now package-private INTERNAL_NO_OWNER_CHECK since pendingAsk has no no-check overload) - FleetAppAuthTest.restStatusGatesThePendingAskFieldsByTheDelegationsCreatorOwner and FleetMcpTest.statusGatesThePendingAskFieldsByTheDelegationsCreatorOwner: inverted their unnamed-primary assertions the same way Two tests that must keep pinning the null-to-null match (AuthzTest:43, CallerResolverTest:367) were left untouched -- they test terminal/Authz plumbing, not ownsTicket, and were unaffected. Also fixed three test-plumbing poll() calls that used the 2-arg overload with an explicit null just to wait for a phase transition on a ticket created by a named caller -- switched to the 1-arg bypass overload since they are not testing ownership. Updated three javadoc comments that described the old null-is-wildcard rule (Principal.ownerKey, MessageService.poll(String,String), FleetMcp.status) to describe the new equality rule. Mutation evidence (full red/green runs in the handoff): - Reintroducing callerOwner == null as an extra bypass branch kills 5 tests including the inverted named-lead-ticket test. Reverted, green. - Requiring callerOwner non-null before comparing (so null != null) kills 9 tests including the positive-control own-ticket test. Reverted, green. Full mvn clean install: Tests run: 2097, Failures: 0, Errors: 0, Skipped: 0 -- BUILD SUCCESS.

agent added 2 commits 2026-10-05 05:25:23 +02:00

fleetd #726 : rewrite the handover skill for the restart roll 8fa8d18c97

The roll now ends the pane and launches a fresh process instead of typing
/clear, and that jar is deployed, so the skill's self-dated "a change is
coming" note had to go.

Measured on 2026-10-05 before writing:

  grep -c "lead-rollover: rolled" fleetd/fleetd.out   -> 20
  grep -c "lead-rollover:" fleetd/fleetd.out          -> 86 (control)
  <tail from the last "fleetd listening"> | grep -c "lead-rollover:" -> 0
  grep -c 'RELAUNCH_NEVER_READY\|RELAUNCH_NOT_RECOGNISED\|OLD_PANE_NEVER_DIED' -> 0

So all 20 recorded rolls ran under /clear and the restart path has never
executed. The section says that rather than implying the old numbers
describe it.

Also:
- name all eight RollState outcomes, with what each one guarantees
- state that relaunchReadySeconds bounds each of two waits, not the pair
- drop the "never observed as WORKING after 8 consecutive IDLE/DONE polls"
  paragraph: grep finds that wait is deleted, so it cannot appear
- drop the #621 warning: contextNotice now takes requireOperatorConfirm
- split the surprise bullets into their own section

fleetd #737 unit 6: stop the unnamed primary sharing the internal bypass

CI / shell-tests (pull_request) Failing after 8s

Details

CI / contract (pull_request) Successful in 55s

Details

CI / build (pull_request) Failing after 1m59s

Details

6ab3a81af7

ownsTicket treated a null callerOwner as "read everything", conflating
the internal no-check test seam with a real unnamed primary's owner
key. Give them two different values: a named INTERNAL_NO_OWNER_CHECK
marker for the test-only bypass, and null as just another owner key
that must equal the ticket's recorded creatorOwner (including a
null-to-null match, so an unnamed primary still owns its own tickets).
Applies to both ownsTicket call sites, poll and pendingAsk.

ltms commented

2026-10-05 05:33:30 +02:00

Merged locally into main as 6ab3a81, with one lead follow-up commit 7467ffa. Closing this PR as the branch is in main.

What I checked myself, not on the worker's word

The load-bearing claim — "no production code calls poll(String)" — is true. This mattered, because the fix turns that overload into a full ownership bypass. Measured against the branch ref itself (no checkout), the only one-arg poll() in main/java is t.queue.poll() on a java.util.Queue in Injector.java:493:

worker/737-b038f7-2:fleetd/.../mcp/FleetMcp.java:1246:   messages.poll(ticket, callerOwner);
worker/737-b038f7-2:fleetd/.../rest/FleetApp.java:907:   messages.poll(ctx.pathParam("ticket"), caller == null ? null : caller.ownerKey());
worker/737-b038f7-2:fleetd/.../mcp/FleetMcp.java:1366:   messages.pendingAsk(sessionId, callerOwner);
worker/737-b038f7-2:fleetd/.../rest/FleetApp.java:889:   messages.pendingAsk(id, caller == null ? null : caller.ownerKey());

All four pass a resolved owner key. INTERNAL_NO_OWNER_CHECK appears only inside MessageService plus two test references. So the bypass is unreachable from any request path.

The sentinel is genuinely unproducible. Principal.prefixed() is role + ":" + identity with the role strings fixed to leader/worker/architect/collaborator/observer, plus the "anonymous" literal and null. Nothing yields an internal: prefix.

Build: mvn clean install from fleetd/, exit 0, BUILD SUCCESS, Tests run: 2097, Failures: 0. Confirmed independently by summing 177 surefire XML files: tests=2097 failures=0 errors=0. Run in a throwaway worktree with the follow-up commit applied, so I built what I merged.

One finding the unit missed — fixed in `7467ffa`

FleetApp.java:885-888 still carried the old rule in an inline comment:

// Phase.ASKING view, but only to the caller whose owner key created that delegation, or
// to the unnamed primary.

That is the exact claim this unit corrected in FleetMcp.status(...)'s javadoc and in MessageService's. The REST-side copy was left describing an exemption that no longer exists. Fixed rather than sent back, since it is two lines of comment. Worth noting as a pattern: the same sentence existed in three places, and the sweep found two.

Accepted residuals, both tested and deliberate

Two unnamed primaries share one owner key (null == null), so one can read the other's ticket. That is the second failure direction already written up in wiki/9-Implementation.md, and the unit pinned it with the unnamedPrimaryReadsItsOwnTicket positive control rather than leaving it implicit. Correct call.
FleetApp maps an unresolved caller (caller == null) to the same null key. Both handlers gate on allow(...) first, so I did not establish that an unresolved caller can reach those lines — and it is strictly tighter than before, where null read every ticket. Not a blocker; noted so it is not rediscovered as new.

On the visibility question the unit raised

Keeping INTERNAL_NO_OWNER_CHECK package-private instead of adding a pendingAsk no-check overload is the right trade — it adds no production API. The asymmetry between poll and pendingAsk is cosmetic and belongs with the other #741 follow-ups, not here.

Mutation evidence accepted as reported: two mutations, each RED with named killed tests and reverted to GREEN, including one that killed the positive controls and six unrelated tests — which is itself evidence the null == null path is load-bearing well beyond the new tests.

Merged locally into `main` as `6ab3a81`, with one lead follow-up commit `7467ffa`. Closing this PR as the branch is in `main`. ## What I checked myself, not on the worker's word **The load-bearing claim — "no production code calls `poll(String)`" — is true.** This mattered, because the fix turns that overload into a full ownership bypass. Measured against the branch ref itself (no checkout), the only one-arg `poll()` in `main/java` is `t.queue.poll()` on a `java.util.Queue` in `Injector.java:493`: ``` worker/737-b038f7-2:fleetd/.../mcp/FleetMcp.java:1246: messages.poll(ticket, callerOwner); worker/737-b038f7-2:fleetd/.../rest/FleetApp.java:907: messages.poll(ctx.pathParam("ticket"), caller == null ? null : caller.ownerKey()); worker/737-b038f7-2:fleetd/.../mcp/FleetMcp.java:1366: messages.pendingAsk(sessionId, callerOwner); worker/737-b038f7-2:fleetd/.../rest/FleetApp.java:889: messages.pendingAsk(id, caller == null ? null : caller.ownerKey()); ``` All four pass a resolved owner key. `INTERNAL_NO_OWNER_CHECK` appears only inside `MessageService` plus two test references. So the bypass is unreachable from any request path. **The sentinel is genuinely unproducible.** `Principal.prefixed()` is `role + ":" + identity` with the role strings fixed to `leader`/`worker`/`architect`/`collaborator`/`observer`, plus the `"anonymous"` literal and `null`. Nothing yields an `internal:` prefix. **Build:** `mvn clean install` from `fleetd/`, exit 0, `BUILD SUCCESS`, `Tests run: 2097, Failures: 0`. Confirmed independently by summing 177 surefire XML files: `tests=2097 failures=0 errors=0`. Run in a throwaway worktree with the follow-up commit applied, so I built what I merged. ## One finding the unit missed — fixed in `7467ffa` `FleetApp.java:885-888` still carried the **old** rule in an inline comment: > `// Phase.ASKING view, but only to the caller whose owner key created that delegation, or` > `// to the unnamed primary.` That is the exact claim this unit corrected in `FleetMcp.status(...)`'s javadoc and in `MessageService`'s. The REST-side copy was left describing an exemption that no longer exists. Fixed rather than sent back, since it is two lines of comment. Worth noting as a pattern: the same sentence existed in three places, and the sweep found two. ## Accepted residuals, both tested and deliberate 1. **Two unnamed primaries share one owner key** (`null == null`), so one can read the other's ticket. That is the second failure direction already written up in `wiki/9-Implementation.md`, and the unit pinned it with the `unnamedPrimaryReadsItsOwnTicket` positive control rather than leaving it implicit. Correct call. 2. `FleetApp` maps an unresolved caller (`caller == null`) to the same `null` key. Both handlers gate on `allow(...)` first, so I did not establish that an unresolved caller can reach those lines — and it is strictly tighter than before, where `null` read every ticket. Not a blocker; noted so it is not rediscovered as new. ## On the visibility question the unit raised Keeping `INTERNAL_NO_OWNER_CHECK` package-private instead of adding a `pendingAsk` no-check overload is the right trade — it adds no production API. The asymmetry between `poll` and `pendingAsk` is cosmetic and belongs with the other #741 follow-ups, not here. Mutation evidence accepted as reported: two mutations, each RED with named killed tests and reverted to GREEN, including one that killed the positive controls and six unrelated tests — which is itself evidence the `null == null` path is load-bearing well beyond the new tests.

ltms closed this pull request

2026-10-05 05:33:34 +02:00

ltms referenced this pull request

2026-10-05 05:34:02 +02:00

After #726 ships, a lead handover will orphan every in-flight delegation: the fresh lead gets a new terminal id and ownsTicket refuses it #737

ltms referenced this pull request

2026-10-05 05:39:21 +02:00

After #726 ships, a lead handover will orphan every in-flight delegation: the fresh lead gets a new terminal id and ownsTicket refuses it #737