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

Three more tool results claim "delivered" for something weaker than delivery #365

New Issue
Closed
opened 2026-09-05 07:49:59 +02:00 by ltms · 1 comment
ltms commented 2026-09-05 07:49:59 +02:00
Owner
Copy Link
Copy Source

Found while implementing #361. Same shape as that issue: a result reports a stronger outcome than the operation actually established. Filed separately so #361 stays scoped; none of these are fixed.

1. fleet_reply always returns the literal "delivered". FleetMcp.reply() returns that string unconditionally, but the underlying messages.reply(...) either resolves a waiting send or just queues the reply in an inbox for a later drain. "Delivered" is true in the first case and not the second, and the caller cannot tell which happened.

2. POST /sessions/{id}/reply returns {"delivered": true} unconditionally. FleetApp.replyMessage, same underlying call, same gap — over REST, where there is no pane to look at either.

3. countNudge("delivered") counts attempts, not arrivals. Both ReplyPushLoop and LeadHeartbeatLoop call it after nudging a pane. It most likely records that a nudge was sent, not that the pane saw it. Unverified — I have not read the counter's consumers, and the metric name may simply be misleading rather than the count being wrong.

Why this matters more than the wording

This makes three independent places, plus the one #361 is fixing, where a green result stands in for an unestablished fact. That is the same family as #359 (a stalled delivery that looked fine) and #360 (a hardening config that silently disabled caller identity). The recurring cost is that an operator cannot distinguish "it worked" from "I could not tell", and those need different responses.

Worth considering as one change rather than three patches: a shared convention for reporting what a call actually established — queued, handed over, or confirmed read — so the next tool that returns a status does not have to re-invent the distinction. A per-site reword fixes today's three and does nothing about the fourth.

Related: #361, #359, #360.

Found while implementing #361. Same shape as that issue: a result reports a stronger outcome than the operation actually established. Filed separately so #361 stays scoped; none of these are fixed. **1. `fleet_reply` always returns the literal `"delivered"`.** `FleetMcp.reply()` returns that string unconditionally, but the underlying `messages.reply(...)` either resolves a waiting send *or* just queues the reply in an inbox for a later drain. "Delivered" is true in the first case and not the second, and the caller cannot tell which happened. **2. `POST /sessions/{id}/reply` returns `{"delivered": true}` unconditionally.** `FleetApp.replyMessage`, same underlying call, same gap — over REST, where there is no pane to look at either. **3. `countNudge("delivered")` counts attempts, not arrivals.** Both `ReplyPushLoop` and `LeadHeartbeatLoop` call it after nudging a pane. It most likely records that a nudge was *sent*, not that the pane saw it. **Unverified** — I have not read the counter's consumers, and the metric name may simply be misleading rather than the count being wrong. ## Why this matters more than the wording This makes three independent places, plus the one #361 is fixing, where a green result stands in for an unestablished fact. That is the same family as #359 (a stalled delivery that looked fine) and #360 (a hardening config that silently disabled caller identity). The recurring cost is that an operator cannot distinguish "it worked" from "I could not tell", and those need different responses. Worth considering as one change rather than three patches: a shared convention for reporting what a call actually established — queued, handed over, or confirmed read — so the next tool that returns a status does not have to re-invent the distinction. A per-site reword fixes today's three and does nothing about the fourth. Related: #361, #359, #360.
ltms commented 2026-09-09 02:41:33 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main in 6f828b8. PR #379. Doc follow-up in cbc7324 and wiki 7f13dbf.

What I verified myself

Full mvn clean install on the merged tree: MVN_EXIT=0, Tests run: 1450, Failures: 0, 0 compile errors.

The worker ran no mutation on this one, so I did

This is the only one of tonight's three that changed production code, and it was the only one with no mutation proof. I mutated the single line that carries the whole ticket — made a queued reply claim it had resolved a send:

return ReplyOutcome.RESOLVED_SEND;      // MUTATED  (was QUEUED)

MessageServiceTest   82 run, 5 failures
FleetMcpTest         75 run, 1 failure
FleetAppTest         37 run, 1 failure

Seven tests kill it, and they are spread across all three doors — the service, MCP and REST:

FleetAppTest.replyWithNoPendingSendQueuesInsteadOfConflict
FleetMcpTest.replyWithNoPendingSendIsQueuedNotError
MessageServiceTest.replyQueuesInInboxWhenNoSendIsOpen
MessageServiceTest.hasStrandedReplyIsTrueWhenNoSendWasWaiting
MessageServiceTest.hasStrandedReplyClearsOnAbandon
MessageServiceTest.hasStrandedReplyClearsOnceTheTargetsNextDeliveryIsAccepted
MessageServiceTest.twoAskTimedOutTicketsOnOneTargetFallBackToTheInboxRatherThanGuess

Restored and confirmed byte-identical. All three ReplyOutcome values are pinned somewhere, including RESOLVED_ASYNC_TICKET, which the report did not claim coverage for.

One false step of my own worth recording: my first mutation run returned exit 1 and I nearly read that as the mutation being killed. It was not. Surefire separates -Dtest patterns with , and I had used +, so no tests ran at all — "No tests matching pattern". A non-zero exit that means "your command was wrong" looks exactly like one that means "the guard works". I re-ran it properly, and the numbers above are from that run.

Something the report implied that is not true

The report reads as though it added tests. It added zero new @Test methods — the net count is 0 added, 0 removed, and the suite total stayed at 1439 on its base. What it actually did was strengthen assertions inside tests that already existed, and rename two (successfulNudgeIncrementsDelivered → ...IncrementsSent).

That is the right approach here and the assertions really do fail against the old code, as the mutation above proves. But "tests that fail against the old code" invites the reader to picture new coverage. I am noting it because a test count that does not move is exactly the signal a reviewer uses, and anyone checking this merge by counting would have found a discrepancy and had to chase it.

The wiki half the worker could not do

The report correctly said it could not check wiki/ — a worker's worktree has no checkout of it. It was right that something was there. Three surfaces documented the old behaviour:

  1. wiki/15-REST-API-Reference.md said Response: 200 {sessionId, delivered: true}. That constant is now wrong. Updated to the real shape, with the three outcome values.
  2. docs/CB-5xx-Hardening.md listed the push-nudge metric as outcome ∈ delivered|exhausted. Now sent|exhausted.
  3. wiki/11-Features.md had no entry. The charter makes one mandatory for a visible behaviour change on an MCP tool or an endpoint, and this is both. Added, covering both doors and the counter rename.

The Features entry calls out the one thing that can break an existing caller: delivered: false is still 200 and still a success. A client that treats it as a failure and retries will queue the reply twice. Branch on outcome, not on delivered. No such client exists in this repo — I grepped — but the REST API is a public door.

Not fixed, reported by the sweep

FleetMcp.ack returns acknowledged <msgId> even when the id never existed; both ReplyInbox implementations silently no-op on a miss. FleetMcp.stop returns stopped <paneId> once herdr's pane.close is accepted, not once the process is confirmed gone. Both are the same attempt-vs-confirmed shape this ticket is about. Neither was traced further, and neither is fixed here.

Closing.

Merged to `main` in 6f828b8. PR #379. Doc follow-up in cbc7324 and wiki 7f13dbf. ## What I verified myself Full `mvn clean install` on the merged tree: `MVN_EXIT=0`, `Tests run: 1450, Failures: 0`, 0 compile errors. ## The worker ran no mutation on this one, so I did This is the only one of tonight's three that changed production code, and it was the only one with no mutation proof. I mutated the single line that carries the whole ticket — made a queued reply claim it had resolved a send: ```java return ReplyOutcome.RESOLVED_SEND; // MUTATED (was QUEUED) ``` ``` MessageServiceTest 82 run, 5 failures FleetMcpTest 75 run, 1 failure FleetAppTest 37 run, 1 failure ``` Seven tests kill it, and they are spread across all three doors — the service, MCP and REST: ``` FleetAppTest.replyWithNoPendingSendQueuesInsteadOfConflict FleetMcpTest.replyWithNoPendingSendIsQueuedNotError MessageServiceTest.replyQueuesInInboxWhenNoSendIsOpen MessageServiceTest.hasStrandedReplyIsTrueWhenNoSendWasWaiting MessageServiceTest.hasStrandedReplyClearsOnAbandon MessageServiceTest.hasStrandedReplyClearsOnceTheTargetsNextDeliveryIsAccepted MessageServiceTest.twoAskTimedOutTicketsOnOneTargetFallBackToTheInboxRatherThanGuess ``` Restored and confirmed byte-identical. All three `ReplyOutcome` values are pinned somewhere, including `RESOLVED_ASYNC_TICKET`, which the report did not claim coverage for. **One false step of my own worth recording:** my first mutation run returned exit 1 and I nearly read that as the mutation being killed. It was not. Surefire separates `-Dtest` patterns with `,` and I had used `+`, so **no tests ran at all** — "No tests matching pattern". A non-zero exit that means "your command was wrong" looks exactly like one that means "the guard works". I re-ran it properly, and the numbers above are from that run. ## Something the report implied that is not true The report reads as though it added tests. It added **zero** new `@Test` methods — the net count is 0 added, 0 removed, and the suite total stayed at 1439 on its base. What it actually did was strengthen assertions inside tests that already existed, and rename two (`successfulNudgeIncrementsDelivered` → `...IncrementsSent`). That is the right approach here and the assertions really do fail against the old code, as the mutation above proves. But "tests that fail against the old code" invites the reader to picture new coverage. I am noting it because a test count that does not move is exactly the signal a reviewer uses, and anyone checking this merge by counting would have found a discrepancy and had to chase it. ## The wiki half the worker could not do The report correctly said it could not check `wiki/` — a worker's worktree has no checkout of it. It was right that something was there. Three surfaces documented the old behaviour: 1. `wiki/15-REST-API-Reference.md` said `Response: 200 {sessionId, delivered: true}`. That constant is now wrong. Updated to the real shape, with the three `outcome` values. 2. `docs/CB-5xx-Hardening.md` listed the push-nudge metric as `outcome ∈ delivered|exhausted`. Now `sent|exhausted`. 3. `wiki/11-Features.md` had no entry. The charter makes one mandatory for a visible behaviour change on an MCP tool or an endpoint, and this is both. Added, covering both doors and the counter rename. The Features entry calls out the one thing that can break an existing caller: **`delivered: false` is still `200` and still a success.** A client that treats it as a failure and retries will queue the reply twice. Branch on `outcome`, not on `delivered`. No such client exists in this repo — I grepped — but the REST API is a public door. ## Not fixed, reported by the sweep `FleetMcp.ack` returns `acknowledged <msgId>` even when the id never existed; both `ReplyInbox` implementations silently no-op on a miss. `FleetMcp.stop` returns `stopped <paneId>` once herdr's `pane.close` is accepted, not once the process is confirmed gone. Both are the same attempt-vs-confirmed shape this ticket is about. Neither was traced further, and neither is fixed here. Closing.
ltms closed this issue 2026-09-09 02:41:39 +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#365
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?