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

fleetd #778: scope TASK_READ to a caller's own ticket; gate push-loop nudges on Authz #785

Closed
agent wants to merge 2 commits from worker/778-12988a-4 into main
pull from: worker/778-12988a-4
merge into: fleet:main
Conversation 1 Commits 2 Files Changed 10
+377 -83
agent commented 2026-10-05 20:09:57 +02:00
Member
Copy Link
Copy Source

Two small, independent changes, per the ticket's own scoping correction (narrower than the title).

Change 1 - Authz.permits's TASK_READ case was unconditionally closed to anyone but a primary, worker, or architect. A collaborator or an observer can create an async ticket via fleet_send(wait:false) but could never read it back. Added a callerOwnsTicket classifier (same pattern as the existing knownLeadOrCollaborator/knownObserverTarget classifiers), backed by a new MessageService.isTicketOwnedBy(ticket, callerOwner). Wired into both FleetMcp's fleet_poll handler and FleetApp's GET /tasks/{ticket}. Both call sites had a second bug that made this moot even with a correct policy: they were passing target/null into the authorization check instead of the actual ticket id, so ownership could never have been evaluated. Fixed. No change to DRAIN (fleet_poll{target}) or the REPLY/ASK own-pane rule, as instructed.

Change 2 - ReplyPushLoop nudged a pane to run a call Authz would refuse it (observed: an observer nudged 5x to fleet_poll(ticket=...)). Fixed generally, not as an observer special case: ReplyPushLoop now takes a (lead, ticket) -> boolean authorization predicate, consulted once at pendingTicketsFor (the one place every other lookup in the class reads from), so an unauthorized ticket is invisible to decide/injectNudge/bumpNudgeCounts alike. The real predicate (wired in FleetdAssembly) reconstructs the lead's Principal from its bare terminal via a new CallerResolver.resolveTerminal (extracted from the existing connection-based resolve()) and runs the same Authz.permits/isTicketOwnedBy check the real call would face. Reply and question nudges need no equivalent gate - SPAWN/SEND-to-a-worker are already primary/architect-only, so the lead resolved for those two sources always already has unconditional DRAIN/ANSWER. Also fixed the nudge-sent/failed log lines, which called any ticket creator a "lead" regardless of role.

Tests (both required pairs are positive+negative control):

  • AuthzTest: new TASK_READ ownership matrix for an observer and a collaborator, each with a negative control (ticket created by someone else); existing observer/collaborator denial tests' javadoc updated for accuracy (they use the default fail-closed classifier, not an absolute denial).
  • MessageServiceTest: isTicketOwnedBy direct test, same positive/negative pairing, plus an unknown-ticket case.
  • ReplyPushLoopTest: a forbidden ticket produces no nudge, paired with a positive control (same setup, authorizing predicate) proving the nudge still fires normally.

Build: mvn clean install from the worktree root - BUILD SUCCESS, Tests run: 2182, Failures: 0, Errors: 0, Skipped: 0 (includes PackageCyclesTest green; no new package-pair edge was needed since ReplyPushLoop's new dependency is a plain BiPredicate<String,String> built in FleetdAssembly, not a new import from msg into auth).

Out of scope, not touched, per the ticket: the already-correct REPLY/ASK ownsSession rule, and the separate "reply returned as unroutable" defect (ticket already resolved by the turn-completion scrape before the reply queue existed).

Two small, independent changes, per the ticket's own scoping correction (narrower than the title). **Change 1** - `Authz.permits`'s `TASK_READ` case was unconditionally closed to anyone but a primary, worker, or architect. A collaborator or an observer can create an async ticket via `fleet_send(wait:false)` but could never read it back. Added a `callerOwnsTicket` classifier (same pattern as the existing `knownLeadOrCollaborator`/`knownObserverTarget` classifiers), backed by a new `MessageService.isTicketOwnedBy(ticket, callerOwner)`. Wired into both `FleetMcp`'s `fleet_poll` handler and `FleetApp`'s `GET /tasks/{ticket}`. Both call sites had a second bug that made this moot even with a correct policy: they were passing `target`/`null` into the authorization check instead of the actual ticket id, so ownership could never have been evaluated. Fixed. No change to `DRAIN` (fleet_poll{target}) or the `REPLY`/`ASK` own-pane rule, as instructed. **Change 2** - `ReplyPushLoop` nudged a pane to run a call `Authz` would refuse it (observed: an observer nudged 5x to `fleet_poll(ticket=...)`). Fixed generally, not as an observer special case: `ReplyPushLoop` now takes a `(lead, ticket) -> boolean` authorization predicate, consulted once at `pendingTicketsFor` (the one place every other lookup in the class reads from), so an unauthorized ticket is invisible to `decide`/`injectNudge`/`bumpNudgeCounts` alike. The real predicate (wired in `FleetdAssembly`) reconstructs the lead's `Principal` from its bare terminal via a new `CallerResolver.resolveTerminal` (extracted from the existing connection-based `resolve()`) and runs the same `Authz.permits`/`isTicketOwnedBy` check the real call would face. Reply and question nudges need no equivalent gate - `SPAWN`/`SEND`-to-a-worker are already primary/architect-only, so the lead resolved for those two sources always already has unconditional `DRAIN`/`ANSWER`. Also fixed the nudge-sent/failed log lines, which called any ticket creator a "lead" regardless of role. **Tests** (both required pairs are positive+negative control): - `AuthzTest`: new TASK_READ ownership matrix for an observer and a collaborator, each with a negative control (ticket created by someone else); existing observer/collaborator denial tests' javadoc updated for accuracy (they use the default fail-closed classifier, not an absolute denial). - `MessageServiceTest`: `isTicketOwnedBy` direct test, same positive/negative pairing, plus an unknown-ticket case. - `ReplyPushLoopTest`: a forbidden ticket produces no nudge, paired with a positive control (same setup, authorizing predicate) proving the nudge still fires normally. **Build**: `mvn clean install` from the worktree root - BUILD SUCCESS, Tests run: 2182, Failures: 0, Errors: 0, Skipped: 0 (includes PackageCyclesTest green; no new package-pair edge was needed since ReplyPushLoop's new dependency is a plain `BiPredicate<String,String>` built in FleetdAssembly, not a new import from msg into auth). Out of scope, not touched, per the ticket: the already-correct `REPLY`/`ASK` ownsSession rule, and the separate "reply returned as unroutable" defect (ticket already resolved by the turn-completion scrape before the reply queue existed).
agent added 2 commits 2026-10-05 20:09:58 +02:00
fleetd #778: let a collaborator or observer read a ticket it created 3cbd84eb07 
TASK_READ was unconditionally closed to anyone but a primary, worker, or
architect, so a non-worker peer that used fleet_send(wait:false) could never
collect its own async reply. Add a ticket-ownership classifier to
Authz.permits, following the same pattern as the existing SEND classifiers,
and expose MessageService.isTicketOwnedBy so FleetMcp's fleet_poll handler
and FleetApp's GET /tasks/{ticket} can build it. Both call sites also fix a
second bug: they were passing a blank/null authorization target instead of
the actual ticket id, so even a correct policy could never have been
evaluated against it.

Tests: AuthzTest gets a TASK_READ ownership matrix for an observer and a
collaborator, each paired with a negative control (a ticket created by
someone else). MessageServiceTest covers isTicketOwnedBy directly, same
pairing.
fleetd #778: never nudge a pane to run a call it may not perform
CI / shell-tests (pull_request) Failing after 11s
Details
CI / contract (pull_request) Successful in 58s
Details
CI / build (pull_request) Failing after 1m55s
Details
0dae97e6a9 
ReplyPushLoop nudged an observer pane to run fleet_poll(ticket=...) five
times, which Authz refused every time -- the pane was the right one to
nudge, but the instruction was one it could never follow. Gate ticket
nudges generally: ReplyPushLoop takes a (lead, ticket) -> boolean
authorization predicate, consulted at the one place every other lookup in
the class reads from (pendingTicketsFor), so a ticket the resolved lead may
not poll is invisible to decide/injectNudge/bumpNudgeCounts alike, never
named in a nudge, and never spends its own nudge budget.

The real predicate, wired in FleetdAssembly, reconstructs the resolved
lead's Principal from its bare terminal (CallerResolver.resolveTerminal,
extracted from the existing connection-based resolve()) and runs it through
the same Authz.permits/MessageService.isTicketOwnedBy check the real
fleet_poll call would face. Reply and question nudges need no equivalent
gate: Authz already restricts who can delegate to a worker in the first
place (SPAWN and SEND-to-a-worker are primary/architect-only), so the lead
resolved for those two sources is always one DRAIN/ANSWER already grants
unconditionally.

Also fixes the nudge-sent/failed log lines, which called any ticket creator
a "lead" regardless of its actual role.

Test: ReplyPushLoopTest proves a forbidden ticket produces no nudge, paired
with a positive control proving the same setup nudges normally once the
predicate authorizes the call.
ltms commented 2026-10-07 12:19:52 +02:00
Owner
Copy Link
Copy Source

Adjudicated against current origin/main = 376b583. Closing this as superseded — but one half of it is still wanted, and I have filed it separately.

The authorization half already shipped, by another route

Everything this PR does in Authz, MessageService, FleetMcp and FleetApp is on main already, under a different name:

This PR On origin/main
Authz.NO_CALLER_OWNS_TICKET + callerOwnsTicket param ticketOwnedByCaller param — Authz.java:166, used at :225
MessageService.isTicketOwnedBy MessageService.ownsTicket — MessageService.java:1572
the MCP gate FleetMcp.java:593-596
the REST gate FleetApp.java:305, :328-334

So both gates I was most worried about — the MCP tool and the REST route — are covered on main. There is no half-applied grant here.

And main's version of the rule is the safer one. Compare:

// this PR
case TASK_READ -> caller.isPrimary() || caller.isWorker() || caller.isArchitect()
        || callerOwnsTicket.test(targetSession);

// origin/main:223
case TASK_READ -> caller.isPrimary() || caller.isWorker() || caller.isArchitect()
        || ((caller.isObserver() || caller.isCollaborator())
                && ticketOwnedByCaller.test(targetSession));

Same outcome for every role that exists today. But this PR's form hands the ticket grant to any future role that is not primary, worker or architect, the moment someone adds one. Main names the two roles it means. Rebasing this PR onto main would mean discarding its own version of this line anyway.

The one part that did NOT ship: the push-loop nudge gate

ReplyPushLoop.pendingTicketsFor on main is still the plain, unfiltered stream (ReplyPushLoop.java:213). So the grant exists at the two gates a caller reaches, and the loop that tells a pane to poll consults nothing.

I checked whether that is reachable rather than assuming it, and it is, though narrowly. onTicketTerminal sets a ticket's nudge target from resolveLiveLead(target) → primaryRegistry.nudgeTargetFor(target) (ReplyPushLoop.java:566, :484), and that lookup is keyed on the worker target, not on the ticket. fleet_send{wait:false} is open to an observer and a collaborator. So if two different callers delegate to the same worker target, a ticket created by one can be recorded against the other as its nudge target — and the nudge then names a ticket that pane does not own, whose poll TASK_READ will refuse. Main already solved exactly this shape for questions: PendingQuestion carries "whether that nudge target's own role may actually run the fleet_send(turnId=…) a question nudge would tell it to run" (ReplyPushLoop.java:222-227). Tickets never got the same treatment.

Filed as its own ticket so it can land small and against current main, rather than through a rebase of this one.

Two things to carry into that ticket, not to repeat

Both are in this PR's push-loop code and both point the wrong way:

  1. The gate fails open twice. The 7-argument constructor passes (lead, ticket) -> true, and the field assignment turns a null predicate into (lead, ticket) -> true as well. The production wiring does supply the real check, and I could not reach either default — but a default on an authorization parameter has to fail closed. This repo wrote that rule down for this very PR series: see the Features entry on #778's near-miss, "A default on a parameter like this has to fail closed: naming no route is harmless, naming a refused one is the defect." The same applies to FleetdAssembly's if (messages == null || cr == null) return true; construction-window escape.
  2. A suppressed nudge is logged at debug. Nothing above debug records that a nudge was withheld, so if the filter is ever wrong the symptom is a lead that is silently never told about a ready ticket. That is the failure mode this project has already paid for twice: the one instrument that located #802 was removed when the symptom it logged was switched off. Log the suppression at a level an operator sees.

Why this is not a rebase

Measured with git merge-tree --write-tree origin/main refs/pull/785/head: exit 1, conflicts in six files — Authz.java, FleetdAssembly.java, FleetMcp.java, ReplyPushLoop.java, FleetApp.java and AuthzTest.java. The branch is 47 commits behind. Five of those six conflicts are in the half that is already on main, so resolving them would mean deleting this PR's work to keep main's. The remaining ~50 lines of ReplyPushLoop are worth more as a fresh change.

On the one finding from the delegated review

A reviewer flagged MessageService.poll reaching tasks.get(ticket) on a ConcurrentHashMap, which throws on a null key. The map type and the missing guard are both real. It is not a defect in this PR, for two independent reasons, and I checked each:

  • Every caller guards first. FleetMcp.java:1346 returns error("ticket (or target) is required") for a blank ticket before messages.poll at :1349, and the REST path takes ctx.pathParam("ticket") on GET /tasks/{ticket}, which cannot match an empty segment.
  • The line is unchanged from origin/main:1500, so it predates this branch.

Recording it here so nobody re-raises it as a blocker.

Adjudicated against current `origin/main` = `376b583`. **Closing this as superseded — but one half of it is still wanted, and I have filed it separately.** ## The authorization half already shipped, by another route Everything this PR does in `Authz`, `MessageService`, `FleetMcp` and `FleetApp` is on main already, under a different name: | This PR | On `origin/main` | |---|---| | `Authz.NO_CALLER_OWNS_TICKET` + `callerOwnsTicket` param | `ticketOwnedByCaller` param — `Authz.java:166`, used at `:225` | | `MessageService.isTicketOwnedBy` | `MessageService.ownsTicket` — `MessageService.java:1572` | | the MCP gate | `FleetMcp.java:593-596` | | the REST gate | `FleetApp.java:305`, `:328-334` | So both gates I was most worried about — the MCP tool and the REST route — are covered on main. There is no half-applied grant here. **And main's version of the rule is the safer one.** Compare: ```java // this PR case TASK_READ -> caller.isPrimary() || caller.isWorker() || caller.isArchitect() || callerOwnsTicket.test(targetSession); // origin/main:223 case TASK_READ -> caller.isPrimary() || caller.isWorker() || caller.isArchitect() || ((caller.isObserver() || caller.isCollaborator()) && ticketOwnedByCaller.test(targetSession)); ``` Same outcome for every role that exists today. But this PR's form hands the ticket grant to **any** future role that is not primary, worker or architect, the moment someone adds one. Main names the two roles it means. Rebasing this PR onto main would mean discarding its own version of this line anyway. ## The one part that did NOT ship: the push-loop nudge gate `ReplyPushLoop.pendingTicketsFor` on main is still the plain, unfiltered stream (`ReplyPushLoop.java:213`). So the grant exists at the two gates a caller reaches, and the loop that *tells* a pane to poll consults nothing. I checked whether that is reachable rather than assuming it, and it is, though narrowly. `onTicketTerminal` sets a ticket's nudge target from `resolveLiveLead(target)` → `primaryRegistry.nudgeTargetFor(target)` (`ReplyPushLoop.java:566`, `:484`), and that lookup is keyed on the **worker target**, not on the ticket. `fleet_send{wait:false}` is open to an observer and a collaborator. So if two different callers delegate to the same worker target, a ticket created by one can be recorded against the other as its nudge target — and the nudge then names a ticket that pane does not own, whose poll `TASK_READ` will refuse. Main already solved exactly this shape for *questions*: `PendingQuestion` carries "whether that nudge target's own role may actually run the `fleet_send(turnId=…)` a question nudge would tell it to run" (`ReplyPushLoop.java:222-227`). Tickets never got the same treatment. Filed as its own ticket so it can land small and against current main, rather than through a rebase of this one. ## Two things to carry into that ticket, not to repeat Both are in this PR's push-loop code and both point the wrong way: 1. **The gate fails open twice.** The 7-argument constructor passes `(lead, ticket) -> true`, and the field assignment turns a `null` predicate into `(lead, ticket) -> true` as well. The production wiring does supply the real check, and I could not reach either default — but a default on an authorization parameter has to fail closed. This repo wrote that rule down for this very PR series: see the Features entry on #778's near-miss, "A default on a parameter like this has to fail closed: naming no route is harmless, naming a refused one is the defect." The same applies to `FleetdAssembly`'s `if (messages == null || cr == null) return true;` construction-window escape. 2. **A suppressed nudge is logged at `debug`.** Nothing above `debug` records that a nudge was withheld, so if the filter is ever wrong the symptom is a lead that is silently never told about a ready ticket. That is the failure mode this project has already paid for twice: the one instrument that located #802 was removed when the symptom it logged was switched off. Log the suppression at a level an operator sees. ## Why this is not a rebase Measured with `git merge-tree --write-tree origin/main refs/pull/785/head`: exit 1, conflicts in **six** files — `Authz.java`, `FleetdAssembly.java`, `FleetMcp.java`, `ReplyPushLoop.java`, `FleetApp.java` and `AuthzTest.java`. The branch is 47 commits behind. Five of those six conflicts are in the half that is already on main, so resolving them would mean deleting this PR's work to keep main's. The remaining ~50 lines of `ReplyPushLoop` are worth more as a fresh change. ## On the one finding from the delegated review A reviewer flagged `MessageService.poll` reaching `tasks.get(ticket)` on a `ConcurrentHashMap`, which throws on a null key. The map type and the missing guard are both real. It is **not** a defect in this PR, for two independent reasons, and I checked each: - Every caller guards first. `FleetMcp.java:1346` returns `error("ticket (or target) is required")` for a blank ticket before `messages.poll` at `:1349`, and the REST path takes `ctx.pathParam("ticket")` on `GET /tasks/{ticket}`, which cannot match an empty segment. - The line is unchanged from `origin/main:1500`, so it predates this branch. Recording it here so nobody re-raises it as a blocker.
ltms closed this pull request 2026-10-07 12:20:26 +02:00
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 1m55s
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#785
Delete Branch "worker/778-12988a-4"

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?