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

Two convenience overloads have dangerous defaults, and one of them fails open: pin that no production code reaches for MessageService.poll(String) #718

New Issue
Closed
opened 2026-10-04 07:35:37 +02:00 by ltms · 3 comments
ltms commented 2026-10-04 07:35:37 +02:00
Owner
Copy Link
Copy Source

Found by me (lead) while verifying PR #716. Everything below is measured in the main clone at
95311c6 unless it names a branch.

The shape

Both MessageService.poll and Authz.permits have a shorter convenience overload that forwards to
the real one with a default argument. A future call site can reach for the short form, compile, pass
the whole suite, and silently get the default. The two differ in which way they fail, and that is
what makes one of them urgent.

MessageService.poll(String) — fails open, silently

public TaskView poll(String ticket) {
    return poll(ticket, null);
}

null is the "no caller terminal" value, and ownsTicket is
callerTerminal == null || callerTerminal.equals(task.creatorTerminal). So the one-argument form
skips the ownership check entirely. That is exactly the hole #705 was filed for: before PR #716,
FleetApp.taskStatus called this overload, and any worker could read any ticket.

After PR #716 merges there are zero production callers. I measured both call sites on that
branch:

$ git grep -n "\.poll(" 0ba597e -- fleetd/src/main/java
FleetMcp.java:1203:  messages.poll(ticket, callerTerminal)
FleetApp.java:898:   messages.poll(ctx.pathParam("ticket"), caller == null ? null : caller.terminal())

So the fix is complete today, and nothing stops it being undone by a one-word edit that no test
notices.

Authz.permits 3-arg — fails closed, loudly

The three-argument form defaults the classifier to NO_KNOWN_LEAD_OR_COLLABORATOR, a deny-all. A
future call site reaching for it would refuse every collaborator send. That is a real defect, but it
is loud and self-correcting: the refusal shows up the first time anyone tries. It has no production
caller — both gates call the four-argument form.

Why not just delete poll(String)

I measured the test usage before proposing anything:

$ grep -rn "\.poll(" fleetd/src/test/java --include="*.java" | grep -v "poll([^)]*,[^)]*)" | wc -l
44

44 test call sites. Deleting the overload means editing all 44 for no behaviour change, and a large
mechanical diff is its own risk. Keep the overload.

What to do

  1. Add a source-scrape test asserting that no file under fleetd/src/main/java calls
    MessageService.poll with a single argument. This codebase already uses that idiom — see
    FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCollaboratorsVisibleTo and the two tests
    PR #716 adds. It needs its own control assertion: prove the scan really reached the production
    sources and found the two-argument calls, so a glob that matches nothing cannot pass as "no
    violation found".
  2. Prove the test fails. Change one production call site back to the one-argument form,
    mvn -o compile green first so the mutation is live and not a compile error, confirm the new test
    goes red, then restore and confirm the file is byte-identical.
  3. Add one comment to the 3-arg Authz.permits saying it is a test convenience and that its
    default classifier denies every collaborator. State the current contract only — no ticket number,
    no history. This is the follow-through on a decision the previous lead recorded and deliberately
    left for the next change in this area.

Not measured

I have not written the test, so I do not know whether the scrape can be written without being
brittle against formatting. If it cannot be made to hold without a control that really fires, say so
rather than shipping a test that passes on an empty scan — a zero match is not a finding.

Blocked on

PR #716. Point 1 is only true once that merges; until then FleetApp.java still has a production
caller of the one-argument form.

Found by me (lead) while verifying PR #716. Everything below is measured in the main clone at `95311c6` unless it names a branch. ## The shape Both `MessageService.poll` and `Authz.permits` have a shorter convenience overload that forwards to the real one with a default argument. A future call site can reach for the short form, compile, pass the whole suite, and silently get the default. The two differ in which way they fail, and that is what makes one of them urgent. ### `MessageService.poll(String)` — fails **open**, silently ```java public TaskView poll(String ticket) { return poll(ticket, null); } ``` `null` is the "no caller terminal" value, and `ownsTicket` is `callerTerminal == null || callerTerminal.equals(task.creatorTerminal)`. So the one-argument form **skips the ownership check entirely**. That is exactly the hole #705 was filed for: before PR #716, `FleetApp.taskStatus` called this overload, and any worker could read any ticket. After PR #716 merges there are **zero** production callers. I measured both call sites on that branch: ``` $ git grep -n "\.poll(" 0ba597e -- fleetd/src/main/java FleetMcp.java:1203: messages.poll(ticket, callerTerminal) FleetApp.java:898: messages.poll(ctx.pathParam("ticket"), caller == null ? null : caller.terminal()) ``` So the fix is complete today, and nothing stops it being undone by a one-word edit that no test notices. ### `Authz.permits` 3-arg — fails **closed**, loudly The three-argument form defaults the classifier to `NO_KNOWN_LEAD_OR_COLLABORATOR`, a deny-all. A future call site reaching for it would refuse every collaborator send. That is a real defect, but it is loud and self-correcting: the refusal shows up the first time anyone tries. It has no production caller — both gates call the four-argument form. ## Why not just delete `poll(String)` I measured the test usage before proposing anything: ``` $ grep -rn "\.poll(" fleetd/src/test/java --include="*.java" | grep -v "poll([^)]*,[^)]*)" | wc -l 44 ``` 44 test call sites. Deleting the overload means editing all 44 for no behaviour change, and a large mechanical diff is its own risk. Keep the overload. ## What to do 1. **Add a source-scrape test** asserting that no file under `fleetd/src/main/java` calls `MessageService.poll` with a single argument. This codebase already uses that idiom — see `FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCollaboratorsVisibleTo` and the two tests PR #716 adds. It needs its own **control assertion**: prove the scan really reached the production sources and found the two-argument calls, so a glob that matches nothing cannot pass as "no violation found". 2. **Prove the test fails.** Change one production call site back to the one-argument form, `mvn -o compile` green first so the mutation is live and not a compile error, confirm the new test goes red, then restore and confirm the file is byte-identical. 3. **Add one comment to the 3-arg `Authz.permits`** saying it is a test convenience and that its default classifier denies every collaborator. State the current contract only — no ticket number, no history. This is the follow-through on a decision the previous lead recorded and deliberately left for the next change in this area. ## Not measured I have not written the test, so I do not know whether the scrape can be written without being brittle against formatting. If it cannot be made to hold without a control that really fires, say so rather than shipping a test that passes on an empty scan — a zero match is not a finding. ## Blocked on PR #716. Point 1 is only true once that merges; until then `FleetApp.java` still has a production caller of the one-argument form.
ltms commented 2026-10-04 07:50:16 +02:00
Author
Owner
Copy Link
Copy Source

No longer blocked, and there is a second instance

PR #716 merged as 9a64d42. So point 1 is now true: MessageService.poll(String) has zero
production callers. I measured it on main at that commit:

$ git grep -n "\.poll(" 9a64d42 -- fleetd/src/main/java
inject/Injector.java:493:   t.queue.poll()                       (a Queue, not this method)
mcp/FleetMcp.java:1203:     messages.poll(ticket, callerTerminal)
rest/FleetApp.java:899:     messages.poll(ctx.pathParam("ticket"), caller == null ? null : caller.terminal())

A second instance of the same shape

FleetMcp.java:973-975 forwards the five-argument sendAsync to the six-argument one with
creatorTerminal defaulted to null:

return sendAsync(messages, sessionId, content, onAccepted, profiles, null);

null is the "no creator" value, and a ticket with no recorded creator matches no terminal-bearing
caller at all — so that default does not merely skip a check, it makes the resulting ticket
unreadable by its own creator. That is the exact failure PR #716's second commit had to fix on the
REST path, where the defaulting happened by accident rather than deliberately.

It is test-only today: the one production caller is FleetMcp.java:493, which passes a real
caller. So it is the same class of hazard as poll(String) — a convenience overload whose default
is wrong in a way the compiler and the suite both accept.

Scope, updated

Cover both methods with the one scrape test rather than writing two: assert that no file under
fleetd/src/main/java calls either MessageService.poll with a single argument or the five-argument
FleetMcp.sendAsync. One control assertion proving the scan really read the production sources and
found the long-form calls still covers both, and a single test is easier to keep honest than two that
drift apart.

The mutation proof in point 2 then has two halves, one per method. Each needs mvn -o compile
confirmed green first so the mutation is live rather than a compile error.

Everything else in the original scope stands, including the Authz.permits comment in point 3.

## No longer blocked, and there is a second instance PR #716 merged as `9a64d42`. So point 1 is now true: `MessageService.poll(String)` has **zero** production callers. I measured it on `main` at that commit: ``` $ git grep -n "\.poll(" 9a64d42 -- fleetd/src/main/java inject/Injector.java:493: t.queue.poll() (a Queue, not this method) mcp/FleetMcp.java:1203: messages.poll(ticket, callerTerminal) rest/FleetApp.java:899: messages.poll(ctx.pathParam("ticket"), caller == null ? null : caller.terminal()) ``` ### A second instance of the same shape `FleetMcp.java:973-975` forwards the five-argument `sendAsync` to the six-argument one with `creatorTerminal` defaulted to `null`: ```java return sendAsync(messages, sessionId, content, onAccepted, profiles, null); ``` `null` is the "no creator" value, and a ticket with no recorded creator **matches no terminal-bearing caller at all** — so that default does not merely skip a check, it makes the resulting ticket unreadable by its own creator. That is the exact failure PR #716's second commit had to fix on the REST path, where the defaulting happened by accident rather than deliberately. It is test-only today: the one production caller is `FleetMcp.java:493`, which passes a real `caller`. So it is the same class of hazard as `poll(String)` — a convenience overload whose default is wrong in a way the compiler and the suite both accept. ### Scope, updated Cover **both** methods with the one scrape test rather than writing two: assert that no file under `fleetd/src/main/java` calls either `MessageService.poll` with a single argument or the five-argument `FleetMcp.sendAsync`. One control assertion proving the scan really read the production sources and found the long-form calls still covers both, and a single test is easier to keep honest than two that drift apart. The mutation proof in point 2 then has two halves, one per method. Each needs `mvn -o compile` confirmed green first so the mutation is live rather than a compile error. Everything else in the original scope stands, including the `Authz.permits` comment in point 3.
ltms commented 2026-10-04 08:01:06 +02:00
Author
Owner
Copy Link
Copy Source

Correction to my own evidence above, before anyone builds a test from it

I wrote this ticket. Re-running its greps on current main (9a64d42) shows two problems with the
evidence, and both change the test this ticket asks for.

1. The quoted grep output is incomplete

The body shows two lines. The real command returns three:

$ git grep -n '\.poll(' origin/main -- fleetd/src/main/java
Injector.java:493:   t.queue.poll();
FleetMcp.java:1231:  messages.poll(ticket, callerTerminal)
FleetApp.java:899:   messages.poll(ctx.pathParam("ticket"), caller == null ? null : caller.terminal())

Injector.java:493 was missing from the quote. It is not new — I checked it is present at
9a64d42~3 and on the PR branch 9a64d42^2, so it was there when I ran the original command and
the output I pasted had been trimmed. t.queue.poll() is a java.util.Queue.poll(), completely
unrelated to MessageService, so the conclusion is unaffected. The evidence was still wrong.

Also note FleetMcp.java:1203 in the body is now :1231 — the line moved in the merge.

2. The obvious regex gives a false positive, not a false negative

This is the part that matters for the test. I tried to count single-argument calls with the pattern
from the body and got 2, when the true answer is 0. Both were false positives:

  • Injector.java:493 — right method name, wrong receiver type.
  • FleetApp.java:899 — genuinely two-argument, but pathParam("ticket") contains a ), so
    poll([^)]*, can never reach the comma. A nested call with parentheses defeats the pattern.

So the usual warning here is inverted. The documented trap is that a broken pattern reads as a
clean zero; this pattern reads as a confident two. Either way the pattern was not measuring what
it claimed.

What this means for the scrape test

The test cannot be a naive method-name-and-comma regex. It must:

  • Distinguish the receiver, not just the method name. queue.poll() must not be a violation and
    messages.poll(...) must be. Matching on the bare name .poll( conflates two unrelated types.
  • Survive a nested call in the first argument. Counting to the first ) is wrong. Either balance
    the parentheses or match on something other than argument-separating commas.
  • Keep the control assertion the body already asks for, and make the control assert the two real
    two-argument sites are found, so a pattern that silently matches nothing cannot pass.

If a text scrape cannot be made to hold on both counts, say so and do not ship it. A test that
reports "no violations" because its pattern is broken is worse than no test — it is the exact defect
this ticket exists to prevent, moved one layer up.

The conclusion of the ticket is unchanged and still measured: zero production callers of the
one-argument MessageService.poll, read off the three lines above by eye rather than by regex.

## Correction to my own evidence above, before anyone builds a test from it I wrote this ticket. Re-running its greps on current `main` (`9a64d42`) shows two problems with the evidence, and both change the test this ticket asks for. ### 1. The quoted grep output is incomplete The body shows two lines. The real command returns **three**: ``` $ git grep -n '\.poll(' origin/main -- fleetd/src/main/java Injector.java:493: t.queue.poll(); FleetMcp.java:1231: messages.poll(ticket, callerTerminal) FleetApp.java:899: messages.poll(ctx.pathParam("ticket"), caller == null ? null : caller.terminal()) ``` `Injector.java:493` was missing from the quote. It is not new — I checked it is present at `9a64d42~3` and on the PR branch `9a64d42^2`, so it was there when I ran the original command and the output I pasted had been trimmed. `t.queue.poll()` is a `java.util.Queue.poll()`, completely unrelated to `MessageService`, so the conclusion is unaffected. The evidence was still wrong. Also note `FleetMcp.java:1203` in the body is now **`:1231`** — the line moved in the merge. ### 2. The obvious regex gives a false positive, not a false negative This is the part that matters for the test. I tried to count single-argument calls with the pattern from the body and got **2**, when the true answer is **0**. Both were false positives: - `Injector.java:493` — right method name, wrong receiver type. - `FleetApp.java:899` — genuinely two-argument, but `pathParam("ticket")` contains a `)`, so `poll([^)]*,` can never reach the comma. A nested call with parentheses defeats the pattern. So the usual warning here is inverted. The documented trap is that a broken pattern reads as a clean zero; this pattern reads as a **confident two**. Either way the pattern was not measuring what it claimed. ### What this means for the scrape test The test cannot be a naive method-name-and-comma regex. It must: - **Distinguish the receiver**, not just the method name. `queue.poll()` must not be a violation and `messages.poll(...)` must be. Matching on the bare name `.poll(` conflates two unrelated types. - **Survive a nested call in the first argument.** Counting to the first `)` is wrong. Either balance the parentheses or match on something other than argument-separating commas. - Keep the **control assertion** the body already asks for, and make the control assert the two real two-argument sites are *found*, so a pattern that silently matches nothing cannot pass. If a text scrape cannot be made to hold on both counts, say so and do not ship it. A test that reports "no violations" because its pattern is broken is worse than no test — it is the exact defect this ticket exists to prevent, moved one layer up. The conclusion of the ticket is unchanged and still measured: **zero production callers of the one-argument `MessageService.poll`**, read off the three lines above by eye rather than by regex.
ltms commented 2026-10-04 08:24:36 +02:00
Author
Owner
Copy Link
Copy Source

Merged locally as 6e06058 and pushed to main.

What landed

Two files, +283 lines, no production behaviour change:

  • fleetd/src/test/java/dev/ltms/fleet/msg/MessageServicePollUsageTest.java (new, 280 lines)
  • fleetd/src/main/java/dev/ltms/fleet/auth/Authz.java (+3, javadoc only)

Verification I ran myself

mvn clean install on e5f4fb8: Tests run: 2020, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS, exit 0. That is 2018 on main plus the 2 new tests.

The merge commit's tree is byte-identical to the branch tree I built:

merge tree:  cb901a8d49beefcf56afab7c1ce16b71b5277d54
branch tree: cb901a8d49beefcf56afab7c1ce16b71b5277d54

So the green build covers the merge exactly, and no post-merge rebuild was needed. Normally a clean auto-merge still needs its own build, because the merge tree differs from both parents. Here it provably does not.

I read the full 280-line test myself. Both tests carry loud control assertions, so a drifted anchor fails instead of passing on an empty match:

  • noProductionFileCallsTheSingleArgumentPollOverload — asserts filesScanned > 0, and assertEquals(2, twoArgSites.size()) naming both FleetMcp.java and FleetApp.java. If the anchor stops matching, that second assertion goes red.
  • everyMessageServiceDeclarationIsNamedMessages — asserts filesScanned > 0 and names.size() > 0. It deliberately does not hardcode a declaration count, so adding a new MessageService field does not make it red for the wrong reason.

Two corrections to my own brief, for the record

  1. My first evidence paste on this ticket showed 2 grep lines where the real command returns 3 — Injector.java:493 t.queue.poll() was missing. The paste had been trimmed. I verified the line is present on the branch.
  2. My naive regex reported a confident 2 single-argument callers. The true answer is 0: both were false positives, one on receiver type and one because a nested ) defeated [^)]*. A broken pattern read as a confident count, not as a clean zero. Those two traps became the acceptance criteria.

I also claimed 43 MessageService declarations. The worker corrected it to 41, and the worker is right. 43 raw hits minus FleetdRuntime.java:106 MessageService messages() (a method declaration) minus FleetMcp.java:486 (a // comment reading "MessageService has won the session lock", which the un-stripped scan would have reported as a violation). I re-checked both exclusions in my own earlier output.

One residual gap, recorded not fixed

The scan's needle is the literal messages.poll(, and the declaration test deliberately skips method declarations. So runtime.messages().poll(ticket) — a call through the FleetdRuntime.messages() accessor — is invisible to both tests.

I measured whether that shape is reachable. Production has zero callers of .messages(). The only call sites are in tests, which this scan does not read:

  • FleetdAssemblyReleaseCleanupBehaviouralTest.java:162
  • FleetdAssemblyHealthFailTargetBehaviouralTest.java:155
  • FleetHealthMonitorTest.java:537, :560, :568

So this is a defect on paper until someone names a path in — the same judgement I applied to #722. It did not block the merge. If a production caller ever reaches MessageService through the accessor, this guard goes quiet rather than red, and that is the thing to remember.

Closing.

Merged locally as `6e06058` and pushed to `main`. ## What landed Two files, +283 lines, no production behaviour change: - `fleetd/src/test/java/dev/ltms/fleet/msg/MessageServicePollUsageTest.java` (new, 280 lines) - `fleetd/src/main/java/dev/ltms/fleet/auth/Authz.java` (+3, javadoc only) ## Verification I ran myself `mvn clean install` on `e5f4fb8`: **Tests run: 2020, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS**, exit 0. That is 2018 on `main` plus the 2 new tests. The merge commit's tree is byte-identical to the branch tree I built: ``` merge tree: cb901a8d49beefcf56afab7c1ce16b71b5277d54 branch tree: cb901a8d49beefcf56afab7c1ce16b71b5277d54 ``` So the green build covers the merge exactly, and no post-merge rebuild was needed. Normally a clean auto-merge still needs its own build, because the merge tree differs from both parents. Here it provably does not. I read the full 280-line test myself. Both tests carry loud control assertions, so a drifted anchor fails instead of passing on an empty match: - `noProductionFileCallsTheSingleArgumentPollOverload` — asserts `filesScanned > 0`, and `assertEquals(2, twoArgSites.size())` naming both `FleetMcp.java` and `FleetApp.java`. If the anchor stops matching, that second assertion goes red. - `everyMessageServiceDeclarationIsNamedMessages` — asserts `filesScanned > 0` and `names.size() > 0`. It deliberately does **not** hardcode a declaration count, so adding a new `MessageService` field does not make it red for the wrong reason. ## Two corrections to my own brief, for the record 1. My first evidence paste on this ticket showed **2** grep lines where the real command returns **3** — `Injector.java:493 t.queue.poll()` was missing. The paste had been trimmed. I verified the line is present on the branch. 2. My naive regex reported a confident **2** single-argument callers. The true answer is **0**: both were false positives, one on receiver type and one because a nested `)` defeated `[^)]*`. A broken pattern read as a confident count, not as a clean zero. Those two traps became the acceptance criteria. I also claimed **43** `MessageService` declarations. The worker corrected it to **41**, and the worker is right. 43 raw hits minus `FleetdRuntime.java:106 MessageService messages()` (a method declaration) minus `FleetMcp.java:486` (a `//` comment reading "MessageService has won the session lock", which the un-stripped scan would have reported as a violation). I re-checked both exclusions in my own earlier output. ## One residual gap, recorded not fixed The scan's needle is the literal `messages.poll(`, and the declaration test deliberately skips *method* declarations. So `runtime.messages().poll(ticket)` — a call through the `FleetdRuntime.messages()` accessor — is invisible to **both** tests. I measured whether that shape is reachable. Production has **zero** callers of `.messages()`. The only call sites are in tests, which this scan does not read: - `FleetdAssemblyReleaseCleanupBehaviouralTest.java:162` - `FleetdAssemblyHealthFailTargetBehaviouralTest.java:155` - `FleetHealthMonitorTest.java:537`, `:560`, `:568` So this is a defect on paper until someone names a path in — the same judgement I applied to #722. It did not block the merge. If a production caller ever reaches `MessageService` through the accessor, this guard goes quiet rather than red, and that is the thing to remember. Closing.
ltms closed this issue 2026-10-04 08:24:41 +02:00
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#718
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?