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

A REST reply with no content field silently becomes an empty reply, and the lead cannot tell it from a member that said nothing #302

New Issue
Closed
opened 2026-09-04 07:24:35 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 07:24:35 +02:00
Owner
Copy Link
Copy Source

From #297's shape check. I verified this myself at all three sites.

Two sibling defects came with it (spawnMember and stopMember lack a catch (HerdrException)); those produce a visible bare 500 and are already an accepted shape in that file with passing tests. This one is different in kind: it writes a wrong value silently. That is why it gets its own ticket.

The three sites

REST reads the field with a default and no check:

// FleetApp.replyMessage
content = mapper.readTree(ctx.body()).path("content").asText("");
...
messages.reply(id, content);

MCP rejects it:

// FleetMcp.reply :817
if (isBlank(content)) {
    return error("content is required");
}

And the shared service in between has no check at all:

// MessageService :414
public boolean reply(String session, String content) {
    if (rendezvous.resolve(session, content)) {

So the guard lives in one handler rather than in the thing both handlers call. .path("content") returns a missing node rather than throwing, and .asText("") turns that into an empty string, which goes straight through to rendezvous.resolve.

Direction of harm

An empty reply resolves the lead's waiter. The turn completes. The lead sees a member that finished and reported nothing — which is indistinguishable from a member that genuinely replied with nothing, and from several real conditions this project already has notes about (a clipped completion scrape, a backend that died mid-turn).

That is worse than the bare 500 its two siblings produce. A 500 is loud, it is visible in the response, and the caller knows the operation did not happen. This one reports success and destroys the information. A malformed client, a typo in a field name, or a JSON body that lost its content key in transit all look exactly like a member with nothing to say.

Where the fix belongs — decide this, do not assume it

There are two defensible places, and I want the choice argued in your report rather than picked by convenience:

  1. In MessageService.reply, so both doors inherit it. This is the "put the rule where it cannot be skipped" answer, and it matches how #297's profilesView correction went. The risk is that reply is on the hot path for every fleet_reply, and a new rejection there changes behaviour for the MCP door too — which today rejects blank content before ever reaching this method, so in principle nothing changes, but you must confirm that rather than assume it.
  2. In FleetApp.replyMessage, mirroring FleetMcp's guard. Smaller blast radius, but it leaves the rule written twice — exactly the drift shape #284 and #297 both were.

My reading is that (1) is right and (2) is what this codebase keeps doing wrong. Check whether any caller legitimately replies with empty content before you commit to (1) — search every call site of MessageService.reply, including the completion fallback and any test helper, and say what you found. If something does rely on it, (2) becomes the honest answer and I want to hear why.

Either way, REST must return the same {error, detail} envelope the rest of FleetApp uses — read herdrError and the surrounding 400 paths for the shape. Do not invent a new one.

Rules

  • Prove it with a test that fails without the fix: a POST /sessions/{id}/reply whose body has no content key must not resolve a waiter.
  • Add the sibling case too: a body where content is present but empty or whitespace. Say in your report whether you treat those the same as missing, and why — isBlank in FleetMcp treats them the same, so diverging from that would itself be a new asymmetry.
  • Do not change FleetMcp. It is the correct side.
  • Do not fix spawnMember or stopMember. They are a separate, lower-severity shape and I will decide on them separately.
  • Mutation proof required: revert the fix, quote the real failure, restore it.

Shape check

When done, look for the same shape — a required field read with a silent default instead of a check — in FleetApp.java only. .asText(""), .asInt(0), .path(...) without a presence check, and anything similar. One line each, do not fix any of it. Note that a default is only a defect where the field is genuinely required; an optional field with a sensible default is correct, so say which each one is.

From #297's shape check. **I verified this myself** at all three sites. Two sibling defects came with it (`spawnMember` and `stopMember` lack a `catch (HerdrException)`); those produce a visible bare 500 and are already an accepted shape in that file with passing tests. **This one is different in kind: it writes a wrong value silently.** That is why it gets its own ticket. ## The three sites REST reads the field with a default and no check: ```java // FleetApp.replyMessage content = mapper.readTree(ctx.body()).path("content").asText(""); ... messages.reply(id, content); ``` MCP rejects it: ```java // FleetMcp.reply :817 if (isBlank(content)) { return error("content is required"); } ``` And the shared service in between has **no check at all**: ```java // MessageService :414 public boolean reply(String session, String content) { if (rendezvous.resolve(session, content)) { ``` So the guard lives in one handler rather than in the thing both handlers call. `.path("content")` returns a missing node rather than throwing, and `.asText("")` turns that into an empty string, which goes straight through to `rendezvous.resolve`. ## Direction of harm An empty reply **resolves the lead's waiter**. The turn completes. The lead sees a member that finished and reported nothing — which is indistinguishable from a member that genuinely replied with nothing, and from several real conditions this project already has notes about (a clipped completion scrape, a backend that died mid-turn). That is worse than the bare 500 its two siblings produce. A 500 is loud, it is visible in the response, and the caller knows the operation did not happen. This one reports success and destroys the information. A malformed client, a typo in a field name, or a JSON body that lost its `content` key in transit all look exactly like a member with nothing to say. ## Where the fix belongs — decide this, do not assume it There are two defensible places, and I want the choice argued in your report rather than picked by convenience: 1. **In `MessageService.reply`**, so both doors inherit it. This is the "put the rule where it cannot be skipped" answer, and it matches how #297's `profilesView` correction went. The risk is that `reply` is on the hot path for every `fleet_reply`, and a new rejection there changes behaviour for the MCP door too — which today rejects blank content *before* ever reaching this method, so in principle nothing changes, but you must confirm that rather than assume it. 2. **In `FleetApp.replyMessage`**, mirroring `FleetMcp`'s guard. Smaller blast radius, but it leaves the rule written twice — exactly the drift shape #284 and #297 both were. My reading is that (1) is right and (2) is what this codebase keeps doing wrong. **Check whether any caller legitimately replies with empty content before you commit to (1)** — search every call site of `MessageService.reply`, including the completion fallback and any test helper, and say what you found. If something does rely on it, (2) becomes the honest answer and I want to hear why. Either way, REST must return the same `{error, detail}` envelope the rest of `FleetApp` uses — read `herdrError` and the surrounding 400 paths for the shape. Do not invent a new one. ## Rules - Prove it with a test that fails without the fix: a `POST /sessions/{id}/reply` whose body has **no** `content` key must not resolve a waiter. - Add the sibling case too: a body where `content` is present but empty or whitespace. Say in your report whether you treat those the same as missing, and why — `isBlank` in `FleetMcp` treats them the same, so diverging from that would itself be a new asymmetry. - Do not change `FleetMcp`. It is the correct side. - Do not fix `spawnMember` or `stopMember`. They are a separate, lower-severity shape and I will decide on them separately. - Mutation proof required: revert the fix, quote the real failure, restore it. ## Shape check When done, look for the same shape — **a required field read with a silent default instead of a check** — in `FleetApp.java` only. `.asText("")`, `.asInt(0)`, `.path(...)` without a presence check, and anything similar. One line each, **do not fix any of it**. Note that a default is only a defect where the field is genuinely required; an optional field with a sensible default is correct, so say which each one is.
ltms commented 2026-09-04 07:36:50 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 4769481, extended in 21844b5. Real merge built green at 1310 tests.

The worker picked option (1) — the guard went into MessageService.reply, which both doors call — and it did the thing that made that choice safe rather than assuming it: it searched every call site first and reported what it found. The javadoc says so in the code, which is where that fact belongs.

The worker corrected my ticket, and it was right

My ticket quoted FleetMcp.reply as guarding with isBlank(content). It does not — it checks content == null. I confirmed that at :816:

if (content == null) {
    return error("content is required");
}

It is the sibling fleet_send at :709 that uses isBlank. I grepped for the error string and assumed the condition matched its sibling. That is a claim I put in a ticket without running the command that would have shown it, and the worker caught it by reading the file instead of trusting my snippet. It also flagged the correction for me to double-check rather than quietly working around it.

What that correction exposed, which the report did not trace

The consequence runs further than "MCP has the same gap, less reachably". MessageService.reply now throws, and fleet_reply's handler is a bare BiFunction with no try/catch around it (:299-306). So after this change the MCP door behaved like this:

content before #302 after #302, before my fix
null clean content is required clean content is required
" " delivered silently uncaught IllegalArgumentException

Not silent any more, which is the important half — but a new asymmetry inside one method, where the same caller mistake gets two different answers depending on which kind of blank it is.

Fixed in 21844b5 by widening that guard to isBlank, matching fleet_send. My ticket's "do not change FleetMcp" rule would have blocked this, which is the second time in this batch a protective constraint of mine blocked the correct fix — noted, and the rule is mine to lift.

My mutation — and the test that was missing

The worker's mutation reverted both source files and quoted both REST tests failing. Correct for what it covers, but its tests only exercise the REST door, so my correction arrived unproven. I wrote the missing test rather than shipping it on reasoning:

FleetMcpTest.replyWithBlankContentIsACleanToolErrorNotAnUncaughtException loops null, "", " ", "\n\t", asserts each returns an error result naming the argument, and asserts nothing reached the inbox.

Then I mutated my own fix back to == null:

FleetMcpTest.replyWithBlankContentIsACleanToolErrorNotAnUncaughtException
  blank content must be refused as a tool error, never thrown out of the handler
  ==> Unexpected exception thrown: java.lang.IllegalArgumentException: content is required
	at FleetMcpTest.java:338

Exactly the predicted failure, from the exact line that would produce it in production.

Shape check

Accepted. The one real instance it found — pong.path("protocol").asInt() in healthz defaulting to 0 with no presence check — is correctly judged low severity: it is diagnostics, and a 0 there reads as a protocol mismatch, which fails loudly rather than silently. Its reasoning on the rest is the part worth keeping: it separated genuinely-optional fields with sensible defaults from required ones, instead of flagging every .asText("") it could find. A sweep that reported all of them would have been noise.

spawnMember and stopMember are still open and untouched, as instructed. I will decide on those separately.

Merged as `4769481`, extended in `21844b5`. Real merge built green at **1310 tests**. The worker picked option (1) — the guard went into `MessageService.reply`, which both doors call — and it did the thing that made that choice safe rather than assuming it: it searched every call site first and reported what it found. The javadoc says so in the code, which is where that fact belongs. ## The worker corrected my ticket, and it was right My ticket quoted `FleetMcp.reply` as guarding with `isBlank(content)`. It does not — it checks `content == null`. I confirmed that at `:816`: ```java if (content == null) { return error("content is required"); } ``` It is the sibling `fleet_send` at `:709` that uses `isBlank`. **I grepped for the error string and assumed the condition matched its sibling.** That is a claim I put in a ticket without running the command that would have shown it, and the worker caught it by reading the file instead of trusting my snippet. It also flagged the correction for me to double-check rather than quietly working around it. ## What that correction exposed, which the report did not trace The consequence runs further than "MCP has the same gap, less reachably". `MessageService.reply` now **throws**, and `fleet_reply`'s handler is a bare `BiFunction` with no try/catch around it (`:299-306`). So after this change the MCP door behaved like this: | `content` | before #302 | after #302, before my fix | |---|---|---| | `null` | clean `content is required` | clean `content is required` | | `" "` | delivered silently | **uncaught `IllegalArgumentException`** | Not silent any more, which is the important half — but a new asymmetry inside one method, where the same caller mistake gets two different answers depending on which kind of blank it is. Fixed in `21844b5` by widening that guard to `isBlank`, matching `fleet_send`. My ticket's "do not change `FleetMcp`" rule would have blocked this, which is the second time in this batch a protective constraint of mine blocked the correct fix — noted, and the rule is mine to lift. ## My mutation — and the test that was missing The worker's mutation reverted both source files and quoted both REST tests failing. Correct for what it covers, but its tests only exercise the REST door, so **my correction arrived unproven**. I wrote the missing test rather than shipping it on reasoning: `FleetMcpTest.replyWithBlankContentIsACleanToolErrorNotAnUncaughtException` loops `null`, `""`, `" "`, `"\n\t"`, asserts each returns an error result naming the argument, and asserts nothing reached the inbox. Then I mutated my own fix back to `== null`: ``` FleetMcpTest.replyWithBlankContentIsACleanToolErrorNotAnUncaughtException blank content must be refused as a tool error, never thrown out of the handler ==> Unexpected exception thrown: java.lang.IllegalArgumentException: content is required at FleetMcpTest.java:338 ``` Exactly the predicted failure, from the exact line that would produce it in production. ## Shape check Accepted. The one real instance it found — `pong.path("protocol").asInt()` in `healthz` defaulting to `0` with no presence check — is correctly judged low severity: it is diagnostics, and a `0` there reads as a protocol mismatch, which fails loudly rather than silently. Its reasoning on the rest is the part worth keeping: it separated genuinely-optional fields with sensible defaults from required ones, instead of flagging every `.asText("")` it could find. A sweep that reported all of them would have been noise. `spawnMember` and `stopMember` are still open and untouched, as instructed. I will decide on those separately.
ltms closed this issue 2026-09-04 07:36:58 +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#302
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?